Skip to content

Epic: converge the start phase into the plan engine #14081

Description

@ndeloof

"This issue was filed by an AI agent on a human's behalf. The human submitter may not have independently verified the report."

Context

Two lifecycle engines coexist today (#14074, section C). The plan-based reconciler (reconcile.go → executor.go, single entry point create.go) is pure, deterministic and golden-tested — but create-only: containers leave the plan in created state, and OpStartContainer is only ever emitted for exotic states (paused/dead). Everything that makes an application actually start lives in the second, imperative engine (InDependencyOrder): waitDependencies (health/completion polling), secret/config injection, pre_start/post_start hooks. up chains the two with two different daemon snapshots, no canonical project object across the phases, two event-emission systems, and latent bugs at the seam (startMx not held on the path actually exercised; the start-phase ContainerList skips the config-hash filter; dependency-wait timeouts swallowed by ctx.Done() → nil).

This epic tracks converging the start phase into the plan engine, one reviewable PR at a time.

Target architecture

Guiding split: decision vs execution — not "plan vs imperative". The Plan is the pivot execution format of Compose; the reconciler is just one producer of plans.

  • One snapshot, one project, one plan with two phases. ReconcileOptions gains a scope (Create, Start, or both); PlanNode gains a Phase field. up -d builds a single Create+Start plan; start/scale/watch-rebuild use scope Start (never recreating); compose create keeps scope Create unchanged — the existing golden tests do not change.
  • New operations (explicit numbering, 40+):
    • OpWaitCondition — one node per (awaited service, condition ∈ healthy / completed_successfully / running_or_healthy), deduplicated across dependents (a waitNodes map, like networkNodes). required: false is absorbed locally: Skipped event, node succeeds — same pattern as the existing BestEffort. condition: service_started needs no node: a plain DAG edge expresses it. Health is re-observed at execution time (the node runs today's waitDependency polling); ObservedState deliberately does not grow a Health field — it would be stale by construction.
    • OpRunPreStart — per service. Emitted at plan time only when no replica was running at observation (today's rule in startService), targeting the lowest-numbered replica (lowestNumberedContainer).
    • OpRunPostStart — per container, after its start.
  • OpStartContainer enriched: secret/config injection folds into execStartContainer (they always run as a pair right before start — a separate node would be noise), and the target resolves either from an observed container or from the CreateNodeID of a create in the same plan (the reconciliationContext mechanism already used by OpRenameContainer). Side effect by construction: every ContainerStart in the codebase now goes through the one call site that holds startMx.
  • Replica chains: inject→start→post_start of replica n+1 depends on the end of replica n's chain — today's sequential start order, preserved and now visible in golden plans. serviceNodes[svc] points at the end of the chain, so a service_started dependent waits for the whole service, matching InDependencyOrder semantics.
  • Events: on converged paths the executor is the only emitter. Exact parity of observable sequences (Waiting→Healthy|Exited|Skipped, Starting→Started with Started emitted after post_start) via a start:<svc>:<n> group on the existing groupTracker.
  • Interactive up: prepare the plan once; execute the Create phase; set up attach/printer/monitor (upSession unchanged); execute the Start phase under context.WithoutCancel with the printer as listener. The phase boundary replaces today's create/start seam without reintroducing a second snapshot.
  • --wait stays post-plan: it is a final verification with a global timeout and synthetic conditions (getDependencyCondition), running on the shared waitDependency primitive.
  • Shared primitives, no decisions inside: waitDependency, injectSecrets/injectConfigs, runHook, createMobyContainer — consumed by the executor and by what stays imperative (restart, run, --wait). startService/startServiceContainer/waitDependencies are deleted at the end of the series.
  • Executor constraint (to document in code): never add errgroup.SetLimit to the plan executor while it schedules one blocking goroutine per node — instant deadlock. A concurrency cap requires moving to a ready-queue scheduler first.

Non-goals (explicit)

  • stop / down: reverse-order teardown converges later as a plan-builder producing a Plan directly (no reconciler — down without a compose file has no desired state to diff; --rmi, anonymous volumes and duplicate-named networks don't fit a diff model). Separate epic.
  • restart: ContainerRestart is atomic on the daemon side; decomposing it into Stop+Start changes semantics. Stays imperative on the shared primitives.
  • kill / pause / unpause: unordered by design; a plan would introduce an ordering that doesn't exist.
  • run one-off container: by definition outside desired state (number=-1, AutoRemove, unique slug). Its dependencies converge for free (they go through Create/Start); its own creation stays imperative on the shared primitives.
  • Parallel replica starts (relaxing today's sequential order): becomes a trivial edge change after this series; not mixed into it.

PR breakdown

Lot 0 — foundations (no behavior change except deliberate bug fixes; independent, can start immediately)

Lot 1 — vocabulary (the plan learns to start; inert code, no consumer)

Lot 2 — migration, consumer by consumer (increasing risk)

Lot 3 — demolition

  • chore: remove the imperative start path — delete startService/startServiceContainer, the InDependencyOrder start path; move waitDependency helpers to a dedicated file (remaining clients: restart, run, --wait). Separate from the interactive-up switchover so its revert stays trivial.

Critical path: reconciler → executor → detached up → interactive up. Up to and including the detached-up switchover, abandoning the effort still leaves the repo strictly better off (bugs fixed, start path unit-locked, vocabulary tested but inert).

Design decisions where maintainer input is requested before Lot 1

  1. Waits as plan nodes (OpWaitCondition, deduplicated, golden-testable) vs conditional edges — this epic proposes nodes; edges cannot emit the Waiting→Healthy events users see and would evaluate conditions once per dependent.
  2. startMx (fix: hold startMx on the start path actually exercised #14106): the global mutex serializing ContainerStart (engine port-range race) is currently only held on a dead code path. Take it on the real path, or drop it entirely? The engine-side fix is Avoid selecting duplicate host ports for mappings to 0.0.0.0 and specific addresses moby/moby#50054 (Engine 28.3.0, explicitly fixes [BUG] docker engine and desktop expose random ports differently #12846, the issue startMx was working around via run ContainerStart sequentially #12851) — and the moby networking maintainer assessed the original problem was not a start race, so the mutex may never have protected anything. This also gates future replica parallelism.
  3. Phase boundary mechanism: single bi-phase plan executed in two steps (proposed) vs two separate plans over the same snapshot.
  4. scale start scope: today scale db=3 also restarts any stopped container of the project (StartOptions.Services is never read). Reproduce first; changing it would be a separate PR.

Verification

Every PR keeps make test and the e2e suites green. The ~48 observable behaviors inventoried from the imperative engine (silent start idempotence, one-offs untouched, leaf/root ordering, event sequences, integer-second timeouts, --wait honoring service_completed_successfully, …) serve as the non-regression checklist for lot 2; #14104 locks the unit-testable part. Event parity is checked against the e2e checks.go vocabulary, which greps actual output.


Supersedes the first exploration in #14082 and #14083 (closed): this design keeps their wait-as-node and soft-fail ideas, but replaces the startWithPlan/listener-based split with explicit plan phases, folds injection into the start operation, and re-sequences the work so every step is independently mergeable on current main.

Activity

  1. added theissue type on Aug 17, 2026
  2. ndeloof commented on Aug 17, 2026

    @ndeloof
    ContributorAuthor

    Design decisions validated (see the discussion on #14083), now implemented on the prototype branch:

    1. Start-candidates stay a plan-time model — the plan is the single source of truth for what starts, accepting the general observe-to-execute drift window. The one case where drift changes observable behavior gets a targeted runtime guard: RunPreStart re-checks the daemon and skips itself if a replica started meanwhile (plan-time placement, runtime guard) — 05258d8.
    2. Waits are nodes, not edge semantics — data over interpreter: the walker stays decision-free and conditions are visible in plan.String() (inspectable, snapshot-testable). The asymmetry with attach (execution hook, not node) is deliberate: a wait gates convergence, attach observes it. Dedup of identical waits by (dependency, condition, required) is deferred to the up extension, where fan-in makes it worthwhile.
    3. Event parity is a written contract, not a feeling — api.EventProcessor now documents the per-resource progressions, the explicitly-not-guaranteed global interleaving, and status texts as stable identifiers (a182931); a recorder test holds the plan executor to it. The one known divergence (skip granularity for optional dependency failures) is fixed: wait failures emit the historical per-container Skipped/Error events (4ea6832).
  3. ndeloof commented on Aug 20, 2026

    @ndeloof
    ContributorAuthor

    @glours the epic body has been fully rewritten with the selected architecture and a 13-PR breakdown. Lot 0 (characterization tests + standalone bug fixes) is behavior-preserving and will open shortly; before investing in lot 1 (the new plan vocabulary), I'd appreciate a design review of the Target architecture and the four Design decisions sections above — in particular the waits-as-nodes choice and the startMx question.

  4. 42 remaining items

  5. ndeloof commented on Oct 6, 2026

    @ndeloof
    ContributorAuthor

    Follow-up noted from #14274's merge review (#14274 (review), glours, non-blocking):

    run()'s failures/failedDependency mechanism (added in #14274 to close the done-before-cancel race for a direct dependency) forwards each node's original, unwrapped error to its own dependents — verified correct by code inspection, including at the point a node relays an already-short-circuited failure further down the chain. But today it's only exercised at one hop (TestExecutePlanFailedPreStartGatesStart, pre_start → start). Nothing currently tests a longer chain (create → pre_start → start → post_start, the shape Lot 1's replica chains actually produce) relaying a failure 2-3 hops deep while keeping the root error legible and unwrapped.

    Worth a regression test with a 3-4 node chain asserting the final surfaced error is still the original, un-prefixed one — low effort, no design question, just missing coverage for a path a future change to failedDependency/recordFailure could silently regress. Picking it up alongside whichever Lot 1/2 PR next touches run()'s dependency-wait logic, rather than as its own PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions