fix(executor): keep dependency-failure comments and wrapping accurate

Two independent reviews (Copilot CLI + a fresh-context agent) of the
previous commit converged on the same finding: execStartContainer's,
execRunPreStart's, and execRunPostStart's ctx.Err() guards still
claimed the TOCTOU window "cannot be closed" and deferred the real fix
to #14081 -- stale now that run()'s failedDependency check closes it
for a direct dependency before executeNode is ever reached. Reworded
to say what's actually still open: defense in depth for ctx canceled
by something other than a direct dependency (an unrelated branch of
the same plan, or an external cancellation).

Also: recordFailure stored the already-wrapped "dependency failed: %w"
error, compounding the prefix at every hop of a multi-node chain.
Store the original error instead, wrapping only the value returned
from this node's own goroutine.

And a stale reference to a test name that was never the one actually
added (TestExecutePlanFailedPreStartGatesStart, not ...GroupDoesNotRestart).

Signed-off-by: Nicolas De Loof <[email protected]>

pkg/compose/executor.go

@@ -124,7 +124,7 @@ func (exec *planExecutor) run(ctx context.Context, plan *Plan) error {
 	// window even though its dependency just failed. Without this, that
 	// dependent runs anyway and spuriously emits its own group Working/
 	// Starting event right after the dependency's Error already went out
-	// (see TestExecutePlanFailedPreStartGatesStartGroupDoesNotRestart).
+	// (see TestExecutePlanFailedPreStartGatesStart).
 	var failuresMu sync.Mutex
 	failures := make(map[int]error, len(plan.Nodes))
 	recordFailure := func(id int, err error) {
@@ -171,9 +171,13 @@ func (exec *planExecutor) run(ctx context.Context, plan *Plan) error {
 			// 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 {
-				recordFailure(node.ID, fmt.Errorf("dependency failed: %w", err))
+				// 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 err
+				return fmt.Errorf("dependency failed: %w", err)
 			}
 
 			if err := acquireSlot(ctx, limiter); err != nil {

pkg/compose/executor_ops.go

@@ -144,19 +144,13 @@ func (exec *planExecutor) execStartContainer(ctx context.Context, op Operation)
 		return err
 	}
 
-	// A dependency's done-channel closes (unblocking this node) before
-	// errgroup cancels ctx on that same dependency's error — cancel() only
-	// runs after the failing goroutine returns, close(done[...]) runs inside
-	// it. This check narrows that window (same guard as
-	// execCreateHookContainer) but cannot close it: a preceding node of this
-	// replica's chain (a wait, pre_start) can fail for a genuine reason a
-	// moment before this goroutine observes ctx.Err(), still nil, and starts
-	// a container whose pre_start hook just failed. The structural fix —
-	// carrying each node's success/failure through what dependents wait on,
-	// instead of inferring it from ctx.Err() — is tracked as its own brick of
-	// the executor lot (#14081, see the epic's "failed-dependency semantics"
-	// comment); every per-operation ctx.Err() guard here, including this one,
-	// is an interim narrowing, not the fix.
+	// run()'s failedDependency check (see its comment) already keeps this
+	// node from ever reaching here when a direct dependency of this replica's
+	// chain (a wait, pre_start) failed for a genuine reason — that window is
+	// closed, not just narrowed. This guard stays as defense in depth for
+	// everything else that can cancel ctx: an unrelated branch of the same
+	// plan failing, or an external cancellation/timeout. Redundant for the
+	// direct-dependency case, authoritative for those others.
 	if err := ctx.Err(); err != nil {
 		return err
 	}
@@ -265,11 +259,10 @@ func (exec *planExecutor) execWaitCondition(ctx context.Context, op Operation) e
 // accepts — skips a second, redundant run of the hooks rather than erroring
 // on runners already consumed.
 func (exec *planExecutor) execRunPreStart(ctx context.Context, op Operation) error {
-	// Same interim narrowing as execStartContainer's guard (see its
-	// comment), not a full fix: this node hangs off the replica's create
-	// node, whose done-channel closes before errgroup's ctx is canceled when
-	// that create fails for a genuine reason, so a narrow window remains
-	// where pre_start hooks could still run against the runner containers.
+	// Same relationship to run()'s failedDependency check as
+	// execStartContainer's guard above (see its comment): closed for this
+	// node's own direct dependency (the replica's create node) failing,
+	// defense in depth for everything else that can cancel ctx.
 	if err := ctx.Err(); err != nil {
 		return err
 	}
@@ -288,11 +281,10 @@ 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 interim narrowing as execStartContainer's guard above, not a full
-	// fix (see its comment): a failed StartContainer's done-channel closes
-	// before errgroup's ctx is canceled, so a narrow window remains where
-	// this could still run post_start against a container whose start just
-	// failed for a genuine reason.
+	// Same relationship to run()'s failedDependency check as
+	// execStartContainer's guard above (see its comment): closed for this
+	// node's own direct dependency (StartContainer) failing, defense in depth
+	// for everything else that can cancel ctx.
 	if err := ctx.Err(); err != nil {
 		return err
 	}