fix: honor --parallel across all bulk engine-call fan-outs

[glours]

What I did
errgroup.SetLimit(0) means "allow zero goroutines", not "unlimited", and maxConcurrency's Go zero-value is 0 — only NewComposeService sets it to -1 explicitly. Any composeService{} literal built without it (common in tests) silently deadlocked at every call site that called SetLimit unconditionally.

Separately, --parallel/COMPOSE_PARALLEL_LIMIT was only ever wired into pull, push, and the dependency-graph traversal, despite the docs promising a generic bound on "concurrent engine calls". Every other bulk operation (kill, pause, down, the up/create plan executor, logs, ps, top, wait, restart, remove, images, model pulls, watch) launched one goroutine per container/image/DAG-node with no cap at all.

Fix both via a shared newLimitedErrgroup helper applied at every fan-out site, threading maxConcurrency through forEachContainerConcurrent and ImagePruner, which had no access to composeService. Add a regression test for the zero-value case.

service_containers.go's waitDependencies is intentionally left unguarded: it's a per-dependency ticker poll, not a burst of engine calls.

Related issue

(not mandatory) A picture of a cute animal, if possible in relation to what you did

[docker-agent]

Assessment: 🟢 APPROVE

The newLimitedErrgroup helper is correctly implemented: it only calls SetLimit when maxConcurrency > 0, properly guarding against the zero-value deadlock. The fan-out at every bulk operation site follows the standard pattern safely. The regression test in compose_test.go correctly exercises the zero-value case. All hypothesized issues were dismissed after verification.

[codecov]

Codecov Report

❌ Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/compose/compose.go	60.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[ndeloof]

Thorough pass over every converted fan-out (result ordering, shared-state races, cancellation, error semantics) plus a local -race run: the mechanics are solid — ps/top fill indexed slices, images/pull guard their maps, pullRequiredImages keeps the caller ctx for post-pull resolution, and the executor's no-deadlock argument in the comment actually holds (topological plan.Nodes by construction + bounded dispatch ⇒ the oldest unfinished node always has finished dependencies). The zero-value test targets exactly the original bug.

One blocker, then two notes:

[high] Logs in --follow starves everything past the cap — including the monitor. Every logContainer call in follow mode streams indefinitely and never frees its errgroup slot, and monitor.Start is itself submitted through the same bounded group after the container loop. With --parallel 2 and 5 services, only two log streams ever open and the other three never appear; with --parallel 1, one stream opens and the monitor never starts at all — silently, for a documented knob. The PR already excludes waitDependencies for exactly this reason (long waits, not bursts of engine calls): follow-mode Logs belongs in the same exclusion. Simplest fix: keep the bounded group for the non-follow case only (or bound just the attach phase).

[low] restart bounds its inner per-service group, but it runs inside InDependencyOrder whose traversal has its own SetLimit(maxConcurrency+1): up to N×N concurrent engine calls. Strictly better than the unbounded inner loop it replaces, but the "bounded engine calls" promise isn't global — a shared semaphore would be the real fix, fine as a follow-up.

[nit] NewImagePruner gains a parameter — exported API, external consumers of pkg/compose break at compile. Consistent with the repo's API posture, just flagging it.

[docker-agent]

Assessment: 🟢 APPROVE

The PR correctly fixes the errgroup.SetLimit(0) footgun via newLimitedErrgroup and applies the concurrency cap consistently across all bulk fan-out sites. The helper and its callers are well-structured; the follow-mode logs path correctly uses a separate semaphore (rather than a bounded errgroup) to avoid starving the monitor goroutine.

One minor concern surfaced by the review:

[docker-agent]

[medium] releaseSlot in doLogContainer is not panic-safe — a panic inside ContainerLogs leaks the semaphore slot

doLogContainer relies on the caller (logContainer) having acquired a semaphore slot via acquireSlot, and releases it unconditionally on line 182 right after ContainerLogs returns:

r, err := s.apiClient().ContainerLogs(...)
releaseSlot(limiter)   // released only if ContainerLogs returns normally

If ContainerLogs panics (e.g. due to a nil dereference inside the Docker client), the releaseSlot call on line 182 is never reached. The slot is never returned to the semaphore, so the effective concurrency limit drops by 1 permanently for the lifetime of the process. With a small --parallel value this can eventually exhaust all slots and cause subsequent containers to block in acquireSlot indefinitely.

The design intentionally avoids defer releaseSlot(limiter) here because the slot must be freed right after the ContainerLogs call opens the stream (not at function exit), so that other containers can begin opening their streams while this goroutine copies its indefinite log stream. A panic guard that re-panics after releasing the slot would preserve both the intent and panic safety:

func panicSafeReleaseSlot(limiter *semaphore.Weighted) {
    if p := recover(); p != nil {
        releaseSlot(limiter)
        panic(p)
    }
}
// at the top of doLogContainer, before ContainerLogs:
defer panicSafeReleaseSlot(limiter)

In practice, panics in the Docker client's HTTP call path are extremely rare, so the real-world impact is low. Still, given that the acquireSlot/releaseSlot split-ownership pattern is new and non-obvious, documenting the panic-safety gap and adding the guard is worth considering.

Confidence	Score
🟡 moderate	62/100