fix: honor --parallel across all bulk engine-call fan-outs - #14177
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
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 Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
a05e2c7 to
e8cc2c2
Compare
ndeloof
left a comment
There was a problem hiding this comment.
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.
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