You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Stacked on #14289 (not yet merged) — only the last commit, "feat: detached up runs on a single plan", belongs to this PR. The diff will shrink to that one commit once #14289 merges and this branch rebases onto main. Draft until then.
What this PR does, in one sentence
docker compose up -d now runs its Create and Start phases as a single plan over a single daemon snapshot, instead of the old create() + start() sequence — two separate snapshots, the second blind to what the first had just done.
Context
Detached up has always run two unrelated things back to back: create() converges the project (networks, volumes, containers) and plans a Plan, then start() re-lists containers from the daemon from scratch and runs the older imperative engine to start them. That second listing can't see anything create()'s plan changed in the same call — it only sees whatever the daemon looked like before this invocation of up began, modulo its own separate converge from the first listing's point of view.
Lot 1 of epic #14081 already taught the reconciler and executor to plan and execute a Start phase in the same Plan as the Create phase (ReconcileOptions.Scope: ScopeCreateStart), precisely so a caller like detached up could stop needing a second pass. Until this PR, nothing actually requested that scope — it shipped as inert code with no consumer.
What the PR brings
preparePlan takes an explicit scope now (create() passes ScopeCreate, byte-identical to its previous implicit behavior). A new upDetached runs preparePlan(..., ScopeCreateStart) + executePlan once for up -d; --wait stays a final, post-plan verification (re-listing containers once execution is done, since the plan may have just created or recreated some) on the same waitDependencies primitive start() already uses. Interactive up is untouched — its own create/start phase boundary is a separate, riskier item of the same lot.
Making ScopeCreateStart a real, exercised path for the first time surfaced two latent bugs in the recreate machinery that nothing had ever driven end to end: a recreated container's Start-phase progress events (and a dependent's health-wait events) kept reporting the container's internal temporary rename-dance name instead of its final one. Both are fixed here, each pinned by a regression test — see the commit message for the detail; caught and confirmed via the full e2e suite (TestRestartWithDependencies) against a locally built binary, not just unit tests.
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>
The reason will be displayed to describe this comment to others. Learn more.
Assessment: 🟢 APPROVE
The PR is a clean, well-scoped refactor: upDetached now runs a single preparePlan(…, ScopeCreateStart) + executePlan pass instead of the old two-snapshot create()/start() sequence. The two latent rename-dance bugs (progress events reporting the temporary name) are properly fixed with regression tests in executor_start_test.go. The executor's thread-safety (containersMu guards all containersByService accesses; reconciliationContext.mu guards pctx) is maintained correctly. The interactive-up path is untouched and behaviourally equivalent to the old code.
Lower-confidence findings (not posted inline)
[low] pkg/compose/executor_ops.go:407 — silent no-op when renamed container is absent from containersByService slice (unverified low; informational only)
[low] pkg/compose/up.go:117 — DeadlineExceeded error message may cite wrong timeout when WaitTimeout==0 and parent ctx carries a deadline (unverified low; informational only)
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>
The reason will be displayed to describe this comment to others. Learn more.
Assessment: 🟡 NEEDS ATTENTION
This review covers only the commits pushed since d39bcc914df9 — the two fixes in commit 0a007fc07 (warning log in execRenameContainer and DeadlineExceeded translation around getContainers).
Both changes from the previous review are correctly addressed. One new finding from the latest commit.
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>
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ This review covers only the commits pushed since 0a007fc07210 (the previous completed review).
Assessment: 🟡 NEEDS ATTENTION
One medium-severity finding in the new deadline-scoping logic.
Lower-confidence findings (not posted inline)
[low] pkg/compose/executor.go:253 — All OpWaitCondition nodes share the same absolute waitDeadline; if preceding create/start/hook ops consume most of the budget, health-wait nodes get little or no effective time (confidence: weak 34/100). This is documented as intentional ("one overall budget") and mirrors the old code's behavior under the previous WithTimeout-wrapped plan; flagged for awareness.
…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>
The reason will be displayed to describe this comment to others. Learn more.
This review covers only the commits pushed since b6b0d598100d2a593a3364969391cc0b979a6fa2.
Assessment: 🟢 APPROVE
The single new commit (570e0944a) introduces applyRemainingDeadline — a well-structured helper that correctly handles all three deadline states: zero (returns a no-op cancel and the unmodified context), already-elapsed (returns a user-friendly error directly, before any daemon call can produce a confusing bare DeadlineExceeded), and future (delegates to context.WithDeadline as normal). The call site in upDetached is wired correctly: the defer cancel() is registered only after a nil-check via the if err != nil { return err } guard, so the nil cancel returned in the error path is never invoked.
This is exactly the fix that was requested in the prior review thread (PRRT_kwDOAOWUd86pi1aP, now resolved), and the implementation matches the suggested pattern while extracting the logic into a named, tested, documented function — an improvement over an inline block.
No bugs were found in the changes introduced by this commit.
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>
The reason will be displayed to describe this comment to others. Learn more.
This review covers only the commits since 650e3f529bfb.
Assessment: 🟢 APPROVE
The incremental diff moves getContainers() to run before the --wait-timeout context window is opened, and removes the now-incorrect DeadlineExceeded-to-friendly-message translation that was guarding the old placement. The refactoring is correct:
In the new placement, getContainers() runs on the caller's original context (before any WaitTimeout is applied), so the removed translation — which named the --wait-timeout duration — would have been wrong there; a plain return err is the right replacement.
The origCtx guard on waitDependencies is unchanged and continues to correctly distinguish a fresh WaitTimeout-derived deadline from the caller's own external deadline.
Container-list freshness is not a concern: getContainers() is called after exec.run() completes, so all containers are already in their final post-execution state.
…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>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Deadline-origin detection remains scheduling-dependent and can misclassify timeout outcomes.
0 open findings
Previously missed (2)
In code that hasn't changed since last review
Parent deadline race loses application health timeout error
pkg/compose/executor.go:413
This post-return origCtx.Err() check has the same deadline-origin race: the wait's own earlier deadline may have caused the return, then the parent can expire before this condition executes. In that case errWaitTimeout is not attached and detached up --wait returns a raw deadline error instead of application not healthy. Derive the child with a distinct timeout cause and classify using context.Cause(ctx) rather than the parent's state at observation time.
origCtx.Err() == nil does not reliably identify which deadline fired. This wait's shorter timeout can expire first, but if the goroutine is not scheduled until after the parent also expires, origCtx.Err() is non-nil and an optional dependency is incorrectly returned as an error instead of being skipped. Use a distinct timeout cause (for example, context.WithTimeoutCause) and test context.Cause(ctx) so the result records which cancellation won, independent of when this branch runs.
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
The reason will be displayed to describe this comment to others. Learn more.
Note: This review covers only the commits pushed since a408a46d8125 (the previous completed review). Earlier commits were already reviewed.
Assessment: 🟢 APPROVE
The errWaitTimeout sentinel + execBoundedWaitCondition + translateWaitTimeout refactor is clean and correct. The dual-wrap fmt.Errorf("%w: %w", errWaitTimeout, err) correctly satisfies both errors.Is(err, errWaitTimeout) and errors.Is(err, context.DeadlineExceeded) (Go 1.20+). The origCtx.Err() == nil guard reliably distinguishes the wait's own per-node derived timeout from an inherited caller deadline in the common case. The new test suite (TestExecutePlanWaitTimeoutIsMarked, TestExecutePlanOtherDeadlinesAreNotWaitTimeouts, TestExecutePlanWaitTimeoutSurvivesDependentSkip, TestTranslateWaitTimeout) pins all the key scenarios correctly, including the regression for the leaked waitTimeout into non-wait node contexts verified by networkCtxHadDeadline.
Lower-confidence findings (not posted inline)
[low] pkg/compose/executor.go:410 — TOCTOU: origCtx cancellation between execWaitCondition returning and the origCtx.Err() == nil guard (confidence: weak 52/100). If origCtx is concurrently cancelled (e.g. by cancelPhase() from a sibling node's failure) in the nanosecond window after the derived ctx times out and before the origCtx.Err() == nil guard evaluates, the error is not wrapped with errWaitTimeout and translateWaitTimeout returns a raw context deadline exceeded instead of the user-friendly message. Impact is UX-only (wrong error message, no data loss); the race window is vanishingly narrow and exploiting it requires a concurrent failure in the same phase within nanoseconds. Listed for awareness rather than as a blocker.
…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>
The reason will be displayed to describe this comment to others. Learn more.
This review covers only the commits pushed since 8d1f255d8826.
Assessment: 🟢 APPROVE
The incremental diff is a clean, minimal simplification: translateWaitTimeout drops its unused wait bool and timeout time.Duration parameters and always returns "timeout waiting for dependencies". The "application not healthy after N" message now lives exclusively in the post-plan waitDependencies block in up.go (line 147–148), which independently gates on options.Start.Wait and --wait-timeout. The single call-site in up.go is correctly updated, the updated function comment accurately describes the design intent, and no other call sites exist. No bugs introduced.
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>
The reason will be displayed to describe this comment to others. Learn more.
This review covers only the commits pushed since b60e75a (the previous completed review).
Assessment: 🟢 APPROVE
The incremental diff refines the scale: 0 vs deploy.replicas: 0 distinction in planServiceStart:
Narrowed early-return (deploy.replicas: 0 only, via the fully-guarded Deploy != nil && Deploy.Replicas != nil && *Deploy.Replicas == 0): correctly restricts the unconditional no-op to the explicit deploy-replicas knob, leaving scale: 0 services to proceed through depends_on evaluation and existing-replica starts.
Added GetScale() > 0 guard on the "no container to start" error path: correctly prevents a spurious error for scale:0 services with no replicas under ScopeStart.
New test TestPlanStart_ScaleZeroIsStillVisited: covers both scopes, verifies exactly one deduplicated OpWaitCondition (from the scale:0 service, not the deploy.replicas:0 one), verifies OpStartContainer for an existing replica under ScopeStart, and verifies no start node under ScopeCreateStart.
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
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.
docker-agent
left a comment
Stacked on #14289 (not yet merged) — only the last commit, "feat: detached up runs on a single plan", belongs to this PR. The diff will shrink to that one commit once #14289 merges and this branch rebases onto main. Draft until then.
What this PR does, in one sentence
docker compose up -dnow runs its Create and Start phases as a single plan over a single daemon snapshot, instead of the oldcreate()+start()sequence — two separate snapshots, the second blind to what the first had just done.Context
Detached
uphas always run two unrelated things back to back:create()converges the project (networks, volumes, containers) and plans aPlan, thenstart()re-lists containers from the daemon from scratch and runs the older imperative engine to start them. That second listing can't see anythingcreate()'s plan changed in the same call — it only sees whatever the daemon looked like before this invocation ofupbegan, modulo its own separate converge from the first listing's point of view.Lot 1 of epic #14081 already taught the reconciler and executor to plan and execute a Start phase in the same
Planas the Create phase (ReconcileOptions.Scope: ScopeCreateStart), precisely so a caller like detachedupcould stop needing a second pass. Until this PR, nothing actually requested that scope — it shipped as inert code with no consumer.What the PR brings
preparePlantakes an explicit scope now (create()passesScopeCreate, byte-identical to its previous implicit behavior). A newupDetachedrunspreparePlan(..., ScopeCreateStart)+executePlanonce forup -d;--waitstays a final, post-plan verification (re-listing containers once execution is done, since the plan may have just created or recreated some) on the samewaitDependenciesprimitivestart()already uses. Interactiveupis untouched — its own create/start phase boundary is a separate, riskier item of the same lot.Making
ScopeCreateStarta real, exercised path for the first time surfaced two latent bugs in the recreate machinery that nothing had ever driven end to end: a recreated container's Start-phase progress events (and a dependent's health-wait events) kept reporting the container's internal temporary rename-dance name instead of its final one. Both are fixed here, each pinned by a regression test — see the commit message for the detail; caught and confirmed via the full e2e suite (TestRestartWithDependencies) against a locally built binary, not just unit tests.Part of #14081.
🤖 Generated with Claude Code