Repository navigation
Conversation
Pure extraction, no behavior change: create() resolved the project model, ensured images/models and external networks/volumes, observed the daemon, and reconciled a Plan, all inline before calling executePlan. That decision-making half is now its own function, preparePlan, returning the canonical project, the daemon snapshot, and the Plan — nothing is created, started, or otherwise touched by it. This is Lot 1's last item (epic docker#14081): interactive up's eventual create/start phase boundary needs to prepare the plan once, run the Create phase, set up attach/printer/monitor, then run the Start phase — which needs the plan and snapshot available between those steps, not buried inside create()'s single call chain. create() itself is unchanged in behavior, just now a two-line call into preparePlan followed by the existing emitRunningEvents + executePlan. Verified with the existing unit suite plus the create/up/down e2e scenarios (TestLocalComposeUp, TestCreateIdempotentDefaultPlatform, TestCreateIdempotentSharedImageMixedPlatforms, TestCreateRefusesJob, TestCreateLocalMultiPlatformImage, TestUpWithAllResources) against a locally built binary. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Detached `up` used to run create() then start() back to back: two separate daemon snapshots, the second blind to what the first just created or recreated. preparePlan already supports planning both phases in one pass (ScopeCreateStart, landed across lot 1) -- this wires it in for the one caller that can use it outright: `up -d`. preparePlan gains an explicit scope parameter (ScopeCreate for create(), unchanged); Up()'s detached branch now calls a new upDetached, which runs preparePlan(..., ScopeCreateStart) + executePlan once instead of create() + start(). --wait stays a post-plan verification: it re-lists containers after the plan runs (the plan may have created or recreated some, so the pre-execution snapshot can't be reused for this) and runs the same waitDependencies check start() already does. Interactive up is untouched: it still runs create() then start() with its printer/monitor listener wired in between, converging onto the plan engine separately (a riskier, later item of the same lot). ScopeCreateStart landed in lot 1 as inert code with no real caller; this is the first one, and it surfaced two latent bugs in the container-recreate path that nothing had exercised end to end before: - planRecreateContainer plans a container's recreation as Create (under a temporary "<shortID>_<name>" to avoid colliding with the old container still holding the final name) -> Stop -> Remove -> Rename(final name). The start phase's event naming and container-ID resolution (groupEventName, resolveContainerID) read the create node's published result by design (plannedReplica keeps every start-phase reference pointed at CreateNodeID, not the rename node), but execRenameContainer never updated that result after renaming -- so a recreated container's Starting/Started events kept reporting the temporary name forever. - execCreateContainer also publishes the new container into containersByService (the live view OpWaitCondition reads for depends_on health waits) under that same temporary name, with nothing refreshing it after rename either -- a dependent waiting on the recreated container's health reported Waiting/Healthy under the temporary name too. Both fixed in execRenameContainer: it now updates the create node's pctx result in place after a successful rename, and (via a new Service field on the rename Operation) patches the matching containersByService entry's name too. Each pinned by a regression test, verified by reverting the fix locally and confirming the test catches it. Verified with the full e2e suite (`go test -tags e2e ./pkg/e2e/...`) against a locally built binary -- including TestRestartWithDependencies, which is what caught both bugs in the first place -- plus the existing unit suite and lint. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot review (High): the plan's own OpWaitCondition nodes (service_healthy / service_completed_successfully dependency waits, executed as part of executePlan) have no timeout of their own -- they block on whatever ctx they're given, unlike the imperative engine's waitDependencies, which already threaded WaitTimeout through every per-dependency wait, not just the final --wait check. upDetached only wrapped ctx with WaitTimeout around that final check, established after executePlan already returned -- so a condition that's never satisfied hung `up -d --wait --wait-timeout N` forever instead of failing after N seconds. Move the deadline to wrap ctx before executePlan runs (gated on Wait && WaitTimeout > 0, matching start()'s own gating), and reuse the same ctx for the final verification instead of opening a second, independent WaitTimeout window -- one overall budget for "becoming healthy" covering both the plan's in-flight waits and the final check. preparePlan is deliberately left outside the deadline: image pulls and the rest of create()'s own work were never bounded by --wait-timeout in the old create()+start() sequence either. Regression-tested at two levels: - TestExecutePlanWaitConditionRespectsContextDeadline drives a real OpWaitCondition node through executePlan/run() under a short deadline and asserts it returns promptly with ctx.Err(), proving the deadline propagates into the plan's blocking nodes. - The existing e2e TestUpWaitTimeout (up --wait --wait-timeout on a service that never turns healthy) is what actually exercises upDetached's own wiring: verified by ablation that reverting this fix makes it hang (test timeout) instead of failing after 3s. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
The previous commit gated the deadline on options.Start.Wait && WaitTimeout > 0, describing it as matching start()'s own gating -- it doesn't. start()'s InDependencyOrder passes options.WaitTimeout into every startService call unconditionally, not only when options.Wait is set, and the CLI never requires --wait alongside --wait-timeout (`up -d --wait-timeout 30` with no --wait is accepted as-is). Under the previous gating, that exact combination would still hang on a stuck OpWaitCondition node -- the same bug the deadline was just added to fix, reintroduced for one flag combination. Drop the options.Start.Wait condition: the deadline now applies whenever WaitTimeout > 0, matching start()'s actual unconditional behavior. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent review: moving the WaitTimeout deadline to wrap ctx before executePlan closed the in-plan hang, but opened a narrow new one -- executePlan can return nil right as the deadline fires, leaving the subsequent getContainers call (for the final --wait verification) to run against an already-expired ctx and propagate a raw context.DeadlineExceeded instead of the existing friendly "application not healthy after %s" message. Same translation the executePlan and waitDependencies error paths already apply. Also, a second docker-agent finding: execRenameContainer's containersByService refresh silently no-ops if the container isn't found at its expected slot (can't happen today -- the DAG guarantees execCreateContainer's append runs first -- but nothing would surface it if a future change broke that). Logs a warning instead of staying silent, so a regression here is visible rather than just quietly reintroducing the stale-name bug this PR fixed. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot review (Medium): wrapping the whole run() ctx with WaitTimeout also capped Create-phase and Start-phase work that was never subject to --wait-timeout in the old create()+start() sequence -- a slow image pull or container create/recreate could now fail as "application not healthy" with no health wait involved. planExecutor gains a waitDeadline field, consulted only in executeNode's OpWaitCondition case to derive a deadline-bound context for that one call; every other operation type keeps running on the plan's plain ctx. upDetached now builds its own planExecutor directly (instead of going through executePlan) so it can set waitDeadline before calling run(), and reuses the same deadline for the final post-plan verification instead of wrapping the whole plan's ctx. TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes pins this: a plan pairing a never-satisfied OpWaitCondition with an OpCreateNetwork confirms the network create isn't raced by the same short deadline. Verified by ablation that removing the OpWaitCondition dispatch's deadline derivation makes the test hang instead of failing promptly. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent review: the deadline set before executePlan is
unconditional (whenever WaitTimeout > 0, matching start()'s own
WaitTimeout threading, regardless of Wait), but the "application not
healthy" message is specifically a --wait message. `up -d
--wait-timeout=30` with no --wait, hitting the deadline during an
OpWaitCondition node, showed that message despite the user never
asking for a health check -- just a hang guard.
Gate the message on options.Start.Wait at this one call site. The two
other DeadlineExceeded-translating sites (the post-plan container
listing and the final waitDependencies check) are unaffected: both
already sit after the `if !options.Start.Wait { return nil }` early
return, so they're structurally unreachable without Wait already
being true.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…heck docker-agent review (strong confidence): exec.run only bounds OpWaitCondition nodes with the wait deadline, not create/start/hook ops (see the previous commit) -- a plan whose non-wait work alone takes longer than --wait-timeout can still return nil past the deadline. context.WithDeadline on an already-past deadline silently produces an immediately-canceled context, failing the next daemon call (getContainers) with a bare context.DeadlineExceeded indistinguishable from "health never came up" -- misleading, since health was never actually checked. Extracted the deadline-wrapping into applyRemainingDeadline, which checks the remaining budget first and fails immediately with a message that says what actually happened, instead of letting a confusing daemon-call failure stand in for it. Pinned by three unit tests (zero deadline is a no-op, a future deadline wraps ctx normally, a past deadline fails immediately with the expected message) -- the past-deadline case verified by ablation to reproduce a false "nil error" without the check. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot review: planProviderStart used a provider's own depends_on wait nodes as its startChainEnds whenever it had any, entirely dropping its own OpRunProvider (deployment) node from that set. A downstream consumer of the provider -- ordered after startChainEnds -- would then only wait for the provider's dependency to become healthy, not for the provider plugin/relay deployment itself to finish. The old create-then-start two-phase barrier guaranteed that ordering for free; ScopeCreateStart (inert until this PR actually exercised it end to end) didn't reproduce it. ends now always includes the provider's own node alongside its dependency prerequisites when it has any, instead of either/or. TestPlanStart_ProviderConsumerWaitsForDeployment pins it: a provider with both an upstream dependency and a downstream consumer, asserting the consumer's start node depends on the provider's OpRunProvider node directly -- verified by ablation to fail (showing the exact missing edge) without the fix. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot review ("previously missed", code unchanged since an earlier
round): the prior fix (applyRemainingDeadline) replaced a confusing
raw DeadlineExceeded with a clearer message, but kept the underlying
design flaw it was reacting to -- a single absolute deadline
(time.Now().Add(WaitTimeout)) shared across the whole plan and the
final verification. Slow, unrelated create/start/hook work (never
meant to be bounded by --wait-timeout -- that's the entire point of
exec.waitDeadline existing) could still exhaust that shared budget on
its own, failing an otherwise healthy deployment as "not healthy" with
no health wait ever actually timing out.
start()'s real behavior never had this failure mode: waitDependencies
opens a fresh context.WithTimeout(ctx, timeout) every time it's
called, so each dependency wait gets its own full WaitTimeout window,
independent of how long anything before it took.
planExecutor.waitDeadline (an absolute time.Time) becomes waitTimeout
(a time.Duration): executeNode's OpWaitCondition case now derives a
fresh context.WithTimeout per wait node instead of a shared
context.WithDeadline. upDetached mirrors this for the final
verification -- its own fresh WaitTimeout window, not whatever's left
of one. applyRemainingDeadline's already-expired-deadline problem
disappears by construction: there's no longer a precomputed deadline
to have already expired.
Also fixes two sibling DeadlineExceeded-translation sites (the
post-plan container listing and waitDependencies' own check) that were
missing the WaitTimeout > 0 guard: without a --wait-timeout set, ctx
can still carry an external deadline (an upstream command timeout, a
cancellation) unrelated to --wait-timeout, and naming "application not
healthy after 0s" would blame a duration nobody configured.
TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes and
TestExecutePlanWaitConditionRespectsContextDeadline updated for the
field rename; both still pin their original scenarios under the new
mechanism. Verified against the full e2e suite (TestUpWaitTimeout,
TestUpWait, TestRestartWithDependencies, the provider suite) on a
locally built binary.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent review (moderate confidence): exec.run propagates any context.DeadlineExceeded up, including one originating from ctx itself (an outer caller's own deadline -- a test harness, a wrapper tool, a client-side timeout) rather than from an OpWaitCondition node's own derived timeout. The WaitTimeout > 0 guard alone doesn't distinguish the two: a user who did configure --wait-timeout could still see "application not healthy after Xs" when what actually happened was the caller's own unrelated deadline firing during plan execution. ctx at this point is still the caller-supplied, undecorated context (the per-wait timeout is derived fresh inside executeNode, never reassigned here) -- so ctx.Err() == nil confirms the DeadlineExceeded didn't come from ctx's own deadline, leaving only an OpWaitCondition node's derived timeout as the source. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot review (High) on docker#14290: run() dispatched every node -- Create and Start phase alike -- under one errgroup.WithContext(ctx), so any node failing canceled the shared context for every other goroutine still in flight. A required OpWaitCondition node hitting --wait-timeout is a Start-phase failure that can now happen while unrelated Create-phase work (a sibling service's container or network create) is still legitimately running -- and that work got aborted too, with nothing to do with the dependency that timed out. The old create()-then-start() sequence never had this failure mode: the whole create phase always finished completely before any dependency wait even began. run() now derives two contexts instead of one shared errgroup-context: createCtx at the root, startCtx as its child. A node picks nodeCtx (and the matching cancelPhase) from its own node.Phase and calls cancelPhase() only on its own genuine failure. A Start-phase failure cancels only startCtx, leaving createCtx and everything still running under it untouched. A Create-phase failure still cancels the whole plan, Start phase included, via startCtx's derivation from createCtx -- matching the old sequence, where start() was never even invoked once create() failed. The node-goroutine body moved out of run()'s loop into a runNode method (golangci-lint's cognitive-complexity limit after this change) taking a new runState struct bundling the per-run bookkeeping every node shares. Two smaller, unrelated fixes bundled in: - upDetached (up.go) keeps an origCtx snapshot so its two DeadlineExceeded-to-friendly-message checks can tell its own WaitTimeout window expiring apart from an inherited, unrelated parent deadline propagating through the derived context -- both report DeadlineExceeded on Err(), only the former earned the "application not healthy" framing. - waitDependency (service_containers.go), shared by the old waitDependencies and the new execWaitCondition -- the latter can now produce a real deadline for the first time via exec.waitTimeout -- tolerates a timeout on an optional (BestEffort) dependency the same way it already tolerates every other definitive failure: skipped with a warning, not a hard failure. Two regression tests pin both cancellation directions: TestExecutePlanStartPhaseFailureDoesNotCancelCreatePhase (a Start-phase wait timing out must not cancel a concurrent, unrelated Create-phase node) and TestExecutePlanCreatePhaseFailureCancelsCreatePhase (a Create-phase failure must still fail-fast its Create-phase siblings, same as before this change). Both verified by ablation. Full e2e suite re-run clean against a locally built binary -- same 3 pre-existing, environment-specific failures as the stack's baseline, no new ones. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot review ("previously missed"): the same either/or bug just
fixed for planProviderStart (e206f05) also affects the ordinary
(non-provider) service branch of planServiceStart. A service whose
only container is in an exceptional state (paused, dead) gets a bare
create-phase restart (OpStartContainer, "not running") and takes the
len(replicas) == 0 path in the start-phase visit. When that service
also declares its own depends_on, startChainEnds used only the
dependency's wait node as ends, dropping the service's own restart
node entirely -- a downstream service_started consumer could then
start before the paused/dead service's only container was actually
running again.
ends now always includes the service's own create-phase node
(serviceNodes[service.Name]) alongside its dependency prerequisites
when it has any, instead of either/or -- the exact same fix shape as
planProviderStart.
TestPlanStart_ExceptionalStateConsumerWaitsForRestart pins it,
mirroring TestPlanStart_ProviderConsumerWaitsForDeployment: a
paused service with both an upstream dependency and a downstream
consumer, asserting the consumer's start node depends on the restart
node directly -- verified by ablation to fail, showing the exact
missing edge, without the fix.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Three related findings from Copilot and docker-agent on the phase-scoped cancellation commit, all stemming from the same root cause: switching run() from errgroup.WithContext's own atomic cancel-with-error-record to an explicit, separate cancelPhase() call reopened timing windows that errgroup.WithContext used to close for free. - Copilot (high): cancelPhase() unblocks every sibling in that phase before this goroutine's own error reaches errgroup.Group's internal errOnce -- a sibling woken by the cancellation can return a bare context.Canceled of its own and win that race, hiding the actual daemon error or DeadlineExceeded (and making upDetached's own DeadlineExceeded translation unreliable with it). - docker-agent (medium): the "is this my own genuine failure" nodeCtx.Err() == nil check and the cancellation it gates aren't atomic -- two genuinely concurrent same-phase failures can both pass it before either's cancelPhase() takes effect, both emitting a group Error event for the same resource. - docker-agent (medium): the dependency-wait loop's early return on ctx cancellation skipped close(rs.done[node.ID]), the only exit path that did. Not reachable today (every dependent in the same phase shares that phase's ctx, already canceled by the time it would matter), but latent: nothing in runNode enforces that no future cross-phase dependency edge ever relies on this channel closing. All three close around one new type, phaseCancel, bundling a phase's context/cancel pair with a sync.Once: the first node in a phase to fail records its error and emits the group event inside that Once, cancels the phase's context, and every later caller -- genuine concurrent failure or cancellation casualty alike -- is a silent no-op. run() returns a phase's recorded error over whatever eg.Wait() itself picked, so errgroup's own race can no longer surface a cancellation casualty's error in its place. The done-channel close moved to a single `defer` at the top of runNode, covering every exit path uniformly instead of three separate explicit calls. TestExecutePlanConcurrentPhaseFailureKeepsOriginatingError exercises the first scenario under real concurrency (one immediately-failing node alongside many siblings a dependency-wait unblocks via its cancellation) -- see its own comment for why this specific race can't be forced to fail by ablation in an in-process test (the Go scheduler never actually lets a sibling win it here), so the fix's correctness rests on reading errgroup's own errOnce.Do source directly, confirmed independently by two reviewers, not on this test alone. Verified: go test -race -count=10 ./pkg/compose/... clean; full e2e suite re-run clean against a locally built binary (same 3 pre-existing, environment-specific failures as the stack's baseline). Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…t the last Copilot review, following up on 374079a's own fix: serviceNodes keeps only the LAST node processed during a service's create-phase convergence -- correct for a single replica, but a scaled service can have more than one exceptional-state (paused, dead) replica, each with its own bare-restart OpStartContainer node. Using serviceNodes alone in planServiceStart's len(replicas) == 0 branch meant a downstream service_started consumer only ended up depending on the highest- numbered replica's restart, not every one of them. exceptionalStateRestartNodes collects every OpStartContainer node from containerNodes[service] (which tracks all of them, unlike serviceNodes), in deterministic replica-number order. planServiceStart now merges all of them into startChainEnds alongside the dependency prerequisites. TestPlanStart_ScaledExceptionalStateConsumerWaitsForEveryRestart pins it: two exceptional-state replicas, asserting the consumer depends on both restart nodes -- verified by ablation to depend on only the last one without the fix. Two more small, independent fixes from the same review round: - execWaitCondition's doc comment (executor_ops.go) described the pre-waitTimeout "no deadline, caller wraps ctx" design; updated to describe planExecutor.waitTimeout's actual per-node window. - waitDependency's new optional-dependency-timeout tolerance (the previous commit) gains its own direct regression test (TestWaitDependencyDeadline/expired deadline on an optional dependency is skipped, not an error) -- the existing tests covered a required timeout, user cancellation, and an optional dependency's definitive (non-timeout) failure, but not this specific path. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…t timeout Copilot review: waitDependency's optional-dependency tolerance (the previous commit) treated ANY DeadlineExceeded on ctx as this call's own timeout -- including one inherited from an ancestor context the caller never gave this specific call a deadline of its own for. A plan whose only outstanding work was an optional, never-satisfied wait could see the API caller's own external deadline (a wrapper tool, a test harness) expire, get silently converted to a Skipped event, and report success despite the caller's context having actually timed out. waitDependency gains a hasOwnDeadline bool: true only when the specific call is responsible for ctx's deadline (waitDependencies passes timeout > 0; execWaitCondition passes exec.waitTimeout > 0, exactly the condition under which executor.go's dispatch wrapped ctx with that same field). Only a DeadlineExceeded under hasOwnDeadline is tolerated for an optional dependency; one without it still propagates, regardless of Required. TestWaitDependencyDeadline gains "an inherited parent deadline is not swallowed for an optional dependency" (timeout=0, an already-expired parent ctx) alongside the existing "own timeout is skipped" case -- verified by ablation that dropping hasOwnDeadline from the condition makes the new test fail exactly as described. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
run() used to dispatch every node concurrently, with Create-phase and Start-phase cancellation scoped separately via phaseCancel so a Start-phase failure couldn't cancel unrelated in-flight Create work. That stopped short of what the interactive up convergence (lot 2's next step) actually needs: its attach/printer session takes exclusive hold of the terminal for the Create phase's own progress display, and hands it over to continuous container log streaming only once the Create phase is entirely done project-wide -- a Start-phase node's log output interleaving with Create's progress bars has nowhere consistent to go. Phase-scoped cancellation alone still let an unrelated Start-phase node begin (and start logging) while a sibling elsewhere in the plan was still being created, or had just failed. run() now splits plan.Nodes into createNodes/startNodes upfront and runs them as two fully sequential runPhase() calls: Start-phase nodes are never even dispatched until every Create-phase node has succeeded, and a Create-phase failure returns immediately without calling runPhase on startNodes at all. This matches the old create()-then- start() sequence's own fail-fast behavior, which the single-plan migration had implicitly relaxed. Added a fast-fail validation: a Create-phase node depending on a Start-phase node would otherwise deadlock forever (the Start phase isn't dispatched until the Create phase blocked on it returns, so nothing would ever close that dependency's done channel). Nothing in the reconciler should produce that edge, but failing fast with a clear error beats a silent hang if it ever does. TestExecutePlanCreatePhaseIsABarrierBeforeStartPhase and TestExecutePlanCreatePhaseFailureNeverDispatchesStartPhase cover both directions of the barrier; both verified by ablation (reverting to a single concurrent runPhase call reliably fails each, 5/5 runs). Verified: go build, go test ./pkg/compose/... (incl. -race), and golangci-lint all clean; full e2e suite re-run clean against a locally built binary (same 3 pre-existing, environment-specific failures as the stack's baseline: TestWatch, TestIPC, TestComposeModel). Local Copilot CLI review found no correctness/deadlock issues in the barrier logic itself; its two actionable findings (missing phase-ordering guard, missing never-dispatched regression test) are both addressed here. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…deadline Copilot review finding on docker#14290: waitDependency decided whether a DeadlineExceeded on an optional dependency wait was safe to tolerate using a bare boolean -- whether the caller had configured a timeout for this wait at all (exec.waitTimeout > 0, or legacy waitDependencies' timeout > 0). That can't distinguish THIS call's own timeout actually firing from an earlier deadline inherited from the caller's own ctx also firing: context.WithTimeout(parent, waitTimeout) still makes both parent and child report DeadlineExceeded once the parent's earlier deadline elapses, even though this wait's own timer never got there. An unsatisfied optional wait could therefore swallow a real external deadline (the process's own context, a test harness timeout) and report success instead of propagating it. Replaced the bool with an origCtx snapshot -- ctx as it stood right before this call's own conditional context.WithTimeout wrap -- threaded from executor.go's OpWaitCondition dispatch through execWaitCondition down to waitDependency, and from waitDependencies' own wrap in service_containers.go. waitDependency now checks origCtx.Err() == nil at the moment ctx.Done() fires: only when the pre-wrap context hasn't failed on its own is this wait's own timeout the one responsible. TestExecutePlanOptionalWaitDoesNotSwallowInheritedDeadline pins the negative case (inherited deadline must still propagate even though exec.waitTimeout is set and the dependency is optional) and TestExecutePlanOptionalWaitTimeoutIsToleratedWithoutInheritedDeadline pins the positive one (this wait's own timeout, no inherited deadline, still tolerated as before) -- both verified by ablation. Verified: go build, go test ./pkg/compose/... (full suite, not just the new tests) and golangci-lint clean. Local Copilot CLI review confirmed the origCtx threading is correct and race-free across all three call sites; its one actionable finding (missing positive-path coverage for the plan-executor route specifically) is addressed by the second new test above. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…-wait Copilot review finding (low) on docker#14290: with --wait-timeout supplied but not --wait, a dependency timeout in upDetached returned the raw "context deadline exceeded" instead of the actionable "timeout waiting for dependencies" the legacy start()+waitDependencies path already reports for the exact same failure. The --wait-specific "application not healthy after %s" translation only ever covered the Wait=true case, leaving the Wait=false, WaitTimeout>0 case (a legal CLI combination -- --wait-timeout alone is a hang guard) with no translation at all. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot flagged a behavior-parity regression on docker#14290's Create->Start barrier: a service's OpWaitCondition node only depended on its own dependency's chain end, so its exec.waitTimeout clock could start ticking earlier than the old imperative engine's single waitDependencies(X) call. That call only begins once EVERY direct dependency of X has itself finished starting (confirmed by reading InDependencyOrder's graphTraversal.run: a service's visitorFn only runs once none of its dependency children are still ServiceStopped). A slow sibling with no wait node of its own (service_started is a plain edge, nothing to poll) could silently eat into a health check's effective grace period compared to the old engine. startPhaseDependencies now computes allTargets, the union of every declared dependency's own chain-end for the requesting service, and passes that into waitConditionNode instead of just the one dependency being checked. Because wait nodes are shared/deduplicated across consuming services by (dep, condition) alone, this widening reopened a second, more serious finding from the same review pass: two consumers of the same shared node can have different sibling sets, and merging a sibling that already (transitively) depends on the shared node -- an entirely ordinary diamond dependency, or a service whose own chain end collapses onto the wait node itself when it has nothing left to start -- creates a dependency cycle. The executor has no cycle detection and no timeout on the dependency-wait phase itself (exec.waitTimeout only applies once a node starts executing), so this was a guaranteed permanent hang, not a correctness nicety. Fixed with wouldCreateCycle: a wait node's merge now skips any candidate that already (transitively) depends on it. This is provably safe, not a workaround -- if a candidate already depends on the wait node, the wait node is necessarily already done by the time the candidate's own chain completes, so there is nothing to gain from also adding the reverse edge; the candidate's own direct dependency on the shared node (or on an intermediate sibling) already encodes everything the consumer's depends_on requires. Five new regression tests in reconcile_start_test.go, all verified by ablation (reverting the relevant guard reliably reproduces the original bug, 3-5/5 runs depending on the test): - TestPlanStart_WaitNodeGatedOnAllSiblingDependencies: the original parity fix (db:service_healthy + migrate:service_started siblings). - TestPlanStart_DiamondDependencyNeverCreatesWaitCycle and TestPlanStart_ExceptionalChainCollapseNeverCreatesWaitCycle: the two concrete cycle shapes found in review. - TestPlanStart_TransitiveChainNeverCreatesWaitCycle: exercises wouldCreateCycle's BFS two hops deep, not just a direct edge. - assertNoCycle, a plan-wide helper used by the cycle tests above. Verified: go build, go test ./pkg/compose/..., and golangci-lint all clean; full e2e suite re-run clean against a locally built binary (same 3 pre-existing, environment-specific failures as the stack's baseline: TestWatch, TestIPC, TestComposeModel). Two rounds of local Copilot CLI review: the first caught the cycle risk in the initial allTargets widening, the second confirmed wouldCreateCycle's BFS is correct and complete, the "skipping is safe" reasoning holds in every case, and no other call site in reconcile.go has the same unguarded sharing risk. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Reverts 365f002, a behavior-parity fix for a Copilot finding that the plan-based start engine's per-(dependency,condition) wait node could start its exec.waitTimeout clock earlier than the old imperative engine's single waitDependencies(X) call -- which only begins once EVERY direct dependency of X has itself finished starting, so a slow sibling with no wait node of its own could silently extend another dependency's effective grace period. On reflection that old-engine coupling is itself the bug: a service's declared --wait-timeout for a given dependency should bound how long THAT dependency takes to satisfy its condition, period -- not be inflated by how long an unrelated sibling dependency happens to take to start. Letting db's health-check timeout silently grow because migrate's own dependency chain is slow means --wait-timeout no longer means what it says. Restoring the narrower, per-dependency gating is a deliberate, accepted behavior change from the old engine, not a parity regression to chase. This also removes the dependency-cycle risk the sibling-widening introduced (an ordinary diamond dependency, or a service whose own chain end collapses onto a shared wait node, could merge a node back into its own transitive dependents -- see 365f002's description) and the wouldCreateCycle machinery and five regression tests that existed solely to guard against it, now dead weight with no reachable scenario to protect against. Verified: go build, go test ./pkg/compose/..., and golangci-lint all clean. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
The final readiness check opened its --wait-timeout window before re-listing the project's containers, so a slow daemon listing could exhaust the budget and report "application not healthy" before waitDependencies had polled once. List first on the caller's context, then derive the timed one, as the imperative path did. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…m DeadlineExceeded up translated any DeadlineExceeded surfacing from the plan into "timeout waiting for dependencies" or "application not healthy" whenever --wait-timeout was set, even one coming from an engine call, a hook or the secret injection, none of which is a dependency-readiness failure. A wait node whose own --wait-timeout window expired now returns errWaitTimeout (still satisfying DeadlineExceeded), and up keys its message on that. A deadline inherited from the caller is never marked. The test that checks --wait-timeout does not leak into non-wait nodes now inspects the context those nodes receive, instead of a mock that would have succeeded either way. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…th --wait too Copilot review finding on docker#14296: a wait node's own timeout is a depends_on condition that did not become satisfied during the plan, not the final --wait readiness check, which runs after the plan and has its own "application not healthy" translation. The imperative path reported "timeout waiting for dependencies" for the former regardless of --wait, but translateWaitTimeout rewrote it as "application not healthy" when --wait was set. It now always reports the dependency timeout. Also mark the test nodes that stand for the Start phase as PhaseStart (the recreate-then-start plan and the wait whose timeout survives a skipped dependent), so they exercise the Create->Start barrier the way the reconciler's plans do instead of running everything in one phase. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot review finding on docker#14296: planServiceStart returned before any dependency planning for every service whose scale is zero. The imperative startService only treats deploy.replicas: 0 as an unconditional no-op: a service with scale: 0 still has its depends_on conditions evaluated first, so an unhealthy or missing required dependency fails `up`/`start`, and whatever is ordered after the service stays ordered after those prerequisites. It is simply never an error for it to have no container to start, and under scope Start the replicas it already owns are started (CreateStart removes them all in the create phase). Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot review finding on docker#14300: if a sibling Create-phase failure ever stopped canceling the other node, VolumeCreate's mock blocks on its ctx while exec.run waits for it, so the test's own five-second guard could never fire and the regression would hang until the suite-wide timeout. Give run a deadline of its own and assert the cancellation is the sibling's (context.Canceled), not that fallback. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Same create() + start() two-snapshot pattern upDetached already replaced for `up -d`, now shared via a new createAndStart helper for the other two callers that had it: - Scale: exact equivalent, same project object was already passed to both calls. - watch's rebuild(): the create() call used the full project (scoped only via CreateOptions.Services/Recreate policy), but start() used a project narrowed to services + their dependents (WithSelectedServices). The narrowed project is now computed once and used for both phases instead. This does shrink the create phase's own scope a little, deliberately: WithSelectedServices only trims the project's Services map (unrelated services move to DisabledServices, not vanish -- orphan detection still recognizes their containers via DisabledServiceNames(), so nothing gets misflagged) -- Networks, Volumes, Configs, Secrets are untouched, so external-resource resolution is unaffected. What does narrow: config-drift checking (RecreateDependencies policy) and the image pull/validate pre-pass no longer consider services outside this rebuild's blast radius. For a file-watch-triggered rebuild of specific services this is arguably more correct than today (a watch rebuild of one service shouldn't be re-validating or drift-checking the rest of the project on every keystroke) -- not a literal byte-for-byte port, a deliberate, narrower scope. The old code also returned a create() failure immediately but logged and swallowed a start() failure; a single combined call can't keep that distinction. Traced as inert either way: the sole caller, rebuildScheduler.run(), discards rebuild()'s return value unconditionally -- only the logged message differs, not control flow. AttachTo: services, also dropped from the old StartOptions, was already dead in this call (only read by start() when Project is nil, which it never was here). Verified with the full e2e suite (TestScale*, TestWatch* in particular) against a locally built binary, plus the existing unit suite and lint. No new failures; the only 3 remaining are the same pre-existing, environment-specific ones from the previous PR in this stack (macOS Docker Desktop restricted-host-mount errors, a model-pull/registry issue). Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
`compose start` (and the two callers sharing its unexported start(): `compose run`'s dependency startup, and interactive up's own start phase) switches from the imperative InDependencyOrder/startService/ startServiceContainer traversal to the plan-based reconciler/executor, via the ScopeStart scope Lot 1 already built and golden-tested but left with no real consumer (docker#14081, lot 2). ScopeStart's planStartPhase works straight off the observed containers -- it never converges anything, matching start()'s own historical contract of starting what exists, never creating (see TestPlanStart_StartOnlyScope). start() now mirrors upDetached's own established shape: collectObservedState -> reconcile(ScopeStart) -> emitRunningEvents -> newPlanExecutor.run, then the same post-plan --wait verification pattern (origCtx snapshot distinguishing the wait's own fresh deadline from an inherited one, exec.waitTimeout threaded unconditionally to match the old engine's per-service-wait-regardless-of---wait semantics). The label-reconstructed project path (no compose file, projectFromName) is unchanged -- ScopeStart only reads project.Services/DependsOn, which that path already populates the same way. Interactive up's own start phase (up.go, listener non-nil for pre_start/ post_start hook log streaming) now runs on the plan engine too, as an unavoidable consequence of start() being shared code -- epic docker#14081 originally scoped that switchover to a later, separate item (interactive up), but there is no way to migrate `compose start` without also affecting every other caller of the same function. This does NOT fix interactive up's own two-snapshot problem (create() and start() still each collect their own observed state) -- that remains a distinct, not-yet-addressed follow-up; this PR only changes which engine runs the second pass, not when it runs. startService/startServiceContainer and the InDependencyOrder-based start traversal are deleted: this migration makes them unreachable from every production call site (confirmed via grep and golangci-lint's unparam/ unused catching the now-always-nil/zero listener and timeout parameters), and the epic's own Lot 3 ("remove the imperative start path") rationale for keeping that deletion separate -- making an interactive-up revert trivial -- no longer applies: interactive up never used this code to begin with once start() itself switched engines, so there is nothing left for Lot 3 to revert around. Their dedicated characterization tests (TestStartService_*, TestStartServiceContainer_*, the relay-hook-skip test in service_containers_test.go) are removed with them; a coverage spot-check (confirmed by local Copilot CLI review) found every behavior they locked already has an equivalent at the executor/plan level (executor_test.go's maxConcurrency tests, executor_start_test.go's event and hook-ordering tests, reconcile_start_test.go's golden plan tests) -- nothing is left unverified. The now-fully-unused isNotRunning helper is removed too. Verified: go build, go test ./pkg/compose/..., and golangci-lint all clean; full e2e suite re-run clean against a locally built binary (same 3 pre-existing, environment-specific failures as the stack's baseline: TestWatch, TestIPC, TestComposeModel). Local Copilot CLI review found no correctness bugs and confirmed the dead-code deletion is complete and safe. A genuine regression this migration exposed in provider-backed services' start-phase planning (planProviderStart never had a path to restart an existing, stopped relay container under pure ScopeStart) is fixed in the next commit, not folded in here, since it is a bug in already-merged Lot 1 vocabulary that this PR's e2e run happened to be the first to actually exercise -- not something introduced by this diff. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Found by the e2e suite (TestProviderPublishEndpoint) the first time ScopeStart became a real, exercised path (previous commit, `compose start`): planProviderStart only ever ordered a provider's consumers after serviceNodes[service.Name], the create-phase OpRunProvider node. That node is never set under pure ScopeStart -- the create phase that emits it doesn't run at all -- so a provider's own relay container, if observed stopped, had no start node planned for it anywhere. `compose start db` on a provider-backed service returned success without actually starting its relay. planProviderRelayRestart emits a bare OpStartContainer for the relay when it is observed but not running, deliberately mirroring the old imperative engine's own behavior for a provider's relay (the now- deleted startService special-cased providers only for the zero- container case, otherwise treating an existing relay container like any other container -- a bare ContainerStart, never a redeploy through ensureServiceRelay/OpRunProvider). A local Copilot CLI review of the first version of this fix caught a real bug before it shipped: "serviceNodes absent" is not a reliable proxy for "the create phase never ran" on its own -- under ScopeCreateStart with SkipProviders (watch's rebuild), reconcileService deliberately skips the provider and leaves serviceNodes unset too, an explicit request to leave the relay alone, not an invitation to restart it regardless. planProviderRelayRestart is now also gated on r.options.SkipProviders. Three golden-plan regression tests, all verified by ablation (reverting the relevant guard reproduces the bug deterministically, 3/3 runs): TestPlanStart_ProviderRelayRestart, TestPlanStart_ProviderRelayAlreadyRunningPlansNothing, and TestPlanStart_ProviderRelayRestartRespectsSkipProviders (the Copilot-caught case). Verified: go build, go test ./pkg/compose/..., and golangci-lint all clean; full e2e suite re-run clean against a locally built binary (same 3 pre-existing, environment-specific failures as the stack's baseline: TestWatch, TestIPC, TestComposeModel) -- TestProviderPublishEndpoint confirmed passing. Two rounds of local Copilot CLI review: the first caught the SkipProviders gap, the second confirmed the ordering and event-grouping of the new node are correct and no other call site shares the same risk. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copilot review finding on docker#14296: planProviderRelayRestart's node was added as a sibling of the provider's own startPhaseDependencies, not their dependent. Under pure ScopeStart there is no Create->Start barrier separating a provider from its siblings the way ScopeCreateStart's OpRunProvider gets one for free (always a create-phase node, run to completion before any start-phase node begins) -- so a stopped relay with a required depends_on condition could start immediately, concurrently with (or before) that condition's own wait resolving. planProviderRelayRestart now takes the provider's own depNodes (startPhaseDependencies' result, already computed by the caller) and passes them through as the restart node's own DependsOn, exactly like an ordinary service's bare-restart-under-ScopeStart already does via its replica chain's `prev := depNodes`. New regression test TestPlanStart_ProviderRelayRestartWaitsOnDependencies, verified by ablation (dropping the deps argument reproduces the missing edge deterministically, 3/3 runs). Verified: go build, go test ./pkg/compose/..., and golangci-lint all clean; full e2e suite re-run clean against a locally built binary (same 3 pre-existing, environment-specific failures as the stack's baseline: TestWatch, TestIPC, TestComposeModel). Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
A Copilot review finding on planProviderRelayRestart: its chain-building loop re-checked "is this replica a relay?" on every iteration to decide whether to run pre_start, so when the lowest-numbered stale container was a relay (correctly skipped) but a later one was an ordinary stale replica, pre_start still ran against that later one. The old imperative engine's own gate -- lowestNumberedContainer(toStart), checked once -- never falls through like that: if its single candidate is a relay, pre_start is skipped entirely for the whole chain, with no fallback to a later non-relay container. Eligibility is now decided once, from the first non-running container only, exactly mirroring planServiceStart's own !isReplicaRelay(replicas[0]) gate (already reviewed and confirmed as faithful parity). The per-replica relay check inside the loop is unchanged and still correct for its own, separate purpose: deciding whether to skip injection/post_start for that specific replica. TestPlanStart_ProviderPathPreStartGateDoesNotRetryPastARelay reproduces the exact shape the finding describes (relay at the lowest number, ordinary stale replica after it) and is verified by ablation (reverting the fix reproduces the bug deterministically, 3/3 runs). Verified: go build, go test ./pkg/compose/..., and golangci-lint all clean; the full provider e2e subset re-run clean against a locally built binary. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
The final readiness check opened its --wait-timeout window before re-listing the project's containers, so a slow daemon listing could exhaust the budget and report "application not healthy" before waitDependencies had polled once. List first on the caller's context, then derive the timed one, as the imperative start path did. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
start() translated any DeadlineExceeded from the plan into a dependency-readiness message when --wait-timeout was set, including one from an engine call or a hook. Use the marker a wait node's own timeout now carries, as upDetached does. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…self run() walked both phases in one call, keeping the state they share local and unreachable. Split it into begin, which prepares the run without dispatching anything, and a planRun whose runCreate and runStart each dispatch their own phase, so a caller can act between the two -- the interactive up will set up its attach/printer/monitor session there. run() is now begin followed by both phases, so every existing caller behaves as before. The start phase takes its hook-log listener when it is entered, since an attached caller only has one once its session exists, and refuses to run before a successful create phase or twice, keeping the create-phase barrier intact for callers that drive the phases themselves. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Most e2e coverage of the start phase runs detached; the foreground `up`, whose create and start phases are driven by separate code paths, had none for hooks, health-gated dependencies, a container that exits as soon as it starts, restarts of exited containers, recreation, dependency timeouts or dry-run. Add scenarios that fix these observables before the machinery behind it changes. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…reams The hook scenario matched "pre-start-hook-ran" in the output, but the service's own line "app-read:pre-start-hook-ran" contains it, so the assertion passed without any pre_start output reaching the session. A foreground up streams post_start's output but not pre_start's today (checked on main too). Lock what actually holds: pre_start's effect, read by the service through a value distinct from anything it logs. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Wait for the long-running service's final state after an abort instead of reading it the instant the command returns, document why the dependent is expected in the created state on a dependency timeout, and narrow the dry-run "no start" check to the service's own lines. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
7 of 17 tasks
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new monitor timestamp parser passes invalid arguments to assert.Check.
1 open finding
What changed in this PR
Moves foreground compose up onto the shared Create+Start plan while preserving attachment and monitoring behavior.
Changes:
- Splits plan execution into independently driven Create and Start phases.
- Migrates start, scale, and watch rebuild paths to the plan engine.
- Adds monitor replay handling and extensive unit/E2E coverage.
| File | Description |
|---|---|
pkg/e2e/up_interactive_test.go |
Adds foreground-up scenarios. |
pkg/e2e/testdata/TestUpInteractiveWaitsForHealthy/compose.yaml |
Health dependency fixture. |
pkg/e2e/testdata/TestUpInteractiveRestartsExitedContainer/compose.yaml |
Exited-container fixture. |
pkg/e2e/testdata/TestUpInteractiveRecreateStreamsNewContainer/compose.yaml |
Recreation fixture. |
pkg/e2e/testdata/TestUpInteractiveInjectsAPISocketConfig/compose.yaml |
API-socket fixture. |
pkg/e2e/testdata/TestUpInteractiveImmediateExit/compose.yaml |
Immediate-exit fixture. |
pkg/e2e/testdata/TestUpInteractiveHookLogs/compose.yaml |
Lifecycle-hook fixture. |
pkg/e2e/testdata/TestUpInteractiveDependencyTimeout/compose.yaml |
Dependency-timeout fixture. |
pkg/e2e/testdata/TestUpDryRunCreatesNothing/compose.yaml |
Dry-run fixture. |
pkg/compose/watch.go |
Uses one plan for rebuilds. |
pkg/compose/up.go |
Drives interactive plan phases around session setup. |
pkg/compose/up_phases_test.go |
Tests interactive phase ordering and failures. |
pkg/compose/start.go |
Migrates start to Start-only plans. |
pkg/compose/start_test.go |
Removes superseded imperative tests. |
pkg/compose/service_containers.go |
Refines dependency timeout handling. |
pkg/compose/service_containers_test.go |
Tests optional dependency deadlines. |
pkg/compose/scale.go |
Uses combined Create+Start planning. |
pkg/compose/restart_test.go |
Updates shared test helpers. |
pkg/compose/reconcile.go |
Expands start planning and provider handling. |
pkg/compose/reconcile_start_test.go |
Covers start-plan edge cases. |
pkg/compose/observed_state.go |
Makes observation phase-aware. |
pkg/compose/observed_state_test.go |
Tests Start-only observation behavior. |
pkg/compose/monitor.go |
Replays events across monitor startup. |
pkg/compose/monitor_test.go |
Tests replay and subscription timing. |
pkg/compose/executor.go |
Adds phased plan execution and wait bounds. |
pkg/compose/executor_start_test.go |
Tests phased execution and timeouts. |
pkg/compose/executor_ops.go |
Updates wait and rename operations. |
pkg/compose/down_test.go |
Updates shared container fixture usage. |
pkg/compose/create.go |
Extracts plan preparation and combined execution. |
pkg/compose/containers.go |
Removes obsolete filtering helper. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Comment on lines
+557
to
+562
| assert.Check(t, err, "since %q", since) | ||
| var n int64 | ||
| if nsec != "" { | ||
| n, err = strconv.ParseInt(nsec, 10, 64) | ||
| assert.Check(t, err, "since %q", since) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Même constat que sur #14304 (le commit monitor est partagé), et je ne le reproduis pas : assert.Check (gotest.tools v3.5.2, internal/assert/assert.go) accepte nil (succès) et error (échec loggé, non fatal) comme comparison. Un err nil est donc une assertion valide, et un err non nil produit un échec lisible. Check est volontaire (appel depuis la goroutine du monitor). Code inchangé ; thread laissé ouvert pour le mainteneur.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
docker-agent
reviewed
Oct 8, 2026
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Lower-confidence findings (not posted inline)
-
[low] pkg/compose/executor.go:474 — execBoundedWaitCondition compound error may leak "context deadline exceeded" text if translateWaitTimeout is not called (confidence: 🟠 weak 52/100)
fmt.Errorf("%w: %w", errWaitTimeout, err)produces a compound error that reads "timeout waiting for dependencies: context deadline exceeded". All current callers ofrun.runStart,upDetached, andstart()correctly calltranslateWaitTimeout, which strips the chain. The residual risk is a future or library caller that calls into the plan executor without wrapping the result — they would see the raw compound message. Not an active bug, but a latent fragility.
ndeloof
force-pushed
the
up-interactive-plan
branch
from
October 8, 2026 11:53
10cac17 to
6844922
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Start can outrun monitor initialization, allowing an immediate container exit to be missed and foreground up to hang.
2 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
docker-agent
reviewed
Oct 8, 2026
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
| // inherited deadline is a different, real failure the caller | ||
| // must still see. | ||
| if errors.Is(ctx.Err(), context.DeadlineExceeded) { | ||
| if !config.Required && origCtx.Err() == nil { |
Contributor
There was a problem hiding this comment.
[low] Concurrent origCtx cancellation while the wait's own deadline fires can mask the cancellation as a timeout error
In waitDependency, the origCtx.Err() == nil guard (line 231) is used to distinguish "this call's own WithTimeout deadline fired" from "a deadline inherited from an ancestor context fired." The code comment covers the inherited-deadline case well.
There is a narrow but real race for optional dependencies: if origCtx is concurrently cancelled (i.e., origCtx.Err() becomes context.Canceled) at the same moment that ctx's own deadline fires, ctx.Done() closes first, ctx.Err() is context.DeadlineExceeded, and origCtx.Err() is now non-nil (context.Canceled). The guard fails, the function returns ctx.Err() (DeadlineExceeded), and the caller in waitDependencies translates it to:
if errors.Is(err, context.DeadlineExceeded) {
return errors.New("timeout waiting for dependencies")
}The user sees "timeout waiting for dependencies" instead of a clean Ctrl-C exit.
The fix is to also allow origCtx having been cancelled (not deadline-expired) to keep the optional-dep skip, since a plain cancellation of the parent should be treated like the optional dependency's own timeout for an optional service:
Suggested change
| if !config.Required && origCtx.Err() == nil { | |
| if errors.Is(ctx.Err(), context.DeadlineExceeded) { | |
| if !config.Required && (origCtx.Err() == nil || errors.Is(origCtx.Err(), context.Canceled)) { |
This preserves the existing behaviour for all required dependencies and for the inherited-deadline case (where origCtx.Err() is context.DeadlineExceeded), while correctly treating a concurrent Ctrl-C cancellation of origCtx as a benign exit for optional dependencies.
| Confidence | Score |
|---|---|
| 🟡 moderate | 62/100 |
monitor.Start lists the project's containers, then opens the event stream. Both are round trips: a container that starts and terminates in between has its die emitted to nobody, the monitor tracks it forever, and --abort-on-container-exit / --exit-code-from never fire. The window is narrow, but a plan running Create and Start back to back makes the first start follow the monitor's attachment closely. Ask the engine for events since a point taken before the listing, and take that point from the daemon (its system time, one extra round trip), not from the client: events are stamped by the daemon, so a client clock that differs from it, as against a remote daemon, would either miss part of the window or replay events from before the monitor was started. There is no falling back on the client's clock: a daemon whose time can't be read or parsed makes Start fail with an explicit error, and so the command. A caller that wants to know before it starts any container can read the time ahead of Start (readDaemonTime), which then doesn't ask again. Replayed events already reflected in the listing are harmless: the listing notifies nothing, so each is reported once, and the tracked set only gets an ID added or removed again. Unit tests drive the monitor against a fake event log that, like the engine, only delivers unsubscribed events to a subscription asking for them with since, and stamps events with a clock of its own. They pin that replay for a die, a start, a destroy and a restart policy, the daemon's time being used whatever its offset from the client's, the failure when it can't be read, and that `logs --follow` returns that failure. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Interactive up used to create() the project, then start() it: two daemon snapshots, the second built from the project as the caller gave it rather than the canonical one the Create phase ran on. It now prepares one Create+Start plan, like detached up. The Create phase runs under the "up" progress scope, then the attach/printer/ monitor session is set up exactly as before, and the Start phase is dispatched last, outside that scope, on a context that survives Ctrl+C, with the printer as the hook-log listener. A failed Create phase reaches neither the session nor the Start phase, and a dry run only ever runs the Create phase. The monitor's starting point is the daemon's time, which is read right before the Start phase: a daemon whose time can't be read fails the up with an explicit error, with no container started. The session, the watcher and the teardown now use the plan's own project, which carries what preparation resolved. That makes use_api_socket work in a foreground up: its config file lives only in that project. A scenario locks it. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
up-interactive-plan
branch
from
October 8, 2026 12:14
6844922 to
66baf1b
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The broad lifecycle, concurrency, event-replay, and dependency-ordering changes warrant final human validation.
1 open finding
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
docker-agent
reviewed
Oct 8, 2026
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #14302, #14300 and #14304 (not yet merged): only the last commit belongs to this PR.
What this PR does, in one sentence
An interactive
compose upnow runs on the same single Create+Start plan asup -d, executing its Create phase, setting up the log session, then executing its Start phase, instead of chainingcreate()andstart()over two daemon snapshots.Context
Foreground
upis the last consumer of the old create-then-start sequence (#14081): it creates the project from one snapshot, sets up attach, the log printer and the monitor, then callsstart(), which re-observes the daemon and rebuilds its project from what the caller passed rather than from what the Create phase actually ran on. Everything else (up -d,scale, watch rebuilds,compose start) already goes through the plan engine.What the PR brings
The plan is prepared once, with both phases. The Create phase runs under the
upprogress scope, as before. The session is then set up unchanged (signals, keyboard menu, watcher, monitor, attach), and the Start phase is dispatched last, outside the progress scope, on a context that survives Ctrl+C, with the printer as the hook-log listener. The guardrails of the old sequence are kept: the Start phase stays out of the progress scope (its refresh would repaint over the logs),signal.Notifystill comes after the Create phase, a failed Create phase reaches neither the session nor the Start phase, a dry run only ever executes the Create phase (message unchanged), andcompose start/compose runare untouched.The monitor replays events from the daemon's time, which an interactive
upreads right before the Start phase: a daemon whose time can't be read fails the command with an explicit error, with no container started.One deliberate behavior change: the session, the watcher and the teardown now use the plan's canonical project instead of the caller's. That project is the one carrying what preparation resolved, in particular the
#apisocketconfig ofuse_api_socket. The effect is a fix: a foregroundupnow gives ause_api_socketservice its/run/secrets/docker/config.json, asup -ddoes since the detached switchover. The defect itself touchesmainin both modes (the file is absent inupforeground and inup -dthere); this PR fixes the foreground half, and a scenario locks it (the file is injected whether or not credentials are configured).Known consequences, accepted: the snapshot is not refreshed between the Create and Start phases; and a library caller combining
AttachwithWaitno longer gets the post-start--waitverification (the CLI already makes--waitimply detached mode). This gap will be closed by validating such combinations at the API level, in a separate PR.The seven characterization scenarios of #14300 pass unchanged against the new path, and unit tests pin the phase order against a mocked engine: create under the progress scope, then attach, then the first start; a container exiting before the event subscription is still seen, the replay starting at the daemon's time (set here an hour off the client's); a daemon whose time can't be read fails the up before any container starts; a failed create stops there; a dry run only creates.
Why this is the right next brick
It is the switchover the whole series builds toward, kept to one commit so it reverts trivially: the characterization scenarios (#14300), the phase handle (#14302) and the monitor replay (#14304) are all in place, and the old
startServicepath can be demolished next.Part of #14081.
🤖 Generated with Claude Code