fix(up): check a depends_on condition before the first poll tick

pkg/compose/executor_start_test.go

@@ -29,6 +29,7 @@ import (
 	"sync"
 	"sync/atomic"
 	"testing"
+	"testing/synctest"
 	"time"
 
 	"github.com/compose-spec/compose-go/v2/types"
@@ -399,11 +400,8 @@ func TestExecutePlanOptionalWaitTimeoutIsToleratedWithoutInheritedDeadline(t *te
 // network create (Phase: PhaseCreate, the default, no DependsOn edge to
 // anything in the Start phase) with an OpStartContainer node (Phase:
 // PhaseStart) and asserts ContainerStart is never called before the network
-// create has fully returned. Uses OpStartContainer rather than
-// OpWaitCondition deliberately: execWaitCondition's underlying poll loop
-// only checks on a 500ms ticker (never on entry), which would make a
-// same-order-of-magnitude Create delay pass this assertion even with no
-// barrier at all, and so couldn't actually catch a regression here.
+// create has fully returned. Uses OpStartContainer: its single
+// ContainerStart call shows exactly when the Start phase began.
 func TestExecutePlanCreatePhaseIsABarrierBeforeStartPhase(t *testing.T) {
 	svc, apiClient := newTestService(t)
 
@@ -1554,61 +1552,76 @@ func TestGroupTrackerOnNodeAborted(t *testing.T) {
 // (Ctrl-C) gets as it was: the plan ends, and no row is rewritten as the
 // victim of a failure there was none of.
 func TestExecutePlanCanceledCallerReportsNothingMore(t *testing.T) {
-	svc, apiClient, recorder := newStartPhaseTestService(t)
-
-	project := &types.Project{Name: "test", Services: types.Services{"db": {Name: "db"}, "api": {Name: "api"}}}
-	dbSummary := container.Summary{
-		ID:     "db-id",
-		Names:  []string{"/test-db-1"},
-		Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"},
-	}
-	apiSummary := container.Summary{
-		ID:     "api-id",
-		Names:  []string{"/test-api-1"},
-		Labels: map[string]string{api.ServiceLabel: "api", api.OneoffLabel: "False"},
-	}
-	ctx, cancel := context.WithCancel(t.Context())
-	defer cancel()
-	apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).
-		DoAndReturn(func(context.Context, string, client.ContainerInspectOptions) (client.ContainerInspectResult, error) {
-			cancel()
-			return client.ContainerInspectResult{Container: container.InspectResponse{
-				ID:   "db-id",
-				Name: "/test-db-1",
-				State: &container.State{
-					Status: container.StateRunning,
-					Health: &container.Health{Status: container.Starting},
-				},
-				Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}},
-			}}, nil
-		}).AnyTimes()
-
-	apiSvc := project.Services["api"]
-	plan := &Plan{}
-	wait := plan.addNode(Operation{
-		Type:       OpWaitCondition,
-		ResourceID: "wait:db:service_healthy",
-		Name:       "db",
-		Condition:  types.ServiceConditionHealthy,
-	}, "")
-	plan.addNode(Operation{
-		Type:       OpStartContainer,
-		ResourceID: "service:api:1",
-		Service:    &apiSvc,
-		Container:  &apiSummary,
-	}, "start:api:1", wait)
-	for _, n := range plan.Nodes {
-		n.Phase = PhaseStart
-	}
+	synctest.Test(t, func(t *testing.T) {
+		svc, apiClient, recorder := newStartPhaseTestService(t)
 
-	exec := svc.newPlanExecutor(project, emptyObservedState("test"))
-	exec.containersByService["db"] = Containers{dbSummary}
-	_ = exec.run(ctx, plan)
+		project := &types.Project{Name: "test", Services: types.Services{"db": {Name: "db"}, "api": {Name: "api"}}}
+		dbSummary := container.Summary{
+			ID:     "db-id",
+			Names:  []string{"/test-db-1"},
+			Labels: map[string]string{api.ServiceLabel: "db", api.OneoffLabel: "False"},
+		}
+		apiSummary := container.Summary{
+			ID:     "api-id",
+			Names:  []string{"/test-api-1"},
+			Labels: map[string]string{api.ServiceLabel: "api", api.OneoffLabel: "False"},
+		}
+		ctx, cancel := context.WithCancel(t.Context())
+		defer cancel()
+		release := make(chan struct{})
+		apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).
+			DoAndReturn(func(context.Context, string, client.ContainerInspectOptions) (client.ContainerInspectResult, error) {
+				<-release
+				return client.ContainerInspectResult{Container: container.InspectResponse{
+					ID:   "db-id",
+					Name: "/test-db-1",
+					State: &container.State{
+						Status: container.StateRunning,
+						Health: &container.Health{Status: container.Starting},
+					},
+					Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}},
+				}}, nil
+			}).AnyTimes()
+
+		apiSvc := project.Services["api"]
+		plan := &Plan{}
+		wait := plan.addNode(Operation{
+			Type:       OpWaitCondition,
+			ResourceID: "wait:db:service_healthy",
+			Name:       "db",
+			Condition:  types.ServiceConditionHealthy,
+		}, "")
+		plan.addNode(Operation{
+			Type:       OpStartContainer,
+			ResourceID: "service:api:1",
+			Service:    &apiSvc,
+			Container:  &apiSummary,
+		}, "start:api:1", wait)
+		for _, n := range plan.Nodes {
+			n.Phase = PhaseStart
+		}
 
-	recorder.mu.Lock()
-	defer recorder.mu.Unlock()
-	assert.DeepEqual(t, recorder.byID["Container test-db-1"], []string{api.StatusWaiting})
-	assert.Equal(t, len(recorder.byID["Container test-api-1"]), 0, "api: %v", recorder.byID["Container test-api-1"])
+		exec := svc.newPlanExecutor(project, emptyObservedState("test"))
+		exec.containersByService["db"] = Containers{dbSummary}
+		ran := make(chan struct{})
+		go func() {
+			defer close(ran)
+			_ = exec.run(ctx, plan)
+		}()
+
+		// the inspection is in flight and api waits on its dependency: cancel
+		// then, and let api see it before the inspection ends
+		synctest.Wait()
+		cancel()
+		synctest.Wait()
+		close(release)
+		<-ran
+
+		recorder.mu.Lock()
+		defer recorder.mu.Unlock()
+		assert.DeepEqual(t, recorder.byID["Container test-db-1"], []string{api.StatusWaiting})
+		assert.Equal(t, len(recorder.byID["Container test-api-1"]), 0, "api: %v", recorder.byID["Container test-api-1"])
+	})
 }
 
 // TestExecutePlanStartFailureClosesAnInterruptedInspection is the same start

