chore(compose): comments state behavior, not history

Eleven comments in the reconciler and its neighbors justified decisions
by pointing at code that no longer exists ('matching the previous
ensureNetwork behavior', 'same way as convergence.go:138-160' — lines
that today hold unrelated code) instead of stating the behavior and its
reason. A reader following those pointers finds nothing, or worse,
something else.

Each is rewritten as a self-contained behavioral statement. Two carry
real weight:
- reconcileService's up-to-date branch now says explicitly that
  starting containers is NOT the plan's job (a separate start phase in
  start.go does it) — the previous wording invited 'fixing' the
  StateCreated case;
- sortContainers now documents that its ordering IS the scale-down
  policy (the tail it produces is what i >= expected trims), instead of
  referencing stale line numbers.

Part of #14074 (A: the code misdescribes its own structure).

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

pkg/compose/create.go

@@ -124,8 +124,8 @@ func (s *composeService) create(ctx context.Context, project *types.Project, opt
 		return err
 	}
 
-	// Emit "Running" events for containers that are already up-to-date,
-	// matching the previous convergence behavior for progress display.
+	// Emit "Running" events for containers that are already up-to-date, so
+	// the progress display accounts for containers the plan will not touch.
 	emitRunningEvents(project, observed, plan, s.events)
 
 	return s.executePlan(ctx, project, observed, plan)
