refactor(compose): drop the timeout parameter of waitDependencies

pkg/compose/executor_ops.go

@@ -220,16 +220,14 @@ func (exec *planExecutor) resolveContainerSummary(op Operation) (container.Summa
 // warning, not a plan failure — matching waitDependencies' own
 // optional-dependency handling.
 //
-// Unlike waitDependencies, this has no timeout of its own to apply here --
-// ctx is used exactly as given, already wrapped (or not) by executeNode's
-// OpWaitCondition dispatch in executor.go. origCtx is ctx as it stood right
-// before that wrap, passed through unchanged to waitDependency, which needs
-// it to tell its own wait timeout apart from an inherited deadline (see its
-// doc comment). upDetached (up.go) is the caller that needs a timeout here
-// at all: planExecutor.waitTimeout gives this node its own fresh per-wait
-// window, the same thing start()'s own WaitTimeout already threads through
-// every waitDependencies call -- without also bounding unrelated Create or
-// Start work the way wrapping the whole plan's ctx would.
+// This applies no timeout of its own -- ctx is used exactly as given,
+// already wrapped (or not) by executeNode's OpWaitCondition dispatch in
+// executor.go. origCtx is ctx as it stood right before that wrap, passed
+// through unchanged to waitDependency, which needs it to tell its own wait
+// timeout apart from an inherited deadline (see its doc comment).
+// planExecutor.waitTimeout (set by upDetached and start) is what gives this
+// node its own fresh per-wait window, without also bounding unrelated Create
+// or Start work the way wrapping the whole plan's ctx would.
 func (exec *planExecutor) execWaitCondition(ctx, origCtx context.Context, op Operation) error {
 	s := exec.compose
 	exec.containersMu.Lock()

pkg/compose/restart.go

@@ -55,7 +55,7 @@ func (s *composeService) restart(ctx context.Context, projectName string, option
 
 	return InDependencyOrder(ctx, project, func(c context.Context, service string) error {
 		config := project.Services[service]
-		err := s.waitDependencies(ctx, project, service, config.DependsOn, containers, 0)
+		err := s.waitDependencies(ctx, project, service, config.DependsOn, containers)
 		if err != nil {
 			return err
 		}

pkg/compose/run.go

@@ -182,7 +182,7 @@ func (s *composeService) doPrepareRun(ctx context.Context, project *types.Projec
 	}
 
 	if !options.NoDeps {
-		if err := s.waitDependencies(ctx, project, service.Name, service.DependsOn, observedState, 0); err != nil {
+		if err := s.waitDependencies(ctx, project, service.Name, service.DependsOn, observedState); err != nil {
 			return prepareRunResult{}, err
 		}
 	}

pkg/compose/service_containers.go

@@ -160,13 +160,8 @@ func containerReasonEvents(containers Containers, eventFunc func(string, string)
 // ServiceConditionRunningOrHealthy is a service condition on status running or healthy
 const ServiceConditionRunningOrHealthy = "running_or_healthy"
 
-func (s *composeService) waitDependencies(ctx context.Context, project *types.Project, dependant string, dependencies types.DependsOnConfig, containers Containers, timeout time.Duration) error {
+func (s *composeService) waitDependencies(ctx context.Context, project *types.Project, dependant string, dependencies types.DependsOnConfig, containers Containers) error {
 	origCtx := ctx
-	if timeout > 0 {
-		withTimeout, cancelFunc := context.WithTimeout(ctx, timeout)
-		defer cancelFunc()
-		ctx = withTimeout
-	}
 	eg, ctx := errgroup.WithContext(ctx)
 	for dep, config := range dependencies {
 		if shouldWait, err := shouldWaitForDependency(dep, config, project); err != nil {
@@ -200,7 +195,8 @@ func (s *composeService) waitDependencies(ctx context.Context, project *types.Pr
 // condition is satisfied (done), definitively failed (err), or ctx is
 // cancelled. Each check reports (done, err): (false, nil) means keep polling.
 // origCtx is ctx as it stood before the caller wrapped it with this wait's
-// own timeout (if any) -- comparing origCtx.Err() against ctx.Err() at the
+// own timeout (if any: waitDependencies sets none, the plan's wait nodes do,
+// see execBoundedWaitCondition) -- comparing origCtx.Err() against ctx.Err() at the
 // moment ctx.Done() fires is how this tells apart a DeadlineExceeded this
 // specific call's own wrap is responsible for from one inherited from an
 // ancestor context the caller never touched. A bare "did the caller

pkg/compose/service_containers_test.go

@@ -244,7 +244,7 @@ func TestWaitDependencies(t *testing.T) {
 			"db":    {Condition: ServiceConditionRunningOrHealthy},
 			"redis": {Condition: ServiceConditionRunningOrHealthy},
 		}
-		assert.NilError(t, tested.(*composeService).waitDependencies(t.Context(), &project, "", dependencies, nil, 0))
+		assert.NilError(t, tested.(*composeService).waitDependencies(t.Context(), &project, "", dependencies, nil))
 	})
 	t.Run("should skip zero-replica dependencies after service hashing", func(t *testing.T) {
 		replicas := 0
@@ -267,7 +267,7 @@ func TestWaitDependencies(t *testing.T) {
 		assert.NilError(t, err)
 
 		assert.NilError(t, tested.(*composeService).waitDependencies(
-			t.Context(), &project, "app", project.Services["app"].DependsOn, nil, 0,
+			t.Context(), &project, "app", project.Services["app"].DependsOn, nil,
 		))
 	})
 	t.Run("should skip dependencies with condition service_started", func(t *testing.T) {
@@ -281,7 +281,7 @@ func TestWaitDependencies(t *testing.T) {
 			"db":    {Condition: types.ServiceConditionStarted, Required: true},
 			"redis": {Condition: types.ServiceConditionStarted, Required: true},
 		}
-		assert.NilError(t, tested.(*composeService).waitDependencies(t.Context(), &project, "", dependencies, nil, 0))
+		assert.NilError(t, tested.(*composeService).waitDependencies(t.Context(), &project, "", dependencies, nil))
 	})
 	t.Run("missing required dependency is an error", func(t *testing.T) {
 		project := types.Project{Name: strings.ToLower(testProject), Services: types.Services{
@@ -290,7 +290,7 @@ func TestWaitDependencies(t *testing.T) {
 		dependencies := types.DependsOnConfig{
 			"db": {Condition: ServiceConditionRunningOrHealthy, Required: true},
 		}
-		err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, nil, 0)
+		err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, nil)
 		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) {
@@ -308,7 +308,7 @@ func TestWaitDependencies(t *testing.T) {
 			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)
+		err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers)
 		assert.Error(t, err, "app is missing dependency db")
 	})
 	t.Run("missing optional dependency is only a warning", func(t *testing.T) {
@@ -318,7 +318,7 @@ func TestWaitDependencies(t *testing.T) {
 		dependencies := types.DependsOnConfig{
 			"db": {Condition: ServiceConditionRunningOrHealthy, Required: false},
 		}
-		assert.NilError(t, tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, nil, 0))
+		assert.NilError(t, tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, nil))
 	})
 	t.Run("failing optional dependency is skipped, not an error", func(t *testing.T) {
 		project := types.Project{Name: strings.ToLower(testProject), Services: types.Services{
@@ -341,7 +341,7 @@ func TestWaitDependencies(t *testing.T) {
 				State: &container.State{Status: container.StateExited, ExitCode: 1},
 			},
 		}, nil)
-		assert.NilError(t, tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers, 0))
+		assert.NilError(t, tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers))
 	})
 	t.Run("dry-run completes service_completed_successfully immediately", func(t *testing.T) {
 		tested.(*composeService).dryRun = true
@@ -359,7 +359,7 @@ func TestWaitDependencies(t *testing.T) {
 		}}
 		// no ContainerInspect expectation: dry-run must not inspect a
 		// container that was never created.
-		assert.NilError(t, tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers, 0))
+		assert.NilError(t, tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers))
 	})
 }
 
@@ -819,6 +819,26 @@ var startingDBInspect = client.ContainerInspectResult{
 	},
 }
 
+// waitWithOwnTimeout runs one depends_on wait the way the plan's
+// OpWaitCondition nodes do under --wait-timeout: bounded by a timeout of its
+// own (planExecutor.waitTimeout), the only way a wait gets one. A watchdog
+// ends a wait that would ignore that timeout.
+func waitWithOwnTimeout(t *testing.T, svc *composeService, project *types.Project, containers Containers, condition string, required bool, timeout time.Duration) error {
+	t.Helper()
+	exec := svc.newPlanExecutor(project, emptyObservedState(project.Name))
+	exec.containersByService["db"] = containers
+	exec.waitTimeout = timeout
+	ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second)
+	defer cancel()
+	return exec.execBoundedWaitCondition(ctx, Operation{
+		Type:       OpWaitCondition,
+		ResourceID: "wait:db:" + condition,
+		Name:       "db",
+		Condition:  condition,
+		BestEffort: !required,
+	})
+}
+
 // TestWaitDependencyDeadline locks the timeout semantics of the dependency
 // wait: an expired deadline surfaces as "timeout waiting for dependencies",
 // while a plain user cancellation is not a wait failure.
@@ -849,46 +869,54 @@ func TestWaitDependencyDeadline(t *testing.T) {
 	}
 
 	t.Run("expired deadline is an error", func(t *testing.T) {
-		// The deadline fires between two checks and must not be swallowed.
+		// The caller's deadline (--wait-timeout of up/start --wait) fires
+		// between two checks and must not be swallowed.
 		stillStarting()
-		err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers, 50*time.Millisecond)
+		ctx, cancel := context.WithTimeout(t.Context(), 50*time.Millisecond)
+		defer cancel()
+		err := tested.(*composeService).waitDependencies(ctx, &project, "app", dependencies, containers)
 		assert.Error(t, err, "timeout waiting for dependencies")
 	})
 
-	// Regression guard: an optional dependency's deadline expiring (new as of
-	// planExecutor.waitTimeout giving execWaitCondition a real timeout for
-	// the first time -- see waitDependency's ctx.Done() branch) must be
-	// tolerated exactly like any other definitive failure of an optional
-	// dependency, not surfaced as "timeout waiting for dependencies".
-	t.Run("expired deadline on an optional dependency is skipped, not an error", func(t *testing.T) {
-		optionalDeps := types.DependsOnConfig{
-			"db": {Condition: types.ServiceConditionHealthy, Required: false},
-		}
+	t.Run("expired own timeout of a plan wait is an error", func(t *testing.T) {
+		// The same, for the timeout of the plan's own wait node.
+		stillStarting()
+		err := waitWithOwnTimeout(t, tested.(*composeService), &project, containers, types.ServiceConditionHealthy, true, 50*time.Millisecond)
+		assert.Assert(t, errors.Is(err, errWaitTimeout), "got %v", err)
+		assert.ErrorIs(t, err, context.DeadlineExceeded)
+	})
+
+	// Regression guard: an optional dependency's deadline expiring (the
+	// timeout of the plan's own wait node, planExecutor.waitTimeout -- see
+	// waitDependency's ctx.Done() branch) must be tolerated exactly like any
+	// other definitive failure of an optional dependency, not surfaced as
+	// "timeout waiting for dependencies".
+	t.Run("expired own timeout on an optional dependency is skipped, not an error", func(t *testing.T) {
 		stillStarting()
-		err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", optionalDeps, containers, 50*time.Millisecond)
+		err := waitWithOwnTimeout(t, tested.(*composeService), &project, containers, types.ServiceConditionHealthy, false, 50*time.Millisecond)
 		assert.NilError(t, err)
 	})
 
 	// Regression guard: the tolerance above must apply only to a deadline
-	// THIS call's own timeout parameter is responsible for -- an unrelated,
-	// already-expired deadline the caller brought in (timeout == 0, so this
-	// call set no deadline of its own) is a different, real failure that
-	// must still propagate even for an optional dependency.
+	// THIS wait's own timeout is responsible for -- an unrelated,
+	// already-expired deadline the caller brought in (waitDependencies sets
+	// none of its own) is a different, real failure that must still
+	// propagate even for an optional dependency.
 	t.Run("an inherited parent deadline is not swallowed for an optional dependency", func(t *testing.T) {
 		optionalDeps := types.DependsOnConfig{
 			"db": {Condition: types.ServiceConditionHealthy, Required: false},
 		}
 		parentCtx, cancel := context.WithTimeout(t.Context(), time.Millisecond)
 		defer cancel()
 		time.Sleep(10 * time.Millisecond) // let the parent's own deadline actually expire first
-		err := tested.(*composeService).waitDependencies(parentCtx, &project, "app", optionalDeps, containers, 0)
+		err := tested.(*composeService).waitDependencies(parentCtx, &project, "app", optionalDeps, containers)
 		assert.Error(t, err, "timeout waiting for dependencies")
 	})
 
 	t.Run("user cancellation is not a wait failure", func(t *testing.T) {
 		ctx, cancel := context.WithCancel(t.Context())
 		cancel()
-		err := tested.(*composeService).waitDependencies(ctx, &project, "app", dependencies, containers, 0)
+		err := tested.(*composeService).waitDependencies(ctx, &project, "app", dependencies, containers)
 		assert.NilError(t, err)
 	})
 
@@ -899,7 +927,10 @@ func TestWaitDependencyDeadline(t *testing.T) {
 		unsupportedDeps := types.DependsOnConfig{
 			"db": {Condition: "some_future_condition", Required: true},
 		}
-		err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", unsupportedDeps, containers, 2*time.Second)
+		// the bound makes a regression that waits fail instead of hanging
+		ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second)
+		defer cancel()
+		err := tested.(*composeService).waitDependencies(ctx, &project, "app", unsupportedDeps, containers)
 		assert.NilError(t, err)
 
 		var messages []string
@@ -968,7 +999,7 @@ func TestWaitDependencyFirstCheck(t *testing.T) {
 
 				dependencies := types.DependsOnConfig{"db": {Condition: condition, Required: true}}
 				start := time.Now()
-				err = tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers, 0)
+				err = tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers)
 				assert.NilError(t, err)
 				assert.Equal(t, time.Since(start), tc.wait)
 			})
@@ -1039,7 +1070,7 @@ func TestOptionalDependencyInspectFailure(t *testing.T) {
 				tested, apiClient, events := newTested(t)
 				apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-ctr", gomock.Any()).
 					Return(client.ContainerInspectResult{}, errors.New("Error response from daemon: No such container: db-ctr"))
-				err := tested.waitDependencies(t.Context(), &project, "app", dependenciesOn(condition, false), dbContainers, 0)
+				err := tested.waitDependencies(t.Context(), &project, "app", dependenciesOn(condition, false), dbContainers)
 				assert.NilError(t, err)
 				assert.Assert(t, wasSkipped(events))
 			})
@@ -1049,7 +1080,7 @@ func TestOptionalDependencyInspectFailure(t *testing.T) {
 				tested, apiClient, events := newTested(t)
 				apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-ctr", gomock.Any()).
 					Return(client.ContainerInspectResult{}, errors.New("Error response from daemon: No such container: db-ctr"))
-				err := tested.waitDependencies(t.Context(), &project, "app", dependenciesOn(condition, true), dbContainers, 0)
+				err := tested.waitDependencies(t.Context(), &project, "app", dependenciesOn(condition, true), dbContainers)
 				assert.ErrorContains(t, err, "No such container: db-ctr")
 				assert.Assert(t, !wasSkipped(events))
 			})
