fix: address the second round of copilot/docker-agent findings

Five more findings landed on the previous commit (three from a fresh
copilot-pull-request-reviewer pass, two from docker-agent):

- **Duplicate hook containers** (observed_state.go): a runner created
  before the ConfigHashLabel exclusion existed (or by an older compose
  version) still carries that label, so it matches getContainers' own
  filters and is already present in `raw` -- appending getHookContainers'
  result unconditionally classified it twice, scheduling two removals for
  one ID. Deduplicated by container ID before classification. Locked in by
  TestCollectObservedState_LegacyHookRunnerNotDuplicated.

- **Hook runners misjudged as a satisfied dependency** (service_containers.go):
  waitDependencies filtered a dependency's containers the same way
  startService's own replica filter used to (isService + isNotOneOff, no
  hook exclusion) -- a dependency's retained/exited runner could be read as
  the dependency itself, either masking a genuinely missing dependency or
  reporting an unhealthy one that's really just a diagnostic leftover.
  Added isNotHookContainer there too.

- **per_replica bypassing the stale-runner purge** (reconcile.go): the
  early return added for #14259 skipped the purge loop entirely, so
  runners from a prior per_replica: false generation would never be
  cleaned up once a hook switched to per_replica: true. Reordered: the
  purge always runs when there's a service to reconcile; only the create
  loop is skipped for per_replica. Locked in by
  TestReconcileContainers_StaleHookRunnersPurgedEvenWhenPerReplica.

- **keptRunning ignores stops already planned elsewhere** (reconcile.go):
  a replica reported running in the observed-state snapshot but already
  scheduled for a stop by planRecreateNetworks or planStopDependents
  (network recreate, a restart: true dependency cascade) would not
  actually be running by the time the start phase evaluates pre_start --
  but keptRunning didn't know that, so no runner got planned and start
  later failed with "no hook runner container found". Only counts a
  replica as kept when stoppedByPlan has no entry for it. Locked in by
  TestReconcileContainers_HookRunnerPlannedWhenCascadeStopsRunningReplica.

- **Success-removal uses the cancellable ctx** (pre_start.go): inconsistent
  with the cancellation-cleanup path in the same file, which already uses
  context.WithoutCancel to guarantee removal survives a Ctrl-C landing in
  the narrow window right after a hook's success. Matched the pattern.

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

pkg/compose/observed_state.go

@@ -174,7 +174,20 @@ func (s *composeService) mergeHookContainers(ctx context.Context, project *types
 	if err != nil {
 		return nil, err
 	}
-	return append(raw, hookRaw...), nil
+	// A legacy runner created before this ConfigHashLabel exclusion existed
+	// (or by an older compose version) can still carry the label, and so
+	// already be present in raw: append only the IDs raw doesn't already
+	// have, or it would be classified -- and scheduled for removal -- twice.
+	seen := make(map[string]bool, len(raw))
+	for _, ctr := range raw {
+		seen[ctr.ID] = true
+	}
+	for _, ctr := range hookRaw {
+		if !seen[ctr.ID] {
+			raw = append(raw, ctr)
+		}
+	}
+	return raw, nil
 }
 
 // The project model is used to classify containers by service and to identify

pkg/compose/observed_state_test.go

@@ -269,6 +269,43 @@ func TestCollectObservedState(t *testing.T) {
 	assert.Equal(t, vol.ConfigHash, "volhash1")
 }
 
+// TestCollectObservedState_LegacyHookRunnerNotDuplicated covers a runner
+// created before the ConfigHashLabel exclusion existed (or by an older
+// compose version): it still carries the label, so it matches getContainers'
+// own filters and is present in BOTH ContainerList responses. It must be
+// classified exactly once, or the reconciler would schedule two removals for
+// the same ID (docker-agent review on #14221).
+func TestCollectObservedState_LegacyHookRunnerNotDuplicated(t *testing.T) {
+	svc, apiClient := newTestService(t)
+	project := &types.Project{Name: "myproject", Services: types.Services{
+		"web": {Name: "web", PreStart: []types.PreStartHook{{}}},
+	}}
+
+	legacyRunner := container.Summary{
+		ID:    "legacy-hook-1",
+		State: container.StateExited,
+		Labels: map[string]string{
+			api.ServiceLabel:    "web",
+			api.ProjectLabel:    "myproject",
+			api.HookLabel:       "pre_start",
+			api.ConfigHashLabel: "stale-legacy-hash",
+		},
+	}
+	apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).Return(client.ContainerListResult{
+		Items: []container.Summary{legacyRunner},
+	}, nil)
+	apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).Return(client.ContainerListResult{
+		Items: []container.Summary{legacyRunner},
+	}, nil)
+	apiClient.EXPECT().NetworkList(gomock.Any(), gomock.Any()).Return(client.NetworkListResult{}, nil)
+	apiClient.EXPECT().VolumeList(gomock.Any(), gomock.Any()).Return(client.VolumeListResult{}, nil)
+
+	state, err := svc.collectObservedState(t.Context(), project)
+	assert.NilError(t, err)
+	assert.Equal(t, len(state.HookContainers["web"]), 1, "the legacy runner must be classified exactly once")
+	assert.Equal(t, len(state.Containers["web"]), 0, "a hook runner must never be classified as a replica")
+}
+
 // TestCollectObservedState_AggregatesDuplicateLabels verifies that two live
 // volumes (or networks) sharing the same compose label are both recorded, with
 // no premature choice — the reconciler resolves the conflict later.