@@ -1461,8 +1461,8 @@ func (s *composeService) createNetwork(ctx context.Context, n *types.NetworkConf
 	if _, err := s.apiClient().NetworkCreate(ctx, n.Name, networkCreateOptions); err != nil {
 		// A concurrent `docker compose up|run` may have created the same network
 		// between the observed-state snapshot and now. Treat the resulting
-		// conflict as success rather than failing hard, mirroring the retry the
-		// previous ensureNetwork performed.
+		// conflict as success rather than failing hard: the network we wanted
+		// exists.
 		if errdefs.IsConflict(err) {
 			s.events.On(createdEvent(networkEventName))
 			return nil

pkg/compose/observed_state.go

@@ -151,8 +151,9 @@ func (s *composeService) collectObservedState(ctx context.Context, project *type
 	}
 
 	// --- Containers ---
-	// Use oneOffInclude to detect orphaned one-off containers (matching the
-	// previous behavior of create() which used oneOffInclude + isOrphaned).
+	// One-off (run) containers are included in the listing on purpose: exited
+	// one-offs are classified as orphans below, so `--remove-orphans` can
+	// clean them up.
 	raw, err := s.getContainers(ctx, project.Name, oneOffInclude, true)
 	if err != nil {
 		return nil, err
@@ -365,8 +366,9 @@ func (s *ObservedState) setResolvedVolumes(volumes map[string]string) {
 }
 
 // emitRunningEvents emits "Running" progress events for containers that are already
-// running and have no operations planned for them. This matches the previous behavior
-// where convergence.ensureService emitted runningEvent for up-to-date containers.
+// running and have no operations planned for them, so the progress display
+// accounts for every container of the project — including the up-to-date ones
+// the plan deliberately leaves alone.
 //
 // Iterates project.Services (not observed.Containers) so that containers of
 // disabled services (e.g. dependencies untouched by `compose run --no-deps`)

pkg/compose/reconcile.go

@@ -192,7 +192,8 @@ func (r *reconciler) resolveObserved() {
 // Divergence is detected by comparing NetworkHash(desired) with the config-hash
 // persisted on the live network (observed.ConfigHash). A network with no
 // recorded hash (e.g. created by an older Compose or manually) is left
-// untouched, matching the previous ensureNetwork behavior.
+// untouched: without a recorded hash there is no reliable way to tell
+// configuration drift apart from deliberate manual setup.
 //
 // A rename (observed.Name != desired.Name) also diverges the hash — NetworkHash
 // includes the name — and is handled by the same recreation path: the old
@@ -242,7 +243,8 @@ func (r *reconciler) planCreateNetwork(key string, networkConfig *types.NetworkC
 // Attached containers must be disconnected before the network can be removed
 // (Docker refuses to remove a network with active endpoints) and are reconnected
 // to the fresh network afterwards — they keep their identity and are not
-// recreated, matching the previous ensureNetwork/removeDivergedNetwork behavior.
+// recreated: a network definition change alone is not a reason to lose
+// container state.
 //
 // Stops are deduplicated through stoppedByPlan so a container attached to several
 // diverged networks (or later recreated by reconcileContainers) is stopped once.
@@ -345,8 +347,9 @@ func (r *reconciler) planRecreateNetworks(keys []string) {
 //
 // Divergence is detected by comparing VolumeHash(desired) with the config-hash
 // persisted on the live volume (observed.ConfigHash). A volume with no recorded
-// hash (e.g. created by an older Compose) is left untouched, matching the
-// previous ensureVolume behavior.
+// hash (e.g. created by an older Compose) is left untouched: without a
+// recorded hash there is no reliable way to tell configuration drift apart
+// from deliberate manual setup, and volumes carry data.
 func (r *reconciler) reconcileVolumes() error {
 	var diverged []string
 	for _, key := range sortedKeys(r.project.Volumes) {
@@ -368,16 +371,16 @@ func (r *reconciler) reconcileVolumes() error {
 		}
 		if observed.Name != desired.Name {
 			// The volume was renamed: the live volume matched by label carries a
-			// different name, i.e. a distinct Docker resource. Match the
-			// historical additive behavior — create the new volume and leave the
-			// old one (and its data) untouched — instead of prompting to delete
-			// data under a name that does not exist yet.
+			// different name, i.e. a distinct Docker resource. A rename is
+			// additive — create the new volume and leave the old one (and its
+			// data) untouched — instead of prompting to delete data under a
+			// name that does not exist yet.
 			r.planCreateVolume(key, &desired, "renamed")
 			// Rewrite the observed name to the desired one so reconcileContainers
 			// detects the mount mismatch and migrates existing containers onto
-			// the new volume within the same up (as the pre-reconcile ensureVolume
-			// path did), and so later runs match deterministically on the new
-			// name rather than split-braining between the two.
+			// the new volume within the same up, and so later runs match
+			// deterministically on the new name rather than split-braining
+			// between the two.
 			observed.Name = desired.Name
 			r.resolvedVolumes[key] = observed
 			continue
@@ -676,8 +679,9 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error {
 	}
 	parentRecreated := r.parentNamespaceRecreated(service)
 
-	// Sort containers: obsolete first, then by number descending, then reverse
-	// to get the same ordering as the existing convergence code.
+	// Order containers so that the scale-down below (i >= expected) trims
+	// obsolete containers and the highest replica numbers first — see
+	// sortContainers for the resulting ordering.
 	r.sortContainers(containers, service, expectedHash, parentRecreated, strategy)
 
 	// Collect dependency nodes that container creation should depend on
@@ -716,7 +720,11 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error {
 		// Container is up-to-date
 		switch oc.State {
 		case container.StateRunning, container.StateCreated, container.StateRestarting, container.StateExited:
-			// Nothing to do (exited containers are left as-is, matching service_containers.go behavior)
+			// Nothing to plan. Starting created/exited containers is NOT the
+			// plan's job: `up` runs a separate start phase afterwards
+			// (start.go), which lists containers again and starts them in
+			// dependency order. Exited containers are deliberately left as-is
+			// here so that phase (or the user) decides.
 		default:
 			// Any other state (paused, dead, ...): attempt to (re)start
 			lastNode = r.plan.addNode(Operation{
@@ -1009,8 +1017,13 @@ func (r *reconciler) infrastructureDeps(service types.ServiceConfig) []*PlanNode
 	return deps
 }
 
-// sortContainers sorts containers the same way as the start path in service_containers.go:
-// obsolete first, then by container number descending, then reversed.
+// sortContainers orders the slice so that, read from the front, up-to-date
+// containers come first in ascending replica-number order, followed by the
+// containers that must be recreated (the comparator sorts obsolete-first and
+// number-descending, then the slice is reversed). Scale-down trims the tail
+// of this slice (i >= expected in reconcileService), so this ordering is what
+// guarantees that obsolete containers and the highest replica numbers are
+// removed first while low-numbered healthy replicas survive.
 //
 // mustRecreate is evaluated once per container before sorting to avoid
 // quadratic re-evaluation in the comparator.