@@ -1066,7 +1097,7 @@ func TestOptionalDependencyInspectFailure(t *testing.T) {
 					}).MinTimes(1) // a pending poll tick may win the select once more
 				// a plain cancellation is not a wait failure (see
 				// TestWaitDependencyDeadline), but it is no skip either
-				err := tested.waitDependencies(ctx, &project, "app", dependenciesOn(condition, false), dbContainers, 0)
+				err := tested.waitDependencies(ctx, &project, "app", dependenciesOn(condition, false), dbContainers)
 				assert.NilError(t, err)
 				assert.Assert(t, !wasSkipped(events))
 			})
@@ -1081,7 +1112,7 @@ func TestOptionalDependencyInspectFailure(t *testing.T) {
 						cancel()
 						return client.ContainerInspectResult{}, fmt.Errorf("inspect db-ctr: %w", context.Canceled)
 					})
-				err := tested.waitDependencies(ctx, &project, "app", dependenciesOn(condition, true), dbContainers, 0)
+				err := tested.waitDependencies(ctx, &project, "app", dependenciesOn(condition, true), dbContainers)
 				assert.Assert(t, errors.Is(err, context.Canceled), "got %v", err)
 			})
 
@@ -1094,7 +1125,7 @@ func TestOptionalDependencyInspectFailure(t *testing.T) {
 					// with the deadline
 					ctx, cancel := context.WithTimeout(t.Context(), time.Second)
 					defer cancel()
-					err := tested.waitDependencies(ctx, &project, "app", dependenciesOn(condition, required), dbContainers, 0)
+					err := tested.waitDependencies(ctx, &project, "app", dependenciesOn(condition, required), dbContainers)
 					assert.Error(t, err, "timeout waiting for dependencies")
 					assert.Assert(t, !wasSkipped(events))
 				})