pkg/compose/pre_start.go

@@ -96,14 +96,25 @@ func lowestNumberedContainer(containers Containers) container.Summary {
 // the volumes of the first non-running replica only — anonymous volumes and
 // tmpfs mounts are per-replica and not shared. Use named volumes or bind
 // mounts for data the hook produces.
-func (s *composeService) runPreStart(ctx context.Context, project *types.Project, service types.ServiceConfig, listener api.ContainerEventListener) error {
-	// Validate every hook up front so an unsupported entry never triggers any I/O.
+// perReplicaHookIndex returns the index of the first pre_start hook declaring
+// per_replica: true, or -1. per_replica is not yet supported
+// (docker/compose#14259): runPreStart rejects the whole service up front on
+// it, and planPreStartHookRunners plans no runner for such a service — the
+// two sides of one co-invariant, sharing this single predicate.
+func perReplicaHookIndex(service types.ServiceConfig) int {
 	for i, hook := range service.PreStart {
 		if hook.PerReplica {
-			// per_replica is not yet supported: docker/compose#14259.
-			return fmt.Errorf("service %q pre_start[%d]: per_replica is not yet supported; remove per_replica or set it to false", service.Name, i)
+			return i
 		}
 	}
+	return -1
+}
+
+func (s *composeService) runPreStart(ctx context.Context, project *types.Project, service types.ServiceConfig, listener api.ContainerEventListener) error {
+	// Validate up front so an unsupported entry never triggers any I/O.
+	if i := perReplicaHookIndex(service); i >= 0 {
+		return fmt.Errorf("service %q pre_start[%d]: per_replica is not yet supported; remove per_replica or set it to false", service.Name, i)
+	}
 	runners, err := s.listPreStartRunners(ctx, project.Name, service.Name)
 	if err != nil {
 		return err
@@ -121,8 +132,10 @@ func (s *composeService) runPreStart(ctx context.Context, project *types.Project
 		}
 		// Success: remove the hook container, mirroring the old AutoRemove behaviour
 		// (including its anonymous volumes). A removal failure is logged but does not
-		// gate service start — the hook already succeeded.
-		if _, removeErr := s.apiClient().ContainerRemove(ctx, runner.ID, client.ContainerRemoveOptions{RemoveVolumes: true}); removeErr != nil {
+		// gate service start — the hook already succeeded. context.WithoutCancel, like
+		// the cancellation-cleanup path above: a cancellation landing in the narrow
+		// window between the hook's success and this call must not abort the removal.
+		if _, removeErr := s.apiClient().ContainerRemove(context.WithoutCancel(ctx), runner.ID, client.ContainerRemoveOptions{RemoveVolumes: true}); removeErr != nil {
 			logrus.Warnf("service %q pre_start[%d]: failed to remove hook container %s: %v", service.Name, i, runner.ID, removeErr)
 		}
 	}
@@ -274,6 +287,11 @@ func (s *composeService) createPreStartContainer(
 		return client.ContainerCreateResult{}, err
 	}
 	cfgs, err := s.getCreateConfigs(ctx, project, hookService, 0, nil, createOptions{
+		// hook runners have no spec-drift concept and must stay invisible to
+		// ps/start's default listing and to down's normal teardown path, both
+		// of which filter on ConfigHashLabel's mere presence (see
+		// removePreStartHookContainers)
+		NoConfigHash: true,
 		// AutoRemove is intentionally false: a failed hook container is
 		// retained so the operator can inspect its logs. execPreStartHook
 		// removes it explicitly on success, and the orphan sweep catches
@@ -299,12 +317,6 @@ func (s *composeService) createPreStartContainer(
 	if err != nil {
 		return client.ContainerCreateResult{}, err
 	}
-	// getCreateConfigs unconditionally stamps a ConfigHashLabel (service
-	// containers use it to detect spec drift) -- a hook runner has no such
-	// concept and must stay invisible to ps/start's default listing and to
-	// down's normal teardown path, both of which filter on this label's mere
-	// presence (see removePreStartHookContainers).
-	delete(cfgs.Container.Labels, api.ConfigHashLabel)
 	// Mounts inherit from the live service container: volumes_from carries
 	// its anonymous and image volumes too. The hook's own mounts, already in
 	// the host config from the merged spec, take precedence on shared

pkg/compose/reconcile.go

@@ -852,7 +852,15 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error {
 			// dependency order. Exited containers are deliberately left as-is
 			// here so that phase (or the user) decides.
 			if oc.State == container.StateRunning {
-				keptRunning = true
+				// Observed state is a snapshot from before this plan ran: a
+				// container reported running here may already be scheduled
+				// to stop elsewhere in the plan (a network recreate, a
+				// restart: true dependency cascade, ...) via stoppedByPlan.
+				// It will not be running by the time the start phase
+				// evaluates pre_start, so it does not count as kept.
+				if _, alreadyStopped := r.stoppedByPlan[oc.ID]; !alreadyStopped {
+					keptRunning = true
+				}
 			}
 		default:
 			// Any other state (paused, dead, ...): attempt to (re)start
@@ -919,17 +927,6 @@ func (r *reconciler) planPreStartHookRunners(service types.ServiceConfig, expect
 	if keptRunning {
 		return
 	}
-	for _, hook := range service.PreStart {
-		if hook.PerReplica {
-			// per_replica is not yet supported (docker/compose#14259):
-			// runPreStart validates every hook up front and rejects the
-			// whole service before executing any of them (see runPreStart).
-			// Planning creates here would only orphan runners that can
-			// never run — nothing to prepare until the hook declares
-			// per_replica: false.
-			return
-		}
-	}
 	serviceCopy := service
 	deps := slices.Concat(containerNodes, infraDeps)
 	stale := slices.Clone(r.observed.HookContainers[service.Name])
@@ -952,6 +949,17 @@ func (r *reconciler) planPreStartHookRunners(service types.ServiceConfig, expect
 		// zero must not leave runners of the prior generation behind.
 		return
 	}
+	if perReplicaHookIndex(service) >= 0 {
+		// per_replica is not yet supported (docker/compose#14259):
+		// runPreStart validates every hook up front and rejects the whole
+		// service before executing any of them. Planning creates here would
+		// only orphan runners that can never run — nothing to prepare until
+		// the hook declares per_replica: false. The stale-runner purge
+		// above still runs unconditionally: a user switching a hook to
+		// per_replica: true must not be left with runners from a prior,
+		// non-per_replica generation accumulating forever.
+		return
+	}
 	for i := range service.PreStart {
 		node := r.plan.addNode(Operation{
 			Type:       OpCreateHookContainer,

pkg/compose/reconcile_test.go

@@ -1534,6 +1534,42 @@ func TestReconcileContainers_HookRunnersNotPlannedWhenPerReplica(t *testing.T) {
 `)+"\n")
 }
 
+// per_replica blocking the create loop (above) must not also block the
+// stale-runner purge: a user switching a hook to per_replica: true must not
+// be left with runners from a prior, non-per_replica generation
+// accumulating forever (docker-agent review on #14221).
+func TestReconcileContainers_StaleHookRunnersPurgedEvenWhenPerReplica(t *testing.T) {
+	project := &types.Project{
+		Name: "myproject",
+		Services: types.Services{
+			"app": {
+				Name:  "app",
+				Scale: intPtr(1),
+				PreStart: []types.PreStartHook{
+					{PerReplica: true},
+				},
+			},
+		},
+	}
+	observed := &ObservedState{
+		ProjectName: "myproject",
+		Containers:  map[string][]ObservedContainer{},
+		HookContainers: map[string][]ObservedContainer{
+			"app": {{ID: "stale-id", Summary: container.Summary{ID: "stale-id"}}},
+		},
+		Networks: map[string][]ObservedNetwork{},
+		Volumes:  map[string][]ObservedVolume{},
+	}
+
+	plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt)
+	assert.NilError(t, err)
+
+	assert.Equal(t, plan.String(), strings.TrimSpace(`
+[] -> #1 service:app:1, CreateContainer, no existing container
+[] -> #2 hook:app:stale:stale-id, RemoveContainer, stale pre_start hook container
+`)+"\n")
+}
+
 // A running replica whose config diverged is recreated, so it will not be
 // running when the start phase evaluates pre_start: the runner must be
 // planned. The observed-running gate applies to replicas that SURVIVE the
@@ -1828,6 +1864,42 @@ func TestReconcileContainers_NamespaceParentRecreated_CascadesToDependent(t *tes
 	assert.Equal(t, strings.Count(planStr, "service:dependent:1, StopContainer"), 1, "duplicate Stop for dependent:\n%s", planStr)
 }
 
+// TestReconcileContainers_HookRunnerPlannedWhenCascadeStopsRunningReplica
+// verifies that a replica reported running in the observed-state snapshot,
+// but already scheduled for a stop elsewhere in the plan (here, a
+// restart: true depends_on cascade off a recreated parent), is not counted
+// as "kept running": it will not actually be running by the time the start
+// phase evaluates pre_start, so a fresh runner must still be planned
+// (docker-agent review on #14221 — the observed state is a snapshot from
+// before this plan ran, and stoppedByPlan is the source of truth for what
+// survives it).
+func TestReconcileContainers_HookRunnerPlannedWhenCascadeStopsRunningReplica(t *testing.T) {
+	parent := types.ServiceConfig{Name: "parent", Scale: intPtr(1), ContainerSpec: types.ContainerSpec{Image: "alpine"}}
+	dependent := types.ServiceConfig{
+		Name: "dependent", Scale: intPtr(1),
+		ContainerSpec: types.ContainerSpec{Image: "alpine"},
+		WorkloadSpec: types.WorkloadSpec{
+			DependsOn: types.DependsOnConfig{"parent": {Condition: types.ServiceConditionStarted, Restart: true, Required: true}},
+		},
+		PreStart: []types.PreStartHook{{}},
+	}
+	project := &types.Project{
+		Name:     "myproject",
+		Services: types.Services{"parent": parent, "dependent": dependent},
+	}
+	observed := parentDependentObserved(t, parent, dependent)
+	observed.Containers["parent"][0].ConfigHash = "stale_parent_hash"
+	observed.Containers["parent"][0].Summary.Labels[api.ConfigHashLabel] = "stale_parent_hash"
+	observed.HookContainers = map[string][]ObservedContainer{}
+
+	plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt)
+	assert.NilError(t, err)
+
+	planStr := plan.String()
+	assert.Assert(t, strings.Contains(planStr, "service:dependent:1, StopContainer"), "dependent must be stopped by the cascade:\n%s", planStr)
+	assert.Assert(t, strings.Contains(planStr, "hook:dependent:pre_start:0, CreateHookContainer"), "a runner must still be planned for the stopped-by-cascade dependent:\n%s", planStr)
+}
+
 // TestReconcileContainers_MultipleParents_EitherTriggersCascade guards
 // parentNamespaceRecreated against a future refactor that early-returns on a
 // non-matching parent: a dependent sharing namespace with two parents must

pkg/compose/service_containers.go

@@ -173,7 +173,7 @@ func (s *composeService) waitDependencies(ctx context.Context, project *types.Pr
 			continue
 		}
 
-		waitingFor := containers.filter(isService(dep), isNotOneOff)
+		waitingFor := containers.filter(isService(dep), isNotOneOff, isNotHookContainer)
 		s.events.On(containerEvents(waitingFor, waiting)...)
 		if len(waitingFor) == 0 {
 			if config.Required {

pkg/compose/service_containers_test.go

@@ -292,6 +292,24 @@ func TestWaitDependencies(t *testing.T) {
 		err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, nil, 0)
 		assert.Error(t, err, "app is missing dependency db")
 	})
+	t.Run("a dependency's pre_start hook runner does not count as the dependency being present", func(t *testing.T) {
+		project := types.Project{Name: strings.ToLower(testProject), Services: types.Services{
+			"db": {Name: "db", Scale: intPtr(1), PreStart: []types.PreStartHook{{}}},
+		}}
+		dependencies := types.DependsOnConfig{
+			"db": {Condition: ServiceConditionRunningOrHealthy, Required: true},
+		}
+		// Only a retained hook runner exists (e.g. its own success removal is
+		// still in flight, or it failed and was kept for inspection) — no
+		// real "db" replica container.
+		containers := Containers{{
+			ID:     "db-hook-runner",
+			Names:  []string{"/db-hook-runner"},
+			Labels: map[string]string{api.ServiceLabel: "db", api.HookLabel: "pre_start"},
+		}}
+		err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers, 0)
+		assert.Error(t, err, "app is missing dependency db")
+	})
 	t.Run("missing optional dependency is only a warning", func(t *testing.T) {
 		project := types.Project{Name: strings.ToLower(testProject), Services: types.Services{
 			"db": {Name: "db", Scale: intPtr(1)},