From 87917c0af289995f254ce2eef527e30de8078d5f Mon Sep 17 00:00:00 2001 From: "hiroto.toyoda" Date: Sun, 25 Oct 2026 00:09:48 +0900 Subject: [PATCH 1/2] fix(up): check a depends_on condition before the first poll tick Signed-off-by: hiroto.toyoda --- pkg/compose/executor_start_test.go | 7 +- pkg/compose/service_containers.go | 13 ++- pkg/compose/service_containers_test.go | 95 +++++++++++++++++-- .../TestUpWithDependencyExit/compose.yaml | 2 + 4 files changed, 102 insertions(+), 15 deletions(-) diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index ba951c5d1f5..c9b581c5562 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -399,11 +399,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) diff --git a/pkg/compose/service_containers.go b/pkg/compose/service_containers.go index 3010658ff0e..6bf0a7f1689 100644 --- a/pkg/compose/service_containers.go +++ b/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 diff --git a/pkg/compose/service_containers_test.go b/pkg/compose/service_containers_test.go index 451b371f26f..b654f807ffc 100644 --- a/pkg/compose/service_containers_test.go +++ b/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 { diff --git a/pkg/e2e/testdata/TestUpWithDependencyExit/compose.yaml b/pkg/e2e/testdata/TestUpWithDependencyExit/compose.yaml index 214804abd3a..8baad917935 100644 --- a/pkg/e2e/testdata/TestUpWithDependencyExit/compose.yaml +++ b/pkg/e2e/testdata/TestUpWithDependencyExit/compose.yaml @@ -9,3 +9,5 @@ services: db: image: alpine command: sh -c "exit 1" + healthcheck: + test: ["CMD", "true"] From f3984c4cfbc55107aff3c314cb446e64c5950c5c Mon Sep 17 00:00:00 2001 From: "hiroto.toyoda" Date: Tue, 27 Oct 2026 17:35:11 +0900 Subject: [PATCH 2/2] test(executor): stop timing the caller's cancel against the wait's first inspection Signed-off-by: hiroto.toyoda --- pkg/compose/executor_start_test.go | 122 ++++++++++++++++------------- 1 file changed, 69 insertions(+), 53 deletions(-) diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index c9b581c5562..b106637a413 100644 --- a/pkg/compose/executor_start_test.go +++ b/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" @@ -1551,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