executor: run start-phase operations (epic #14081, lot 1)

[ndeloof]

Part of #14081 — Lot 1, item 3 ("executor runs start-phase operations").

What this PR does, in one sentence

It teaches the plan executor to actually run the start-phase operations the reconciler has already known how to describe since #14200 — waiting on a dependency's condition, running pre_start hooks, starting a container with the right injections, running post_start hooks — closing the gap between "the plan can say it" and "the plan can do it".

Context

#14200 taught the reconciler to plan the start phase: OpWaitCondition, OpRunPreStart, an enriched OpStartContainer, OpRunPostStart. Nobody consumed them — the executor's dispatch had no case for any of the four, so a plan carrying them would fail with "unknown operation type". The vocabulary existed; nothing could speak it.

What the PR brings

The executor now runs every start-phase operation — by reusing the exact primitive the imperative engine (start.go/service_containers.go) already uses for it, rather than reimplementing it: a wait delegates to the same polling/condition-check functions waitDependencies calls, pre_start/post_start hooks run through the same runPreStart/runHook, and a start injects secrets/configs through the same injectSecrets/injectConfigs before ContainerStart. Behavior cannot drift between the two engines because there is only one implementation of each behavior.

Two things follow from that shared-primitive design:

• One progress line per replica. A replica's pre_start → start → post_start chain reports a single Starting → Started progression — exactly what startServiceContainer reports today — rather than three separate, confusing lines. pkg/api/event.go now documents this per-resource progression contract, so the promise is written down, not just implied by the code.
• A replica the plan itself creates has no daemon-observed container yet. The enriched OpStartContainer resolves its target from the CreateContainer node's result when there is no prior observation — the same mechanism OpRenameContainer already relies on.

Guardrail unchanged from #14200: inert by construction. create() — the only caller of executePlan — never builds a plan with start-phase scope, so none of this runs outside tests yet. It lands now, proven correct in isolation, so the next bricks (the executor lot's remaining piece, then the actual migration lots) can build on a tested foundation instead of growing it and wiring a consumer in the same change.

Why this is the right next brick

This is the second item of Lot 1 of epic #14081 (issue checklist): the vocabulary from #14200 gets an engine to run it before anything is asked to depend on it working.

🤖 Generated with Claude Code

[docker-agent]

Assessment: 🟢 APPROVE

[codecov]

Codecov Report

❌ Patch coverage is 78.16901% with 31 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/compose/executor_ops.go	61.19%	15 Missing and 11 partials ⚠️
pkg/compose/executor_events.go	93.02%	2 Missing and 1 partial ⚠️
pkg/compose/executor.go	93.75%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[ndeloof]

Fixed in 3cecc1e: both execStartContainer's enriched branch and execRunPostStart now bail out on ctx.Err() first, mirroring the guard execCreateHookContainer already had — closing the exact race described (a failed pre_start/start no longer lets its dependent run). Pinned by TestExecutePlanFailedPreStartGatesStart, which reproduces it deterministically.

[docker-agent]

This review covers the commits pushed since ed531fc6b35d (the incremental diff for this cycle).

Assessment: 🟡 NEEDS ATTENTION

The fix correctly applies the ctx.Err() guard pattern (already used by execCreateHookContainer) to execStartContainer and execRunPostStart, and the regression test is well-structured. However, the guard does not fully close the race it targets: close(done[node.ID]) in run() fires inside the failing goroutine before errgroup calls cancel(), so there is a real window where a dependent goroutine sees the done-channel ready but ctx.Err() still returns nil.

[ndeloof]

Good catch — you're right that the ctx.Err() guard narrows the window rather than closing it (close(done[...]) still runs inside the failing goroutine before errgroup's cancel()). That structural fix (carrying each node's success/failure through what dependents wait on, uniformly across every op type — including the already-merged execCreateHookContainer) is exactly what the epic's #14081 "failed-dependency semantics" comment already reserves for a dedicated brick of the executor lot, not this PR's diff. 341a5ac36 spells that out explicitly at both guard sites and in the regression test, so it can't be mistaken for the full fix later.

[docker-agent]

This review covers only the commits pushed since 3cecc1e.

Assessment: 🟢 APPROVE

The incremental diff is purely comment/documentation changes — no code logic was modified. The two comment blocks in executor_ops.go now accurately describe the ctx.Err() checks as "interim narrowing" rather than "guards against" the race, explicitly acknowledge the window where the race still exists, and point to #14081 for the structural fix. The test doc comment in executor_start_test.go updates to matching language.

These comment updates are a direct and accurate response to the two prior review threads on this file (the ctx.Err() race at lines 160 and 287). The technical claims in the new comments are factually correct and consistent with the code.

[docker-agent]

This review covers only the commits pushed since 341a5ac.

Assessment: 🟢 APPROVE

The incremental diff adds a single ctx.Err() guard to execRunPreStart — the same interim-narrowing pattern already present at execStartContainer (line 160) and execRunPostStart (line 296), with an explicit comment acknowledging the residual race window. The guard is correct Go, introduces no new behavior relative to the existing two guards, and is consistent with the codebase convention. The comment accurately describes both the interim nature of the fix and the remaining race window. No bugs introduced by the added lines.

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

[docker-agent]

This review covers only the commits pushed since b1a06a5de263.

Assessment: 🟢 APPROVE

The single change in this incremental diff — replacing startMx.Unlock() with defer startMx.Unlock() immediately after startMx.Lock() in the enriched execStartContainer path — is correct. The defer fires at the same point (return err) on the non-panic path, so runtime behavior is unchanged; it adds panic safety that the bare path already had. No new issues found.

[docker-agent]

This review covers only the commits pushed since de7ce858ad13.

Assessment: 🟢 APPROVE

The incremental changes implement the structural "failed-dependency semantics" fix that previous review cycles deferred (closing the ctx.Err() race window documented in the prior review threads). The implementation is sound:

• The failures map is written before close(done[node.ID]) under failuresMu, so a dependent that unblocks always sees the failure if one was recorded — the critical ordering invariant is correct.
• recordFailure and close(done[node.ID]) are both called on the acquireSlot failure path (the diff adds the missing close call there), preventing dependent goroutine deadlocks.
• Storing the original (unwrapped) error in failures on the failedDependency path correctly prevents cascading "dependency failed:" prefix accumulation across multi-node chains.
• The working bool field on groupState is always accessed under gt.mu, so there are no data races; and the onNodeError Working-before-Error guard keeps event progressions well-formed when OpRunPreStart fails before ContainerStart is reached.
• The ctx.Err() guards in executor_ops.go are now accurately described as defense-in-depth rather than interim narrowings, since the structural fix is in place.

[glours]

The failures/failedDependency mechanism in run() genuinely closes the done-before-cancel race for direct dependencies — traced the happens-before (recordFailure always precedes close(done[node.ID]) on every exit path) and the multi-hop unwrapped-error forwarding (each hop re-records what it read verbatim, no wrapping, so no compounding "dependency failed: " prefixes). The working flag fix on groupState correctly keeps the Working→Error progression well-formed when OpRunPreStart fails before triggering it. Both hold up under code inspection.

One gap: the multi-hop forwarding claim (create→pre_start→start→post_start keeps the original error legible at any depth) is only tested at one hop (TestExecutePlanFailedPreStartGatesStart) — nothing exercises a node relaying an already-short-circuited failure to a further dependent. The code is correct as traced, but a regression (e.g. wrapping inside the failedDependency branch before recordFailure) would ship silently today. Worth a follow-up test with a 3-4 node chain asserting the final error is still the root cause, unwrapped — not a blocker on this PR.