From de85d399ee9892e741a93217d5e11cd787aadcf7 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Wed, 30 Sep 2026 14:53:30 +0200 Subject: [PATCH] fix: a failed pre_start/start no longer lets its dependent run A node's done-channel closes strictly before errgroup cancels its derived context on that same error - the close happens synchronously inside the failing node's goroutine, ctx cancellation only after it returns. A dependent blocked in run()'s select { case <-done[dep.ID]: ...; case <-ctx.Done(): ... } therefore always sees its dependency's done-channel ready first, regardless of whether that dependency failed for a genuine reason or was cancelled. execCreateHookContainer already guarded against exactly this. The two other new dispatch functions from the previous commit did not: execStartContainer's enriched branch could call ContainerStart right after its OpRunPreStart failed for real (e.g. "no hook runner container found"), contradicting pre_start.go's documented guarantee that a non-zero hook exit gates service start - and symmetrically for execRunPostStart after a failed OpStartContainer. Both now bail out on ctx.Err() first, mirroring the existing guard. Pinned by a regression test reproducing the race deterministically: no ContainerStart expectation is registered, so the unguarded code calls it from inside the errgroup worker goroutine, where gomock's missing- expectation failure hangs the test binary instead of failing it cleanly - a stronger tell than a plain assertion failure would have been. Signed-off-by: Nicolas De Loof --- pkg/compose/executor_ops.go | 16 +++++++ pkg/compose/executor_start_test.go | 71 ++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/pkg/compose/executor_ops.go b/pkg/compose/executor_ops.go index 9b3031091..794337756 100644 --- a/pkg/compose/executor_ops.go +++ b/pkg/compose/executor_ops.go @@ -144,6 +144,14 @@ func (exec *planExecutor) execStartContainer(ctx context.Context, op Operation) return err } + // A dependency's done-channel also closes on failure: when a preceding + // node of this replica's chain (a wait, pre_start) failed and canceled + // the run, bail out on the cancellation instead of starting a container + // whose pre_start hook just failed — same race as execCreateHookContainer. + if err := ctx.Err(); err != nil { + return err + } + id, err := exec.resolveContainerID(op) if err != nil { return err @@ -262,6 +270,14 @@ func (exec *planExecutor) execRunPreStart(ctx context.Context, op Operation) err // execRunPostStart runs the service's post_start hooks against the replica // the start chain just brought up. func (exec *planExecutor) execRunPostStart(ctx context.Context, op Operation) error { + // Same race as execCreateHookContainer/execStartContainer: a failed + // StartContainer's done-channel closes before errgroup's ctx is + // canceled, so this must not run post_start against a container that + // never actually started. + if err := ctx.Err(); err != nil { + return err + } + ctr, err := exec.resolveContainerSummary(op) if err != nil { return err diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index 2e61707c2..27852ddad 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -435,3 +435,74 @@ func TestExecutePlanMissingRequiredDependencyFailsSilently(t *testing.T) { assert.ErrorContains(t, err, "missing dependency db") assert.Equal(t, len(recorder.byID), 0, "a missing-dependency failure must not emit a spurious event") } + +// TestExecutePlanFailedPreStartGatesStart is a regression test for a race in +// run()'s DAG walk: a node's done-channel closes (unblocking its dependents) +// strictly before errgroup cancels its derived context on that same error — +// the close happens synchronously inside the failing node's goroutine, ctx +// cancellation only after it returns. A dependent blocked in run()'s +// `select { case <-done[dep.ID]: ...; case <-ctx.Done(): ... }` therefore +// always sees its dependency's done-channel ready first, deterministically, +// regardless of whether that dependency failed for a genuine reason or was +// itself cancelled. +// +// execCreateHookContainer already guards against this with a leading +// ctx.Err() check; this test pins the same guard on execStartContainer's +// enriched branch: when OpRunPreStart fails for a real reason (here: no hook +// runner container found — never a context cancellation), the OpStartContainer +// depending on it must not call ContainerStart. No ContainerStart expectation +// is registered below: an unexpected call fails the test. +func TestExecutePlanFailedPreStartGatesStart(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + + service := types.ServiceConfig{ + Name: "web", + PreStart: []types.PreStartHook{{ContainerSpec: types.ContainerSpec{Command: types.ShellCommand{"init"}}}}, + } + project := &types.Project{Name: "test", Services: types.Services{"web": service}} + + 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: "/test-web-1", + Config: &container.Config{}, + NetworkSettings: &container.NetworkSettings{}, + }}, nil) + // Both the pre_start drift-check and the hook-runner scan list no + // container: no replica is running, and no runner was prepared for the + // declared hook — runPreStart fails with "no hook runner container + // found", a genuine error, not a cancellation. + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). + Return(client.ContainerListResult{}, nil).Times(2) + // No ContainerStart expectation: registering one here would hide the + // bug by silently accepting the call this test exists to forbid. + + plan := &Plan{} + create := plan.addNode(Operation{ + Type: OpCreateContainer, + ResourceID: "service:web:1", + Cause: "no existing container", + Service: &service, + Name: "test-web-1", + Number: 1, + }, "") + preStart := plan.addNode(Operation{ + Type: OpRunPreStart, + ResourceID: "service:web:1", + Cause: "pre_start hooks", + Service: &service, + CreateNodeID: create.ID, + }, "start:web:1", create) + plan.addNode(Operation{ + Type: OpStartContainer, + ResourceID: "service:web:1", + Cause: "start", + Service: &service, + CreateNodeID: create.ID, + }, "start:web:1", preStart) + + err := svc.executePlan(t.Context(), project, emptyObservedState("test"), plan) + assert.ErrorContains(t, err, "no hook runner container found") +}