pkg/compose/service_containers.go

@@ -223,13 +223,18 @@ func (s *composeService) waitDependency(ctx, origCtx context.Context, dependant,
 func (s *composeService) waitDependencyReached(ctx, origCtx context.Context, dependant, dep string, config types.ServiceDependency, waitingFor Containers) (bool, error) {
 	ticker := time.NewTicker(500 * time.Millisecond)
 	defer ticker.Stop()
-	for {
+	// running_or_healthy accepts a running container without a healthcheck:
+	// checked right after it starts, one that exits at once would pass.
+	checkOnEntry := config.Condition == types.ServiceConditionHealthy || config.Condition == types.ServiceConditionCompletedSuccessfully
+	for first := true; ; first = false {
 		// a tick and the end of ctx can be ready together, and select would
 		// then pick either: ctx wins, so that a canceled wait is never run
 		// through one more check
-		select {
-		case <-ticker.C:
-		case <-ctx.Done():
+		if !first || !checkOnEntry {
+			select {
+			case <-ticker.C:
+			case <-ctx.Done():
+			}
 		}
 		if ctx.Err() != nil {
 			// An expired deadline is precisely the failure this wait is meant

pkg/compose/service_containers_test.go

@@ -23,6 +23,7 @@ import (
 	"net/netip"
 	"strings"
 	"testing"
+	"testing/synctest"
 	"time"
 
 	"github.com/compose-spec/compose-go/v2/types"
@@ -808,6 +809,16 @@ func TestRuntimeAPIVersionRetriesOnTransientError(t *testing.T) {
 	assert.Equal(t, version, "1.44")
 }
 
+// startingDBInspect is db-ctr running with its healthcheck still starting.
+var startingDBInspect = client.ContainerInspectResult{
+	Container: container.InspectResponse{
+		ID:     "db-ctr",
+		Name:   "/db-ctr",
+		Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}},
+		State:  &container.State{Status: container.StateRunning, Health: &container.Health{Status: container.Starting}},
+	},
+}
+
 // 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.
@@ -832,10 +843,14 @@ func TestWaitDependencyDeadline(t *testing.T) {
 		Names:  []string{"/db-ctr"},
 		Labels: map[string]string{api.ServiceLabel: "db"},
 	}}
+	// stillStarting lets the timeout below expire between two checks.
+	stillStarting := func() {
+		apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-ctr", gomock.Any()).Return(startingDBInspect, nil)
+	}
 
 	t.Run("expired deadline is an error", func(t *testing.T) {
-		// Timeout shorter than the first 500ms poll tick: the deadline fires
-		// before any condition check, and must not be swallowed.
+		// The deadline fires between two checks and must not be swallowed.
+		stillStarting()
 		err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers, 50*time.Millisecond)
 		assert.Error(t, err, "timeout waiting for dependencies")
 	})
@@ -849,6 +864,7 @@ func TestWaitDependencyDeadline(t *testing.T) {
 		optionalDeps := types.DependsOnConfig{
 			"db": {Condition: types.ServiceConditionHealthy, Required: false},
 		}
+		stillStarting()
 		err := tested.(*composeService).waitDependencies(t.Context(), &project, "app", optionalDeps, containers, 50*time.Millisecond)
 		assert.NilError(t, err)
 	})
@@ -895,6 +911,71 @@ func TestWaitDependencyDeadline(t *testing.T) {
 	})
 }
 