@@ -1109,16 +1140,22 @@ func TestOptionalDependencyInspectFailure(t *testing.T) {
 					t.Parallel()
 					tested, apiClient, _ := newTested(t)
 					inspectFailsWithContext(apiClient)
-					duringInspect := tested.waitDependencies(t.Context(), &project, "app", dependenciesOn(condition, required), dbContainers, time.Second)
+					duringInspect := waitWithOwnTimeout(t, tested, &project, dbContainers, condition, required, time.Second)
 
 					// no check in flight when the timeout expires: at most the one on entry
 					polled, polledClient, _ := newTested(t)
 					polledClient.EXPECT().ContainerInspect(gomock.Any(), "db-ctr", gomock.Any()).Return(startingDBInspect, nil).MaxTimes(1)
-					betweenPolls := polled.waitDependencies(t.Context(), &project, "app", dependenciesOn(condition, required), dbContainers, 50*time.Millisecond)
+					betweenPolls := waitWithOwnTimeout(t, polled, &project, dbContainers, condition, required, 50*time.Millisecond)
 
-					assert.Equal(t, fmt.Sprint(duringInspect), fmt.Sprint(betweenPolls))
+					// the same end, whatever the instant the timeout expired at
+					assert.Equal(t, duringInspect == nil, betweenPolls == nil)
+					assert.Equal(t, errors.Is(duringInspect, errWaitTimeout), errors.Is(betweenPolls, errWaitTimeout))
+					assert.Equal(t, errors.Is(duringInspect, context.DeadlineExceeded), errors.Is(betweenPolls, context.DeadlineExceeded))
 					if required {
-						assert.Error(t, duringInspect, "timeout waiting for dependencies")
+						assert.Assert(t, errors.Is(duringInspect, errWaitTimeout), "got %v", duringInspect)
+						assert.ErrorIs(t, duringInspect, context.DeadlineExceeded)
+					} else {
+						assert.NilError(t, duringInspect)
 					}
 				})
 			}

pkg/compose/start.go

@@ -134,7 +134,7 @@ func (s *composeService) start(ctx context.Context, projectName string, options
 			Required:  true,
 		}
 	}
-	if err := s.waitDependencies(ctx, project, project.Name, depends, containers, 0); err != nil {
+	if err := s.waitDependencies(ctx, project, project.Name, depends, containers); err != nil {
 		if options.WaitTimeout > 0 && origCtx.Err() == nil && errors.Is(ctx.Err(), context.DeadlineExceeded) {
 			return fmt.Errorf("application not healthy after %s", options.WaitTimeout)
 		}