From 9ff7182c3e30727979f4466ad70ea25dab3dbf30 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 14:16:57 +0200 Subject: [PATCH 01/26] refactor: split create() into preparePlan + executePlan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 --- pkg/compose/create.go | 44 ++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/pkg/compose/create.go b/pkg/compose/create.go index 01cc3aacc7..6f175dd346 100644 --- a/pkg/compose/create.go +++ b/pkg/compose/create.go @@ -71,6 +71,28 @@ func (s *composeService) Create(ctx context.Context, project *types.Project, opt } func (s *composeService) create(ctx context.Context, project *types.Project, options api.CreateOptions) error { + project, observed, plan, err := s.preparePlan(ctx, project, options) + if err != nil { + return err + } + + // Emit "Running" events for containers that are already up-to-date, so + // the progress display accounts for containers the plan will not touch. + emitRunningEvents(project, observed, plan, s.events) + + return s.executePlan(ctx, project, observed, plan) +} + +// preparePlan resolves the project model, ensures the resources the plan +// itself doesn't own (images, models, external networks/volumes), observes +// the daemon, and reconciles all of it into a Plan — without creating, +// starting, or otherwise touching a single container, network, or volume. +// create's split from this is pure extraction: callers that need the +// canonical project, the daemon snapshot, or the plan itself before deciding +// how to run it (interactive up's create/start phase boundary, #14081) call +// this directly instead of create; create itself still just chains this with +// executePlan. +func (s *composeService) preparePlan(ctx context.Context, project *types.Project, options api.CreateOptions) (*types.Project, *ObservedState, *Plan, error) { if len(options.Services) == 0 { options.Services = project.ServiceNames() } @@ -82,40 +104,40 @@ func (s *composeService) create(ctx context.Context, project *types.Project, opt err := project.CheckContainerNameUnicity() if err != nil { - return err + return nil, nil, nil, err } err = s.ensureImagesExists(ctx, project, options.Build, options.QuietPull, options.SkipProviders) if err != nil { - return err + return nil, nil, nil, err } err = s.ensureModels(ctx, project, options.QuietPull) if err != nil { - return err + return nil, nil, nil, err } prepareNetworks(project) externalNetworks, err := s.checkExternalNetworks(ctx, project) if err != nil { - return err + return nil, nil, nil, err } prepareVolumes(project) externalVolumes, err := s.checkExternalVolumes(ctx, project) if err != nil { - return err + return nil, nil, nil, err } // Temporary implementation of use_api_socket until we get actual support inside docker engine project, err = s.useAPISocket(project) if err != nil { - return err + return nil, nil, nil, err } observed, err := s.collectObservedState(ctx, project) if err != nil { - return err + return nil, nil, nil, err } observed.setResolvedNetworks(externalNetworks, project) observed.setResolvedVolumes(externalVolumes) @@ -131,14 +153,10 @@ func (s *composeService) create(ctx context.Context, project *types.Project, opt plan, err := reconcile(ctx, project, observed, toReconcileOptions(options), s.prompt) if err != nil { - return err + return nil, nil, nil, err } - // Emit "Running" events for containers that are already up-to-date, so - // the progress display accounts for containers the plan will not touch. - emitRunningEvents(project, observed, plan, s.events) - - return s.executePlan(ctx, project, observed, plan) + return project, observed, plan, nil } func prepareNetworks(project *types.Project) { From 76a7391e10b129a6debf9c4ffe1be53ade83313e Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 16:32:06 +0200 Subject: [PATCH 02/26] feat: detached up runs on a single plan 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 "_" 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 --- pkg/compose/create.go | 14 ++-- pkg/compose/executor_ops.go | 30 +++++++- pkg/compose/executor_start_test.go | 107 +++++++++++++++++++++++++++++ pkg/compose/reconcile.go | 7 +- pkg/compose/up.go | 67 ++++++++++++++++-- 5 files changed, 211 insertions(+), 14 deletions(-) diff --git a/pkg/compose/create.go b/pkg/compose/create.go index 6f175dd346..1d2add52b5 100644 --- a/pkg/compose/create.go +++ b/pkg/compose/create.go @@ -71,7 +71,7 @@ func (s *composeService) Create(ctx context.Context, project *types.Project, opt } func (s *composeService) create(ctx context.Context, project *types.Project, options api.CreateOptions) error { - project, observed, plan, err := s.preparePlan(ctx, project, options) + project, observed, plan, err := s.preparePlan(ctx, project, options, ScopeCreate) if err != nil { return err } @@ -89,10 +89,10 @@ func (s *composeService) create(ctx context.Context, project *types.Project, opt // starting, or otherwise touching a single container, network, or volume. // create's split from this is pure extraction: callers that need the // canonical project, the daemon snapshot, or the plan itself before deciding -// how to run it (interactive up's create/start phase boundary, #14081) call -// this directly instead of create; create itself still just chains this with -// executePlan. -func (s *composeService) preparePlan(ctx context.Context, project *types.Project, options api.CreateOptions) (*types.Project, *ObservedState, *Plan, error) { +// how to run it (detached up's single-plan Create+Start, interactive up's +// phase boundary, #14081) call this directly instead of create with a wider +// scope; create itself still just chains ScopeCreate with executePlan. +func (s *composeService) preparePlan(ctx context.Context, project *types.Project, options api.CreateOptions, scope ReconcileScope) (*types.Project, *ObservedState, *Plan, error) { if len(options.Services) == 0 { options.Services = project.ServiceNames() } @@ -151,7 +151,9 @@ func (s *composeService) preparePlan(ctx context.Context, project *types.Project "--remove-orphans flag to clean it up.", observed.orphanNames()) } - plan, err := reconcile(ctx, project, observed, toReconcileOptions(options), s.prompt) + reconcileOptions := toReconcileOptions(options) + reconcileOptions.Scope = scope + plan, err := reconcile(ctx, project, observed, reconcileOptions, s.prompt) if err != nil { return nil, nil, nil, err } diff --git a/pkg/compose/executor_ops.go b/pkg/compose/executor_ops.go index 49921b725f..f2e8fd6e84 100644 --- a/pkg/compose/executor_ops.go +++ b/pkg/compose/executor_ops.go @@ -383,5 +383,33 @@ func (exec *planExecutor) execRenameContainer(ctx context.Context, node *PlanNod _, err := exec.compose.apiClient().ContainerRename(ctx, createdID, client.ContainerRenameOptions{ NewName: op.Name, }) - return err + if err != nil { + return err + } + // Update the create node's own result in place rather than publishing a + // separate entry under this node's ID: plannedReplica deliberately keeps + // every start-phase reference (resolveContainerID, groupEventName) + // pointed at CreateNodeID, not at this rename node, so they all see the + // rename take effect without each needing their own case for "was this + // replica's container ever renamed". The ID is unchanged by a rename; + // only the name this node is renaming-TO replaces the create's own + // temporary one. + exec.pctx.set(op.CreateNodeID, operationResult{ContainerID: createdID, ContainerName: op.Name}) + + // execCreateContainer published this container into the live view under + // its temporary name (the only name it had at that point); refresh it in + // place now that the rename landed, or OpWaitCondition and any sibling + // execCreateContainer resolving a service reference keep seeing the + // temporary one for the rest of the plan's execution. + if op.Service != nil { + exec.containersMu.Lock() + for i, ctr := range exec.containersByService[op.Service.Name] { + if ctr.ID == createdID { + exec.containersByService[op.Service.Name][i].Names = []string{"/" + op.Name} + break + } + } + exec.containersMu.Unlock() + } + return nil } diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index f0c821bc11..b24776d577 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -189,6 +189,113 @@ func TestExecStartContainer_EnrichedResolvesCreateNodeAndStarts(t *testing.T) { assert.NilError(t, err) } +// TestExecutePlanRecreateThenStartUsesFinalName is a regression test: a +// recreate's create node is planned under a temporary name +// (planRecreateContainer's "_" dance, to avoid colliding with +// the old container still holding the final name), and the final name is +// applied by a separate OpRenameContainer node. The start phase's event +// naming (groupEventName) and resolveContainerID both read +// pctx.get(CreateNodeID) by design (plannedReplica keeps every start-phase +// reference pointed at the create node, not the rename node) -- so +// execRenameContainer must update that same entry in place once it renames +// the container, or the start phase reports progress under the stale +// temporary name forever. Caught via e2e (TestRestartWithDependencies) +// before this node-result update existed. +func TestExecutePlanRecreateThenStartUsesFinalName(t *testing.T) { + svc, apiClient, recorder := newStartPhaseTestService(t) + + service := types.ServiceConfig{Name: "web", ContainerSpec: types.ContainerSpec{Image: "alpine"}} + project := &types.Project{Name: "test", Services: types.Services{"web": service}} + + const tmpName = "abc123456789_test-web-1" + const finalName = "test-web-1" + + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "new-id"}, nil) + apiClient.EXPECT().ContainerInspect(gomock.Any(), "new-id", gomock.Any()). + Return(client.ContainerInspectResult{Container: container.InspectResponse{ + ID: "new-id", + Name: "/" + tmpName, + Config: &container.Config{}, + NetworkSettings: &container.NetworkSettings{}, + }}, nil) + apiClient.EXPECT().ContainerRename(gomock.Any(), "new-id", client.ContainerRenameOptions{NewName: finalName}). + Return(client.ContainerRenameResult{}, nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), "new-id", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + + plan := &Plan{} + create := plan.addNode(Operation{ + Type: OpCreateContainer, + ResourceID: "service:web:1", + Cause: "config changed (tmpName)", + Service: &service, + Name: tmpName, + Number: 1, + }, "") + rename := plan.addNode(Operation{ + Type: OpRenameContainer, + ResourceID: "service:web:1", + Cause: "finalize recreate", + Name: finalName, + CreateNodeID: create.ID, + }, "", create) + plan.addNode(Operation{ + Type: OpStartContainer, + ResourceID: "service:web:1", + Cause: "start", + Service: &service, + CreateNodeID: create.ID, + }, "start:web:1", rename) + + err := svc.executePlan(t.Context(), project, emptyObservedState("test"), plan) + assert.NilError(t, err) + + // The start group's eventName is resolved lazily from + // pctx.get(CreateNodeID) (see groupEventName) once the start node + // actually runs -- by then the rename has already completed, so this + // must be the post-rename name, not create's own temporary one. + assert.DeepEqual(t, recorder.byID["Container "+finalName], []string{api.StatusStarting, api.StatusStarted}) +} + +// TestExecRenameContainer_RefreshesLiveViewName is the sibling regression to +// TestExecutePlanRecreateThenStartUsesFinalName: execCreateContainer +// publishes the new container into containersByService (the live view +// OpWaitCondition and sibling execCreateContainer calls read by service +// name) under its temporary name -- the only one it had at that point. +// Without execRenameContainer refreshing that same entry, a dependent +// waiting on this service's health (depends_on: condition: service_healthy) +// reports Waiting/Healthy under the stale temporary name for the rest of the +// plan's execution, exactly like TestRestartWithDependencies caught in e2e. +func TestExecRenameContainer_RefreshesLiveViewName(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + + service := types.ServiceConfig{Name: "web"} + project := &types.Project{Name: "test", Services: types.Services{"web": service}} + + const tmpName = "abc123456789_test-web-1" + const finalName = "test-web-1" + + apiClient.EXPECT().ContainerRename(gomock.Any(), "new-id", client.ContainerRenameOptions{NewName: finalName}). + Return(client.ContainerRenameResult{}, nil) + + exec := svc.newPlanExecutor(project, emptyObservedState("test"), nil) + exec.pctx.set(1, operationResult{ContainerID: "new-id", ContainerName: tmpName}) + exec.containersByService["web"] = Containers{{ID: "new-id", Names: []string{"/" + tmpName}}} + + node := &PlanNode{ID: 2, Operation: Operation{ + Type: OpRenameContainer, + Name: finalName, + Service: &service, + CreateNodeID: 1, + }} + + err := exec.execRenameContainer(t.Context(), node) + assert.NilError(t, err) + + assert.DeepEqual(t, exec.containersByService["web"][0].Names, []string{"/" + finalName}) +} + // TestExecRunPreStart_SkipsWhenReplicaAlreadyRunning covers the // observe-to-execute drift guard: the plan scheduled RunPreStart because no // replica was running at observation time, but a replica started in the diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index cb2b805b21..c460dfff75 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -1511,13 +1511,18 @@ func (r *reconciler) planRecreateContainer(service types.ServiceConfig, oc *Obse }, group, removeDeps...) // 4. Rename to final name. Link to the create node so the executor can - // fetch the resulting container ID directly. + // fetch the resulting container ID directly. Service is set so + // execRenameContainer can refresh that container's entry in the live + // containersByService view (OpWaitCondition and sibling create calls + // read it by service name) once the rename actually lands — otherwise + // they'd keep seeing the temporary name past this node. finalName := getContainerName(r.project.Name, service, oc.Number) renameNode := r.plan.addNode(Operation{ Type: OpRenameContainer, ResourceID: resID, Cause: "finalize recreate", Name: finalName, + Service: &serviceCopy, CreateNodeID: createNode.ID, }, group, removeNode) diff --git a/pkg/compose/up.go b/pkg/compose/up.go index 2c7998a46b..b44420e8f1 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -44,14 +44,10 @@ import ( func (s *composeService) Up(ctx context.Context, project *types.Project, options api.UpOptions) error { err := Run(ctx, tracing.SpanWrapFunc("project/up", tracing.ProjectOptions(ctx, project), func(ctx context.Context) error { - err := s.create(ctx, project, options.Create) - if err != nil { - return err - } if options.Start.Attach == nil { - return s.start(ctx, project.Name, options.Start, nil) + return s.upDetached(ctx, project, options) } - return nil + return s.create(ctx, project, options.Create) }), "up", s.events) if err != nil { return err @@ -67,6 +63,65 @@ func (s *composeService) Up(ctx context.Context, project *types.Project, options return s.runInteractiveUp(ctx, project, options) } +// upDetached runs detached `up`'s Create and Start phases as a single plan: +// the semantic switchover from create() + start() -- two separate daemon +// snapshots, the second blind to what the first just did -- to one +// preparePlan/executePlan pass covering both phases (epic #14081, lot 2). +// Interactive up keeps the create()+start() sequence for now: its own +// create/start phase boundary (attach/printer/monitor setup in between) is a +// separate, riskier item of the same lot. +func (s *composeService) upDetached(ctx context.Context, project *types.Project, options api.UpOptions) error { + project, observed, plan, err := s.preparePlan(ctx, project, options.Create, ScopeCreateStart) + if err != nil { + return err + } + + // Must run against the pre-execution snapshot: observed only labels a + // container Running if it already was one before this plan touched + // anything, exactly what "the plan won't touch it" is supposed to mean. + emitRunningEvents(project, observed, plan, s.events) + + if err := s.executePlan(ctx, project, observed, plan); err != nil { + return err + } + + if !options.Start.Wait { + return nil + } + + // The plan may have created or recreated containers: the verification + // below needs their current IDs, which the pre-execution observed + // snapshot above doesn't have for a container that didn't exist yet at + // observation time. --wait stays a final, post-plan check on the shared + // waitDependency primitive (synthetic conditions, global timeout) -- + // matching start()'s own Wait handling below, which this intentionally + // mirrors. + containers, err := s.getContainers(ctx, project.Name, oneOffExclude, true) + if err != nil { + return err + } + + depends := types.DependsOnConfig{} + for _, svc := range project.Services { + depends[svc.Name] = types.ServiceDependency{ + Condition: getDependencyCondition(svc, project), + Required: true, + } + } + if options.Start.WaitTimeout > 0 { + withTimeout, cancel := context.WithTimeout(ctx, options.Start.WaitTimeout) + defer cancel() + ctx = withTimeout + } + if err := s.waitDependencies(ctx, project, project.Name, depends, containers, 0); err != nil { + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) + } + return err + } + return nil +} + // upSession carries the state shared between the goroutines driving an // interactive `compose up` once services are created: the errgroup running // them, the collected errors, and the application exit status. From d39bcc914df9480108b970dc395f105939f7d166 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 17:00:58 +0200 Subject: [PATCH 03/26] fix(up): apply --wait-timeout before executing the plan, not after 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 --- pkg/compose/executor_start_test.go | 56 ++++++++++++++++++++++++++++++ pkg/compose/up.go | 38 +++++++++++++------- 2 files changed, 82 insertions(+), 12 deletions(-) diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index b24776d577..73b6179c36 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -23,6 +23,7 @@ import ( "net" "sync" "testing" + "time" "github.com/compose-spec/compose-go/v2/types" "github.com/docker/cli/cli/config/configfile" @@ -145,6 +146,61 @@ func TestExecWaitCondition_HealthyConditionSatisfied(t *testing.T) { assert.DeepEqual(t, recorder.byID["Container test-db-1"], []string{api.StatusWaiting, api.StatusHealthy}) } +// TestExecutePlanWaitConditionRespectsContextDeadline is a regression test +// for a gap a reviewer caught on upDetached (pkg/compose/up.go): the plan's +// own OpWaitCondition nodes have no timeout of their own (unlike the +// imperative engine's waitDependencies, which already threaded WaitTimeout +// through every per-dependency wait, not just the final --wait check) -- +// they rely entirely on the ctx run() and execWaitCondition are given. If +// nothing establishes a deadline on that ctx before executePlan runs, a +// condition that's never satisfied blocks forever instead of failing after +// --wait-timeout. This drives a real OpWaitCondition node (service_healthy, +// never satisfied) through the full executePlan/run() DAG path -- not +// execWaitCondition in isolation -- under a short deadline, and asserts it +// returns promptly with ctx.Err(), proving the deadline really does +// propagate from the caller into the plan's blocking nodes. +func TestExecutePlanWaitConditionRespectsContextDeadline(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + + dbSummary := container.Summary{ + ID: "db-id", + Names: []string{"/test-db-1"}, + Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"}, + } + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(client.ContainerInspectResult{ + Container: container.InspectResponse{ + ID: "db-id", + Name: "/test-db-1", + State: &container.State{ + Status: container.StateRunning, + Health: &container.Health{Status: container.Starting}, + }, + Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}}, + }, + }, nil).AnyTimes() + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: "wait:db:service_healthy", + Name: "db", + Condition: types.ServiceConditionHealthy, + }, "") + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + exec.containersByService["db"] = Containers{dbSummary} + + ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) + defer cancel() + + start := time.Now() + err := exec.run(ctx, plan) + elapsed := time.Since(start) + + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.Assert(t, elapsed < 5*time.Second, "execWaitCondition's polling loop did not stop at the deadline (took %s)", elapsed) +} + // TestExecStartContainer_EnrichedResolvesCreateNodeAndStarts verifies that a // start-phase OpStartContainer (Service set) resolves its target through the // CreateContainer node it references — the same reconciliationContext diff --git a/pkg/compose/up.go b/pkg/compose/up.go index b44420e8f1..ff281ab79d 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -81,7 +81,26 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // anything, exactly what "the plan won't touch it" is supposed to mean. emitRunningEvents(project, observed, plan, s.events) + // The plan's own OpWaitCondition nodes (service_healthy / + // service_completed_successfully dependency waits) block on ctx with no + // timeout of their own -- unlike start()'s per-dependency + // waitDependencies call, which already threaded WaitTimeout through + // every wait, not just the final one below. Establish the deadline + // before executing the plan, not after, or a condition that's never + // satisfied hangs the whole command instead of failing after + // WaitTimeout. preparePlan above is deliberately left out of it: image + // pulls and the rest of create()'s own work were never bounded by + // --wait-timeout either, in the old create()+start() sequence. + if options.Start.Wait && options.Start.WaitTimeout > 0 { + withTimeout, cancel := context.WithTimeout(ctx, options.Start.WaitTimeout) + defer cancel() + ctx = withTimeout + } + if err := s.executePlan(ctx, project, observed, plan); err != nil { + if options.Start.Wait && errors.Is(ctx.Err(), context.DeadlineExceeded) { + return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) + } return err } @@ -89,13 +108,13 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, return nil } - // The plan may have created or recreated containers: the verification - // below needs their current IDs, which the pre-execution observed - // snapshot above doesn't have for a container that didn't exist yet at - // observation time. --wait stays a final, post-plan check on the shared - // waitDependency primitive (synthetic conditions, global timeout) -- - // matching start()'s own Wait handling below, which this intentionally - // mirrors. + // The plan may have created or recreated containers: this verification + // needs their current IDs, which the pre-execution observed snapshot + // above doesn't have for a container that didn't exist yet at + // observation time. It shares ctx's deadline (established above) rather + // than opening a fresh WaitTimeout window of its own -- one overall + // budget for "becoming healthy", covering both the plan's own waits and + // this final check, not one budget per wait. containers, err := s.getContainers(ctx, project.Name, oneOffExclude, true) if err != nil { return err @@ -108,11 +127,6 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, Required: true, } } - if options.Start.WaitTimeout > 0 { - withTimeout, cancel := context.WithTimeout(ctx, options.Start.WaitTimeout) - defer cancel() - ctx = withTimeout - } if err := s.waitDependencies(ctx, project, project.Name, depends, containers, 0); err != nil { if errors.Is(ctx.Err(), context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) From 88b07ee38e65a9aea7f0e03c750c85bf616ccc01 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 17:03:50 +0200 Subject: [PATCH 04/26] fix(up): apply the plan deadline regardless of --wait 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 --- pkg/compose/up.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/pkg/compose/up.go b/pkg/compose/up.go index ff281ab79d..3078dbeb2a 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -84,21 +84,23 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // The plan's own OpWaitCondition nodes (service_healthy / // service_completed_successfully dependency waits) block on ctx with no // timeout of their own -- unlike start()'s per-dependency - // waitDependencies call, which already threaded WaitTimeout through - // every wait, not just the final one below. Establish the deadline - // before executing the plan, not after, or a condition that's never - // satisfied hangs the whole command instead of failing after - // WaitTimeout. preparePlan above is deliberately left out of it: image - // pulls and the rest of create()'s own work were never bounded by - // --wait-timeout either, in the old create()+start() sequence. - if options.Start.Wait && options.Start.WaitTimeout > 0 { + // waitDependencies call, which already threads WaitTimeout through every + // wait unconditionally (InDependencyOrder passes it to startService + // regardless of options.Wait -- --wait-timeout alone, with no --wait, is + // a legal CLI combination nothing rejects). Establish the same + // unconditional deadline before executing the plan, not after, or a + // condition that's never satisfied hangs the whole command instead of + // failing after WaitTimeout. preparePlan above is deliberately left out + // of it: image pulls and the rest of create()'s own work were never + // bounded by --wait-timeout either, in the old create()+start() sequence. + if options.Start.WaitTimeout > 0 { withTimeout, cancel := context.WithTimeout(ctx, options.Start.WaitTimeout) defer cancel() ctx = withTimeout } if err := s.executePlan(ctx, project, observed, plan); err != nil { - if options.Start.Wait && errors.Is(ctx.Err(), context.DeadlineExceeded) { + if options.Start.WaitTimeout > 0 && errors.Is(ctx.Err(), context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) } return err From 0a007fc072106007936fb24a8a88466a8fde0050 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 17:10:56 +0200 Subject: [PATCH 05/26] fix(up): translate a deadline hit during the post-plan container listing 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 --- pkg/compose/executor_ops.go | 10 ++++++++++ pkg/compose/up.go | 3 +++ 2 files changed, 13 insertions(+) diff --git a/pkg/compose/executor_ops.go b/pkg/compose/executor_ops.go index f2e8fd6e84..9c999e711a 100644 --- a/pkg/compose/executor_ops.go +++ b/pkg/compose/executor_ops.go @@ -403,13 +403,23 @@ func (exec *planExecutor) execRenameContainer(ctx context.Context, node *PlanNod // temporary one for the rest of the plan's execution. if op.Service != nil { exec.containersMu.Lock() + found := false for i, ctr := range exec.containersByService[op.Service.Name] { if ctr.ID == createdID { exec.containersByService[op.Service.Name][i].Names = []string{"/" + op.Name} + found = true break } } exec.containersMu.Unlock() + if !found { + // Shouldn't happen: the DAG guarantees execCreateContainer's + // append ran before this node does. Logged rather than silently + // skipped, so a future regression that breaks that invariant is + // at least visible instead of just quietly serving the + // temporary name downstream again. + logrus.Warnf("execRenameContainer: container %s not found in live view for service %s; dependents may see the stale temporary name", createdID, op.Service.Name) + } } return nil } diff --git a/pkg/compose/up.go b/pkg/compose/up.go index 3078dbeb2a..21268531d9 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -119,6 +119,9 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // this final check, not one budget per wait. containers, err := s.getContainers(ctx, project.Name, oneOffExclude, true) if err != nil { + if options.Start.WaitTimeout > 0 && errors.Is(ctx.Err(), context.DeadlineExceeded) { + return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) + } return err } From 1e807eeb4643539d9f5308b0e2a9398ec0e456d4 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 17:20:17 +0200 Subject: [PATCH 06/26] fix(up): scope the wait deadline to OpWaitCondition nodes only 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 --- pkg/compose/executor.go | 14 ++++++ pkg/compose/executor_start_test.go | 77 ++++++++++++++++++++++++++---- pkg/compose/up.go | 49 +++++++++++-------- 3 files changed, 111 insertions(+), 29 deletions(-) diff --git a/pkg/compose/executor.go b/pkg/compose/executor.go index b9b7cd9bd1..86837d667a 100644 --- a/pkg/compose/executor.go +++ b/pkg/compose/executor.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "sync" + "time" "github.com/compose-spec/compose-go/v2/types" "golang.org/x/sync/errgroup" @@ -47,6 +48,14 @@ type planExecutor struct { // round-trip per create. containersMu sync.Mutex containersByService map[string]Containers + + // waitDeadline, when non-zero, bounds OpWaitCondition nodes only (see + // executeNode) -- not the rest of the plan. up -d --wait-timeout must + // cap how long a dependency's health/completion condition is waited on, + // the same thing start()'s own WaitTimeout already does today, without + // also capping unrelated create/start/hook work the way wrapping the + // whole run() ctx would (docker/compose#14290 review). + waitDeadline time.Time } // reconciliationContext holds results produced by completed nodes so that downstream @@ -239,6 +248,11 @@ func (exec *planExecutor) executeNode(ctx context.Context, node *PlanNode) error case OpCreateHookContainer: return exec.execCreateHookContainer(ctx, node) case OpWaitCondition: + if !exec.waitDeadline.IsZero() { + var cancel context.CancelFunc + ctx, cancel = context.WithDeadline(ctx, exec.waitDeadline) + defer cancel() + } return exec.execWaitCondition(ctx, op) case OpRunPreStart: return exec.execRunPreStart(ctx, op) diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index 73b6179c36..e331b39cfd 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -151,14 +151,14 @@ func TestExecWaitCondition_HealthyConditionSatisfied(t *testing.T) { // own OpWaitCondition nodes have no timeout of their own (unlike the // imperative engine's waitDependencies, which already threaded WaitTimeout // through every per-dependency wait, not just the final --wait check) -- -// they rely entirely on the ctx run() and execWaitCondition are given. If -// nothing establishes a deadline on that ctx before executePlan runs, a -// condition that's never satisfied blocks forever instead of failing after -// --wait-timeout. This drives a real OpWaitCondition node (service_healthy, -// never satisfied) through the full executePlan/run() DAG path -- not -// execWaitCondition in isolation -- under a short deadline, and asserts it -// returns promptly with ctx.Err(), proving the deadline really does -// propagate from the caller into the plan's blocking nodes. +// they rely entirely on whatever ctx they're given (directly, or via +// exec.waitDeadline -- see TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes for +// that path specifically). This drives a real OpWaitCondition node +// (service_healthy, never satisfied) through the full executePlan/run() DAG +// path -- not execWaitCondition in isolation -- under a short deadline on +// the ctx run() itself is given, and asserts it returns promptly with +// ctx.Err(), proving the deadline really does propagate from the caller into +// the plan's blocking nodes. func TestExecutePlanWaitConditionRespectsContextDeadline(t *testing.T) { svc, apiClient, _ := newStartPhaseTestService(t) @@ -201,6 +201,67 @@ func TestExecutePlanWaitConditionRespectsContextDeadline(t *testing.T) { assert.Assert(t, elapsed < 5*time.Second, "execWaitCondition's polling loop did not stop at the deadline (took %s)", elapsed) } +// TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes is a regression test for a +// Copilot review finding on #14290: upDetached originally bounded +// --wait-timeout by wrapping the whole run() ctx, which would have also +// capped unrelated Create-phase and Start-phase work (network/container +// creation, hooks) that was never subject to --wait-timeout in the old +// create()+start() sequence. exec.waitDeadline (set by upDetached, consulted +// only in executeNode's OpWaitCondition case) must bound exclusively the +// blocking dependency wait, leaving every other node on the plan's plain, +// deadline-free ctx. This plan pairs a never-satisfied OpWaitCondition with +// an OpCreateNetwork that has no deadline of its own: if waitDeadline leaked +// into the whole run(), the network create would race the same short +// deadline instead of completing normally. +func TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + + dbSummary := container.Summary{ + ID: "db-id", + Names: []string{"/test-db-1"}, + Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"}, + } + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(client.ContainerInspectResult{ + Container: container.InspectResponse{ + ID: "db-id", + Name: "/test-db-1", + State: &container.State{ + Status: container.StateRunning, + Health: &container.Health{Status: container.Starting}, + }, + Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}}, + }, + }, nil).AnyTimes() + nw := types.NetworkConfig{Name: "test_default"} + apiClient.EXPECT().NetworkCreate(gomock.Any(), "test_default", gomock.Any()). + Return(client.NetworkCreateResult{ID: "net-id"}, nil) + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpCreateNetwork, + ResourceID: "network:default", + Name: nw.Name, + Network: &nw, + }, "") + plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: "wait:db:service_healthy", + Name: "db", + Condition: types.ServiceConditionHealthy, + }, "") + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + exec.containersByService["db"] = Containers{dbSummary} + exec.waitDeadline = time.Now().Add(200 * time.Millisecond) + + start := time.Now() + err := exec.run(t.Context(), plan) + elapsed := time.Since(start) + + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.Assert(t, elapsed < 5*time.Second, "waitDeadline did not bound the stuck OpWaitCondition node (took %s)", elapsed) +} + // TestExecStartContainer_EnrichedResolvesCreateNodeAndStarts verifies that a // start-phase OpStartContainer (Service set) resolves its target through the // CreateContainer node it references — the same reconciliationContext diff --git a/pkg/compose/up.go b/pkg/compose/up.go index 21268531d9..b25040f690 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -82,25 +82,26 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, emitRunningEvents(project, observed, plan, s.events) // The plan's own OpWaitCondition nodes (service_healthy / - // service_completed_successfully dependency waits) block on ctx with no - // timeout of their own -- unlike start()'s per-dependency - // waitDependencies call, which already threads WaitTimeout through every - // wait unconditionally (InDependencyOrder passes it to startService - // regardless of options.Wait -- --wait-timeout alone, with no --wait, is - // a legal CLI combination nothing rejects). Establish the same - // unconditional deadline before executing the plan, not after, or a - // condition that's never satisfied hangs the whole command instead of - // failing after WaitTimeout. preparePlan above is deliberately left out - // of it: image pulls and the rest of create()'s own work were never - // bounded by --wait-timeout either, in the old create()+start() sequence. + // service_completed_successfully dependency waits) block with no timeout + // of their own -- unlike start()'s per-dependency waitDependencies call, + // which already threads WaitTimeout through every wait unconditionally + // (InDependencyOrder passes it to startService regardless of + // options.Wait -- --wait-timeout alone, with no --wait, is a legal CLI + // combination nothing rejects). exec.waitDeadline bounds exactly those + // nodes (see executeNode) and nothing else in the plan: wrapping the + // whole run() ctx instead would also cap unrelated create/start/hook + // work that was never subject to --wait-timeout in the old + // create()+start() sequence (a slow image pull or container create could + // then fail as "application not healthy" with no health wait involved). + exec := s.newPlanExecutor(project, observed, nil) + var deadline time.Time if options.Start.WaitTimeout > 0 { - withTimeout, cancel := context.WithTimeout(ctx, options.Start.WaitTimeout) - defer cancel() - ctx = withTimeout + deadline = time.Now().Add(options.Start.WaitTimeout) + exec.waitDeadline = deadline } - if err := s.executePlan(ctx, project, observed, plan); err != nil { - if options.Start.WaitTimeout > 0 && errors.Is(ctx.Err(), context.DeadlineExceeded) { + if err := exec.run(ctx, plan); err != nil { + if options.Start.WaitTimeout > 0 && errors.Is(err, context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) } return err @@ -113,13 +114,19 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // The plan may have created or recreated containers: this verification // needs their current IDs, which the pre-execution observed snapshot // above doesn't have for a container that didn't exist yet at - // observation time. It shares ctx's deadline (established above) rather - // than opening a fresh WaitTimeout window of its own -- one overall - // budget for "becoming healthy", covering both the plan's own waits and - // this final check, not one budget per wait. + // observation time. It shares the same deadline established above + // (when set) rather than opening a fresh WaitTimeout window of its own + // -- one overall budget for "becoming healthy", covering both the + // plan's own waits and this final check, not one budget per wait. + if !deadline.IsZero() { + withDeadline, cancel := context.WithDeadline(ctx, deadline) + defer cancel() + ctx = withDeadline + } + containers, err := s.getContainers(ctx, project.Name, oneOffExclude, true) if err != nil { - if options.Start.WaitTimeout > 0 && errors.Is(ctx.Err(), context.DeadlineExceeded) { + if !deadline.IsZero() && errors.Is(ctx.Err(), context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) } return err From b6b0d598100d2a593a3364969391cc0b979a6fa2 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 17:34:01 +0200 Subject: [PATCH 07/26] fix(up): don't claim "application not healthy" without --wait 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 --- pkg/compose/up.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/compose/up.go b/pkg/compose/up.go index b25040f690..34cbd3d4ef 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -101,7 +101,13 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, } if err := exec.run(ctx, plan); err != nil { - if options.Start.WaitTimeout > 0 && errors.Is(err, context.DeadlineExceeded) { + // The deadline itself is unconditional (set above whenever + // WaitTimeout > 0, regardless of Wait) -- but "application not + // healthy" is specifically a --wait message. Without --wait, a + // user who only set --wait-timeout as a hang guard sees the raw + // error instead of a message implying a health check they never + // asked for. + if options.Start.Wait && errors.Is(err, context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) } return err From 570e0944a75ceefea22d30be47e1c81c168e6f11 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 17:48:35 +0200 Subject: [PATCH 08/26] fix(up): don't derive an already-expired context for the final wait check 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 --- pkg/compose/up.go | 38 ++++++++++++++++++++++++++++++++++---- pkg/compose/up_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/pkg/compose/up.go b/pkg/compose/up.go index 34cbd3d4ef..df17fa309c 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -124,11 +124,20 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // (when set) rather than opening a fresh WaitTimeout window of its own // -- one overall budget for "becoming healthy", covering both the // plan's own waits and this final check, not one budget per wait. - if !deadline.IsZero() { - withDeadline, cancel := context.WithDeadline(ctx, deadline) - defer cancel() - ctx = withDeadline + // + // exec.run only bounds OpWaitCondition nodes with that deadline (see + // above): a plan whose create/start/hook ops alone take longer than + // WaitTimeout can still return nil past it. applyRemainingDeadline + // catches that case explicitly instead of deriving an already-expired + // context.WithDeadline, which would fail getContainers below before it + // ever touches the daemon -- "not healthy" when health was never + // actually checked. + var cancel context.CancelFunc + ctx, cancel, err = applyRemainingDeadline(ctx, deadline, options.Start.WaitTimeout) + if err != nil { + return err } + defer cancel() containers, err := s.getContainers(ctx, project.Name, oneOffExclude, true) if err != nil { @@ -154,6 +163,27 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, return nil } +// applyRemainingDeadline derives ctx bound by deadline, the no-op +// cancel/unmodified ctx when deadline is zero (no --wait-timeout set). +// Deadline may already be in the past -- the plan's create/start/hook ops +// aren't bound by it (see upDetached), so they can run long enough on their +// own to exhaust the whole --wait-timeout budget before this is ever called. +// context.WithDeadline on an already-past deadline would silently produce an +// immediately-canceled context, failing the caller's very next daemon call +// with a bare context.DeadlineExceeded that looks identical to "health never +// came up" -- misleading when health was never actually checked. Returning +// the familiar message directly here, instead, keeps that distinction clear. +func applyRemainingDeadline(ctx context.Context, deadline time.Time, waitTimeout time.Duration) (context.Context, context.CancelFunc, error) { + if deadline.IsZero() { + return ctx, func() {}, nil + } + if time.Until(deadline) <= 0 { + return nil, nil, fmt.Errorf("application not healthy after %s: deadline exceeded during plan execution", waitTimeout) + } + withDeadline, cancel := context.WithDeadline(ctx, deadline) + return withDeadline, cancel, nil +} + // upSession carries the state shared between the goroutines driving an // interactive `compose up` once services are created: the errgroup running // them, the collected errors, and the application exit status. diff --git a/pkg/compose/up_test.go b/pkg/compose/up_test.go index 23a5b08682..e2763c8fa6 100644 --- a/pkg/compose/up_test.go +++ b/pkg/compose/up_test.go @@ -228,3 +228,40 @@ func TestStreamContainerLogs_ConcurrencyIsBounded(t *testing.T) { assert.NilError(t, eg.Wait()) assert.Equal(t, tracker.Peak(), 1, "streamContainerLogs must share --parallel's budget for concurrent log-attach opens") } + +// TestApplyRemainingDeadline_ZeroDeadlineIsNoOp covers upDetached's +// --wait-timeout-unset path: no deadline, ctx passed through unchanged. +func TestApplyRemainingDeadline_ZeroDeadlineIsNoOp(t *testing.T) { + got, cancel, err := applyRemainingDeadline(t.Context(), time.Time{}, 0) + defer cancel() + assert.NilError(t, err) + _, hasDeadline := got.Deadline() + assert.Assert(t, !hasDeadline) +} + +// TestApplyRemainingDeadline_FutureDeadlineWraps covers the common case: +// budget remains, ctx comes back bound by it. +func TestApplyRemainingDeadline_FutureDeadlineWraps(t *testing.T) { + deadline := time.Now().Add(time.Minute) + got, cancel, err := applyRemainingDeadline(t.Context(), deadline, 5*time.Second) + defer cancel() + assert.NilError(t, err) + gotDeadline, hasDeadline := got.Deadline() + assert.Assert(t, hasDeadline) + assert.Equal(t, gotDeadline, deadline) +} + +// TestApplyRemainingDeadline_PastDeadlineFailsImmediately is a regression +// test for a docker-agent review finding on #14290: a plan whose +// create/start/hook ops alone (unbounded by the wait deadline -- see +// upDetached) take longer than --wait-timeout can still return nil past it. +// Without this check, context.WithDeadline on an already-past deadline +// would silently produce an immediately-canceled context, and the next +// daemon call (getContainers) would fail with a bare context.DeadlineExceeded +// indistinguishable from "health never came up" -- misleading, since health +// was never actually checked. This must fail immediately with a message +// that says so, before any daemon call happens. +func TestApplyRemainingDeadline_PastDeadlineFailsImmediately(t *testing.T) { + _, _, err := applyRemainingDeadline(t.Context(), time.Now().Add(-time.Second), 5*time.Second) + assert.ErrorContains(t, err, "application not healthy after 5s: deadline exceeded during plan execution") +} From e206f0566565c1af0e0a3b1185d001e5a1207075 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 18:09:04 +0200 Subject: [PATCH 09/26] fix(reconcile): a provider's consumer waits for its deployment too 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 --- pkg/compose/reconcile.go | 15 ++++++--- pkg/compose/reconcile_start_test.go | 52 +++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index c460dfff75..e27f7ce738 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -1305,17 +1305,22 @@ func (r *reconciler) waitConditionNode(dep string, cfg types.ServiceDependency, // planProviderStart is the start-phase visit of a provider service: no // container to start, but its own depends_on still applies — the imperative // startService waits on it for every service, providers included. Its chain -// end is those prerequisites, else its create-phase RunProvider node. +// end is its create-phase RunProvider node (the deployment itself), plus +// those dependency prerequisites when it has any -- a consumer must wait for +// both: the provider's own dependencies satisfied AND the provider actually +// deployed, not either in isolation. Before this ordering was corrected, a +// provider with both an upstream dependency and a downstream consumer let +// the dependency wait substitute for the deploy node entirely, so the +// consumer could start while the provider's plugin/relay deployment was +// still running. func (r *reconciler) planProviderStart(service types.ServiceConfig) error { depNodes, err := r.startPhaseDependencies(service) if err != nil { return err } ends := depNodes - if len(ends) == 0 { - if node, ok := r.serviceNodes[service.Name]; ok { - ends = []*PlanNode{node} - } + if node, ok := r.serviceNodes[service.Name]; ok && !slices.Contains(ends, node) { + ends = append(ends, node) } if len(ends) > 0 { r.startChainEnds[service.Name] = ends diff --git a/pkg/compose/reconcile_start_test.go b/pkg/compose/reconcile_start_test.go index 7064197fb2..395d49e960 100644 --- a/pkg/compose/reconcile_start_test.go +++ b/pkg/compose/reconcile_start_test.go @@ -381,6 +381,58 @@ func TestPlanStart_ProviderWaitsOnDependencies(t *testing.T) { assert.Equal(t, wait.Operation.Condition, types.ServiceConditionHealthy) } +// TestPlanStart_ProviderConsumerWaitsForDeployment is a regression test for +// a Copilot review finding on #14290: once ScopeCreateStart became the real, +// exercised path for detached up, planProviderStart's startChainEnds for a +// provider WITH its own depends_on was just that dependency's wait node -- +// dropping the provider's own OpRunProvider (deployment) node entirely. A +// downstream consumer of the provider could therefore start while the +// provider plugin/relay deployment was still running, a barrier the old +// create-then-start two-phase sequence guaranteed for free. +func TestPlanStart_ProviderConsumerWaitsForDeployment(t *testing.T) { + db := types.ServiceConfig{Name: "db"} + db.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} + prov := serviceWithDeps("prov", types.DependsOnConfig{"db": {Condition: types.ServiceConditionHealthy, Required: true}}) + prov.Provider = &types.ServiceProviderConfig{Type: "test"} + consumer := serviceWithDeps("consumer", types.DependsOnConfig{"prov": {Condition: types.ServiceConditionStarted, Required: true}}) + project := &types.Project{ + Name: "myproject", + Services: types.Services{"db": db, "prov": prov, "consumer": consumer}, + } + dbHash, err := serviceHashWithResolvedRefs(db, nil) + assert.NilError(t, err) + consumerHash, err := serviceHashWithResolvedRefs(consumer, nil) + assert.NilError(t, err) + observed := emptyObserved() + observed.Containers["db"] = []ObservedContainer{ + observedServiceContainer("db", 1, container.StateRunning, dbHash), + } + observed.Containers["consumer"] = []ObservedContainer{ + observedServiceContainer("consumer", 1, container.StateExited, consumerHash), + } + + plan, err := reconcile(t.Context(), project, observed, startScopeOptions(ScopeCreateStart), noPrompt) + assert.NilError(t, err) + + var providerNode, consumerStartNode *PlanNode + for _, n := range plan.Nodes { + switch { + case n.Operation.Type == OpRunProvider: + providerNode = n + case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:consumer:1": + consumerStartNode = n + } + } + if providerNode == nil { + t.Fatalf("expected an OpRunProvider node for prov:\n%s", plan) + } + if consumerStartNode == nil { + t.Fatalf("expected a start node for consumer:\n%s", plan) + } + assert.Assert(t, slices.Contains(consumerStartNode.DependsOn, providerNode), + "consumer's start must depend on the provider's own deployment node, not just its depends_on wait:\n%s", plan) +} + // A replica condemned by scale-down must never receive a start-phase node: // the imperative engine only starts what survives the convergence — and the // condemned replica does not count as running for the pre_start gating. From e58a51499804adb0cababc5e5ebacd6dba3c2011 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 18:34:17 +0200 Subject: [PATCH 10/26] fix(up): give each wait its own fresh --wait-timeout window 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 --- pkg/compose/executor.go | 21 ++++--- pkg/compose/executor_start_test.go | 10 ++-- pkg/compose/up.go | 93 +++++++++++------------------- pkg/compose/up_test.go | 37 ------------ 4 files changed, 52 insertions(+), 109 deletions(-) diff --git a/pkg/compose/executor.go b/pkg/compose/executor.go index 86837d667a..fb4859e12f 100644 --- a/pkg/compose/executor.go +++ b/pkg/compose/executor.go @@ -49,13 +49,16 @@ type planExecutor struct { containersMu sync.Mutex containersByService map[string]Containers - // waitDeadline, when non-zero, bounds OpWaitCondition nodes only (see - // executeNode) -- not the rest of the plan. up -d --wait-timeout must - // cap how long a dependency's health/completion condition is waited on, - // the same thing start()'s own WaitTimeout already does today, without - // also capping unrelated create/start/hook work the way wrapping the - // whole run() ctx would (docker/compose#14290 review). - waitDeadline time.Time + // waitTimeout, when non-zero, bounds OpWaitCondition nodes only (see + // executeNode) -- not the rest of the plan, and with its OWN fresh + // window per wait node rather than a single shared deadline counted + // down from before the plan started. up -d --wait-timeout must cap how + // long a dependency's health/completion condition is waited on, the + // same thing start()'s own per-dependency WaitTimeout already does + // today -- without also capping unrelated create/start/hook work, and + // without letting that unrelated work's duration eat into (or exhaust) + // a wait's own budget (docker/compose#14290 review). + waitTimeout time.Duration } // reconciliationContext holds results produced by completed nodes so that downstream @@ -248,9 +251,9 @@ func (exec *planExecutor) executeNode(ctx context.Context, node *PlanNode) error case OpCreateHookContainer: return exec.execCreateHookContainer(ctx, node) case OpWaitCondition: - if !exec.waitDeadline.IsZero() { + if exec.waitTimeout > 0 { var cancel context.CancelFunc - ctx, cancel = context.WithDeadline(ctx, exec.waitDeadline) + ctx, cancel = context.WithTimeout(ctx, exec.waitTimeout) defer cancel() } return exec.execWaitCondition(ctx, op) diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index e331b39cfd..5ae7bc2f72 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -152,7 +152,7 @@ func TestExecWaitCondition_HealthyConditionSatisfied(t *testing.T) { // imperative engine's waitDependencies, which already threaded WaitTimeout // through every per-dependency wait, not just the final --wait check) -- // they rely entirely on whatever ctx they're given (directly, or via -// exec.waitDeadline -- see TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes for +// exec.waitTimeout -- see TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes for // that path specifically). This drives a real OpWaitCondition node // (service_healthy, never satisfied) through the full executePlan/run() DAG // path -- not execWaitCondition in isolation -- under a short deadline on @@ -206,11 +206,11 @@ func TestExecutePlanWaitConditionRespectsContextDeadline(t *testing.T) { // --wait-timeout by wrapping the whole run() ctx, which would have also // capped unrelated Create-phase and Start-phase work (network/container // creation, hooks) that was never subject to --wait-timeout in the old -// create()+start() sequence. exec.waitDeadline (set by upDetached, consulted +// create()+start() sequence. exec.waitTimeout (set by upDetached, consulted // only in executeNode's OpWaitCondition case) must bound exclusively the // blocking dependency wait, leaving every other node on the plan's plain, // deadline-free ctx. This plan pairs a never-satisfied OpWaitCondition with -// an OpCreateNetwork that has no deadline of its own: if waitDeadline leaked +// an OpCreateNetwork that has no deadline of its own: if waitTimeout leaked // into the whole run(), the network create would race the same short // deadline instead of completing normally. func TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes(t *testing.T) { @@ -252,14 +252,14 @@ func TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes(t *testing.T) { exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) exec.containersByService["db"] = Containers{dbSummary} - exec.waitDeadline = time.Now().Add(200 * time.Millisecond) + exec.waitTimeout = 200 * time.Millisecond start := time.Now() err := exec.run(t.Context(), plan) elapsed := time.Since(start) assert.ErrorIs(t, err, context.DeadlineExceeded) - assert.Assert(t, elapsed < 5*time.Second, "waitDeadline did not bound the stuck OpWaitCondition node (took %s)", elapsed) + assert.Assert(t, elapsed < 5*time.Second, "waitTimeout did not bound the stuck OpWaitCondition node (took %s)", elapsed) } // TestExecStartContainer_EnrichedResolvesCreateNodeAndStarts verifies that a diff --git a/pkg/compose/up.go b/pkg/compose/up.go index df17fa309c..c97ae704db 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -87,27 +87,29 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // which already threads WaitTimeout through every wait unconditionally // (InDependencyOrder passes it to startService regardless of // options.Wait -- --wait-timeout alone, with no --wait, is a legal CLI - // combination nothing rejects). exec.waitDeadline bounds exactly those - // nodes (see executeNode) and nothing else in the plan: wrapping the - // whole run() ctx instead would also cap unrelated create/start/hook - // work that was never subject to --wait-timeout in the old - // create()+start() sequence (a slow image pull or container create could - // then fail as "application not healthy" with no health wait involved). + // combination nothing rejects). exec.waitTimeout bounds exactly those + // nodes (see executeNode), each with its OWN fresh window starting when + // that wait begins -- not a single shared budget counted down from + // before the plan even started. Sharing one absolute deadline across + // create/start/hook work and every wait would let slow, unrelated work + // alone exhaust it, failing an otherwise healthy deployment as "not + // healthy" with no wait ever actually timing out -- start()'s real, + // per-wait-independent behavior never had that failure mode. exec := s.newPlanExecutor(project, observed, nil) - var deadline time.Time - if options.Start.WaitTimeout > 0 { - deadline = time.Now().Add(options.Start.WaitTimeout) - exec.waitDeadline = deadline - } + exec.waitTimeout = options.Start.WaitTimeout if err := exec.run(ctx, plan); err != nil { - // The deadline itself is unconditional (set above whenever - // WaitTimeout > 0, regardless of Wait) -- but "application not - // healthy" is specifically a --wait message. Without --wait, a - // user who only set --wait-timeout as a hang guard sees the raw - // error instead of a message implying a health check they never - // asked for. - if options.Start.Wait && errors.Is(err, context.DeadlineExceeded) { + // options.Start.WaitTimeout alone is unconditional (threaded above + // regardless of Wait, matching start()'s own InDependencyOrder) -- + // but "application not healthy" is specifically a --wait message. + // Without --wait, a user who only set --wait-timeout as a hang + // guard sees the raw error instead of a message implying a health + // check they never asked for. And without a WaitTimeout at all, + // exec.waitTimeout is 0 -- a DeadlineExceeded here can only have + // come from ctx's own external deadline, unrelated to + // --wait-timeout, so naming a "0s" duration nobody configured would + // be just as wrong. + if options.Start.Wait && options.Start.WaitTimeout > 0 && errors.Is(err, context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) } return err @@ -120,28 +122,24 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // The plan may have created or recreated containers: this verification // needs their current IDs, which the pre-execution observed snapshot // above doesn't have for a container that didn't exist yet at - // observation time. It shares the same deadline established above - // (when set) rather than opening a fresh WaitTimeout window of its own - // -- one overall budget for "becoming healthy", covering both the - // plan's own waits and this final check, not one budget per wait. - // - // exec.run only bounds OpWaitCondition nodes with that deadline (see - // above): a plan whose create/start/hook ops alone take longer than - // WaitTimeout can still return nil past it. applyRemainingDeadline - // catches that case explicitly instead of deriving an already-expired - // context.WithDeadline, which would fail getContainers below before it - // ever touches the daemon -- "not healthy" when health was never - // actually checked. - var cancel context.CancelFunc - ctx, cancel, err = applyRemainingDeadline(ctx, deadline, options.Start.WaitTimeout) - if err != nil { - return err + // observation time. It opens its own fresh WaitTimeout window, same as + // every OpWaitCondition node above and start()'s own final --wait check + // did -- not whatever's left of a shared budget, which unrelated plan + // work could already have exhausted with no wait ever at risk. + if options.Start.WaitTimeout > 0 { + withTimeout, cancel := context.WithTimeout(ctx, options.Start.WaitTimeout) + defer cancel() + ctx = withTimeout } - defer cancel() containers, err := s.getContainers(ctx, project.Name, oneOffExclude, true) if err != nil { - if !deadline.IsZero() && errors.Is(ctx.Err(), context.DeadlineExceeded) { + // Without a WaitTimeout, ctx above is still whatever the caller + // passed in -- it can carry its own external deadline (an upstream + // command timeout, a cancellation) unrelated to --wait-timeout. + // Translating that into "application not healthy after 0s" would + // name a duration nobody configured. + if options.Start.WaitTimeout > 0 && errors.Is(ctx.Err(), context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) } return err @@ -155,7 +153,7 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, } } if err := s.waitDependencies(ctx, project, project.Name, depends, containers, 0); err != nil { - if errors.Is(ctx.Err(), context.DeadlineExceeded) { + if options.Start.WaitTimeout > 0 && errors.Is(ctx.Err(), context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) } return err @@ -163,27 +161,6 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, return nil } -// applyRemainingDeadline derives ctx bound by deadline, the no-op -// cancel/unmodified ctx when deadline is zero (no --wait-timeout set). -// Deadline may already be in the past -- the plan's create/start/hook ops -// aren't bound by it (see upDetached), so they can run long enough on their -// own to exhaust the whole --wait-timeout budget before this is ever called. -// context.WithDeadline on an already-past deadline would silently produce an -// immediately-canceled context, failing the caller's very next daemon call -// with a bare context.DeadlineExceeded that looks identical to "health never -// came up" -- misleading when health was never actually checked. Returning -// the familiar message directly here, instead, keeps that distinction clear. -func applyRemainingDeadline(ctx context.Context, deadline time.Time, waitTimeout time.Duration) (context.Context, context.CancelFunc, error) { - if deadline.IsZero() { - return ctx, func() {}, nil - } - if time.Until(deadline) <= 0 { - return nil, nil, fmt.Errorf("application not healthy after %s: deadline exceeded during plan execution", waitTimeout) - } - withDeadline, cancel := context.WithDeadline(ctx, deadline) - return withDeadline, cancel, nil -} - // upSession carries the state shared between the goroutines driving an // interactive `compose up` once services are created: the errgroup running // them, the collected errors, and the application exit status. diff --git a/pkg/compose/up_test.go b/pkg/compose/up_test.go index e2763c8fa6..23a5b08682 100644 --- a/pkg/compose/up_test.go +++ b/pkg/compose/up_test.go @@ -228,40 +228,3 @@ func TestStreamContainerLogs_ConcurrencyIsBounded(t *testing.T) { assert.NilError(t, eg.Wait()) assert.Equal(t, tracker.Peak(), 1, "streamContainerLogs must share --parallel's budget for concurrent log-attach opens") } - -// TestApplyRemainingDeadline_ZeroDeadlineIsNoOp covers upDetached's -// --wait-timeout-unset path: no deadline, ctx passed through unchanged. -func TestApplyRemainingDeadline_ZeroDeadlineIsNoOp(t *testing.T) { - got, cancel, err := applyRemainingDeadline(t.Context(), time.Time{}, 0) - defer cancel() - assert.NilError(t, err) - _, hasDeadline := got.Deadline() - assert.Assert(t, !hasDeadline) -} - -// TestApplyRemainingDeadline_FutureDeadlineWraps covers the common case: -// budget remains, ctx comes back bound by it. -func TestApplyRemainingDeadline_FutureDeadlineWraps(t *testing.T) { - deadline := time.Now().Add(time.Minute) - got, cancel, err := applyRemainingDeadline(t.Context(), deadline, 5*time.Second) - defer cancel() - assert.NilError(t, err) - gotDeadline, hasDeadline := got.Deadline() - assert.Assert(t, hasDeadline) - assert.Equal(t, gotDeadline, deadline) -} - -// TestApplyRemainingDeadline_PastDeadlineFailsImmediately is a regression -// test for a docker-agent review finding on #14290: a plan whose -// create/start/hook ops alone (unbounded by the wait deadline -- see -// upDetached) take longer than --wait-timeout can still return nil past it. -// Without this check, context.WithDeadline on an already-past deadline -// would silently produce an immediately-canceled context, and the next -// daemon call (getContainers) would fail with a bare context.DeadlineExceeded -// indistinguishable from "health never came up" -- misleading, since health -// was never actually checked. This must fail immediately with a message -// that says so, before any daemon call happens. -func TestApplyRemainingDeadline_PastDeadlineFailsImmediately(t *testing.T) { - _, _, err := applyRemainingDeadline(t.Context(), time.Now().Add(-time.Second), 5*time.Second) - assert.ErrorContains(t, err, "application not healthy after 5s: deadline exceeded during plan execution") -} From ffcc91e9e790544906013639a6c4e37c1fe3b9c5 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 18:39:43 +0200 Subject: [PATCH 11/26] fix(up): don't blame --wait-timeout for an external ctx deadline 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 --- pkg/compose/up.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/compose/up.go b/pkg/compose/up.go index c97ae704db..1e190e9bc4 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -108,8 +108,12 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // exec.waitTimeout is 0 -- a DeadlineExceeded here can only have // come from ctx's own external deadline, unrelated to // --wait-timeout, so naming a "0s" duration nobody configured would - // be just as wrong. - if options.Start.Wait && options.Start.WaitTimeout > 0 && errors.Is(err, context.DeadlineExceeded) { + // be just as wrong. ctx.Err() == nil additionally rules out ctx + // itself (the caller-supplied, un-derived context) having its own + // independent deadline expire during exec.run -- that DeadlineExceeded + // didn't come from any OpWaitCondition node's own derived timeout + // either, so it isn't a health-readiness failure. + if options.Start.Wait && options.Start.WaitTimeout > 0 && ctx.Err() == nil && errors.Is(err, context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) } return err From cd238333838e3a447526fe21e8c91b95f517910b Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 19:29:26 +0200 Subject: [PATCH 12/26] fix(executor): scope plan cancellation to each node's own phase Copilot review (High) on #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 --- pkg/compose/executor.go | 164 ++++++++++++++++++++--------- pkg/compose/executor_start_test.go | 134 +++++++++++++++++++++++ pkg/compose/service_containers.go | 12 ++- pkg/compose/up.go | 12 ++- 4 files changed, 268 insertions(+), 54 deletions(-) diff --git a/pkg/compose/executor.go b/pkg/compose/executor.go index fb4859e12f..3729be5cd9 100644 --- a/pkg/compose/executor.go +++ b/pkg/compose/executor.go @@ -24,6 +24,7 @@ import ( "github.com/compose-spec/compose-go/v2/types" "golang.org/x/sync/errgroup" + "golang.org/x/sync/semaphore" "github.com/docker/compose/v5/pkg/api" ) @@ -159,67 +160,128 @@ func (exec *planExecutor) run(ctx context.Context, plan *Plan) error { groups := exec.buildGroupTracker(plan) events := exec.compose.events - // Every node's goroutine is dispatched unconditionally, so one waiting on - // a dependency never occupies a concurrency slot -- only the actual - // executeNode call does, via the semaphore below. This also means - // deadlock-freedom no longer depends on plan.Nodes staying topologically - // sorted: a goroutine blocked on <-done[dep.ID] holds no slot for a - // still-pending dependency to be starved on. - eg, ctx := errgroup.WithContext(ctx) - limiter := newOptionalLimiter(exec.compose.maxConcurrency) + // Cancellation is phase-scoped, not plan-wide: createCtx is canceled by a + // Create-phase node failing; startCtx, derived from it, is ALSO canceled + // by a Start-phase node failing. A Start-phase failure (an OpWaitCondition + // hitting --wait-timeout, a container failing to start, anything) must + // never cancel createCtx -- doing so would abort unrelated, still + // in-flight Create-phase work (a sibling service's container create, a + // network create) that has nothing to do with it. The old + // create()-then-start() sequence never had this failure mode: the whole + // create phase had always finished before any dependency wait, or + // anything else in the start phase, even began. A Create-phase failure + // still cancels the Start phase too, via createCtx's cancellation + // propagating to the startCtx derived from it -- matching the old + // sequence, where start() was never even called once create() failed. + createCtx, cancelCreateCtx := context.WithCancel(ctx) + defer cancelCreateCtx() + startCtx, cancelStartCtx := context.WithCancel(createCtx) + defer cancelStartCtx() + + rs := &runState{ + done: done, + recordFailure: recordFailure, + failedDependency: failedDependency, + groups: groups, + events: events, + limiter: newOptionalLimiter(exec.compose.maxConcurrency), + createCtx: createCtx, + cancelCreateCtx: cancelCreateCtx, + startCtx: startCtx, + cancelStartCtx: cancelStartCtx, + } + eg := errgroup.Group{} for _, node := range plan.Nodes { eg.Go(func() error { - // Wait for all dependencies - for _, dep := range node.DependsOn { - select { - case <-done[dep.ID]: - case <-ctx.Done(): - return ctx.Err() - } - } + return exec.runNode(node, rs) + }) + } - // A dependency may have just failed without ctx being canceled - // yet (see failures' comment above) -- skip this node exactly - // like a canceled one: no slot acquired, no event emitted, its - // own dependents unblocked with the same failure recorded. - if err := failedDependency(node.DependsOn); err != nil { - // Store the original error, not a wrapped one: this node's own - // dependents look it up the same way, and wrapping it again at - // every hop would compound "dependency failed: " prefixes down - // a multi-node chain (create -> pre_start -> start -> post_start). - recordFailure(node.ID, err) - close(done[node.ID]) - return fmt.Errorf("dependency failed: %w", err) - } + return eg.Wait() +} - if err := acquireSlot(ctx, limiter); err != nil { - recordFailure(node.ID, err) - close(done[node.ID]) - return err - } - defer releaseSlot(limiter) +// runState carries the per-run() state every node's goroutine shares: the +// done-channel per node, dependency-failure bookkeeping, group event +// tracking, the concurrency limiter, and the phase-scoped cancellation a +// node joins depending on its own Phase. Split out of run() purely to keep +// runNode a plain method instead of a closure captured in a loop. +type runState struct { + done map[int]chan struct{} + recordFailure func(id int, err error) + failedDependency func(deps []*PlanNode) error + groups *groupTracker + events api.EventProcessor + limiter *semaphore.Weighted + createCtx context.Context + cancelCreateCtx context.CancelFunc + startCtx context.Context + cancelStartCtx context.CancelFunc +} - // Emit group start event if this is the first node of a group - groups.onNodeStart(node, events) +// runNode executes one plan node: waits for its dependencies, skips it if +// one of them failed, then dispatches it and records the outcome. See run() +// for the phase-scoped cancellation (createCtx/startCtx) this draws from. +func (exec *planExecutor) runNode(node *PlanNode, rs *runState) error { + nodeCtx, cancelPhase := rs.createCtx, rs.cancelCreateCtx + if node.Phase == PhaseStart { + nodeCtx, cancelPhase = rs.startCtx, rs.cancelStartCtx + } - err := exec.executeNode(ctx, node) + // Wait for all dependencies + for _, dep := range node.DependsOn { + select { + case <-rs.done[dep.ID]: + case <-nodeCtx.Done(): + return nodeCtx.Err() + } + } - if err == nil { - // Emit group done event if this is the last node of a group - groups.onNodeDone(node, events) - } else { - recordFailure(node.ID, err) - if ctx.Err() == nil { - groups.onNodeError(node, events, err) - } - } + // A dependency may have just failed without nodeCtx being canceled yet + // (see run()'s failures comment) -- skip this node exactly like a + // canceled one: no slot acquired, no event emitted, its own dependents + // unblocked with the same failure recorded. + if err := rs.failedDependency(node.DependsOn); err != nil { + // Store the original error, not a wrapped one: this node's own + // dependents look it up the same way, and wrapping it again at + // every hop would compound "dependency failed: " prefixes down a + // multi-node chain (create -> pre_start -> start -> post_start). + rs.recordFailure(node.ID, err) + close(rs.done[node.ID]) + return fmt.Errorf("dependency failed: %w", err) + } - close(done[node.ID]) - return err - }) + if err := acquireSlot(nodeCtx, rs.limiter); err != nil { + rs.recordFailure(node.ID, err) + close(rs.done[node.ID]) + return err } + defer releaseSlot(rs.limiter) - return eg.Wait() + // Emit group start event if this is the first node of a group + rs.groups.onNodeStart(node, rs.events) + + err := exec.executeNode(nodeCtx, node) + + if err == nil { + // Emit group done event if this is the last node of a group + rs.groups.onNodeDone(node, rs.events) + } else { + rs.recordFailure(node.ID, err) + if nodeCtx.Err() == nil { + rs.groups.onNodeError(node, rs.events, err) + // This node's own failure, not an inherited cancellation: + // propagate it to this node's own phase (and, if it's a + // Create-phase node, transitively to Start via startCtx's + // derivation) -- never the other way around. Gated the same as + // the event above: a node whose nodeCtx is already canceled + // failed because of that cancellation, not a new failure of its + // own to propagate. + cancelPhase() + } + } + + close(rs.done[node.ID]) + return err } // executeNode dispatches a single plan node to the appropriate API call. diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index 5ae7bc2f72..d07a24a822 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -20,6 +20,7 @@ package compose import ( "context" + "errors" "net" "sync" "testing" @@ -262,6 +263,139 @@ func TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes(t *testing.T) { assert.Assert(t, elapsed < 5*time.Second, "waitTimeout did not bound the stuck OpWaitCondition node (took %s)", elapsed) } +// TestExecutePlanStartPhaseFailureDoesNotCancelCreatePhase is a regression +// test for a Copilot review finding on #14290: run() used to dispatch every +// node under one shared errgroup.WithContext, so a Start-phase node failing +// (a required OpWaitCondition hitting its own waitTimeout, here) canceled +// ctx for every other goroutine still in flight -- including a completely +// unrelated Create-phase node (a sibling service's network create) that has +// nothing to do with the dependency that timed out. The old +// create()-then-start() sequence never had this failure mode: the whole +// create phase had always finished before any dependency wait began. This +// pairs a never-satisfied required wait (Phase: PhaseStart, matching what +// planStartPhase actually produces) with a network create (Phase: +// PhaseCreate, the default) deliberately slower than the wait's timeout, and +// asserts the network create's own context is never canceled -- it must +// complete normally, on its own schedule, even though the sibling wait +// already failed and the plan as a whole still fails too. +func TestExecutePlanStartPhaseFailureDoesNotCancelCreatePhase(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + + dbSummary := container.Summary{ + ID: "db-id", + Names: []string{"/test-db-1"}, + Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"}, + } + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(client.ContainerInspectResult{ + Container: container.InspectResponse{ + ID: "db-id", + Name: "/test-db-1", + State: &container.State{ + Status: container.StateRunning, + Health: &container.Health{Status: container.Starting}, + }, + Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}}, + }, + }, nil).AnyTimes() + + nw := types.NetworkConfig{Name: "test_default"} + var networkCreateCtxErr error + networkCreateDone := make(chan struct{}) + apiClient.EXPECT().NetworkCreate(gomock.Any(), "test_default", gomock.Any()). + DoAndReturn(func(ctx context.Context, _ string, _ client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + defer close(networkCreateDone) + // Deliberately outlives the wait's timeout below: if the + // Create-phase ctx were canceled by the sibling Start-phase wait + // failing, this ctx would already be done well before this + // returns. + time.Sleep(300 * time.Millisecond) + networkCreateCtxErr = ctx.Err() + return client.NetworkCreateResult{ID: "net-id"}, nil + }) + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpCreateNetwork, + ResourceID: "network:default", + Name: nw.Name, + Network: &nw, + }, "") + wait := plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: "wait:db:service_healthy", + Name: "db", + Condition: types.ServiceConditionHealthy, + }, "") + wait.Phase = PhaseStart + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + exec.containersByService["db"] = Containers{dbSummary} + exec.waitTimeout = 100 * time.Millisecond + + err := exec.run(t.Context(), plan) + assert.ErrorIs(t, err, context.DeadlineExceeded) + + select { + case <-networkCreateDone: + case <-time.After(5 * time.Second): + t.Fatal("NetworkCreate never completed -- its context was likely canceled by the sibling Start-phase wait timing out") + } + assert.NilError(t, networkCreateCtxErr, "NetworkCreate's own ctx must not be canceled by an unrelated Start-phase failure") +} + +// TestExecutePlanCreatePhaseFailureCancelsCreatePhase is the mirror image of +// TestExecutePlanStartPhaseFailureDoesNotCancelCreatePhase: phase-scoped +// cancellation must not weaken the Create phase's own existing fail-fast +// behavior -- two independent (no DependsOn edge) Create-phase nodes, one +// failing immediately, must still cancel the other's in-flight work. Both +// nodes default to Phase: PhaseCreate (addNode's zero value), so they share +// createCtx. +func TestExecutePlanCreatePhaseFailureCancelsCreatePhase(t *testing.T) { + svc, apiClient := newTestService(t) + + apiClient.EXPECT().NetworkCreate(gomock.Any(), "test_default", gomock.Any()). + Return(client.NetworkCreateResult{}, errors.New("boom")) + + var volumeCreateCtxErr error + volumeCreateDone := make(chan struct{}) + apiClient.EXPECT().VolumeCreate(gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, _ client.VolumeCreateOptions) (client.VolumeCreateResult, error) { + defer close(volumeCreateDone) + <-ctx.Done() + volumeCreateCtxErr = ctx.Err() + return client.VolumeCreateResult{}, ctx.Err() + }) + + nw := types.NetworkConfig{Name: "test_default"} + vol := types.VolumeConfig{Name: "data", Driver: "local"} + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpCreateNetwork, + ResourceID: "network:default", + Name: nw.Name, + Network: &nw, + }, "") + plan.addNode(Operation{ + Type: OpCreateVolume, + ResourceID: "volume:data", + Name: vol.Name, + Volume: &vol, + }, "") + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + + err := exec.run(t.Context(), plan) + assert.ErrorContains(t, err, "boom") + + select { + case <-volumeCreateDone: + case <-time.After(5 * time.Second): + t.Fatal("VolumeCreate's ctx was never canceled -- a sibling Create-phase failure should still fail fast") + } + assert.ErrorIs(t, volumeCreateCtxErr, context.Canceled, "VolumeCreate's ctx must be canceled by a sibling Create-phase node failing") +} + // TestExecStartContainer_EnrichedResolvesCreateNodeAndStarts verifies that a // start-phase OpStartContainer (Service set) resolves its target through the // CreateContainer node it references — the same reconciliationContext diff --git a/pkg/compose/service_containers.go b/pkg/compose/service_containers.go index 0ac19ee9b7..dda9b3d135 100644 --- a/pkg/compose/service_containers.go +++ b/pkg/compose/service_containers.go @@ -205,8 +205,18 @@ func (s *composeService) waitDependency(ctx context.Context, dependant, dep stri case <-ticker.C: case <-ctx.Done(): // An expired deadline is precisely the failure this wait is meant - // to detect; only a plain cancellation (Ctrl-C) stays silent. + // to detect for a required dependency; only a plain cancellation + // (Ctrl-C) stays silent either way. An optional dependency + // tolerates a timeout exactly like it tolerates any other + // definitive failure the check* functions below report — + // skipped, not aborted. if errors.Is(ctx.Err(), context.DeadlineExceeded) { + if !config.Required { + s.events.On(containerReasonEvents(waitingFor, skippedEvent, + fmt.Sprintf("optional dependency %q did not satisfy %q before the timeout", dep, config.Condition))...) + logrus.Warnf("optional dependency %q did not satisfy %q before the timeout", dep, config.Condition) + return nil + } return ctx.Err() } return nil diff --git a/pkg/compose/up.go b/pkg/compose/up.go index 1e190e9bc4..f94c2b9e8a 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -130,6 +130,14 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // every OpWaitCondition node above and start()'s own final --wait check // did -- not whatever's left of a shared budget, which unrelated plan // work could already have exhausted with no wait ever at risk. + // + // origCtx is kept so the two DeadlineExceeded checks below can tell this + // fresh window expiring (origCtx still fine) apart from origCtx's own, + // independent deadline propagating through the derived one (origCtx + // already done): a parent deadline firing first makes the derived + // context's Err() report DeadlineExceeded too, inherited from the + // parent, even though this window's own timer never fired. + origCtx := ctx if options.Start.WaitTimeout > 0 { withTimeout, cancel := context.WithTimeout(ctx, options.Start.WaitTimeout) defer cancel() @@ -143,7 +151,7 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // command timeout, a cancellation) unrelated to --wait-timeout. // Translating that into "application not healthy after 0s" would // name a duration nobody configured. - if options.Start.WaitTimeout > 0 && errors.Is(ctx.Err(), context.DeadlineExceeded) { + if options.Start.WaitTimeout > 0 && origCtx.Err() == nil && errors.Is(ctx.Err(), context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) } return err @@ -157,7 +165,7 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, } } if err := s.waitDependencies(ctx, project, project.Name, depends, containers, 0); err != nil { - if options.Start.WaitTimeout > 0 && errors.Is(ctx.Err(), context.DeadlineExceeded) { + if options.Start.WaitTimeout > 0 && origCtx.Err() == nil && errors.Is(ctx.Err(), context.DeadlineExceeded) { return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) } return err From 374079a321bc2553e4cc6ff0c06cd28feaa78669 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 19:54:33 +0200 Subject: [PATCH 13/26] fix(reconcile): a restarted service's consumer waits for the restart too Copilot review ("previously missed"): the same either/or bug just fixed for planProviderStart (e206f0566) 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 --- pkg/compose/reconcile.go | 13 ++++--- pkg/compose/reconcile_start_test.go | 57 +++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index e27f7ce738..968f739fac 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -1354,12 +1354,15 @@ func (r *reconciler) planServiceStart(service types.ServiceConfig) error { } if len(replicas) == 0 { // the visit still happened: dependents order after its prerequisites - // (waits, service_started edges), else after the create phase + // (waits, service_started edges) AND after the create phase's own + // exceptional-state restart node when it planned one (anyRunning via + // a bare OpStartContainer, same reasoning as planProviderStart) -- + // not either/or: a consumer must wait for both the dependency and + // the restart actually happening, or it can start before a + // paused/dead service's only container is running again. ends := depNodes - if len(ends) == 0 { - if node, ok := r.serviceNodes[service.Name]; ok { - ends = []*PlanNode{node} - } + if node, ok := r.serviceNodes[service.Name]; ok && !slices.Contains(ends, node) { + ends = append(ends, node) } if len(ends) > 0 { r.startChainEnds[service.Name] = ends diff --git a/pkg/compose/reconcile_start_test.go b/pkg/compose/reconcile_start_test.go index 395d49e960..8c20f8695f 100644 --- a/pkg/compose/reconcile_start_test.go +++ b/pkg/compose/reconcile_start_test.go @@ -433,6 +433,63 @@ func TestPlanStart_ProviderConsumerWaitsForDeployment(t *testing.T) { "consumer's start must depend on the provider's own deployment node, not just its depends_on wait:\n%s", plan) } +// TestPlanStart_ExceptionalStateConsumerWaitsForRestart is the sibling +// regression to TestPlanStart_ProviderConsumerWaitsForDeployment for an +// ordinary (non-provider) service: the same either/or bug in +// planServiceStart's len(replicas) == 0 branch dropped the service's own +// create-phase bare-restart node (TestPlanStart_ExceptionalStateReplicaCountsAsRunning) +// from startChainEnds whenever the service also had its own depends_on, +// keeping only the dependency wait. A downstream service_started consumer +// could then start before a paused/dead service's only container was +// actually running again. +func TestPlanStart_ExceptionalStateConsumerWaitsForRestart(t *testing.T) { + db := types.ServiceConfig{Name: "db"} + db.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} + app := serviceWithDeps("app", types.DependsOnConfig{"db": {Condition: types.ServiceConditionHealthy, Required: true}}) + consumer := serviceWithDeps("consumer", types.DependsOnConfig{"app": {Condition: types.ServiceConditionStarted, Required: true}}) + project := &types.Project{ + Name: "myproject", + Services: types.Services{"db": db, "app": app, "consumer": consumer}, + } + dbHash, err := serviceHashWithResolvedRefs(db, nil) + assert.NilError(t, err) + appHash, err := serviceHashWithResolvedRefs(app, nil) + assert.NilError(t, err) + consumerHash, err := serviceHashWithResolvedRefs(consumer, nil) + assert.NilError(t, err) + observed := emptyObserved() + observed.Containers["db"] = []ObservedContainer{ + observedServiceContainer("db", 1, container.StateRunning, dbHash), + } + observed.Containers["app"] = []ObservedContainer{ + observedServiceContainer("app", 1, container.StatePaused, appHash), + } + observed.Containers["consumer"] = []ObservedContainer{ + observedServiceContainer("consumer", 1, container.StateExited, consumerHash), + } + + plan, err := reconcile(t.Context(), project, observed, startScopeOptions(ScopeCreateStart), noPrompt) + assert.NilError(t, err) + + var appRestartNode, consumerStartNode *PlanNode + for _, n := range plan.Nodes { + switch { + case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:app:1": + appRestartNode = n + case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:consumer:1": + consumerStartNode = n + } + } + if appRestartNode == nil { + t.Fatalf("expected the create-phase bare-restart node for app:\n%s", plan) + } + if consumerStartNode == nil { + t.Fatalf("expected a start node for consumer:\n%s", plan) + } + assert.Assert(t, slices.Contains(consumerStartNode.DependsOn, appRestartNode), + "consumer's start must depend on app's own restart node, not just its depends_on wait:\n%s", plan) +} + // A replica condemned by scale-down must never receive a start-phase node: // the imperative engine only starts what survives the convergence — and the // condemned replica does not count as running for the pre_start gating. From b9efd1e0477da6660f645a0cd6c19f051cbafdba Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 20:05:36 +0200 Subject: [PATCH 14/26] fix(executor): make phase cancellation race-free end to end 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 --- pkg/compose/executor.go | 150 ++++++++++++++++++++--------- pkg/compose/executor_start_test.go | 53 ++++++++++ 2 files changed, 158 insertions(+), 45 deletions(-) diff --git a/pkg/compose/executor.go b/pkg/compose/executor.go index 3729be5cd9..fea00a4d2c 100644 --- a/pkg/compose/executor.go +++ b/pkg/compose/executor.go @@ -160,23 +160,24 @@ func (exec *planExecutor) run(ctx context.Context, plan *Plan) error { groups := exec.buildGroupTracker(plan) events := exec.compose.events - // Cancellation is phase-scoped, not plan-wide: createCtx is canceled by a - // Create-phase node failing; startCtx, derived from it, is ALSO canceled - // by a Start-phase node failing. A Start-phase failure (an OpWaitCondition - // hitting --wait-timeout, a container failing to start, anything) must - // never cancel createCtx -- doing so would abort unrelated, still - // in-flight Create-phase work (a sibling service's container create, a - // network create) that has nothing to do with it. The old - // create()-then-start() sequence never had this failure mode: the whole - // create phase had always finished before any dependency wait, or - // anything else in the start phase, even began. A Create-phase failure - // still cancels the Start phase too, via createCtx's cancellation - // propagating to the startCtx derived from it -- matching the old - // sequence, where start() was never even called once create() failed. - createCtx, cancelCreateCtx := context.WithCancel(ctx) - defer cancelCreateCtx() - startCtx, cancelStartCtx := context.WithCancel(createCtx) - defer cancelStartCtx() + // Cancellation is phase-scoped, not plan-wide: the Create phase is + // canceled by a Create-phase node failing; the Start phase, derived from + // it, is ALSO canceled by a Start-phase node failing. A Start-phase + // failure (an OpWaitCondition hitting --wait-timeout, a container failing + // to start, anything) must never cancel the Create phase -- doing so + // would abort unrelated, still in-flight Create-phase work (a sibling + // service's container create, a network create) that has nothing to do + // with it. The old create()-then-start() sequence never had this failure + // mode: the whole create phase had always finished before any dependency + // wait, or anything else in the start phase, even began. A Create-phase + // failure still cancels the Start phase too, via the Create phase's + // cancellation propagating to the Start phase derived from it -- + // matching the old sequence, where start() was never even called once + // create() failed. + createPhase := newPhaseCancel(ctx) + defer createPhase.cancel() + startPhase := newPhaseCancel(createPhase.ctx) + defer startPhase.cancel() rs := &runState{ done: done, @@ -185,10 +186,8 @@ func (exec *planExecutor) run(ctx context.Context, plan *Plan) error { groups: groups, events: events, limiter: newOptionalLimiter(exec.compose.maxConcurrency), - createCtx: createCtx, - cancelCreateCtx: cancelCreateCtx, - startCtx: startCtx, - cancelStartCtx: cancelStartCtx, + createPhase: createPhase, + startPhase: startPhase, } eg := errgroup.Group{} for _, node := range plan.Nodes { @@ -197,7 +196,63 @@ func (exec *planExecutor) run(ctx context.Context, plan *Plan) error { }) } - return eg.Wait() + err := eg.Wait() + // A phase's own recorded error -- set by whichever node first triggered + // that phase's cancellation (see phaseCancel.fail) -- is authoritative + // over whatever eg.Wait() itself picked: once cancellation starts + // unblocking siblings, more than one goroutine can be mid-return at the + // same time, and errgroup.Group's own first-error-wins race has no idea + // which of them was the actual cause versus just a cancellation casualty + // (a sibling's <-nodeCtx.Done() bail returning a bare context.Canceled + // that reveals nothing). Create takes precedence over Start: a Create + // failure is the more fundamental one of the two (it's what canceled + // Start in the first place, transitively, when both end up set). + if createPhase.err != nil { + return createPhase.err + } + if startPhase.err != nil { + return startPhase.err + } + return err +} + +// phaseCancel bundles one phase's cancellation context with the bookkeeping +// needed to make exactly one node's failure authoritative for it: fail is +// called by every node in this phase that errors out, but its body -- the +// event emission and the actual ctx cancellation -- runs for only the first +// caller, via once. That single gate point fixes two related races a plain +// "if nodeCtx.Err() == nil" check can't: two genuinely concurrent failures +// in the same phase racing to emit onNodeError (the old errgroup.WithContext +// made this vanishingly unlikely by canceling ctx atomically with recording +// the error; splitting cancellation into our own explicit call reopened the +// window), and errgroup.Group's own first-error-wins race picking a +// cancellation casualty's bare context.Canceled over the actual originating +// error once cancellation starts unblocking siblings (see run()'s use of +// err, set here, instead of eg.Wait()'s own return value). +type phaseCancel struct { + ctx context.Context + cancel context.CancelFunc + once sync.Once + err error +} + +func newPhaseCancel(parent context.Context) *phaseCancel { + p := &phaseCancel{} + p.ctx, p.cancel = context.WithCancel(parent) + return p +} + +// fail registers node's failure as this phase's cause, for the first caller +// only: it emits the group error event and cancels the phase's context. Any +// later caller's err is silently dropped -- by the time it could run, this +// phase is already canceled, so it is never anything but a cancellation +// casualty of the first failure, not a new one of its own. +func (p *phaseCancel) fail(node *PlanNode, events api.EventProcessor, groups *groupTracker, err error) { + p.once.Do(func() { + p.err = err + groups.onNodeError(node, events, err) + p.cancel() + }) } // runState carries the per-run() state every node's goroutine shares: the @@ -212,20 +267,29 @@ type runState struct { groups *groupTracker events api.EventProcessor limiter *semaphore.Weighted - createCtx context.Context - cancelCreateCtx context.CancelFunc - startCtx context.Context - cancelStartCtx context.CancelFunc + createPhase *phaseCancel + startPhase *phaseCancel } // runNode executes one plan node: waits for its dependencies, skips it if // one of them failed, then dispatches it and records the outcome. See run() -// for the phase-scoped cancellation (createCtx/startCtx) this draws from. +// for the phase-scoped cancellation (createPhase/startPhase) this draws +// from. func (exec *planExecutor) runNode(node *PlanNode, rs *runState) error { - nodeCtx, cancelPhase := rs.createCtx, rs.cancelCreateCtx + phase := rs.createPhase if node.Phase == PhaseStart { - nodeCtx, cancelPhase = rs.startCtx, rs.cancelStartCtx + phase = rs.startPhase } + nodeCtx := phase.ctx + + // Every exit path below must close rs.done[node.ID] exactly once, or a + // dependent blocks on it forever: deferred once here instead of before + // each individual return, so a future added exit path can't reintroduce + // the gap the original dependency-wait early-return had (no reachable + // hang today -- every dependent in the same phase shares this same + // nodeCtx, already canceled by the time it would matter -- but nothing + // enforces that staying true as the plan DAG grows new shapes). + defer close(rs.done[node.ID]) // Wait for all dependencies for _, dep := range node.DependsOn { @@ -246,13 +310,11 @@ func (exec *planExecutor) runNode(node *PlanNode, rs *runState) error { // every hop would compound "dependency failed: " prefixes down a // multi-node chain (create -> pre_start -> start -> post_start). rs.recordFailure(node.ID, err) - close(rs.done[node.ID]) return fmt.Errorf("dependency failed: %w", err) } if err := acquireSlot(nodeCtx, rs.limiter); err != nil { rs.recordFailure(node.ID, err) - close(rs.done[node.ID]) return err } defer releaseSlot(rs.limiter) @@ -265,22 +327,20 @@ func (exec *planExecutor) runNode(node *PlanNode, rs *runState) error { if err == nil { // Emit group done event if this is the last node of a group rs.groups.onNodeDone(node, rs.events) - } else { - rs.recordFailure(node.ID, err) - if nodeCtx.Err() == nil { - rs.groups.onNodeError(node, rs.events, err) - // This node's own failure, not an inherited cancellation: - // propagate it to this node's own phase (and, if it's a - // Create-phase node, transitively to Start via startCtx's - // derivation) -- never the other way around. Gated the same as - // the event above: a node whose nodeCtx is already canceled - // failed because of that cancellation, not a new failure of its - // own to propagate. - cancelPhase() - } + return nil } - close(rs.done[node.ID]) + rs.recordFailure(node.ID, err) + if nodeCtx.Err() == nil { + // This node's own failure, not an inherited cancellation: a node + // whose nodeCtx is already canceled failed because of that + // cancellation, not a new failure of its own to propagate. phase.fail + // itself gates on being the first such failure in this phase (see its + // doc comment) -- this outer check only spares every later, + // cancellation-casualty caller the cost of building the error-event + // payload it would be dropped anyway. + phase.fail(node, rs.events, rs.groups, err) + } return err } diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index d07a24a822..294b6cc118 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -21,6 +21,7 @@ package compose import ( "context" "errors" + "fmt" "net" "sync" "testing" @@ -396,6 +397,58 @@ func TestExecutePlanCreatePhaseFailureCancelsCreatePhase(t *testing.T) { assert.ErrorIs(t, volumeCreateCtxErr, context.Canceled, "VolumeCreate's ctx must be canceled by a sibling Create-phase node failing") } +// TestExecutePlanConcurrentPhaseFailureKeepsOriginatingError exercises a +// Copilot/docker-agent finding on the phase-scoped cancellation fix: +// errgroup.Group's plain Wait() returns whichever goroutine's error reaches +// its internal sync.Once first, with no idea which one actually caused the +// others to fail -- a sibling unblocked by a phase's cancellation and +// returning a bare context.Canceled of its own could in principle win that +// race over the originating node's real error. phaseCancel's own sync.Once +// records the first failure explicitly, and run() prefers it over +// eg.Wait()'s own pick, closing that window regardless of goroutine +// scheduling. This test runs the scenario under real concurrency (one +// immediately-failing node alongside many siblings unblocked by its +// cancellation, via dependency-wait on a PlanNode ID never added to the +// plan -- rs.done[dep.ID] is then a nil map entry, so the only way out of +// the select is <-nodeCtx.Done(), firing the instant the phase cancels) and +// asserts the real error always surfaces. Note: in practice the Go runtime +// never actually lets a sibling win this race in this in-process test -- +// the originating goroutine keeps running uninterrupted through its own +// short return path before the scheduler gets around to any of the +// newly-runnable siblings -- so reverting the fix does not make this test +// fail; its value is exercising the mechanism under load, not proving the +// fix by ablation. The fix's correctness instead rests on reading +// errgroup's own errOnce.Do source directly (golang.org/x/sync/errgroup), +// confirmed independently by two reviewers. +func TestExecutePlanConcurrentPhaseFailureKeepsOriginatingError(t *testing.T) { + svc, _ := newTestService(t) + + plan := &Plan{} + fail := plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: "wait:missing:service_healthy", + Name: "missing", // no registered containers under this name -> immediate failure + Condition: types.ServiceConditionHealthy, + }, "") + fail.Phase = PhaseStart + + phantom := &PlanNode{ID: -1} // never added to plan.Nodes; its done-channel is never created + const siblings = 30 + for i := range siblings { + blocked := plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: fmt.Sprintf("wait:phantom:%d", i), + Name: "phantom", + }, "", phantom) + blocked.Phase = PhaseStart + } + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + + err := exec.run(t.Context(), plan) + assert.ErrorContains(t, err, "missing dependency missing") +} + // TestExecStartContainer_EnrichedResolvesCreateNodeAndStarts verifies that a // start-phase OpStartContainer (Service set) resolves its target through the // CreateContainer node it references — the same reconciliationContext From db1922b8fb652b1b185e39f0028c45c4f8b80f7f Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 20:25:43 +0200 Subject: [PATCH 15/26] fix(reconcile): include every exceptional-state restart node, not just the last Copilot review, following up on 374079a32'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 --- pkg/compose/executor_ops.go | 13 ++++--- pkg/compose/reconcile.go | 45 +++++++++++++++++---- pkg/compose/reconcile_start_test.go | 54 ++++++++++++++++++++++++++ pkg/compose/service_containers_test.go | 13 +++++++ 4 files changed, 111 insertions(+), 14 deletions(-) diff --git a/pkg/compose/executor_ops.go b/pkg/compose/executor_ops.go index 9c999e711a..ff3791f657 100644 --- a/pkg/compose/executor_ops.go +++ b/pkg/compose/executor_ops.go @@ -219,12 +219,13 @@ func (exec *planExecutor) resolveContainerSummary(op Operation) (container.Summa // warning, not a plan failure — matching waitDependencies' own // optional-dependency handling. // -// Unlike waitDependencies, this applies no deadline of its own: nothing -// produces one yet (no ReconcileOptions field feeds a per-wait timeout the -// way api.CreateOptions.WaitTimeout does today). A future caller needing that -// — e.g. `up --wait` once it runs on the plan — wraps ctx before executing -// the plan, or adds a Timeout to the operation for execWaitCondition to wrap -// here. +// Unlike waitDependencies, this has no timeout of its own to apply here -- +// ctx is used exactly as given. upDetached (up.go) is the caller that needs +// one: planExecutor.waitTimeout gives this node its own fresh per-wait +// window, derived in executeNode's OpWaitCondition dispatch rather than +// here, the same thing start()'s own WaitTimeout already threads through +// every waitDependencies call -- without also bounding unrelated Create or +// Start work the way wrapping the whole plan's ctx would. func (exec *planExecutor) execWaitCondition(ctx context.Context, op Operation) error { s := exec.compose exec.containersMu.Lock() diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index 968f739fac..af70e484de 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -1172,6 +1172,30 @@ func (r *reconciler) setContainerNode(service string, number int, node *PlanNode r.containerNodes[service][number] = node } +// exceptionalStateRestartNodes returns every create-phase bare-restart node +// (OpStartContainer) planned for this service's exceptional-state (paused, +// dead) replicas, in deterministic replica-number order. serviceNodes keeps +// only the LAST node processed for a service's create-phase convergence -- +// correct for a single replica, but a scaled service with more than one +// exceptional-state replica needs every one of them in startChainEnds, or a +// service_started consumer could run while another replica's restart is +// still pending. +func (r *reconciler) exceptionalStateRestartNodes(service string) []*PlanNode { + planned := r.containerNodes[service] + numbers := make([]int, 0, len(planned)) + for number, node := range planned { + if node.Operation.Type == OpStartContainer { + numbers = append(numbers, number) + } + } + sort.Ints(numbers) + nodes := make([]*PlanNode, len(numbers)) + for i, number := range numbers { + nodes[i] = planned[number] + } + return nodes +} + // startPhaseReplicas collects the replicas to start, ascending number: // containers materialized by the create phase plus observed up-to-date // containers not running — exactly the isNotRunning set the imperative start @@ -1354,15 +1378,20 @@ func (r *reconciler) planServiceStart(service types.ServiceConfig) error { } if len(replicas) == 0 { // the visit still happened: dependents order after its prerequisites - // (waits, service_started edges) AND after the create phase's own - // exceptional-state restart node when it planned one (anyRunning via - // a bare OpStartContainer, same reasoning as planProviderStart) -- - // not either/or: a consumer must wait for both the dependency and - // the restart actually happening, or it can start before a - // paused/dead service's only container is running again. + // (waits, service_started edges) AND after every create phase's own + // exceptional-state restart node (anyRunning via a bare + // OpStartContainer, same reasoning as planProviderStart) -- not + // either/or: a consumer must wait for both the dependency and every + // such restart actually happening, or it can start before a + // paused/dead replica's container is running again. A scaled + // service can have more than one exceptional-state replica, so this + // collects all of them (exceptionalStateRestartNodes), not just the + // single last node serviceNodes retains. ends := depNodes - if node, ok := r.serviceNodes[service.Name]; ok && !slices.Contains(ends, node) { - ends = append(ends, node) + for _, node := range r.exceptionalStateRestartNodes(service.Name) { + if !slices.Contains(ends, node) { + ends = append(ends, node) + } } if len(ends) > 0 { r.startChainEnds[service.Name] = ends diff --git a/pkg/compose/reconcile_start_test.go b/pkg/compose/reconcile_start_test.go index 8c20f8695f..82b243a603 100644 --- a/pkg/compose/reconcile_start_test.go +++ b/pkg/compose/reconcile_start_test.go @@ -490,6 +490,60 @@ func TestPlanStart_ExceptionalStateConsumerWaitsForRestart(t *testing.T) { "consumer's start must depend on app's own restart node, not just its depends_on wait:\n%s", plan) } +// TestPlanStart_ScaledExceptionalStateConsumerWaitsForEveryRestart is a +// regression test for a Copilot review finding on +// TestPlanStart_ExceptionalStateConsumerWaitsForRestart's own fix: for a +// scaled service, serviceNodes keeps only the LAST create-phase node +// processed, so a consumer's start only ended up depending on the highest- +// numbered replica's restart. With two exceptional-state replicas, the +// consumer must depend on BOTH restart nodes, not just one. +func TestPlanStart_ScaledExceptionalStateConsumerWaitsForEveryRestart(t *testing.T) { + two := 2 + app := types.ServiceConfig{Name: "app", Deploy: &types.DeployConfig{Replicas: &two}} + consumer := serviceWithDeps("consumer", types.DependsOnConfig{"app": {Condition: types.ServiceConditionStarted, Required: true}}) + project := &types.Project{ + Name: "myproject", + Services: types.Services{"app": app, "consumer": consumer}, + } + appHash, err := serviceHashWithResolvedRefs(app, nil) + assert.NilError(t, err) + consumerHash, err := serviceHashWithResolvedRefs(consumer, nil) + assert.NilError(t, err) + observed := emptyObserved() + observed.Containers["app"] = []ObservedContainer{ + observedServiceContainer("app", 1, container.StatePaused, appHash), + observedServiceContainer("app", 2, container.StateDead, appHash), + } + observed.Containers["consumer"] = []ObservedContainer{ + observedServiceContainer("consumer", 1, container.StateExited, consumerHash), + } + + plan, err := reconcile(t.Context(), project, observed, startScopeOptions(ScopeCreateStart), noPrompt) + assert.NilError(t, err) + + var restart1, restart2, consumerStartNode *PlanNode + for _, n := range plan.Nodes { + switch { + case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:app:1": + restart1 = n + case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:app:2": + restart2 = n + case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:consumer:1": + consumerStartNode = n + } + } + if restart1 == nil || restart2 == nil { + t.Fatalf("expected both replicas' bare-restart nodes:\n%s", plan) + } + if consumerStartNode == nil { + t.Fatalf("expected a start node for consumer:\n%s", plan) + } + assert.Assert(t, slices.Contains(consumerStartNode.DependsOn, restart1), + "consumer's start must depend on replica 1's restart node:\n%s", plan) + assert.Assert(t, slices.Contains(consumerStartNode.DependsOn, restart2), + "consumer's start must depend on replica 2's restart node too, not just the last one processed:\n%s", plan) +} + // A replica condemned by scale-down must never receive a start-phase node: // the imperative engine only starts what survives the convergence — and the // condemned replica does not count as running for the pre_start gating. diff --git a/pkg/compose/service_containers_test.go b/pkg/compose/service_containers_test.go index d6e3527c50..31bd01ba5b 100644 --- a/pkg/compose/service_containers_test.go +++ b/pkg/compose/service_containers_test.go @@ -865,6 +865,19 @@ func TestWaitDependencyDeadline(t *testing.T) { assert.Error(t, err, "timeout waiting for dependencies") }) + // Regression guard: an optional dependency's deadline expiring (new as of + // planExecutor.waitTimeout giving execWaitCondition a real timeout for + // the first time -- see waitDependency's ctx.Done() branch) must be + // tolerated exactly like any other definitive failure of an optional + // dependency, not surfaced as "timeout waiting for dependencies". + t.Run("expired deadline on an optional dependency is skipped, not an error", func(t *testing.T) { + optionalDeps := types.DependsOnConfig{ + "db": {Condition: types.ServiceConditionHealthy, Required: false}, + } + err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", optionalDeps, containers, 50*time.Millisecond) + assert.NilError(t, err) + }) + t.Run("user cancellation is not a wait failure", func(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) cancel() From 6fc92dc388994e701039514bc44ee7cc8668f3c7 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 20:31:54 +0200 Subject: [PATCH 16/26] fix(service_containers): don't swallow an inherited deadline as a wait 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 --- pkg/compose/executor_ops.go | 7 ++++++- pkg/compose/service_containers.go | 16 ++++++++++++---- pkg/compose/service_containers_test.go | 16 ++++++++++++++++ 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/pkg/compose/executor_ops.go b/pkg/compose/executor_ops.go index ff3791f657..fc4f153ff0 100644 --- a/pkg/compose/executor_ops.go +++ b/pkg/compose/executor_ops.go @@ -249,7 +249,12 @@ func (exec *planExecutor) execWaitCondition(ctx context.Context, op Operation) e // itself instead. waitDependency only ever reads this for one // practically unreachable log line (an unsupported depends_on condition, // filtered out before a plan is ever built). - return s.waitDependency(ctx, op.ResourceID, op.Name, config, waitingFor) + // + // exec.waitTimeout > 0 is exactly the condition under which executeNode's + // OpWaitCondition dispatch wrapped ctx with its own per-node timeout (see + // executor.go) -- the same flag waitDependency needs to tell that wrap + // apart from a deadline inherited from further up the chain. + return s.waitDependency(ctx, exec.waitTimeout > 0, op.ResourceID, op.Name, config, waitingFor) } // execRunPreStart runs the service's pre_start hooks against the runner diff --git a/pkg/compose/service_containers.go b/pkg/compose/service_containers.go index dda9b3d135..3f3d628171 100644 --- a/pkg/compose/service_containers.go +++ b/pkg/compose/service_containers.go @@ -184,7 +184,7 @@ func (s *composeService) waitDependencies(ctx context.Context, project *types.Pr } eg.Go(func() error { - return s.waitDependency(ctx, dependant, dep, config, waitingFor) + return s.waitDependency(ctx, timeout > 0, dependant, dep, config, waitingFor) }) } err := eg.Wait() @@ -197,7 +197,13 @@ func (s *composeService) waitDependencies(ctx context.Context, project *types.Pr // waitDependency polls the dependency's containers until its depends_on // condition is satisfied (done), definitively failed (err), or ctx is // cancelled. Each check reports (done, err): (false, nil) means keep polling. -func (s *composeService) waitDependency(ctx context.Context, dependant, dep string, config types.ServiceDependency, waitingFor Containers) error { +// hasOwnDeadline tells apart a DeadlineExceeded this specific call is +// responsible for (the caller wrapped ctx with its own timeout for this +// wait) from one inherited from an ancestor context the caller never +// touched -- only the former is safe to tolerate on an optional dependency; +// the latter means something else entirely (an external deadline, a test +// harness timeout) fired, and must keep propagating regardless of Required. +func (s *composeService) waitDependency(ctx context.Context, hasOwnDeadline bool, dependant, dep string, config types.ServiceDependency, waitingFor Containers) error { ticker := time.NewTicker(500 * time.Millisecond) defer ticker.Stop() for { @@ -209,9 +215,11 @@ func (s *composeService) waitDependency(ctx context.Context, dependant, dep stri // (Ctrl-C) stays silent either way. An optional dependency // tolerates a timeout exactly like it tolerates any other // definitive failure the check* functions below report — - // skipped, not aborted. + // skipped, not aborted — but only this call's OWN timeout: an + // inherited deadline is a different, real failure the caller + // must still see. if errors.Is(ctx.Err(), context.DeadlineExceeded) { - if !config.Required { + if !config.Required && hasOwnDeadline { s.events.On(containerReasonEvents(waitingFor, skippedEvent, fmt.Sprintf("optional dependency %q did not satisfy %q before the timeout", dep, config.Condition))...) logrus.Warnf("optional dependency %q did not satisfy %q before the timeout", dep, config.Condition) diff --git a/pkg/compose/service_containers_test.go b/pkg/compose/service_containers_test.go index 31bd01ba5b..42e20290a7 100644 --- a/pkg/compose/service_containers_test.go +++ b/pkg/compose/service_containers_test.go @@ -878,6 +878,22 @@ func TestWaitDependencyDeadline(t *testing.T) { assert.NilError(t, err) }) + // Regression guard: the tolerance above must apply only to a deadline + // THIS call's own timeout parameter is responsible for -- an unrelated, + // already-expired deadline the caller brought in (timeout == 0, so this + // call set no deadline of its own) is a different, real failure that + // must still propagate even for an optional dependency. + t.Run("an inherited parent deadline is not swallowed for an optional dependency", func(t *testing.T) { + optionalDeps := types.DependsOnConfig{ + "db": {Condition: types.ServiceConditionHealthy, Required: false}, + } + parentCtx, cancel := context.WithTimeout(t.Context(), time.Millisecond) + defer cancel() + time.Sleep(10 * time.Millisecond) // let the parent's own deadline actually expire first + err := tested.(*composeService).waitDependencies(parentCtx, &project, "app", optionalDeps, containers, 0) + assert.Error(t, err, "timeout waiting for dependencies") + }) + t.Run("user cancellation is not a wait failure", func(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) cancel() From 2c517f6e10fece821c02ed1c55ffd955b10187ea Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 22:33:23 +0200 Subject: [PATCH 17/26] fix(executor): make Create->Start a hard sequential barrier 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 --- pkg/compose/executor.go | 125 ++++++++++++++----------- pkg/compose/executor_start_test.go | 144 ++++++++++++++++------------- 2 files changed, 152 insertions(+), 117 deletions(-) diff --git a/pkg/compose/executor.go b/pkg/compose/executor.go index fea00a4d2c..67d98fe44c 100644 --- a/pkg/compose/executor.go +++ b/pkg/compose/executor.go @@ -160,25 +160,6 @@ func (exec *planExecutor) run(ctx context.Context, plan *Plan) error { groups := exec.buildGroupTracker(plan) events := exec.compose.events - // Cancellation is phase-scoped, not plan-wide: the Create phase is - // canceled by a Create-phase node failing; the Start phase, derived from - // it, is ALSO canceled by a Start-phase node failing. A Start-phase - // failure (an OpWaitCondition hitting --wait-timeout, a container failing - // to start, anything) must never cancel the Create phase -- doing so - // would abort unrelated, still in-flight Create-phase work (a sibling - // service's container create, a network create) that has nothing to do - // with it. The old create()-then-start() sequence never had this failure - // mode: the whole create phase had always finished before any dependency - // wait, or anything else in the start phase, even began. A Create-phase - // failure still cancels the Start phase too, via the Create phase's - // cancellation propagating to the Start phase derived from it -- - // matching the old sequence, where start() was never even called once - // create() failed. - createPhase := newPhaseCancel(ctx) - defer createPhase.cancel() - startPhase := newPhaseCancel(createPhase.ctx) - defer startPhase.cancel() - rs := &runState{ done: done, recordFailure: recordFailure, @@ -186,32 +167,76 @@ func (exec *planExecutor) run(ctx context.Context, plan *Plan) error { groups: groups, events: events, limiter: newOptionalLimiter(exec.compose.maxConcurrency), - createPhase: createPhase, - startPhase: startPhase, } - eg := errgroup.Group{} + + var createNodes, startNodes []*PlanNode for _, node := range plan.Nodes { + if node.Phase == PhaseStart { + startNodes = append(startNodes, node) + } else { + createNodes = append(createNodes, node) + } + } + + // A Create-phase node depending on a Start-phase node would deadlock + // forever: runPhase(createNodes, ...) only closes rs.done for nodes in + // createNodes, so runNode's `<-rs.done[dep.ID]` would block on a channel + // nothing ever closes, since the Start phase isn't even dispatched until + // the Create phase this node is blocking returns. Nothing in the + // reconciler should ever produce this edge, but failing fast with a + // clear error beats a silent hang if it ever does. + for _, node := range createNodes { + for _, dep := range node.DependsOn { + if dep.Phase == PhaseStart { + return fmt.Errorf("invalid plan: create-phase node %d depends on start-phase node %d", node.ID, dep.ID) + } + } + } + + // The Create phase is a hard barrier: no Start-phase node is even + // dispatched until every Create-phase node has succeeded. This isn't + // just about not canceling unrelated in-flight Create work on a Start + // failure (phaseCancel already keeps that scoped per phase, see its own + // doc comment) -- it's that a Start-phase node whose own DependsOn + // edges are all satisfied has no reason to wait for a COMPLETELY + // UNRELATED Create-phase node elsewhere in the plan, so without this + // barrier a service could already be starting while a sibling it has no + // relationship with is still being created, or fails. The old + // create()-then-start() sequence never allowed that: the whole create + // phase, project-wide, always finished (or failed, with start() never + // invoked at all) before any dependency wait, or anything else in the + // start phase, even began. Interactive up's attach/printer session + // depends on this too: it takes exclusive hold of the terminal for the + // create phase's own progress display, handing it over to continuous + // container log streaming only once the create phase is entirely done + // -- a Start-phase node's log output interleaving with Create's + // progress bars has nowhere consistent to go. + if err := exec.runPhase(createNodes, rs, newPhaseCancel(ctx)); err != nil { + return err + } + return exec.runPhase(startNodes, rs, newPhaseCancel(ctx)) +} + +// runPhase dispatches every node in nodes concurrently (respecting their own +// DependsOn edges via rs.done, same as a single-phase plan always did) and +// waits for them all to finish. phase is this call's own cancellation scope: +// a node failing here cancels phase.ctx for its siblings in THIS call only, +// never a concurrently-running call for a different phase (not that there +// is one today -- run() calls this sequentially -- but runNode has no way +// to tell, so this still matters if that ever changes). See phaseCancel's +// doc comment for why a node's failure goes through phase.fail instead of +// errgroup.Group's own first-error-wins race. +func (exec *planExecutor) runPhase(nodes []*PlanNode, rs *runState, phase *phaseCancel) error { + defer phase.cancel() + eg := errgroup.Group{} + for _, node := range nodes { eg.Go(func() error { - return exec.runNode(node, rs) + return exec.runNode(node, rs, phase) }) } - err := eg.Wait() - // A phase's own recorded error -- set by whichever node first triggered - // that phase's cancellation (see phaseCancel.fail) -- is authoritative - // over whatever eg.Wait() itself picked: once cancellation starts - // unblocking siblings, more than one goroutine can be mid-return at the - // same time, and errgroup.Group's own first-error-wins race has no idea - // which of them was the actual cause versus just a cancellation casualty - // (a sibling's <-nodeCtx.Done() bail returning a bare context.Canceled - // that reveals nothing). Create takes precedence over Start: a Create - // failure is the more fundamental one of the two (it's what canceled - // Start in the first place, transitively, when both end up set). - if createPhase.err != nil { - return createPhase.err - } - if startPhase.err != nil { - return startPhase.err + if phase.err != nil { + return phase.err } return err } @@ -257,9 +282,11 @@ func (p *phaseCancel) fail(node *PlanNode, events api.EventProcessor, groups *gr // runState carries the per-run() state every node's goroutine shares: the // done-channel per node, dependency-failure bookkeeping, group event -// tracking, the concurrency limiter, and the phase-scoped cancellation a -// node joins depending on its own Phase. Split out of run() purely to keep -// runNode a plain method instead of a closure captured in a loop. +// tracking, and the concurrency limiter. Shared across both phases (built +// once in run(), before either runs) since done/failures must stay visible +// to a Start-phase node depending on a Create-phase one across the barrier +// between them. Split out of run() purely to keep runNode a plain method +// instead of a closure captured in a loop. type runState struct { done map[int]chan struct{} recordFailure func(id int, err error) @@ -267,19 +294,13 @@ type runState struct { groups *groupTracker events api.EventProcessor limiter *semaphore.Weighted - createPhase *phaseCancel - startPhase *phaseCancel } // runNode executes one plan node: waits for its dependencies, skips it if -// one of them failed, then dispatches it and records the outcome. See run() -// for the phase-scoped cancellation (createPhase/startPhase) this draws -// from. -func (exec *planExecutor) runNode(node *PlanNode, rs *runState) error { - phase := rs.createPhase - if node.Phase == PhaseStart { - phase = rs.startPhase - } +// one of them failed, then dispatches it and records the outcome. phase is +// the cancellation scope of the runPhase call driving it -- see runPhase and +// phaseCancel's own doc comments. +func (exec *planExecutor) runNode(node *PlanNode, rs *runState, phase *phaseCancel) error { nodeCtx := phase.ctx // Every exit path below must close rs.done[node.ID] exactly once, or a diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index 294b6cc118..61490a3ed4 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -24,6 +24,7 @@ import ( "fmt" "net" "sync" + "sync/atomic" "testing" "time" @@ -264,56 +265,44 @@ func TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes(t *testing.T) { assert.Assert(t, elapsed < 5*time.Second, "waitTimeout did not bound the stuck OpWaitCondition node (took %s)", elapsed) } -// TestExecutePlanStartPhaseFailureDoesNotCancelCreatePhase is a regression -// test for a Copilot review finding on #14290: run() used to dispatch every -// node under one shared errgroup.WithContext, so a Start-phase node failing -// (a required OpWaitCondition hitting its own waitTimeout, here) canceled -// ctx for every other goroutine still in flight -- including a completely -// unrelated Create-phase node (a sibling service's network create) that has -// nothing to do with the dependency that timed out. The old -// create()-then-start() sequence never had this failure mode: the whole -// create phase had always finished before any dependency wait began. This -// pairs a never-satisfied required wait (Phase: PhaseStart, matching what -// planStartPhase actually produces) with a network create (Phase: -// PhaseCreate, the default) deliberately slower than the wait's timeout, and -// asserts the network create's own context is never canceled -- it must -// complete normally, on its own schedule, even though the sibling wait -// already failed and the plan as a whole still fails too. -func TestExecutePlanStartPhaseFailureDoesNotCancelCreatePhase(t *testing.T) { - svc, apiClient, _ := newStartPhaseTestService(t) - - dbSummary := container.Summary{ - ID: "db-id", - Names: []string{"/test-db-1"}, - Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"}, - } - apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(client.ContainerInspectResult{ - Container: container.InspectResponse{ - ID: "db-id", - Name: "/test-db-1", - State: &container.State{ - Status: container.StateRunning, - Health: &container.Health{Status: container.Starting}, - }, - Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}}, - }, - }, nil).AnyTimes() +// TestExecutePlanCreatePhaseIsABarrierBeforeStartPhase is a regression test +// for a Copilot review finding on #14291: a Start-phase node's own DependsOn +// edges being satisfied is not enough to let it run -- nothing stopped it +// from starting while a COMPLETELY UNRELATED Create-phase node elsewhere in +// the plan was still in flight (or about to fail), something the old +// create()-then-start() sequence never allowed (the whole create phase, +// project-wide, always finished -- or failed, with start() never invoked at +// all -- before any dependency wait, or anything else in the start phase, +// even began). Interactive up's attach/printer session needs that same +// guarantee: it takes exclusive hold of the terminal for the create phase's +// progress display, handing it to continuous container log streaming only +// once the create phase is entirely done. This pairs a slow, unrelated +// network create (Phase: PhaseCreate, the default, no DependsOn edge to +// anything in the Start phase) with an OpStartContainer node (Phase: +// PhaseStart) and asserts ContainerStart is never called before the network +// create has fully returned. Uses OpStartContainer rather than +// OpWaitCondition deliberately: execWaitCondition's underlying poll loop +// only checks on a 500ms ticker (never on entry), which would make a +// same-order-of-magnitude Create delay pass this assertion even with no +// barrier at all, and so couldn't actually catch a regression here. +func TestExecutePlanCreatePhaseIsABarrierBeforeStartPhase(t *testing.T) { + svc, apiClient := newTestService(t) + var createDone atomic.Bool nw := types.NetworkConfig{Name: "test_default"} - var networkCreateCtxErr error - networkCreateDone := make(chan struct{}) apiClient.EXPECT().NetworkCreate(gomock.Any(), "test_default", gomock.Any()). - DoAndReturn(func(ctx context.Context, _ string, _ client.NetworkCreateOptions) (client.NetworkCreateResult, error) { - defer close(networkCreateDone) - // Deliberately outlives the wait's timeout below: if the - // Create-phase ctx were canceled by the sibling Start-phase wait - // failing, this ctx would already be done well before this - // returns. - time.Sleep(300 * time.Millisecond) - networkCreateCtxErr = ctx.Err() + DoAndReturn(func(context.Context, string, client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + time.Sleep(100 * time.Millisecond) + createDone.Store(true) return client.NetworkCreateResult{ID: "net-id"}, nil }) + apiClient.EXPECT().ContainerStart(gomock.Any(), "db-id", gomock.Any()). + DoAndReturn(func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) { + assert.Assert(t, createDone.Load(), "the Start-phase container start began before the unrelated Create-phase network create returned") + return client.ContainerStartResult{}, nil + }) + plan := &Plan{} plan.addNode(Operation{ Type: OpCreateNetwork, @@ -321,36 +310,25 @@ func TestExecutePlanStartPhaseFailureDoesNotCancelCreatePhase(t *testing.T) { Name: nw.Name, Network: &nw, }, "") - wait := plan.addNode(Operation{ - Type: OpWaitCondition, - ResourceID: "wait:db:service_healthy", - Name: "db", - Condition: types.ServiceConditionHealthy, + start := plan.addNode(Operation{ + Type: OpStartContainer, + ResourceID: "service:db:1", + Container: &container.Summary{ID: "db-id", Names: []string{"/test-db-1"}}, }, "") - wait.Phase = PhaseStart + start.Phase = PhaseStart exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) - exec.containersByService["db"] = Containers{dbSummary} - exec.waitTimeout = 100 * time.Millisecond - - err := exec.run(t.Context(), plan) - assert.ErrorIs(t, err, context.DeadlineExceeded) - select { - case <-networkCreateDone: - case <-time.After(5 * time.Second): - t.Fatal("NetworkCreate never completed -- its context was likely canceled by the sibling Start-phase wait timing out") - } - assert.NilError(t, networkCreateCtxErr, "NetworkCreate's own ctx must not be canceled by an unrelated Start-phase failure") + assert.NilError(t, exec.run(t.Context(), plan)) + assert.Assert(t, createDone.Load()) } -// TestExecutePlanCreatePhaseFailureCancelsCreatePhase is the mirror image of -// TestExecutePlanStartPhaseFailureDoesNotCancelCreatePhase: phase-scoped -// cancellation must not weaken the Create phase's own existing fail-fast +// TestExecutePlanCreatePhaseFailureCancelsCreatePhase verifies phase-scoped +// cancellation didn't weaken the Create phase's own existing fail-fast // behavior -- two independent (no DependsOn edge) Create-phase nodes, one // failing immediately, must still cancel the other's in-flight work. Both // nodes default to Phase: PhaseCreate (addNode's zero value), so they share -// createCtx. +// the same runPhase call's phaseCancel. func TestExecutePlanCreatePhaseFailureCancelsCreatePhase(t *testing.T) { svc, apiClient := newTestService(t) @@ -397,6 +375,42 @@ func TestExecutePlanCreatePhaseFailureCancelsCreatePhase(t *testing.T) { assert.ErrorIs(t, volumeCreateCtxErr, context.Canceled, "VolumeCreate's ctx must be canceled by a sibling Create-phase node failing") } +// TestExecutePlanCreatePhaseFailureNeverDispatchesStartPhase complements +// TestExecutePlanCreatePhaseIsABarrierBeforeStartPhase: that test proves the +// Start phase waits for a successful Create phase, this one proves a failed +// Create phase skips the Start phase entirely rather than dispatching it and +// canceling it. run() must return the Create-phase error before runPhase is +// ever called on startNodes -- ContainerStart's .Times(0) fails the test the +// instant it's called at all, not just if it fails to be called. +func TestExecutePlanCreatePhaseFailureNeverDispatchesStartPhase(t *testing.T) { + svc, apiClient := newTestService(t) + + apiClient.EXPECT().NetworkCreate(gomock.Any(), "test_default", gomock.Any()). + Return(client.NetworkCreateResult{}, errors.New("boom")) + apiClient.EXPECT().ContainerStart(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + nw := types.NetworkConfig{Name: "test_default"} + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpCreateNetwork, + ResourceID: "network:default", + Name: nw.Name, + Network: &nw, + }, "") + start := plan.addNode(Operation{ + Type: OpStartContainer, + ResourceID: "service:db:1", + Container: &container.Summary{ID: "db-id", Names: []string{"/test-db-1"}}, + }, "") + start.Phase = PhaseStart + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + + err := exec.run(t.Context(), plan) + assert.ErrorContains(t, err, "boom") +} + // TestExecutePlanConcurrentPhaseFailureKeepsOriginatingError exercises a // Copilot/docker-agent finding on the phase-scoped cancellation fix: // errgroup.Group's plain Wait() returns whichever goroutine's error reaches From d9097197f9a5b7a465dae1963b67eb03f83104da Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 22:54:13 +0200 Subject: [PATCH 18/26] fix(executor): tell an optional wait's own timeout from an inherited deadline Copilot review finding on #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 --- pkg/compose/executor.go | 3 +- pkg/compose/executor_ops.go | 19 +++-- pkg/compose/executor_start_test.go | 113 ++++++++++++++++++++++++++++- pkg/compose/service_containers.go | 27 ++++--- 4 files changed, 139 insertions(+), 23 deletions(-) diff --git a/pkg/compose/executor.go b/pkg/compose/executor.go index 67d98fe44c..29df329077 100644 --- a/pkg/compose/executor.go +++ b/pkg/compose/executor.go @@ -394,12 +394,13 @@ func (exec *planExecutor) executeNode(ctx context.Context, node *PlanNode) error case OpCreateHookContainer: return exec.execCreateHookContainer(ctx, node) case OpWaitCondition: + origCtx := ctx if exec.waitTimeout > 0 { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, exec.waitTimeout) defer cancel() } - return exec.execWaitCondition(ctx, op) + return exec.execWaitCondition(ctx, origCtx, op) case OpRunPreStart: return exec.execRunPreStart(ctx, op) case OpRunPostStart: diff --git a/pkg/compose/executor_ops.go b/pkg/compose/executor_ops.go index fc4f153ff0..7c36e909ed 100644 --- a/pkg/compose/executor_ops.go +++ b/pkg/compose/executor_ops.go @@ -220,13 +220,16 @@ func (exec *planExecutor) resolveContainerSummary(op Operation) (container.Summa // optional-dependency handling. // // Unlike waitDependencies, this has no timeout of its own to apply here -- -// ctx is used exactly as given. upDetached (up.go) is the caller that needs -// one: planExecutor.waitTimeout gives this node its own fresh per-wait -// window, derived in executeNode's OpWaitCondition dispatch rather than -// here, the same thing start()'s own WaitTimeout already threads through +// ctx is used exactly as given, already wrapped (or not) by executeNode's +// OpWaitCondition dispatch in executor.go. origCtx is ctx as it stood right +// before that wrap, passed through unchanged to waitDependency, which needs +// it to tell its own wait timeout apart from an inherited deadline (see its +// doc comment). upDetached (up.go) is the caller that needs a timeout here +// at all: planExecutor.waitTimeout gives this node its own fresh per-wait +// window, the same thing start()'s own WaitTimeout already threads through // every waitDependencies call -- without also bounding unrelated Create or // Start work the way wrapping the whole plan's ctx would. -func (exec *planExecutor) execWaitCondition(ctx context.Context, op Operation) error { +func (exec *planExecutor) execWaitCondition(ctx, origCtx context.Context, op Operation) error { s := exec.compose exec.containersMu.Lock() waitingFor := exec.containersByService[op.Name].filter(isNotOneOff, isNotHookContainer) @@ -250,11 +253,7 @@ func (exec *planExecutor) execWaitCondition(ctx context.Context, op Operation) e // practically unreachable log line (an unsupported depends_on condition, // filtered out before a plan is ever built). // - // exec.waitTimeout > 0 is exactly the condition under which executeNode's - // OpWaitCondition dispatch wrapped ctx with its own per-node timeout (see - // executor.go) -- the same flag waitDependency needs to tell that wrap - // apart from a deadline inherited from further up the chain. - return s.waitDependency(ctx, exec.waitTimeout > 0, op.ResourceID, op.Name, config, waitingFor) + return s.waitDependency(ctx, origCtx, op.ResourceID, op.Name, config, waitingFor) } // execRunPreStart runs the service's pre_start hooks against the runner diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index 61490a3ed4..5900125683 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -84,7 +84,7 @@ func TestExecWaitCondition_RequiredMissingDependencyFails(t *testing.T) { svc, _, _ := newStartPhaseTestService(t) exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) - err := exec.execWaitCondition(t.Context(), Operation{ + err := exec.execWaitCondition(t.Context(), t.Context(), Operation{ Name: "db", Condition: types.ServiceConditionHealthy, }) @@ -99,7 +99,7 @@ func TestExecWaitCondition_OptionalMissingDependencyIsTolerated(t *testing.T) { svc, _, recorder := newStartPhaseTestService(t) exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) - err := exec.execWaitCondition(t.Context(), Operation{ + err := exec.execWaitCondition(t.Context(), t.Context(), Operation{ Name: "db", Condition: types.ServiceConditionHealthy, BestEffort: true, @@ -141,7 +141,7 @@ func TestExecWaitCondition_HealthyConditionSatisfied(t *testing.T) { exec := svc.newPlanExecutor(&types.Project{Name: "test"}, observed, nil) exec.containersByService["db"] = Containers{dbSummary} - err := exec.execWaitCondition(t.Context(), Operation{ + err := exec.execWaitCondition(t.Context(), t.Context(), Operation{ Name: "db", Condition: types.ServiceConditionHealthy, }) @@ -265,6 +265,113 @@ func TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes(t *testing.T) { assert.Assert(t, elapsed < 5*time.Second, "waitTimeout did not bound the stuck OpWaitCondition node (took %s)", elapsed) } +// TestExecutePlanOptionalWaitDoesNotSwallowInheritedDeadline is a regression +// test for a Copilot review finding on #14290: waitDependency originally +// tolerated any DeadlineExceeded on an optional dependency as long as the +// caller had configured a timeout at all (exec.waitTimeout > 0), regardless +// of whether THIS wait's own timeout, or an earlier deadline inherited from +// the caller's own ctx, was the one that actually fired. Both make +// ctx.Done() report DeadlineExceeded once the earlier of the two elapses, so +// a bare "did I configure a timeout" flag can't tell them apart -- an +// optional dependency would then silently swallow a real external deadline +// (e.g. the process's own context, or a test harness timeout) and report +// success. This plan's single OpWaitCondition node never resolves +// (BestEffort: true) and exec.waitTimeout is set far longer than the ctx +// passed into exec.run, so the INHERITED deadline is always the one that +// fires first: the error must still propagate, not be tolerated as if +// exec.waitTimeout itself had elapsed. +func TestExecutePlanOptionalWaitDoesNotSwallowInheritedDeadline(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + + dbSummary := container.Summary{ + ID: "db-id", + Names: []string{"/test-db-1"}, + Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"}, + } + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(client.ContainerInspectResult{ + Container: container.InspectResponse{ + ID: "db-id", + Name: "/test-db-1", + State: &container.State{ + Status: container.StateRunning, + Health: &container.Health{Status: container.Starting}, + }, + Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}}, + }, + }, nil).AnyTimes() + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: "wait:db:service_healthy", + Name: "db", + Condition: types.ServiceConditionHealthy, + BestEffort: true, + }, "") + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + exec.containersByService["db"] = Containers{dbSummary} + exec.waitTimeout = 10 * time.Second + + ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) + defer cancel() + + start := time.Now() + err := exec.run(ctx, plan) + elapsed := time.Since(start) + + assert.ErrorIs(t, err, context.DeadlineExceeded, "an inherited deadline must not be swallowed just because this wait is optional and exec.waitTimeout is set") + assert.Assert(t, elapsed < 5*time.Second, "took too long to fail on the inherited deadline (took %s)", elapsed) +} + +// TestExecutePlanOptionalWaitTimeoutIsToleratedWithoutInheritedDeadline is +// the positive-case counterpart to +// TestExecutePlanOptionalWaitDoesNotSwallowInheritedDeadline: with no +// inherited deadline at all (plain t.Context() passed into exec.run), this +// wait's own exec.waitTimeout elapsing on an optional dependency must still +// be tolerated as before (nil, not an error) -- the origCtx refactor must +// not have turned every DeadlineExceeded into a hard failure. +func TestExecutePlanOptionalWaitTimeoutIsToleratedWithoutInheritedDeadline(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + + dbSummary := container.Summary{ + ID: "db-id", + Names: []string{"/test-db-1"}, + Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"}, + } + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(client.ContainerInspectResult{ + Container: container.InspectResponse{ + ID: "db-id", + Name: "/test-db-1", + State: &container.State{ + Status: container.StateRunning, + Health: &container.Health{Status: container.Starting}, + }, + Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}}, + }, + }, nil).AnyTimes() + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: "wait:db:service_healthy", + Name: "db", + Condition: types.ServiceConditionHealthy, + BestEffort: true, + }, "") + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + exec.containersByService["db"] = Containers{dbSummary} + exec.waitTimeout = 200 * time.Millisecond + + start := time.Now() + err := exec.run(t.Context(), plan) + elapsed := time.Since(start) + + assert.NilError(t, err, "exec.waitTimeout elapsing on its own, with no inherited deadline, must still be tolerated for an optional dependency") + assert.Assert(t, elapsed < 5*time.Second, "took too long (took %s)", elapsed) +} + // TestExecutePlanCreatePhaseIsABarrierBeforeStartPhase is a regression test // for a Copilot review finding on #14291: a Start-phase node's own DependsOn // edges being satisfied is not enough to let it run -- nothing stopped it diff --git a/pkg/compose/service_containers.go b/pkg/compose/service_containers.go index 3f3d628171..c13ff9eef1 100644 --- a/pkg/compose/service_containers.go +++ b/pkg/compose/service_containers.go @@ -160,6 +160,7 @@ func containerReasonEvents(containers Containers, eventFunc func(string, string) const ServiceConditionRunningOrHealthy = "running_or_healthy" func (s *composeService) waitDependencies(ctx context.Context, project *types.Project, dependant string, dependencies types.DependsOnConfig, containers Containers, timeout time.Duration) error { + origCtx := ctx if timeout > 0 { withTimeout, cancelFunc := context.WithTimeout(ctx, timeout) defer cancelFunc() @@ -184,7 +185,7 @@ func (s *composeService) waitDependencies(ctx context.Context, project *types.Pr } eg.Go(func() error { - return s.waitDependency(ctx, timeout > 0, dependant, dep, config, waitingFor) + return s.waitDependency(ctx, origCtx, dependant, dep, config, waitingFor) }) } err := eg.Wait() @@ -197,13 +198,21 @@ func (s *composeService) waitDependencies(ctx context.Context, project *types.Pr // waitDependency polls the dependency's containers until its depends_on // condition is satisfied (done), definitively failed (err), or ctx is // cancelled. Each check reports (done, err): (false, nil) means keep polling. -// hasOwnDeadline tells apart a DeadlineExceeded this specific call is -// responsible for (the caller wrapped ctx with its own timeout for this -// wait) from one inherited from an ancestor context the caller never -// touched -- only the former is safe to tolerate on an optional dependency; -// the latter means something else entirely (an external deadline, a test -// harness timeout) fired, and must keep propagating regardless of Required. -func (s *composeService) waitDependency(ctx context.Context, hasOwnDeadline bool, dependant, dep string, config types.ServiceDependency, waitingFor Containers) error { +// origCtx is ctx as it stood before the caller wrapped it with this wait's +// own timeout (if any) -- comparing origCtx.Err() against ctx.Err() at the +// moment ctx.Done() fires is how this tells apart a DeadlineExceeded this +// specific call's own wrap is responsible for from one inherited from an +// ancestor context the caller never touched. A bare "did the caller +// configure a timeout at all" flag isn't enough: if origCtx itself already +// carries an earlier deadline, wrapping it with context.WithTimeout still +// makes both origCtx and ctx report DeadlineExceeded once that earlier +// deadline fires, even though this call's own timeout never actually +// elapsed -- only origCtx.Err() == nil proves this wrap's own timeout is +// the one that fired. Only that case is safe to tolerate on an optional +// dependency; an inherited deadline means something else entirely (an +// external deadline, a test harness timeout) fired, and must keep +// propagating regardless of Required. +func (s *composeService) waitDependency(ctx, origCtx context.Context, dependant, dep string, config types.ServiceDependency, waitingFor Containers) error { ticker := time.NewTicker(500 * time.Millisecond) defer ticker.Stop() for { @@ -219,7 +228,7 @@ func (s *composeService) waitDependency(ctx context.Context, hasOwnDeadline bool // inherited deadline is a different, real failure the caller // must still see. if errors.Is(ctx.Err(), context.DeadlineExceeded) { - if !config.Required && hasOwnDeadline { + if !config.Required && origCtx.Err() == nil { s.events.On(containerReasonEvents(waitingFor, skippedEvent, fmt.Sprintf("optional dependency %q did not satisfy %q before the timeout", dep, config.Condition))...) logrus.Warnf("optional dependency %q did not satisfy %q before the timeout", dep, config.Condition) From cb11968490056daed93055c097e0473952f2e8f6 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 6 Oct 2026 23:36:00 +0200 Subject: [PATCH 19/26] fix(up): preserve the actionable dependency-timeout message without --wait Copilot review finding (low) on #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 --- pkg/compose/up.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/compose/up.go b/pkg/compose/up.go index f94c2b9e8a..5f958622eb 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -113,8 +113,17 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // independent deadline expire during exec.run -- that DeadlineExceeded // didn't come from any OpWaitCondition node's own derived timeout // either, so it isn't a health-readiness failure. - if options.Start.Wait && options.Start.WaitTimeout > 0 && ctx.Err() == nil && errors.Is(err, context.DeadlineExceeded) { - return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) + if options.Start.WaitTimeout > 0 && ctx.Err() == nil && errors.Is(err, context.DeadlineExceeded) { + if options.Start.Wait { + return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) + } + // No --wait: this is exactly the dependency-timeout failure + // start()'s own waitDependencies already reports as "timeout + // waiting for dependencies" (service_containers.go) -- matching + // that established, actionable message instead of leaking the + // raw "context deadline exceeded" a user never configured in + // those terms. + return errors.New("timeout waiting for dependencies") } return err } From 365f0026f41395827362ff68ef2f1e5c12210d11 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Wed, 7 Oct 2026 07:47:31 +0200 Subject: [PATCH 20/26] fix(reconcile): gate wait nodes on every sibling dependency, cycle-safe Copilot flagged a behavior-parity regression on #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 --- pkg/compose/reconcile.go | 95 ++++++++++-- pkg/compose/reconcile_start_test.go | 224 ++++++++++++++++++++++++++++ 2 files changed, 307 insertions(+), 12 deletions(-) diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index af70e484de..6395b317f5 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -1269,24 +1269,52 @@ func (r *reconciler) startPhaseReplicas(service types.ServiceConfig) (replicas [ // gate — errors included: a dependency that is neither declared nor // disabled is the same hard failure it is today. func (r *reconciler) startPhaseDependencies(service types.ServiceConfig) ([]*PlanNode, error) { - var depNodes []*PlanNode - for _, dep := range sortedKeys(service.DependsOn) { - cfg := service.DependsOn[dep] + deps := sortedKeys(service.DependsOn) + depTargets := make(map[string][]*PlanNode, len(deps)) + var allTargets []*PlanNode + for _, dep := range deps { targets := r.startChainEnds[dep] if len(targets) == 0 { if node, ok := r.serviceNodes[dep]; ok { targets = []*PlanNode{node} } } + depTargets[dep] = targets + for _, target := range targets { + if !slices.Contains(allTargets, target) { + allTargets = append(allTargets, target) + } + } + } + + var depNodes []*PlanNode + for _, dep := range deps { + cfg := service.DependsOn[dep] shouldWait, err := shouldWaitForDependency(dep, cfg, r.project) if err != nil { return nil, err } if !shouldWait { - depNodes = append(depNodes, targets...) + for _, target := range depTargets[dep] { + if !slices.Contains(depNodes, target) { + depNodes = append(depNodes, target) + } + } continue } - depNodes = append(depNodes, r.waitConditionNode(dep, cfg, targets)) + // The wait node's own exec.waitTimeout window must not open until + // EVERY dependency this service declares has itself finished + // starting, not just the one this particular condition checks -- + // matching the old imperative engine's startService, which only + // calls waitDependencies(X) once every direct dependency's own + // startService call has already returned (see InDependencyOrder's + // graphTraversal.run: a node is dispatched only once none of its + // children are still ServiceStopped, so X's siblings all finish + // starting before X's own wait clock ever starts). Passing just this + // dep's own target here would let an unrelated, slower sibling + // dependency silently shrink the effective grace period this + // condition's health check gets, compared to the old engine. + depNodes = append(depNodes, r.waitConditionNode(dep, cfg, allTargets)) } return depNodes, nil } @@ -1294,7 +1322,13 @@ func (r *reconciler) startPhaseDependencies(service types.ServiceConfig) ([]*Pla // waitConditionNode returns the shared wait node for (dep, condition), // creating it on first use. required:false marks it best-effort — a missing // dependency is skipped, not fatal; one required dependent upgrades the -// shared node for everyone. +// shared node for everyone. targets is now each caller's FULL sibling +// dependency set (see startPhaseDependencies), not just dep's own target, so +// two dependents of the same (dep, condition) with different sibling sets +// routinely diverge here: the merge below unions them onto the one shared +// node rather than creating a second one, at the cost of a dependent with +// fewer siblings waiting a little longer than its own depends_on strictly +// requires -- preferable to losing the dedup this node exists for. func (r *reconciler) waitConditionNode(dep string, cfg types.ServiceDependency, targets []*PlanNode) *PlanNode { key := dep + ":" + cfg.Condition wait, ok := r.waitNodes[key] @@ -1314,18 +1348,55 @@ func (r *reconciler) waitConditionNode(dep string, cfg types.ServiceDependency, if cfg.Required && wait.Operation.BestEffort { wait.Operation.BestEffort = false } - // merge this caller's prerequisites into the shared node. Today every - // caller passes the same targets (startChainEnds[dep] is fixed before - // any dependent is visited), so this is defensive — but relying on that - // silently would break the day the targets diverge per caller. + // merge this caller's prerequisites into the shared node, skipping + // anything that would create a cycle: an ordinary diamond dependency + // (X depends on dep:cond, Z depends on X AND independently on the same + // dep:cond) puts X's own chain end -- which already depends on this very + // wait node -- into Z's sibling set. Adding it back as wait's own + // prerequisite would deadlock the executor forever (runNode never applies + // exec.waitTimeout until its own DependsOn are satisfied, and nothing + // detects a node depending on itself at a distance). Skipping it loses + // nothing: X cannot even start until wait has already succeeded, so by + // the time X's chain completes wait is necessarily already done. for _, target := range targets { - if !slices.Contains(wait.DependsOn, target) { - wait.DependsOn = append(wait.DependsOn, target) + if slices.Contains(wait.DependsOn, target) { + continue } + if wouldCreateCycle(target, wait) { + continue + } + wait.DependsOn = append(wait.DependsOn, target) } return wait } +// wouldCreateCycle reports whether candidate already (transitively) depends +// on node, via DependsOn edges -- i.e. whether appending candidate to +// node.DependsOn would close a cycle. Only reachable from waitConditionNode's +// merge, where candidate is a sibling target pulled in for an already-shared +// wait node. +func wouldCreateCycle(candidate, node *PlanNode) bool { + if candidate == node { + return true + } + seen := map[*PlanNode]bool{candidate: true} + queue := []*PlanNode{candidate} + for len(queue) > 0 { + n := queue[0] + queue = queue[1:] + for _, dep := range n.DependsOn { + if dep == node { + return true + } + if !seen[dep] { + seen[dep] = true + queue = append(queue, dep) + } + } + } + return false +} + // planProviderStart is the start-phase visit of a provider service: no // container to start, but its own depends_on still applies — the imperative // startService waits on it for every service, providers included. Its chain diff --git a/pkg/compose/reconcile_start_test.go b/pkg/compose/reconcile_start_test.go index 82b243a603..2769d6980e 100644 --- a/pkg/compose/reconcile_start_test.go +++ b/pkg/compose/reconcile_start_test.go @@ -132,6 +132,230 @@ func TestPlanStart_HealthyConditionDeduplicated(t *testing.T) { `)+"\n") } +// TestPlanStart_WaitNodeGatedOnAllSiblingDependencies is a regression test +// for a Copilot review finding on #14290's Create->Start barrier work: a +// wait node's own exec.waitTimeout window used to open as soon as ITS OWN +// dependency's chain finished, ignoring the requesting service's other +// depends_on entries entirely. The old imperative engine never allowed that +// -- InDependencyOrder only calls startService(app), and so only starts +// app's own waitDependencies clock, once EVERY one of app's direct +// dependencies has itself finished starting (see graphTraversal.run: a node +// is dispatched only once none of its children are still ServiceStopped). A +// slow sibling with no wait node of its own (service_started is a plain +// edge, no condition to poll) could silently eat into a health check's +// effective grace period. Here app depends on db:service_healthy (needs a +// wait node) and migrate:service_started (a plain edge, no wait node) -- the +// db wait node must still depend on migrate's own start, or its +// exec.waitTimeout clock starts too early relative to the old engine. +func TestPlanStart_WaitNodeGatedOnAllSiblingDependencies(t *testing.T) { + db := types.ServiceConfig{Name: "db"} + db.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} + migrate := types.ServiceConfig{Name: "migrate"} + app := serviceWithDeps("app", types.DependsOnConfig{ + "db": {Condition: types.ServiceConditionHealthy, Required: true}, + "migrate": {Condition: types.ServiceConditionStarted, Required: true}, + }) + project := &types.Project{ + Name: "myproject", + Services: types.Services{"db": db, "migrate": migrate, "app": app}, + } + + plan, err := reconcile(t.Context(), project, emptyObserved(), startScopeOptions(ScopeCreateStart), noPrompt) + assert.NilError(t, err) + + var wait, dbStart, migrateStart *PlanNode + for _, n := range plan.Nodes { + switch { + case n.Operation.Type == OpWaitCondition: + wait = n + case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:db:1": + dbStart = n + case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:migrate:1": + migrateStart = n + } + } + if wait == nil || dbStart == nil || migrateStart == nil { + t.Fatalf("expected a db wait node and both db/migrate start nodes:\n%s", plan) + } + assert.Assert(t, slices.Contains(wait.DependsOn, dbStart), + "db's wait node must depend on db's own start:\n%s", plan) + assert.Assert(t, slices.Contains(wait.DependsOn, migrateStart), + "db's wait node must also depend on app's other sibling dependency (migrate) finishing its own start, not just db's:\n%s", plan) +} + +// nodeInOwnDependencyClosure reports whether n is reachable from itself by +// following DependsOn edges -- i.e. whether the plan has a dependency cycle +// through n. +func nodeInOwnDependencyClosure(n *PlanNode) bool { + seen := map[*PlanNode]bool{} + queue := slices.Clone(n.DependsOn) + for len(queue) > 0 { + cur := queue[0] + queue = queue[1:] + if cur == n { + return true + } + if seen[cur] { + continue + } + seen[cur] = true + queue = append(queue, cur.DependsOn...) + } + return false +} + +func assertNoCycle(t *testing.T, plan *Plan) { + t.Helper() + for _, n := range plan.Nodes { + if nodeInOwnDependencyClosure(n) { + t.Fatalf("plan has a dependency cycle through node #%d (%s):\n%s", n.ID, n.Operation.ResourceID, plan) + } + } +} + +// TestPlanStart_DiamondDependencyNeverCreatesWaitCycle is a regression test +// for a Copilot review finding on the allTargets widening above: an entirely +// ordinary diamond dependency -- app depends on db:service_healthy, worker +// depends on BOTH app:service_started and, independently, db:service_healthy +// -- shares db's wait node between app and worker (same (dep,condition) +// key). app's own start node already depends on that wait node. Widening the +// wait node's own prerequisites with worker's sibling set (app's start node, +// pulled in because worker also depends on app) would append app's start +// node back into the wait node's own DependsOn -- a direct cycle the +// executor can never resolve (runNode blocks on rs.done[dep.ID] for every +// dependency before closing its own, with nothing to detect or time out a +// cycle). The wait node must keep depending on db alone; worker's own start +// node separately depends on both the wait node and app's start node, which +// already encodes everything worker's depends_on requires -- nothing is +// lost by not also routing it through the shared wait node. +func TestPlanStart_DiamondDependencyNeverCreatesWaitCycle(t *testing.T) { + db := types.ServiceConfig{Name: "db"} + db.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} + app := serviceWithDeps("app", types.DependsOnConfig{ + "db": {Condition: types.ServiceConditionHealthy, Required: true}, + }) + worker := serviceWithDeps("worker", types.DependsOnConfig{ + "app": {Condition: types.ServiceConditionStarted, Required: true}, + "db": {Condition: types.ServiceConditionHealthy, Required: true}, + }) + project := &types.Project{ + Name: "myproject", + Services: types.Services{"db": db, "app": app, "worker": worker}, + } + + plan, err := reconcile(t.Context(), project, emptyObserved(), startScopeOptions(ScopeCreateStart), noPrompt) + assert.NilError(t, err) + assertNoCycle(t, plan) + + var wait, appStart *PlanNode + for _, n := range plan.Nodes { + switch { + case n.Operation.Type == OpWaitCondition: + wait = n + case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:app:1": + appStart = n + } + } + if wait == nil || appStart == nil { + t.Fatalf("expected db's wait node and app's start node:\n%s", plan) + } + assert.Assert(t, !slices.Contains(wait.DependsOn, appStart), + "db's wait node must not depend on app's start node -- app's own start already depends on the wait node, so this would be a cycle:\n%s", plan) + assert.Assert(t, slices.Contains(appStart.DependsOn, wait), + "app's own start must still depend on db's wait node:\n%s", plan) +} + +// TestPlanStart_ExceptionalChainCollapseNeverCreatesWaitCycle is a second +// regression test for the same cycle risk, reproducing the exact shape found +// during review: a steady-state up where every service is already running. +// "a" depends only on db:service_healthy and has nothing left to start, so +// planServiceStart's "nothing to start" branch sets startChainEnds["a"] to +// literally BE the shared db:service_healthy wait node (ends := depNodes). +// "b" depends on both db:service_healthy and a:service_started -- its +// allTargets then contains that very wait node as one of "a"'s targets, and +// merging it into itself would be a direct self-loop with no other node +// in between. +func TestPlanStart_ExceptionalChainCollapseNeverCreatesWaitCycle(t *testing.T) { + db := types.ServiceConfig{Name: "db"} + db.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} + a := serviceWithDeps("a", types.DependsOnConfig{ + "db": {Condition: types.ServiceConditionHealthy, Required: true}, + }) + b := serviceWithDeps("b", types.DependsOnConfig{ + "db": {Condition: types.ServiceConditionHealthy, Required: true}, + "a": {Condition: types.ServiceConditionStarted, Required: true}, + }) + project := &types.Project{ + Name: "myproject", + Services: types.Services{"db": db, "a": a, "b": b}, + } + observed := emptyObserved() + for _, svc := range []types.ServiceConfig{db, a, b} { + hash, err := serviceHashWithResolvedRefs(svc, nil) + assert.NilError(t, err) + observed.Containers[svc.Name] = []ObservedContainer{ + observedServiceContainer(svc.Name, 1, container.StateRunning, hash), + } + } + + plan, err := reconcile(t.Context(), project, observed, startScopeOptions(ScopeCreateStart), noPrompt) + assert.NilError(t, err) + assertNoCycle(t, plan) + + for _, n := range plan.Nodes { + if n.Operation.Type == OpWaitCondition { + assert.Assert(t, !slices.Contains(n.DependsOn, n), + "wait node #%d must not depend on itself:\n%s", n.ID, plan) + } + } +} + +// TestPlanStart_TransitiveChainNeverCreatesWaitCycle exercises +// wouldCreateCycle's BFS beyond a single hop: consumer's sibling "app" isn't +// itself the shared wait node, nor does it depend on it directly -- app's +// own start node depends on mid's start node, which depends on the shared +// db:service_healthy wait node two hops away (app -> mid -> wait). Merging +// app's start node into the wait node's own DependsOn would still close a +// (longer) cycle, so wouldCreateCycle must walk the full closure, not just +// candidate's immediate DependsOn. +func TestPlanStart_TransitiveChainNeverCreatesWaitCycle(t *testing.T) { + db := types.ServiceConfig{Name: "db"} + db.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} + mid := serviceWithDeps("mid", types.DependsOnConfig{ + "db": {Condition: types.ServiceConditionHealthy, Required: true}, + }) + app := serviceWithDeps("app", types.DependsOnConfig{ + "mid": {Condition: types.ServiceConditionStarted, Required: true}, + }) + consumer := serviceWithDeps("consumer", types.DependsOnConfig{ + "app": {Condition: types.ServiceConditionStarted, Required: true}, + "db": {Condition: types.ServiceConditionHealthy, Required: true}, + }) + project := &types.Project{ + Name: "myproject", + Services: types.Services{"db": db, "mid": mid, "app": app, "consumer": consumer}, + } + + plan, err := reconcile(t.Context(), project, emptyObserved(), startScopeOptions(ScopeCreateStart), noPrompt) + assert.NilError(t, err) + assertNoCycle(t, plan) + + var wait, appStart *PlanNode + for _, n := range plan.Nodes { + switch { + case n.Operation.Type == OpWaitCondition: + wait = n + case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:app:1": + appStart = n + } + } + if wait == nil || appStart == nil { + t.Fatalf("expected db's wait node and app's start node:\n%s", plan) + } + assert.Assert(t, !slices.Contains(wait.DependsOn, appStart), + "db's wait node must not depend on app's start node two hops away (app -> mid -> wait) -- that would still be a cycle:\n%s", plan) +} + // pre_start runs once per service before the first replica start, only when // no replica was running at observation; replicas start sequentially, each // chain link (start, then post_start when declared) gating the next. From 650e3f529bfb42ab98d9975d4b121e7b49c754c1 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Wed, 7 Oct 2026 07:56:27 +0200 Subject: [PATCH 21/26] fix(reconcile): keep each dependency's wait timeout independent Reverts 365f0026f, 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 365f0026f'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 --- pkg/compose/reconcile.go | 95 ++---------- pkg/compose/reconcile_start_test.go | 224 ---------------------------- 2 files changed, 12 insertions(+), 307 deletions(-) diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index 6395b317f5..af70e484de 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -1269,52 +1269,24 @@ func (r *reconciler) startPhaseReplicas(service types.ServiceConfig) (replicas [ // gate — errors included: a dependency that is neither declared nor // disabled is the same hard failure it is today. func (r *reconciler) startPhaseDependencies(service types.ServiceConfig) ([]*PlanNode, error) { - deps := sortedKeys(service.DependsOn) - depTargets := make(map[string][]*PlanNode, len(deps)) - var allTargets []*PlanNode - for _, dep := range deps { + var depNodes []*PlanNode + for _, dep := range sortedKeys(service.DependsOn) { + cfg := service.DependsOn[dep] targets := r.startChainEnds[dep] if len(targets) == 0 { if node, ok := r.serviceNodes[dep]; ok { targets = []*PlanNode{node} } } - depTargets[dep] = targets - for _, target := range targets { - if !slices.Contains(allTargets, target) { - allTargets = append(allTargets, target) - } - } - } - - var depNodes []*PlanNode - for _, dep := range deps { - cfg := service.DependsOn[dep] shouldWait, err := shouldWaitForDependency(dep, cfg, r.project) if err != nil { return nil, err } if !shouldWait { - for _, target := range depTargets[dep] { - if !slices.Contains(depNodes, target) { - depNodes = append(depNodes, target) - } - } + depNodes = append(depNodes, targets...) continue } - // The wait node's own exec.waitTimeout window must not open until - // EVERY dependency this service declares has itself finished - // starting, not just the one this particular condition checks -- - // matching the old imperative engine's startService, which only - // calls waitDependencies(X) once every direct dependency's own - // startService call has already returned (see InDependencyOrder's - // graphTraversal.run: a node is dispatched only once none of its - // children are still ServiceStopped, so X's siblings all finish - // starting before X's own wait clock ever starts). Passing just this - // dep's own target here would let an unrelated, slower sibling - // dependency silently shrink the effective grace period this - // condition's health check gets, compared to the old engine. - depNodes = append(depNodes, r.waitConditionNode(dep, cfg, allTargets)) + depNodes = append(depNodes, r.waitConditionNode(dep, cfg, targets)) } return depNodes, nil } @@ -1322,13 +1294,7 @@ func (r *reconciler) startPhaseDependencies(service types.ServiceConfig) ([]*Pla // waitConditionNode returns the shared wait node for (dep, condition), // creating it on first use. required:false marks it best-effort — a missing // dependency is skipped, not fatal; one required dependent upgrades the -// shared node for everyone. targets is now each caller's FULL sibling -// dependency set (see startPhaseDependencies), not just dep's own target, so -// two dependents of the same (dep, condition) with different sibling sets -// routinely diverge here: the merge below unions them onto the one shared -// node rather than creating a second one, at the cost of a dependent with -// fewer siblings waiting a little longer than its own depends_on strictly -// requires -- preferable to losing the dedup this node exists for. +// shared node for everyone. func (r *reconciler) waitConditionNode(dep string, cfg types.ServiceDependency, targets []*PlanNode) *PlanNode { key := dep + ":" + cfg.Condition wait, ok := r.waitNodes[key] @@ -1348,55 +1314,18 @@ func (r *reconciler) waitConditionNode(dep string, cfg types.ServiceDependency, if cfg.Required && wait.Operation.BestEffort { wait.Operation.BestEffort = false } - // merge this caller's prerequisites into the shared node, skipping - // anything that would create a cycle: an ordinary diamond dependency - // (X depends on dep:cond, Z depends on X AND independently on the same - // dep:cond) puts X's own chain end -- which already depends on this very - // wait node -- into Z's sibling set. Adding it back as wait's own - // prerequisite would deadlock the executor forever (runNode never applies - // exec.waitTimeout until its own DependsOn are satisfied, and nothing - // detects a node depending on itself at a distance). Skipping it loses - // nothing: X cannot even start until wait has already succeeded, so by - // the time X's chain completes wait is necessarily already done. + // merge this caller's prerequisites into the shared node. Today every + // caller passes the same targets (startChainEnds[dep] is fixed before + // any dependent is visited), so this is defensive — but relying on that + // silently would break the day the targets diverge per caller. for _, target := range targets { - if slices.Contains(wait.DependsOn, target) { - continue + if !slices.Contains(wait.DependsOn, target) { + wait.DependsOn = append(wait.DependsOn, target) } - if wouldCreateCycle(target, wait) { - continue - } - wait.DependsOn = append(wait.DependsOn, target) } return wait } -// wouldCreateCycle reports whether candidate already (transitively) depends -// on node, via DependsOn edges -- i.e. whether appending candidate to -// node.DependsOn would close a cycle. Only reachable from waitConditionNode's -// merge, where candidate is a sibling target pulled in for an already-shared -// wait node. -func wouldCreateCycle(candidate, node *PlanNode) bool { - if candidate == node { - return true - } - seen := map[*PlanNode]bool{candidate: true} - queue := []*PlanNode{candidate} - for len(queue) > 0 { - n := queue[0] - queue = queue[1:] - for _, dep := range n.DependsOn { - if dep == node { - return true - } - if !seen[dep] { - seen[dep] = true - queue = append(queue, dep) - } - } - } - return false -} - // planProviderStart is the start-phase visit of a provider service: no // container to start, but its own depends_on still applies — the imperative // startService waits on it for every service, providers included. Its chain diff --git a/pkg/compose/reconcile_start_test.go b/pkg/compose/reconcile_start_test.go index 2769d6980e..82b243a603 100644 --- a/pkg/compose/reconcile_start_test.go +++ b/pkg/compose/reconcile_start_test.go @@ -132,230 +132,6 @@ func TestPlanStart_HealthyConditionDeduplicated(t *testing.T) { `)+"\n") } -// TestPlanStart_WaitNodeGatedOnAllSiblingDependencies is a regression test -// for a Copilot review finding on #14290's Create->Start barrier work: a -// wait node's own exec.waitTimeout window used to open as soon as ITS OWN -// dependency's chain finished, ignoring the requesting service's other -// depends_on entries entirely. The old imperative engine never allowed that -// -- InDependencyOrder only calls startService(app), and so only starts -// app's own waitDependencies clock, once EVERY one of app's direct -// dependencies has itself finished starting (see graphTraversal.run: a node -// is dispatched only once none of its children are still ServiceStopped). A -// slow sibling with no wait node of its own (service_started is a plain -// edge, no condition to poll) could silently eat into a health check's -// effective grace period. Here app depends on db:service_healthy (needs a -// wait node) and migrate:service_started (a plain edge, no wait node) -- the -// db wait node must still depend on migrate's own start, or its -// exec.waitTimeout clock starts too early relative to the old engine. -func TestPlanStart_WaitNodeGatedOnAllSiblingDependencies(t *testing.T) { - db := types.ServiceConfig{Name: "db"} - db.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} - migrate := types.ServiceConfig{Name: "migrate"} - app := serviceWithDeps("app", types.DependsOnConfig{ - "db": {Condition: types.ServiceConditionHealthy, Required: true}, - "migrate": {Condition: types.ServiceConditionStarted, Required: true}, - }) - project := &types.Project{ - Name: "myproject", - Services: types.Services{"db": db, "migrate": migrate, "app": app}, - } - - plan, err := reconcile(t.Context(), project, emptyObserved(), startScopeOptions(ScopeCreateStart), noPrompt) - assert.NilError(t, err) - - var wait, dbStart, migrateStart *PlanNode - for _, n := range plan.Nodes { - switch { - case n.Operation.Type == OpWaitCondition: - wait = n - case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:db:1": - dbStart = n - case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:migrate:1": - migrateStart = n - } - } - if wait == nil || dbStart == nil || migrateStart == nil { - t.Fatalf("expected a db wait node and both db/migrate start nodes:\n%s", plan) - } - assert.Assert(t, slices.Contains(wait.DependsOn, dbStart), - "db's wait node must depend on db's own start:\n%s", plan) - assert.Assert(t, slices.Contains(wait.DependsOn, migrateStart), - "db's wait node must also depend on app's other sibling dependency (migrate) finishing its own start, not just db's:\n%s", plan) -} - -// nodeInOwnDependencyClosure reports whether n is reachable from itself by -// following DependsOn edges -- i.e. whether the plan has a dependency cycle -// through n. -func nodeInOwnDependencyClosure(n *PlanNode) bool { - seen := map[*PlanNode]bool{} - queue := slices.Clone(n.DependsOn) - for len(queue) > 0 { - cur := queue[0] - queue = queue[1:] - if cur == n { - return true - } - if seen[cur] { - continue - } - seen[cur] = true - queue = append(queue, cur.DependsOn...) - } - return false -} - -func assertNoCycle(t *testing.T, plan *Plan) { - t.Helper() - for _, n := range plan.Nodes { - if nodeInOwnDependencyClosure(n) { - t.Fatalf("plan has a dependency cycle through node #%d (%s):\n%s", n.ID, n.Operation.ResourceID, plan) - } - } -} - -// TestPlanStart_DiamondDependencyNeverCreatesWaitCycle is a regression test -// for a Copilot review finding on the allTargets widening above: an entirely -// ordinary diamond dependency -- app depends on db:service_healthy, worker -// depends on BOTH app:service_started and, independently, db:service_healthy -// -- shares db's wait node between app and worker (same (dep,condition) -// key). app's own start node already depends on that wait node. Widening the -// wait node's own prerequisites with worker's sibling set (app's start node, -// pulled in because worker also depends on app) would append app's start -// node back into the wait node's own DependsOn -- a direct cycle the -// executor can never resolve (runNode blocks on rs.done[dep.ID] for every -// dependency before closing its own, with nothing to detect or time out a -// cycle). The wait node must keep depending on db alone; worker's own start -// node separately depends on both the wait node and app's start node, which -// already encodes everything worker's depends_on requires -- nothing is -// lost by not also routing it through the shared wait node. -func TestPlanStart_DiamondDependencyNeverCreatesWaitCycle(t *testing.T) { - db := types.ServiceConfig{Name: "db"} - db.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} - app := serviceWithDeps("app", types.DependsOnConfig{ - "db": {Condition: types.ServiceConditionHealthy, Required: true}, - }) - worker := serviceWithDeps("worker", types.DependsOnConfig{ - "app": {Condition: types.ServiceConditionStarted, Required: true}, - "db": {Condition: types.ServiceConditionHealthy, Required: true}, - }) - project := &types.Project{ - Name: "myproject", - Services: types.Services{"db": db, "app": app, "worker": worker}, - } - - plan, err := reconcile(t.Context(), project, emptyObserved(), startScopeOptions(ScopeCreateStart), noPrompt) - assert.NilError(t, err) - assertNoCycle(t, plan) - - var wait, appStart *PlanNode - for _, n := range plan.Nodes { - switch { - case n.Operation.Type == OpWaitCondition: - wait = n - case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:app:1": - appStart = n - } - } - if wait == nil || appStart == nil { - t.Fatalf("expected db's wait node and app's start node:\n%s", plan) - } - assert.Assert(t, !slices.Contains(wait.DependsOn, appStart), - "db's wait node must not depend on app's start node -- app's own start already depends on the wait node, so this would be a cycle:\n%s", plan) - assert.Assert(t, slices.Contains(appStart.DependsOn, wait), - "app's own start must still depend on db's wait node:\n%s", plan) -} - -// TestPlanStart_ExceptionalChainCollapseNeverCreatesWaitCycle is a second -// regression test for the same cycle risk, reproducing the exact shape found -// during review: a steady-state up where every service is already running. -// "a" depends only on db:service_healthy and has nothing left to start, so -// planServiceStart's "nothing to start" branch sets startChainEnds["a"] to -// literally BE the shared db:service_healthy wait node (ends := depNodes). -// "b" depends on both db:service_healthy and a:service_started -- its -// allTargets then contains that very wait node as one of "a"'s targets, and -// merging it into itself would be a direct self-loop with no other node -// in between. -func TestPlanStart_ExceptionalChainCollapseNeverCreatesWaitCycle(t *testing.T) { - db := types.ServiceConfig{Name: "db"} - db.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} - a := serviceWithDeps("a", types.DependsOnConfig{ - "db": {Condition: types.ServiceConditionHealthy, Required: true}, - }) - b := serviceWithDeps("b", types.DependsOnConfig{ - "db": {Condition: types.ServiceConditionHealthy, Required: true}, - "a": {Condition: types.ServiceConditionStarted, Required: true}, - }) - project := &types.Project{ - Name: "myproject", - Services: types.Services{"db": db, "a": a, "b": b}, - } - observed := emptyObserved() - for _, svc := range []types.ServiceConfig{db, a, b} { - hash, err := serviceHashWithResolvedRefs(svc, nil) - assert.NilError(t, err) - observed.Containers[svc.Name] = []ObservedContainer{ - observedServiceContainer(svc.Name, 1, container.StateRunning, hash), - } - } - - plan, err := reconcile(t.Context(), project, observed, startScopeOptions(ScopeCreateStart), noPrompt) - assert.NilError(t, err) - assertNoCycle(t, plan) - - for _, n := range plan.Nodes { - if n.Operation.Type == OpWaitCondition { - assert.Assert(t, !slices.Contains(n.DependsOn, n), - "wait node #%d must not depend on itself:\n%s", n.ID, plan) - } - } -} - -// TestPlanStart_TransitiveChainNeverCreatesWaitCycle exercises -// wouldCreateCycle's BFS beyond a single hop: consumer's sibling "app" isn't -// itself the shared wait node, nor does it depend on it directly -- app's -// own start node depends on mid's start node, which depends on the shared -// db:service_healthy wait node two hops away (app -> mid -> wait). Merging -// app's start node into the wait node's own DependsOn would still close a -// (longer) cycle, so wouldCreateCycle must walk the full closure, not just -// candidate's immediate DependsOn. -func TestPlanStart_TransitiveChainNeverCreatesWaitCycle(t *testing.T) { - db := types.ServiceConfig{Name: "db"} - db.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} - mid := serviceWithDeps("mid", types.DependsOnConfig{ - "db": {Condition: types.ServiceConditionHealthy, Required: true}, - }) - app := serviceWithDeps("app", types.DependsOnConfig{ - "mid": {Condition: types.ServiceConditionStarted, Required: true}, - }) - consumer := serviceWithDeps("consumer", types.DependsOnConfig{ - "app": {Condition: types.ServiceConditionStarted, Required: true}, - "db": {Condition: types.ServiceConditionHealthy, Required: true}, - }) - project := &types.Project{ - Name: "myproject", - Services: types.Services{"db": db, "mid": mid, "app": app, "consumer": consumer}, - } - - plan, err := reconcile(t.Context(), project, emptyObserved(), startScopeOptions(ScopeCreateStart), noPrompt) - assert.NilError(t, err) - assertNoCycle(t, plan) - - var wait, appStart *PlanNode - for _, n := range plan.Nodes { - switch { - case n.Operation.Type == OpWaitCondition: - wait = n - case n.Operation.Type == OpStartContainer && n.Operation.ResourceID == "service:app:1": - appStart = n - } - } - if wait == nil || appStart == nil { - t.Fatalf("expected db's wait node and app's start node:\n%s", plan) - } - assert.Assert(t, !slices.Contains(wait.DependsOn, appStart), - "db's wait node must not depend on app's start node two hops away (app -> mid -> wait) -- that would still be a cycle:\n%s", plan) -} - // pre_start runs once per service before the first replica start, only when // no replica was running at observation; replicas start sequentially, each // chain link (start, then post_start when declared) gating the next. From a408a46d8125984cf2f158b30054984aac71b27d Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Thu, 8 Oct 2026 09:25:24 +0200 Subject: [PATCH 22/26] fix(up): list containers before opening the final --wait-timeout window 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 --- pkg/compose/up.go | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/pkg/compose/up.go b/pkg/compose/up.go index 5f958622eb..3c7a6d72f2 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -135,13 +135,22 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, // The plan may have created or recreated containers: this verification // needs their current IDs, which the pre-execution observed snapshot // above doesn't have for a container that didn't exist yet at - // observation time. It opens its own fresh WaitTimeout window, same as - // every OpWaitCondition node above and start()'s own final --wait check - // did -- not whatever's left of a shared budget, which unrelated plan - // work could already have exhausted with no wait ever at risk. + // observation time. The listing runs on the caller's context, before the + // --wait-timeout window opens, so daemon-listing latency never eats into + // the budget the readiness polling is owed (any deadline it hits is the + // caller's own, unrelated to --wait-timeout, and is returned as such). + containers, err := s.getContainers(ctx, project.Name, oneOffExclude, true) + if err != nil { + return err + } + + // The check then opens its own fresh WaitTimeout window, same as every + // OpWaitCondition node above and start()'s own final --wait check did -- + // not whatever's left of a shared budget, which unrelated plan work could + // already have exhausted with no wait ever at risk. // - // origCtx is kept so the two DeadlineExceeded checks below can tell this - // fresh window expiring (origCtx still fine) apart from origCtx's own, + // origCtx is kept so the DeadlineExceeded check below can tell this fresh + // window expiring (origCtx still fine) apart from origCtx's own, // independent deadline propagating through the derived one (origCtx // already done): a parent deadline firing first makes the derived // context's Err() report DeadlineExceeded too, inherited from the @@ -153,19 +162,6 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, ctx = withTimeout } - containers, err := s.getContainers(ctx, project.Name, oneOffExclude, true) - if err != nil { - // Without a WaitTimeout, ctx above is still whatever the caller - // passed in -- it can carry its own external deadline (an upstream - // command timeout, a cancellation) unrelated to --wait-timeout. - // Translating that into "application not healthy after 0s" would - // name a duration nobody configured. - if options.Start.WaitTimeout > 0 && origCtx.Err() == nil && errors.Is(ctx.Err(), context.DeadlineExceeded) { - return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) - } - return err - } - depends := types.DependsOnConfig{} for _, svc := range project.Services { depends[svc.Name] = types.ServiceDependency{ From 8d1f255d88264c7e786c2513c1e0806e54f8d32f Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Thu, 8 Oct 2026 11:39:58 +0200 Subject: [PATCH 23/26] fix(executor): mark a wait node's own timeout instead of guessing from 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 --- pkg/compose/executor.go | 57 +++++++-- pkg/compose/executor_start_test.go | 180 ++++++++++++++++++++++++++++- pkg/compose/up.go | 28 +---- 3 files changed, 230 insertions(+), 35 deletions(-) diff --git a/pkg/compose/executor.go b/pkg/compose/executor.go index 29df329077..6d82864d32 100644 --- a/pkg/compose/executor.go +++ b/pkg/compose/executor.go @@ -18,6 +18,7 @@ package compose import ( "context" + "errors" "fmt" "sync" "time" @@ -365,6 +366,54 @@ func (exec *planExecutor) runNode(node *PlanNode, rs *runState, phase *phaseCanc return err } +// errWaitTimeout marks the failure of an OpWaitCondition node whose own +// --wait-timeout window (planExecutor.waitTimeout) expired, as opposed to any +// other DeadlineExceeded that can surface from a plan: an engine call timing +// out, a hook, or an external deadline on the caller's context. Callers that +// translate a wait timeout into a user-facing message key on this rather than +// on DeadlineExceeded, which is not specific to waits. The wrapped error still +// satisfies errors.Is(err, context.DeadlineExceeded). +var errWaitTimeout = errors.New("timeout waiting for dependencies") + +// translateWaitTimeout turns a plan failure caused by a wait node's own +// --wait-timeout window (see errWaitTimeout) into the user-facing message the +// imperative start path always reported, and leaves any other error as is. +// "application not healthy" is specifically a --wait message: without --wait, +// a user who only set --wait-timeout as a hang guard (a legal combination +// nothing rejects) would otherwise see a message implying a health check they +// never asked for, and gets the dependency-timeout message waitDependencies +// has always used instead of a raw "context deadline exceeded". +func translateWaitTimeout(err error, wait bool, timeout time.Duration) error { + if !errors.Is(err, errWaitTimeout) { + return err + } + if wait { + return fmt.Errorf("application not healthy after %s", timeout) + } + return errors.New("timeout waiting for dependencies") +} + +// execBoundedWaitCondition runs an OpWaitCondition node under its own fresh +// planExecutor.waitTimeout window, if one is set, and marks a failure caused +// by that window expiring with errWaitTimeout. origCtx.Err() == nil rules out +// an earlier deadline inherited from the caller having fired instead: both +// make the derived context report DeadlineExceeded, but only the former is +// this wait's own timeout. +func (exec *planExecutor) execBoundedWaitCondition(origCtx context.Context, op Operation) error { + ctx := origCtx + if exec.waitTimeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(origCtx, exec.waitTimeout) + defer cancel() + } + err := exec.execWaitCondition(ctx, origCtx, op) + if err != nil && exec.waitTimeout > 0 && origCtx.Err() == nil && + errors.Is(ctx.Err(), context.DeadlineExceeded) && errors.Is(err, context.DeadlineExceeded) { + return fmt.Errorf("%w: %w", errWaitTimeout, err) + } + return err +} + // executeNode dispatches a single plan node to the appropriate API call. func (exec *planExecutor) executeNode(ctx context.Context, node *PlanNode) error { op := node.Operation @@ -394,13 +443,7 @@ func (exec *planExecutor) executeNode(ctx context.Context, node *PlanNode) error case OpCreateHookContainer: return exec.execCreateHookContainer(ctx, node) case OpWaitCondition: - origCtx := ctx - if exec.waitTimeout > 0 { - var cancel context.CancelFunc - ctx, cancel = context.WithTimeout(ctx, exec.waitTimeout) - defer cancel() - } - return exec.execWaitCondition(ctx, origCtx, op) + return exec.execBoundedWaitCondition(ctx, op) case OpRunPreStart: return exec.execRunPreStart(ctx, op) case OpRunPostStart: diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index 5900125683..91e53fdc4c 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -236,8 +236,16 @@ func TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes(t *testing.T) { }, }, nil).AnyTimes() nw := types.NetworkConfig{Name: "test_default"} + // the network create must run on a context with no deadline at all: the + // wait's own window is derived for the OpWaitCondition node only, so a + // deadline showing up here would mean waitTimeout leaked into the plan + var networkCtxHadDeadline atomic.Bool apiClient.EXPECT().NetworkCreate(gomock.Any(), "test_default", gomock.Any()). - Return(client.NetworkCreateResult{ID: "net-id"}, nil) + DoAndReturn(func(ctx context.Context, _ string, _ client.NetworkCreateOptions) (client.NetworkCreateResult, error) { + _, has := ctx.Deadline() + networkCtxHadDeadline.Store(has) + return client.NetworkCreateResult{ID: "net-id"}, nil + }) plan := &Plan{} plan.addNode(Operation{ @@ -262,6 +270,7 @@ func TestExecutePlanWaitDeadlineOnlyBoundsWaitNodes(t *testing.T) { elapsed := time.Since(start) assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.Assert(t, !networkCtxHadDeadline.Load(), "waitTimeout leaked into a non-wait node's context") assert.Assert(t, elapsed < 5*time.Second, "waitTimeout did not bound the stuck OpWaitCondition node (took %s)", elapsed) } @@ -1073,3 +1082,172 @@ func TestExecutePlanFailedPreStartGatesStart(t *testing.T) { "Creating", "Created", api.StatusStarting, preStartErr, }) } + +// TestExecutePlanWaitTimeoutIsMarked verifies that a wait node's own +// --wait-timeout window expiring is reported as errWaitTimeout -- the marker +// upDetached and start() translate into a user-facing message -- while still +// satisfying context.DeadlineExceeded. +func TestExecutePlanWaitTimeoutIsMarked(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + + dbSummary := container.Summary{ + ID: "db-id", + Names: []string{"/test-db-1"}, + Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"}, + } + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(client.ContainerInspectResult{ + Container: container.InspectResponse{ + ID: "db-id", + Name: "/test-db-1", + State: &container.State{ + Status: container.StateRunning, + Health: &container.Health{Status: container.Starting}, + }, + Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}}, + }, + }, nil).AnyTimes() + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: "wait:db:service_healthy", + Name: "db", + Condition: types.ServiceConditionHealthy, + }, "") + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + exec.containersByService["db"] = Containers{dbSummary} + exec.waitTimeout = 200 * time.Millisecond + + err := exec.run(t.Context(), plan) + assert.ErrorIs(t, err, errWaitTimeout) + assert.ErrorIs(t, err, context.DeadlineExceeded) +} + +// TestExecutePlanOtherDeadlinesAreNotWaitTimeouts verifies that a +// DeadlineExceeded that is not a wait node's own window expiring is never +// marked errWaitTimeout, even with --wait-timeout set: an engine call timing +// out in a node that is not a wait, and a deadline inherited from the +// caller's context firing during a wait. Translating either into a +// dependency-readiness message would blame the wrong thing. +func TestExecutePlanOtherDeadlinesAreNotWaitTimeouts(t *testing.T) { + t.Run("engine call timing out in a non-wait node", func(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + apiClient.EXPECT().ContainerStart(gomock.Any(), "db-id", gomock.Any()). + Return(client.ContainerStartResult{}, context.DeadlineExceeded) + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpStartContainer, + ResourceID: "service:db:1", + Container: &container.Summary{ID: "db-id", Names: []string{"/test-db-1"}}, + }, "").Phase = PhaseStart + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + exec.waitTimeout = time.Minute + + err := exec.run(t.Context(), plan) + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.Assert(t, !errors.Is(err, errWaitTimeout), "an engine-call timeout was marked as a wait timeout: %v", err) + }) + + t.Run("deadline inherited from the caller during a wait", func(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + dbSummary := container.Summary{ + ID: "db-id", + Names: []string{"/test-db-1"}, + Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"}, + } + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(client.ContainerInspectResult{ + Container: container.InspectResponse{ + ID: "db-id", + Name: "/test-db-1", + State: &container.State{ + Status: container.StateRunning, + Health: &container.Health{Status: container.Starting}, + }, + Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}}, + }, + }, nil).AnyTimes() + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: "wait:db:service_healthy", + Name: "db", + Condition: types.ServiceConditionHealthy, + }, "") + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + exec.containersByService["db"] = Containers{dbSummary} + exec.waitTimeout = time.Minute // far longer than the caller's own deadline + + ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) + defer cancel() + err := exec.run(ctx, plan) + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.Assert(t, !errors.Is(err, errWaitTimeout), "an inherited deadline was marked as a wait timeout: %v", err) + }) +} + +// TestExecutePlanWaitTimeoutSurvivesDependentSkip verifies the marker is still +// what run() returns when a node depends on the wait that timed out: the +// dependent is skipped with "dependency failed: ...", but it is the wait's own +// failure that is reported, so the caller still translates it. +func TestExecutePlanWaitTimeoutSurvivesDependentSkip(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + + dbSummary := container.Summary{ + ID: "db-id", + Names: []string{"/test-db-1"}, + Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"}, + } + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(client.ContainerInspectResult{ + Container: container.InspectResponse{ + ID: "db-id", + Name: "/test-db-1", + State: &container.State{ + Status: container.StateRunning, + Health: &container.Health{Status: container.Starting}, + }, + Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}}, + }, + }, nil).AnyTimes() + // no ContainerStart expectation: the dependent must never run + + plan := &Plan{} + wait := plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: "wait:db:service_healthy", + Name: "db", + Condition: types.ServiceConditionHealthy, + }, "") + plan.addNode(Operation{ + Type: OpStartContainer, + ResourceID: "service:app:1", + Container: &container.Summary{ID: "app-id", Names: []string{"/test-app-1"}}, + }, "", wait).Phase = PhaseStart + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) + exec.containersByService["db"] = Containers{dbSummary} + exec.waitTimeout = 200 * time.Millisecond + + err := exec.run(t.Context(), plan) + assert.ErrorIs(t, err, errWaitTimeout) +} + +func TestTranslateWaitTimeout(t *testing.T) { + marked := fmt.Errorf("%w: %w", errWaitTimeout, context.DeadlineExceeded) + other := errors.New("boom") + + assert.ErrorContains(t, translateWaitTimeout(marked, true, 3*time.Second), "application not healthy after 3s") + assert.Error(t, translateWaitTimeout(marked, false, 3*time.Second), "timeout waiting for dependencies") + + // anything that is not a wait's own timeout comes back untouched, with or + // without --wait: a bare DeadlineExceeded included + for _, wait := range []bool{true, false} { + assert.Equal(t, translateWaitTimeout(other, wait, time.Second), other) + assert.Equal(t, translateWaitTimeout(context.DeadlineExceeded, wait, time.Second), context.DeadlineExceeded) + } + assert.NilError(t, translateWaitTimeout(nil, true, time.Second)) +} diff --git a/pkg/compose/up.go b/pkg/compose/up.go index 3c7a6d72f2..78962c63b0 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -99,33 +99,7 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, exec.waitTimeout = options.Start.WaitTimeout if err := exec.run(ctx, plan); err != nil { - // options.Start.WaitTimeout alone is unconditional (threaded above - // regardless of Wait, matching start()'s own InDependencyOrder) -- - // but "application not healthy" is specifically a --wait message. - // Without --wait, a user who only set --wait-timeout as a hang - // guard sees the raw error instead of a message implying a health - // check they never asked for. And without a WaitTimeout at all, - // exec.waitTimeout is 0 -- a DeadlineExceeded here can only have - // come from ctx's own external deadline, unrelated to - // --wait-timeout, so naming a "0s" duration nobody configured would - // be just as wrong. ctx.Err() == nil additionally rules out ctx - // itself (the caller-supplied, un-derived context) having its own - // independent deadline expire during exec.run -- that DeadlineExceeded - // didn't come from any OpWaitCondition node's own derived timeout - // either, so it isn't a health-readiness failure. - if options.Start.WaitTimeout > 0 && ctx.Err() == nil && errors.Is(err, context.DeadlineExceeded) { - if options.Start.Wait { - return fmt.Errorf("application not healthy after %s", options.Start.WaitTimeout) - } - // No --wait: this is exactly the dependency-timeout failure - // start()'s own waitDependencies already reports as "timeout - // waiting for dependencies" (service_containers.go) -- matching - // that established, actionable message instead of leaking the - // raw "context deadline exceeded" a user never configured in - // those terms. - return errors.New("timeout waiting for dependencies") - } - return err + return translateWaitTimeout(err, options.Start.Wait, options.Start.WaitTimeout) } if !options.Start.Wait { From b60e75ad04867b0b6a6b38c3739da77597e72f2a Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Thu, 8 Oct 2026 12:17:09 +0200 Subject: [PATCH 24/26] fix(up): report a depends_on wait timeout as a dependency timeout, with --wait too Copilot review finding on #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 --- pkg/compose/executor.go | 18 +++++++----------- pkg/compose/executor_start_test.go | 23 +++++++++++++---------- pkg/compose/up.go | 2 +- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/pkg/compose/executor.go b/pkg/compose/executor.go index 6d82864d32..3f3ed85134 100644 --- a/pkg/compose/executor.go +++ b/pkg/compose/executor.go @@ -376,20 +376,16 @@ func (exec *planExecutor) runNode(node *PlanNode, rs *runState, phase *phaseCanc var errWaitTimeout = errors.New("timeout waiting for dependencies") // translateWaitTimeout turns a plan failure caused by a wait node's own -// --wait-timeout window (see errWaitTimeout) into the user-facing message the -// imperative start path always reported, and leaves any other error as is. -// "application not healthy" is specifically a --wait message: without --wait, -// a user who only set --wait-timeout as a hang guard (a legal combination -// nothing rejects) would otherwise see a message implying a health check they -// never asked for, and gets the dependency-timeout message waitDependencies -// has always used instead of a raw "context deadline exceeded". -func translateWaitTimeout(err error, wait bool, timeout time.Duration) error { +// --wait-timeout window (see errWaitTimeout) into the "timeout waiting for +// dependencies" message the imperative start path always reported for a +// depends_on wait timing out, with or without --wait, and leaves any other +// error as is. It is deliberately not the "application not healthy" message: +// that one belongs to the final --wait readiness check that runs after the +// plan, which has its own translation. +func translateWaitTimeout(err error) error { if !errors.Is(err, errWaitTimeout) { return err } - if wait { - return fmt.Errorf("application not healthy after %s", timeout) - } return errors.New("timeout waiting for dependencies") } diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index 91e53fdc4c..787dd12928 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -680,7 +680,7 @@ func TestExecutePlanRecreateThenStartUsesFinalName(t *testing.T) { Cause: "start", Service: &service, CreateNodeID: create.ID, - }, "start:web:1", rename) + }, "start:web:1", rename).Phase = PhaseStart err := svc.executePlan(t.Context(), project, emptyObservedState("test"), plan) assert.NilError(t, err) @@ -1222,6 +1222,9 @@ func TestExecutePlanWaitTimeoutSurvivesDependentSkip(t *testing.T) { Name: "db", Condition: types.ServiceConditionHealthy, }, "") + // both in the Start phase, like the reconciler plans them: the skipped + // dependent only exists if the wait that fails is in its own phase + wait.Phase = PhaseStart plan.addNode(Operation{ Type: OpStartContainer, ResourceID: "service:app:1", @@ -1240,14 +1243,14 @@ func TestTranslateWaitTimeout(t *testing.T) { marked := fmt.Errorf("%w: %w", errWaitTimeout, context.DeadlineExceeded) other := errors.New("boom") - assert.ErrorContains(t, translateWaitTimeout(marked, true, 3*time.Second), "application not healthy after 3s") - assert.Error(t, translateWaitTimeout(marked, false, 3*time.Second), "timeout waiting for dependencies") + // a depends_on wait timing out is a dependency timeout, whether or not + // --wait was given: "application not healthy" is the final readiness + // check's message, not this one's + assert.Error(t, translateWaitTimeout(marked), "timeout waiting for dependencies") - // anything that is not a wait's own timeout comes back untouched, with or - // without --wait: a bare DeadlineExceeded included - for _, wait := range []bool{true, false} { - assert.Equal(t, translateWaitTimeout(other, wait, time.Second), other) - assert.Equal(t, translateWaitTimeout(context.DeadlineExceeded, wait, time.Second), context.DeadlineExceeded) - } - assert.NilError(t, translateWaitTimeout(nil, true, time.Second)) + // anything that is not a wait's own timeout comes back untouched: a bare + // DeadlineExceeded included + assert.Equal(t, translateWaitTimeout(other), other) + assert.Equal(t, translateWaitTimeout(context.DeadlineExceeded), context.DeadlineExceeded) + assert.NilError(t, translateWaitTimeout(nil)) } diff --git a/pkg/compose/up.go b/pkg/compose/up.go index 78962c63b0..2f6aafef1b 100644 --- a/pkg/compose/up.go +++ b/pkg/compose/up.go @@ -99,7 +99,7 @@ func (s *composeService) upDetached(ctx context.Context, project *types.Project, exec.waitTimeout = options.Start.WaitTimeout if err := exec.run(ctx, plan); err != nil { - return translateWaitTimeout(err, options.Start.Wait, options.Start.WaitTimeout) + return translateWaitTimeout(err) } if !options.Start.Wait { From f5c9bb4850b20f781b0b612c978a2567b2a28d38 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Thu, 8 Oct 2026 12:39:03 +0200 Subject: [PATCH 25/26] fix(reconcile): still visit a scale: 0 service in the start phase Copilot review finding on #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 --- pkg/compose/reconcile.go | 10 ++++- pkg/compose/reconcile_start_test.go | 63 +++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index af70e484de..c1a9421dc3 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -1356,7 +1356,13 @@ func (r *reconciler) planServiceStart(service types.ServiceConfig) error { if service.Provider != nil { return r.planProviderStart(service) } - if service.GetScale() == 0 { + // imperative parity (startService): deploy.replicas: 0 is an unconditional + // no-op, but a service with scale: 0 is still visited -- its depends_on + // conditions are evaluated and its dependents ordered after them -- and + // whatever replicas it already owns are started (scope Start only: the + // create phase removes them all under CreateStart). It is just never an + // error for it to have no container to start. + if service.Deploy != nil && service.Deploy.Replicas != nil && *service.Deploy.Replicas == 0 { return nil } @@ -1370,7 +1376,7 @@ func (r *reconciler) planServiceStart(service types.ServiceConfig) error { if err != nil { return err } - if len(replicas) == 0 && !anyRunning && r.options.Scope == ScopeStart { + if len(replicas) == 0 && !anyRunning && r.options.Scope == ScopeStart && service.GetScale() > 0 { // imperative parity (startService): a scale>0 service with no // container at all cannot be started — only reachable under scope // Start, since CreateStart would have planned the missing creates diff --git a/pkg/compose/reconcile_start_test.go b/pkg/compose/reconcile_start_test.go index 82b243a603..dc4abbd425 100644 --- a/pkg/compose/reconcile_start_test.go +++ b/pkg/compose/reconcile_start_test.go @@ -307,6 +307,69 @@ func TestPlanStart_AllRunningStillWaitsOnConditions(t *testing.T) { assert.Assert(t, !wait.Operation.BestEffort) } +// Imperative parity (startService): a service with scale: 0 is still visited +// -- its depends_on conditions are evaluated before the no-op, so an unhealthy +// dependency still fails `up`/`start` and whatever is ordered after the +// service is ordered after those prerequisites -- and under scope Start the +// replicas it already owns are started. Only deploy.replicas: 0 is an +// unconditional no-op. Having no container is never an error for it. +func TestPlanStart_ScaleZeroIsStillVisited(t *testing.T) { + zero := 0 + db := types.ServiceConfig{Name: "db"} + scaled := serviceWithDeps("app", types.DependsOnConfig{"db": {Condition: types.ServiceConditionHealthy, Required: true}}) + scaled.Scale = &zero + deployed := serviceWithDeps("job", types.DependsOnConfig{"db": {Condition: types.ServiceConditionHealthy, Required: true}}) + deployed.Deploy = &types.DeployConfig{Replicas: &zero} + project := &types.Project{ + Name: "myproject", + Services: types.Services{"db": db, "app": scaled, "job": deployed}, + } + dbHash, err := serviceHashWithResolvedRefs(db, nil) + assert.NilError(t, err) + observed := emptyObserved() + observed.Containers["db"] = []ObservedContainer{observedServiceContainer("db", 1, container.StateRunning, dbHash)} + + // no container for the scale-zero service: its dependency wait is planned + // and nothing is an error; deploy.replicas: 0 plans nothing at all + for _, scope := range []ReconcileScope{ScopeStart, ScopeCreateStart} { + plan, err := reconcile(t.Context(), project, observed, startScopeOptions(scope), noPrompt) + assert.NilError(t, err) + waits := 0 + for _, n := range plan.Nodes { + switch n.Operation.Type { + case OpWaitCondition: + waits++ + assert.Equal(t, n.Operation.Name, "db") + case OpStartContainer, OpCreateContainer: + t.Fatalf("no container expected for scale-zero services, got %s %s:\n%s", n.Operation.Type, n.Operation.ResourceID, plan) + } + } + assert.Equal(t, waits, 1, "one deduplicated db wait, from app only:\n%s", plan) + } + + // scope Start also starts the replicas the scale-zero service still owns + appHash, err := serviceHashWithResolvedRefs(scaled, nil) + assert.NilError(t, err) + observed.Containers["app"] = []ObservedContainer{observedServiceContainer("app", 1, container.StateExited, appHash)} + plan, err := reconcile(t.Context(), project, observed, startScopeOptions(ScopeStart), noPrompt) + assert.NilError(t, err) + var started []string + for _, n := range plan.Nodes { + if n.Operation.Type == OpStartContainer { + started = append(started, n.Operation.ResourceID) + } + } + assert.DeepEqual(t, started, []string{"service:app:1"}) + + // the create phase removes every replica of a scale-zero service before + // the start phase looks: nothing to start under CreateStart + plan, err = reconcile(t.Context(), project, observed, startScopeOptions(ScopeCreateStart), noPrompt) + assert.NilError(t, err) + for _, n := range plan.Nodes { + assert.Assert(t, n.Operation.Type != OpStartContainer, "unexpected start node:\n%s", plan) + } +} + // Imperative parity (startService): under scope Start, a scale>0 service // with no container at all fails the plan the way `compose start` fails. func TestPlanStart_NoContainerToStartFails(t *testing.T) { From 38a069df89ec7c6efde0ffcaa156378518268831 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Thu, 8 Oct 2026 12:40:44 +0200 Subject: [PATCH 26/26] test(executor): bound the phase-cancellation test with its own deadline Copilot review finding on #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 --- pkg/compose/executor_start_test.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index 787dd12928..2981f4c5b2 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -480,15 +480,22 @@ func TestExecutePlanCreatePhaseFailureCancelsCreatePhase(t *testing.T) { exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test"), nil) - err := exec.run(t.Context(), plan) + // run waits for every node, and VolumeCreate blocks until its ctx ends: if + // the sibling failure ever stopped canceling it, run would hang until the + // suite-wide timeout. This deadline turns that into a local failure, and + // the context.Canceled assertion below tells it apart from the sibling + // cancellation the test is about. + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + err := exec.run(ctx, plan) assert.ErrorContains(t, err, "boom") select { case <-volumeCreateDone: - case <-time.After(5 * time.Second): - t.Fatal("VolumeCreate's ctx was never canceled -- a sibling Create-phase failure should still fail fast") + default: + t.Fatal("run returned before VolumeCreate did") } - assert.ErrorIs(t, volumeCreateCtxErr, context.Canceled, "VolumeCreate's ctx must be canceled by a sibling Create-phase node failing") + assert.ErrorIs(t, volumeCreateCtxErr, context.Canceled, "VolumeCreate's ctx must be canceled by a sibling Create-phase node failing, not by the test's own deadline") } // TestExecutePlanCreatePhaseFailureNeverDispatchesStartPhase complements