Skip to content

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

Open
glours wants to merge 1 commit into
docker:mainfrom
glours:fix/parallel-limit-guard
Open

fix: honor --parallel across all bulk engine-call fan-outs#14177
glours wants to merge 1 commit into
docker:mainfrom
glours:fix/parallel-limit-guard

Conversation

@glours

@glours glours commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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

@glours
glours requested review from a team as code owners September 3, 2026 15:14
@glours
glours requested a review from ndeloof September 3, 2026 15:14

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Bot commented Sep 3, 2026

Copy link
Copy Markdown

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
ndeloof previously approved these changes Sep 9, 2026
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>

@ndeloof ndeloof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants