plan the purge of stale pre_start hook runners

[ndeloof]

What this PR does, in one sentence

The reconciliation plan now covers the purge of stale pre_start hook runners, and observed state stops mistaking those runners for service replicas.

Context

pre_start hooks run in ephemeral containers. When a hook fails, its runner is deliberately retained so the operator can inspect it, and the next run purges it — but that purge lives entirely inside the imperative primitive, invisible to the plan. Worse, observed-state collection had no notion of hook containers at all: carrying no container-number label, a leftover runner was classified as a service replica numbered 0.

What the PR brings

• Observed state classifies hook containers into a dedicated HookContainers bucket — they no longer masquerade as replica 0.
• When pre_start is going to run again (hooks declared, no replica running at observation — the exact imperative gating), the plan emits one best-effort RemoveContainer per stale runner, dropping its anonymous volumes. Failure is warn-only and a running service keeps its retained-for-inspection runner untouched, exactly like today; the imperative purge remains in place as backstop, so no behavior is lost in the cases the plan cannot see.
• The imperative primitive is split into a lifecycle-free execution piece (execPreStartHook: start, wait, logs, retain-on-failure) and the create/remove pieces around it, recomposed identically in runPreStart — locked by the existing characterization tests.

Why this is the right next brick

Once the start phase lands in the plan (#14200), hook-container creation and post-success removal can join it as plan nodes chained around a run-only RunPreStart — this PR puts the classification, the purge, and the split primitive in place without depending on it.

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

[codecov]

Codecov Report

❌ Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/compose/pre_start.go	85.71%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

One medium-severity correctness finding confirmed (strong confidence, 100/100).

Summary

The PR correctly classifies hook containers into a dedicated HookContainers bucket so they no longer masquerade as replica-0, and the plan-level purge of stale hook runners is well-gated and best-effort. The refactoring of runPreStartHook into execPreStartHook + caller-managed lifecycle is clean.

One issue was found: the early continue in collectObservedState bypasses the orphan check for hook containers, so stale hook runners from deleted services escape compose up --remove-orphans cleanup.

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

Two findings introduced by this PR's + lines:

Inline comments (2):

1. pkg/compose/reconcile.go:774 [medium] — planPurgeStaleHookRunners purges stale hook runners even when scale=0, contrary to the stated contract (confidence: 🟡 moderate 57/100)
2. pkg/compose/observed_state.go:180 [medium] — Hook containers of disabled services are siloed into HookContainers with no cleanup path (confidence: 🟢 strong 97/100)

[docker-agent]

[medium] Hook containers of disabled services are siloed into HookContainers with no cleanup path

The new classification block guards on knownServices[svcName], and knownServices is populated for both active AND disabled services:

for _, ds := range project.DisabledServices {
    knownServices[ds.Name] = true   // ← disabled services included
}

So a stale pre_start hook container whose ServiceLabel refers to a disabled service hits the new continue and lands in HookContainers[disabled-svc] — never reaching the orphan check below.

planPurgeStaleHookRunners is called only from reconcileService, which is driven by visitInDependencyOrder over NewGraph(project.Services) — disabled services are not in that graph. As a result, HookContainers["disabled-svc"] is filled at collection time and never read again.

Result: stale hook containers of disabled services have no cleanup path:

• Not an orphan → --remove-orphans ignores them.
• Not processed by the plan → planPurgeStaleHookRunners never fires for them.

They accumulate indefinitely until manually removed.

Fix: use a separate activeServices set (active services only, not disabled ones) for the hook-container gate, so disabled-service hook containers still reach the orphan check:

_, isActive := project.Services[svcName]
if ctr.Labels[api.HookLabel] != "" && isActive {

Or inline:

Suggested change

	if ctr.Labels[api.HookLabel] != "" && knownServices[svcName] {
	if ctr.Labels[api.HookLabel] != "" && project.Services[svcName].Name != "" {

Confidence	Score
🟢 strong	97/100

[ndeloof]

Deliberately kept as-is — the accumulation predates this PR rather than being introduced by it: before this change, a disabled service's hook runner sat in Containers[svc] as a bogus replica-0 entry (knownServices includes disabled services), never visited by reconcileService and never orphaned, so --remove-orphans did not clean it either. Routing it to the orphan check would be a behavior CHANGE: --remove-orphans would delete a retained-for-inspection runner while its service is merely profile-disabled — and while the service's real replicas are kept. The PR actually improves the cleanup path: once the service is re-enabled, reconcileService visits it and the plan purges the stale runner (as the imperative purge would). down keeps cleaning them via its label query. Leaving this thread open for the maintainer to weigh in.

[docker-agent]

Assessment: 🟢 APPROVE

The PR is well-structured and the changes are correct.

Summary of changes reviewed:

• observed_state.go: Hook containers (identified by HookLabel) are now correctly classified into HookContainers map keyed by service name, preventing stale runners from masquerading as replica 0. The guard knownServices[svcName] ensures containers for services removed from the model fall through to the orphan path as expected.
• reconcile.go: planPurgeStaleHookRunners correctly mirrors the imperative gate (len(service.PreStart) > 0 && expectedScale > 0 && no running replica). The best-effort/warn-only semantics and the RemoveVolumes: true flag are both correct.
• pre_start.go: The refactor cleanly separates execPreStartHook (start + wait + logs + retain-on-failure) from runPreStart (create → exec → remove-on-success). The success-path removal is now correctly owned by the caller, and the failure retention / cancellation removal paths are unchanged.
• executor_ops.go: The BestEffort check and RemoveVolumes forwarding are placed correctly — a warn-only failure returns nil so the plan continues, and the live-view comment explains the container stays in the cache.
• Test coverage is thorough: executor, observed-state, and reconciler all have dedicated tests for the new behaviour.