+// TestWaitDependencyFirstCheck locks when a satisfied dependency is first
+// checked: on entry, except running_or_healthy (see waitDependencyReached).
+func TestWaitDependencyFirstCheck(t *testing.T) {
+	project := types.Project{Name: strings.ToLower(testProject), Services: types.Services{
+		"db": {Name: "db", Scale: intPtr(1)},
+	}}
+	containers := Containers{{
+		ID:     "db-ctr",
+		Names:  []string{"/db-ctr"},
+		Labels: map[string]string{api.ServiceLabel: "db"},
+	}}
+	satisfied := map[string]struct {
+		inspect container.InspectResponse
+		wait    time.Duration
+	}{
+		types.ServiceConditionHealthy: {
+			inspect: container.InspectResponse{
+				ID:     "db-ctr",
+				Name:   "/db-ctr",
+				Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}},
+				State:  &container.State{Status: container.StateRunning, Health: &container.Health{Status: container.Healthy}},
+			},
+			wait: 0,
+		},
+		types.ServiceConditionCompletedSuccessfully: {
+			inspect: container.InspectResponse{
+				ID:    "db-ctr",
+				Name:  "/db-ctr",
+				State: &container.State{Status: container.StateExited, ExitCode: 0},
+			},
+			wait: 0,
+		},
+		ServiceConditionRunningOrHealthy: {
+			inspect: container.InspectResponse{
+				ID:     "db-ctr",
+				Name:   "/db-ctr",
+				Config: &container.Config{},
+				State:  &container.State{Status: container.StateRunning},
+			},
+			wait: 500 * time.Millisecond,
+		},
+	}
+
+	for condition, tc := range satisfied {
+		t.Run(condition, func(t *testing.T) {
+			synctest.Test(t, func(t *testing.T) {
+				mockCtrl := gomock.NewController(t)
+				apiClient := mocks.NewMockAPIClient(mockCtrl)
+				cli := mocks.NewMockCli(mockCtrl)
+				cli.EXPECT().Client().Return(apiClient).AnyTimes()
+				tested, err := NewComposeService(cli)
+				assert.NilError(t, err)
+				apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-ctr", gomock.Any()).
+					Return(client.ContainerInspectResult{Container: tc.inspect}, nil)
+
+				dependencies := types.DependsOnConfig{"db": {Condition: condition, Required: true}}
+				start := time.Now()
+				err = tested.(*composeService).waitDependencies(t.Context(), &project, "app", dependencies, containers, 0)
+				assert.NilError(t, err)
+				assert.Equal(t, time.Since(start), tc.wait)
+			})
+		})
+	}
+}
+
 // TestOptionalDependencyInspectFailure locks what an optional (required:
 // false) dependency's failing inspection means. A genuine inspection error
 // (the daemon doesn't know the container, the API call fails) says the
@@ -1009,8 +1090,8 @@ func TestOptionalDependencyInspectFailure(t *testing.T) {
 					t.Parallel()
 					tested, apiClient, events := newTested(t)
 					inspectFailsWithContext(apiClient)
-					// the first poll (500ms) starts the inspection, which only
-					// ends with the deadline
+					// the first check starts the inspection, which only ends
+					// with the deadline
 					ctx, cancel := context.WithTimeout(t.Context(), time.Second)
 					defer cancel()
 					err := tested.waitDependencies(ctx, &project, "app", dependenciesOn(condition, required), dbContainers, 0)
@@ -1030,8 +1111,10 @@ func TestOptionalDependencyInspectFailure(t *testing.T) {
 					inspectFailsWithContext(apiClient)
 					duringInspect := tested.waitDependencies(t.Context(), &project, "app", dependenciesOn(condition, required), dbContainers, time.Second)
 
-					// shorter than the first poll: expires before any check
-					betweenPolls := tested.waitDependencies(t.Context(), &project, "app", dependenciesOn(condition, required), dbContainers, 50*time.Millisecond)
+					// 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)
 
 					assert.Equal(t, fmt.Sprint(duringInspect), fmt.Sprint(betweenPolls))
 					if required {

pkg/e2e/testdata/TestUpWithDependencyExit/compose.yaml

@@ -9,3 +9,5 @@ services:
   db:
     image: alpine
     command: sh -c "exit 1"
+    healthcheck:
+      test: ["CMD", "true"]