From 35328d56e58c600d5fcc62b3658e3b87072aad7b Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 27 Oct 2026 14:26:07 +0100 Subject: [PATCH 1/8] test(monitor): lock which containers the monitor follows The monitor ends with the application it follows: pin that its initial listing and its event subscription are scoped to the project's own containers (no one-offs, no hook runners without a config hash), and that a provider's relay, which never terminates, is not waited for, whether it is there at the start or created later. Part of #14081. Signed-off-by: Nicolas De Loof --- pkg/compose/monitor_test.go | 80 ++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/pkg/compose/monitor_test.go b/pkg/compose/monitor_test.go index a3d1d5ccff..750af5e3b1 100644 --- a/pkg/compose/monitor_test.go +++ b/pkg/compose/monitor_test.go @@ -264,6 +264,14 @@ func monitorDieEvent(exitCode int) events.Message { // newMonitorTestFixture wires a monitor against a mocked API client, with the // goroutine-leak guard and the standard initial ContainerList expectation. func newMonitorTestFixture(t *testing.T) (*monitor, *mocks.MockAPIClient) { + t.Helper() + return newMonitorTestFixtureListing(t, []container.Summary{testContainer("service1", "123", false)}, nil) +} + +// newMonitorTestFixtureListing is newMonitorTestFixture for given containers +// in the initial listing, whose request it records in listed when it is not +// nil. +func newMonitorTestFixtureListing(t *testing.T, initial []container.Summary, listed *client.ContainerListOptions) (*monitor, *mocks.MockAPIClient) { t.Helper() ignoreExisting := goleak.IgnoreCurrent() t.Cleanup(func() { @@ -275,7 +283,12 @@ func newMonitorTestFixture(t *testing.T) (*monitor, *mocks.MockAPIClient) { expectNowAsDaemonTime(apiMock) apiMock.EXPECT().ContainerList(gomock.Any(), gomock.Any()). - Return(client.ContainerListResult{Items: []container.Summary{testContainer("service1", "123", false)}}, nil) + DoAndReturn(func(_ context.Context, opts client.ContainerListOptions) (client.ContainerListResult, error) { + if listed != nil { + *listed = opts + } + return client.ContainerListResult{Items: initial}, nil + }) m := newMonitor(apiMock, strings.ToLower(testProject)) return m, apiMock @@ -952,3 +965,68 @@ func TestMonitorReadDaemonTimeOnce(t *testing.T) { assert.DeepEqual(t, log.since, []string{"981153306.789012345"}) } + +// TestMonitorFollowsTheContainersOfTheApplicationOnly pins which containers +// the monitor follows: the project's containers that are not one-off, and, in +// the initial listing, that carry a config hash, running or not. The hook +// runners of pre_start carry none: a runner's exit is not the end of the +// application. +func TestMonitorFollowsTheContainersOfTheApplicationOnly(t *testing.T) { + var listed client.ContainerListOptions + m, apiMock := newMonitorTestFixtureListing(t, []container.Summary{testContainer("service1", "123", false)}, &listed) + var subscribed client.EventsListOptions + messages := make(chan events.Message, 1) + messages <- monitorEvent(events.ActionDestroy) + apiMock.EXPECT().Events(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, opts client.EventsListOptions) client.EventsResult { + subscribed = opts + return client.EventsResult{Messages: messages, Err: make(chan error)} + }) + + _, err := runMonitor(t, m) + + assert.NilError(t, err) + project := labelFilter(api.ProjectLabel, strings.ToLower(testProject)) + assert.Assert(t, listed.All, "stopped containers are part of the application") + labels := listed.Filters["label"] + assert.Assert(t, labels[project], "listing filters: %v", listed.Filters) + assert.Assert(t, labels[oneOffFilter(false)], "one-off containers are not part of the application: %v", listed.Filters) + assert.Assert(t, labels[api.ConfigHashLabel], "a container without a config hash (a hook runner) is not: %v", listed.Filters) + labels = subscribed.Filters["label"] + assert.Assert(t, subscribed.Filters["type"]["container"], "events filters: %v", subscribed.Filters) + assert.Assert(t, labels[project], "events filters: %v", subscribed.Filters) + assert.Assert(t, labels[oneOffFilter(false)], "the events of one-off containers are not wanted: %v", subscribed.Filters) +} + +// TestMonitorDoesNotWaitForARelay pins that the relay container a provider +// service may leave in the project -- it never terminates on its own -- is not +// part of the application the monitor follows, whether it is there at the +// start or created later: the monitor ends with the application's own +// containers, instead of waiting forever for the relay. +func TestMonitorDoesNotWaitForARelay(t *testing.T) { + relayLabels := func() map[string]string { + labels := containerLabels("provider", false) + labels[api.RelayLabel] = "relay-identity" + return labels + } + + t.Run("already there at the start", func(t *testing.T) { + relay := container.Summary{ID: "relay", Labels: relayLabels()} + m, apiMock := newMonitorTestFixtureListing(t, []container.Summary{testContainer("service1", "123", false), relay}, nil) + expectEvents(apiMock, monitorEvent(events.ActionDestroy)) + + _, err := runMonitor(t, m) + + assert.NilError(t, err) + }) + + t.Run("created while the application runs", func(t *testing.T) { + m, apiMock := newMonitorTestFixture(t) + relayCreated := containerMessage(events.ActionCreate, "relay", "testproject-provider-1", "provider", map[string]string{api.RelayLabel: "relay-identity"}) + expectEvents(apiMock, relayCreated, monitorEvent(events.ActionDestroy)) + + _, err := runMonitor(t, m) + + assert.NilError(t, err) + }) +} From 41ccb0db1d1e3cfc9f68364c27feb067895f5e4d Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 27 Oct 2026 14:26:51 +0100 Subject: [PATCH 2/8] test(start): lock compose start's --wait, project rebuild and replica order Pin what compose start does around its plan: the --wait verification (ready or not, timeout message, one-shot services satisfied by their completion), the project rebuilt from the project's non-one-off containers when no compose file is given, only the selected services started, and the replicas started in numeric order. Part of #14081. Signed-off-by: Nicolas De Loof --- pkg/compose/start_test.go | 165 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 165 insertions(+) diff --git a/pkg/compose/start_test.go b/pkg/compose/start_test.go index da358cf64b..19131e0f8b 100644 --- a/pkg/compose/start_test.go +++ b/pkg/compose/start_test.go @@ -26,6 +26,7 @@ import ( "sync" "sync/atomic" "testing" + "testing/synctest" "time" "github.com/compose-spec/compose-go/v2/types" @@ -308,3 +309,167 @@ func TestGetDependencyCondition(t *testing.T) { assert.Equal(t, getDependencyCondition(oneShot, project), types.ServiceConditionCompletedSuccessfully) assert.Equal(t, getDependencyCondition(web, project), ServiceConditionRunningOrHealthy) } + +func inspectResultOf(id, name string, status container.ContainerState, exitCode int, health *container.Health) client.ContainerInspectResult { + config := &container.Config{} + if health != nil { + config.Healthcheck = &container.HealthConfig{Test: []string{"CMD", "true"}} + } + return client.ContainerInspectResult{Container: container.InspectResponse{ + ID: id, + Name: "/" + name, + Config: config, + State: &container.State{Status: status, ExitCode: exitCode, Health: health}, + }} +} + +// TestStart_WaitVerifiesTheApplicationIsReady pins what --wait adds to start: +// once the containers are started, every service has to be up (running, and +// healthy when it has a healthcheck) before start returns, or start fails +// naming the --wait-timeout that elapsed. Without --wait, start returns as soon +// as the containers are started, and checks nothing. +func TestStart_WaitVerifiesTheApplicationIsReady(t *testing.T) { + web := types.ServiceConfig{Name: "web", ContainerSpec: types.ContainerSpec{Image: "alpine"}} + web.HealthCheck = &types.HealthCheckConfig{Test: []string{"CMD", "true"}} + project := &types.Project{Name: "prj", Services: types.Services{"web": web}} + webCtr := stoppedServiceContainer("web", 1, container.StateRunning) + + newService := func(t *testing.T, health container.HealthStatus) *composeService { + svc, apiClient, _ := newStartTestService(t) + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). + Return(client.ContainerListResult{Items: []container.Summary{webCtr}}, nil).AnyTimes() + apiClient.EXPECT().ContainerInspect(gomock.Any(), webCtr.ID, gomock.Any()).AnyTimes(). + Return(inspectResultOf(webCtr.ID, "prj-web-1", container.StateRunning, 0, &container.Health{Status: health}), nil) + return svc + } + + t.Run("a service not ready in time fails the start", func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + svc := newService(t, container.Starting) + + err := svc.start(t.Context(), "prj", api.StartOptions{Project: project, Wait: true, WaitTimeout: 2 * time.Second}) + + assert.Error(t, err, "application not healthy after 2s") + }) + }) + + t.Run("a service that is ready lets it succeed", func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + svc := newService(t, container.Healthy) + + assert.NilError(t, svc.start(t.Context(), "prj", api.StartOptions{Project: project, Wait: true, WaitTimeout: 5 * time.Second})) + }) + }) + + t.Run("without --wait nothing is verified", func(t *testing.T) { + svc := newService(t, container.Starting) + + assert.NilError(t, svc.start(t.Context(), "prj", api.StartOptions{Project: project})) + }) +} + +// TestStart_WaitSatisfiesAOneShotServiceByItsCompletion pins that the --wait +// verification expects a service that others depend on with +// service_completed_successfully to have exited with 0, not to be running: a +// one-shot service is done once it has run, and would otherwise make --wait +// fail forever. One that completes with an error code fails it, naming the +// service and the code. +func TestStart_WaitSatisfiesAOneShotServiceByItsCompletion(t *testing.T) { + for _, tt := range []struct { + name string + exitCode int + err string + }{ + {name: "the one-shot service completed", exitCode: 0}, + {name: "the one-shot service failed", exitCode: 3, err: `service "init" didn't complete successfully: exit 3`}, + } { + t.Run(tt.name, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + svc, apiClient, _ := newStartTestService(t) + + initService := types.ServiceConfig{Name: "init", ContainerSpec: types.ContainerSpec{Image: "alpine"}} + app := types.ServiceConfig{Name: "app", ContainerSpec: types.ContainerSpec{Image: "alpine"}} + app.DependsOn = types.DependsOnConfig{"init": {Condition: types.ServiceConditionCompletedSuccessfully, Required: true}} + project := &types.Project{Name: "prj", Services: types.Services{"init": initService, "app": app}} + + initCtr := stoppedServiceContainer("init", 1, container.StateExited) + appCtr := stoppedServiceContainer("app", 1, container.StateRunning) + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).AnyTimes(). + Return(client.ContainerListResult{Items: []container.Summary{initCtr, appCtr}}, nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(client.ContainerStartResult{}, nil) + apiClient.EXPECT().ContainerInspect(gomock.Any(), initCtr.ID, gomock.Any()).AnyTimes(). + Return(inspectResultOf(initCtr.ID, "prj-init-1", container.StateExited, tt.exitCode, nil), nil) + apiClient.EXPECT().ContainerInspect(gomock.Any(), appCtr.ID, gomock.Any()).AnyTimes(). + Return(inspectResultOf(appCtr.ID, "prj-app-1", container.StateRunning, 0, nil), nil) + + err := svc.start(t.Context(), "prj", api.StartOptions{Project: project, Wait: true, WaitTimeout: 5 * time.Second}) + + if tt.err == "" { + assert.NilError(t, err) + } else { + assert.ErrorContains(t, err, tt.err) + } + }) + }) + } +} + +// TestStart_StartsTheReplicasOneAfterTheOther pins the order a service's +// stopped replicas start in: by replica number, each start waiting for the +// previous one's. +func TestStart_StartsTheReplicasOneAfterTheOther(t *testing.T) { + svc, apiClient, _ := newStartTestService(t) + web := types.ServiceConfig{Name: "web", ContainerSpec: types.ContainerSpec{Image: "alpine"}, Scale: intPtr(3)} + project := &types.Project{Name: "prj", Services: types.Services{"web": web}} + replicas := []container.Summary{ + stoppedServiceContainer("web", 3, container.StateExited), + stoppedServiceContainer("web", 1, container.StateExited), + stoppedServiceContainer("web", 2, container.StateExited), + } + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).AnyTimes(). + Return(client.ContainerListResult{Items: replicas}, nil) + first := apiClient.EXPECT().ContainerStart(gomock.Any(), "prj-web-1-id", gomock.Any()).Return(client.ContainerStartResult{}, nil) + second := apiClient.EXPECT().ContainerStart(gomock.Any(), "prj-web-2-id", gomock.Any()).Return(client.ContainerStartResult{}, nil).After(first) + apiClient.EXPECT().ContainerStart(gomock.Any(), "prj-web-3-id", gomock.Any()).Return(client.ContainerStartResult{}, nil).After(second) + + assert.NilError(t, svc.start(t.Context(), "prj", api.StartOptions{Project: project})) +} + +// TestStart_WithoutAProjectStartsTheSelectedServicesOnly pins `compose start +// web` run without a compose file: the project is rebuilt from the project's +// containers, and only the services asked for (with what they depend on) are +// started, the other containers of the project staying as they are. +func TestStart_WithoutAProjectStartsTheSelectedServicesOnly(t *testing.T) { + svc, apiClient, _ := newStartTestService(t) + webCtr := stoppedServiceContainer("web", 1, container.StateExited) + jobCtr := stoppedServiceContainer("job", 1, container.StateExited) + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).AnyTimes(). + Return(client.ContainerListResult{Items: []container.Summary{webCtr, jobCtr}}, nil) + // no expectation to start the job's container: it would fail the test + apiClient.EXPECT().ContainerStart(gomock.Any(), webCtr.ID, gomock.Any()).Return(client.ContainerStartResult{}, nil) + + assert.NilError(t, svc.start(t.Context(), "prj", api.StartOptions{AttachTo: []string{"web"}})) +} + +// TestStart_WithoutAProjectListsTheContainersWithoutTheOneOffs pins that the +// project `compose start` rebuilds from the containers it finds is made of the +// project's own containers only, never of the one-off ones (the ones `compose +// run` left behind): the listing it asks the daemon for leaves them out. +func TestStart_WithoutAProjectListsTheContainersWithoutTheOneOffs(t *testing.T) { + svc, apiClient, _ := newStartTestService(t) + webCtr := stoppedServiceContainer("web", 1, container.StateExited) + var firstListing client.ContainerListOptions + var listings atomic.Int32 + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(_ context.Context, opts client.ContainerListOptions) (client.ContainerListResult, error) { + if listings.Add(1) == 1 { + firstListing = opts + } + return client.ContainerListResult{Items: []container.Summary{webCtr}}, nil + }) + apiClient.EXPECT().ContainerStart(gomock.Any(), webCtr.ID, gomock.Any()).Return(client.ContainerStartResult{}, nil) + + assert.NilError(t, svc.start(t.Context(), "prj", api.StartOptions{})) + + assert.Assert(t, firstListing.Filters["label"][oneOffFilter(false)], "the listing must exclude one-off containers: %v", firstListing.Filters) +} From 505bf5156e84e2fafed8110d9d8c1b043af3f513 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 27 Oct 2026 14:27:34 +0100 Subject: [PATCH 3/8] test(wait): lock the depends_on gate, health statuses and wait events Pin which dependencies are worth a wait (shouldWaitForDependency), how each container state reads for service_healthy and service_completed_successfully (required and optional, dry-run), the Waiting/Healthy rows of a wait outside a plan, and that the end of a wait's context wins over a ready tick. Add the plan shapes for a wait following its dependency's start, a project of scale: 0 services, and the Start phase covering every service of the project (the current behavior of scale, whose change would be a decision of its own). Part of #14081. Signed-off-by: Nicolas De Loof --- pkg/compose/dependency_wait_test.go | 232 ++++++++++++++++++++++++++++ pkg/compose/reconcile_start_test.go | 81 ++++++++++ 2 files changed, 313 insertions(+) diff --git a/pkg/compose/dependency_wait_test.go b/pkg/compose/dependency_wait_test.go index 0e22ebfe62..c0256acfd3 100644 --- a/pkg/compose/dependency_wait_test.go +++ b/pkg/compose/dependency_wait_test.go @@ -794,3 +794,235 @@ func TestCheckDependencyDoneContext(t *testing.T) { assert.Assert(t, !done) }) } + +// TestShouldWaitForDependency pins which depends_on entries are worth a wait, +// the gate the plan's wait nodes and the waits outside a plan (restart, run, +// --wait) share: only a condition other than service_started, on a declared +// service that really runs containers of its own. +func TestShouldWaitForDependency(t *testing.T) { + project := &types.Project{ + Name: "myproject", + Services: types.Services{ + "db": {Name: "db"}, + "idle": {Name: "idle", Scale: intPtr(0)}, + "provider": {Name: "provider", Provider: &types.ServiceProviderConfig{Type: "test"}}, + }, + DisabledServices: types.Services{"cache": {Name: "cache"}}, + } + healthy := types.ServiceDependency{Condition: types.ServiceConditionHealthy, Required: true} + + tests := []struct { + name string + dependency string + config types.ServiceDependency + wait bool + err string + }{ + {name: "a started condition is the dependency order itself", dependency: "db", config: types.ServiceDependency{Condition: types.ServiceConditionStarted, Required: true}}, + {name: "a healthy dependency is waited for", dependency: "db", config: healthy, wait: true}, + {name: "a service completing is waited for", dependency: "db", config: types.ServiceDependency{Condition: types.ServiceConditionCompletedSuccessfully, Required: true}, wait: true}, + {name: "a dependency with no container (scale 0) is not", dependency: "idle", config: healthy}, + {name: "a provider service is not", dependency: "provider", config: healthy}, + {name: "a disabled service (--no-deps) is not", dependency: "cache", config: healthy}, + {name: "an undeclared service is an error", dependency: "ghost", config: healthy, err: "ghost"}, + {name: "an undeclared service with a started condition is not even looked up", dependency: "ghost", config: types.ServiceDependency{Condition: types.ServiceConditionStarted}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + wait, err := shouldWaitForDependency(tt.dependency, tt.config, project) + if tt.err != "" { + assert.ErrorContains(t, err, tt.err) + return + } + assert.NilError(t, err) + assert.Equal(t, wait, tt.wait) + }) + } +} + +func inspectOf(status container.ContainerState, health *container.Health, healthcheck *container.HealthConfig, exitCode int) client.ContainerInspectResult { + return client.ContainerInspectResult{Container: container.InspectResponse{ + ID: "db-id", + Name: "/test-db-1", + Config: &container.Config{Healthcheck: healthcheck}, + State: &container.State{Status: status, Health: health, ExitCode: exitCode}, + }} +} + +// TestIsServiceHealthyStatuses pins how each state of a container with a +// healthcheck reads for service_healthy: healthy is the only way to satisfy +// the condition, starting means keep polling, and anything else -- exited, +// unhealthy, no health state at all, a status the daemon should not report -- +// is a definitive failure naming the container. +func TestIsServiceHealthyStatuses(t *testing.T) { + check := &container.HealthConfig{Test: []string{"CMD", "true"}} + tests := []struct { + name string + inspect client.ContainerInspectResult + healthy bool + err string + }{ + {name: "healthy", inspect: inspectOf(container.StateRunning, &container.Health{Status: container.Healthy}, check, 0), healthy: true}, + {name: "still starting", inspect: inspectOf(container.StateRunning, &container.Health{Status: container.Starting}, check, 0)}, + {name: "unhealthy", inspect: inspectOf(container.StateRunning, &container.Health{Status: container.Unhealthy}, check, 0), err: "container test-db-1 is unhealthy"}, + {name: "exited", inspect: inspectOf(container.StateExited, nil, check, 3), err: "container test-db-1 exited (3)"}, + {name: "no health state", inspect: inspectOf(container.StateRunning, nil, check, 0), err: "container test-db-1 has no healthcheck configured"}, + {name: "unexpected status", inspect: inspectOf(container.StateRunning, &container.Health{Status: "weird"}, check, 0), err: `container test-db-1 had unexpected health status "weird"`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(tt.inspect, nil) + + healthy, err := svc.isServiceHealthy(t.Context(), Containers{{ID: "db-id"}}, false) + + if tt.err != "" { + assert.ErrorContains(t, err, tt.err) + } else { + assert.NilError(t, err) + } + assert.Equal(t, healthy, tt.healthy) + }) + } + + t.Run("every container of the service has to be healthy", func(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-1", gomock.Any()). + Return(inspectOf(container.StateRunning, &container.Health{Status: container.Healthy}, check, 0), nil) + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-2", gomock.Any()). + Return(inspectOf(container.StateRunning, &container.Health{Status: container.Starting}, check, 0), nil) + + healthy, err := svc.isServiceHealthy(t.Context(), Containers{{ID: "db-1"}, {ID: "db-2"}}, false) + assert.NilError(t, err) + assert.Assert(t, !healthy) + }) +} + +// TestCheckDependencyCompleted pins service_completed_successfully: a +// dependency that exited with 0 satisfies it, one still running does not yet, +// one that exited with another code fails a required dependency -- naming the +// service and the code, and marking its rows in error -- and is skipped, with a warning, +// when the dependency is optional. +func TestCheckDependencyCompleted(t *testing.T) { + waitingFor := Containers{{ID: "db-id", Names: []string{"/test-db-1"}, Labels: map[string]string{api.ServiceLabel: "db"}}} + const row = "Container test-db-1" + + tests := []struct { + name string + inspect client.ContainerInspectResult + required bool + done bool + err string + events []string + }{ + { + name: "exited with 0", inspect: inspectOf(container.StateExited, nil, nil, 0), required: true, + done: true, events: []string{api.StatusExited}, + }, + { + name: "still running", inspect: inspectOf(container.StateRunning, nil, nil, 0), required: true, + }, + { + name: "required, exited with an error code", inspect: inspectOf(container.StateExited, nil, nil, 3), required: true, + err: `service "db" didn't complete successfully: exit 3`, + events: []string{api.StatusError}, + }, + { + name: "optional, exited with an error code", inspect: inspectOf(container.StateExited, nil, nil, 3), + done: true, + events: []string{`Skipped: optional dependency "db" didn't complete successfully: exit 3`}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, apiClient, recorder := newStartPhaseTestService(t) + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Return(tt.inspect, nil) + + done, err := svc.checkDependencyCompleted(t.Context(), "db", types.ServiceDependency{Required: tt.required}, waitingFor) + + if tt.err != "" { + assert.ErrorContains(t, err, tt.err) + } else { + assert.NilError(t, err) + } + assert.Equal(t, done, tt.done) + assert.DeepEqual(t, recorder.byID[row], tt.events) + }) + } + + t.Run("a dry run never sees a dependency exit: it is taken as completed", func(t *testing.T) { + svc, _, _ := newStartPhaseTestService(t) + svc.dryRun = true + + done, err := svc.checkDependencyCompleted(t.Context(), "db", types.ServiceDependency{Required: true}, waitingFor) + assert.NilError(t, err) + assert.Assert(t, done) + }) +} + +// TestWaitDependenciesReportsTheWait pins what a wait outside a plan (restart, +// run, the --wait verification) shows of a dependency's containers: Waiting +// first, then Healthy once the condition holds -- the progression a plan's wait +// node gives the same rows -- and only the dependency's own containers, never +// a one-off or a hook runner of the service. +func TestWaitDependenciesReportsTheWait(t *testing.T) { + svc, apiClient, recorder := newStartPhaseTestService(t) + project := &types.Project{Name: "test", Services: types.Services{"db": {Name: "db"}}} + labels := func(oneOff string) map[string]string { + return map[string]string{api.ServiceLabel: "db", api.OneoffLabel: oneOff} + } + containers := Containers{ + {ID: "db-id", Names: []string{"/test-db-1"}, Labels: labels("False")}, + {ID: "run-id", Names: []string{"/test-db-run-1"}, Labels: labels("True")}, + } + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()). + Return(inspectOf(container.StateRunning, &container.Health{Status: container.Healthy}, &container.HealthConfig{Test: []string{"CMD", "true"}}, 0), nil) + + dependencies := types.DependsOnConfig{"db": {Condition: types.ServiceConditionHealthy, Required: true}} + assert.NilError(t, svc.waitDependencies(t.Context(), project, "web", dependencies, containers)) + + assert.DeepEqual(t, recorder.byID["Container test-db-1"], []string{api.StatusWaiting, api.StatusHealthy}) + assert.Equal(t, len(recorder.byID["Container test-db-run-1"]), 0, "a one-off container is not waited for") +} + +// TestWaitDependencyEndOfContextWinsOverATick pins that a wait whose context +// ended is never run through one more check because a tick happened to be +// ready as well: select picks at random between the two, so the wait looks at +// its context after it, and the end of the context wins. Here a check is in +// flight while the context ends and a tick is pending; the scenario is run +// repeatedly, as a wait that let the tick win would only miss it one time in +// two. The one check allowed is the first, on entry. +func TestWaitDependencyEndOfContextWinsOverATick(t *testing.T) { + waitingFor := Containers{{ID: "db-id", Names: []string{"/test-db-1"}, Labels: map[string]string{api.ServiceLabel: "db"}}} + dependency := types.ServiceDependency{Condition: types.ServiceConditionHealthy, Required: true} + + for range 40 { + synctest.Test(t, func(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + entered, release := make(chan struct{}), make(chan struct{}) + apiClient.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).Times(1). + DoAndReturn(func(context.Context, string, client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + close(entered) + <-release + return inspectOf(container.StateRunning, &container.Health{Status: container.Starting}, &container.HealthConfig{Test: []string{"CMD", "true"}}, 0), nil + }) + result := make(chan error, 1) + go func() { + reached, err := svc.waitDependencyReached(ctx, ctx, "web", "db", dependency, waitingFor) + if err == nil && reached { + err = errors.New("the wait reported the dependency as reached") + } + result <- err + }() + + <-entered + time.Sleep(time.Second) // the ticker fires while the check is in flight + cancel() + close(release) + + assert.NilError(t, <-result) + }) + } +} diff --git a/pkg/compose/reconcile_start_test.go b/pkg/compose/reconcile_start_test.go index 768f442102..6d087d3534 100644 --- a/pkg/compose/reconcile_start_test.go +++ b/pkg/compose/reconcile_start_test.go @@ -1307,3 +1307,84 @@ func TestPlanStart_DefaultScopeIsInert(t *testing.T) { } assert.Equal(t, plan.String(), "[] -> #1 service:app:1, CreateContainer, no existing container\n") } + +// A wait node is ordered after the start of the dependency it waits for: the +// condition is only worth checking once the dependency's container has been +// started. Here a single dependent uses the wait, so nothing but the node's +// own prerequisites can order it (a second dependent would add them to the +// shared node). +func TestPlanStart_WaitFollowsTheDependencyStart(t *testing.T) { + project := &types.Project{ + Name: "myproject", + Services: types.Services{ + "db": {Name: "db"}, + "web": serviceWithDeps("web", types.DependsOnConfig{ + "db": {Condition: types.ServiceConditionHealthy, Required: true}, + }), + }, + } + + plan, err := reconcile(t.Context(), project, emptyObserved(), startScopeOptions(ScopeCreateStart), noPrompt) + assert.NilError(t, err) + + assert.Equal(t, plan.String(), strings.TrimSpace(` +[] -> #1 service:db:1, CreateContainer, no existing container +[1] -> #2 service:web:1, CreateContainer, no existing container +[1] -> #3 service:db:1, StartContainer, start [start:db:1] {start} +[3] -> #4 wait:db:service_healthy, WaitCondition, depends_on condition {start} +[2,4] -> #5 service:web:1, StartContainer, start [start:web:1] {start} +`)+"\n") +} + +// A project whose services all have scale: 0 has no container to start, and +// that is not an error: `compose start` has nothing to do. +func TestPlanStart_ScaleZeroServiceWithoutAnyContainerIsNotAnError(t *testing.T) { + zero := 0 + project := &types.Project{ + Name: "myproject", + Services: types.Services{"app": {Name: "app", Scale: &zero}}, + } + + plan, err := reconcile(t.Context(), project, emptyObserved(), startScopeOptions(ScopeStart), noPrompt) + assert.NilError(t, err) + assert.Equal(t, len(plan.Nodes), 0, "nothing to plan:\n%s", plan) +} + +// The Start phase covers every service of the project the plan is built for, +// whichever services the create options select: Services only decides the +// services whose containers may be recreated (see api.ScaleOptions.Services), +// so scaling a service starts, with the replicas it creates, the stopped +// containers of the project's other services. The CLI hands the plan a +// project narrowed to the services it names and their dependencies, which is +// why a user doesn't meet it. Starting only what was selected would be a +// decision of its own (#14081, "scale start scope"), not the side effect of +// another change: this pins the behavior as it stands. +func TestPlanStart_StartPhaseCoversEveryServiceOfTheProject(t *testing.T) { + two := 2 + db := types.ServiceConfig{Name: "db", Scale: &two} + web := serviceWithDeps("web", types.DependsOnConfig{"db": {Condition: types.ServiceConditionStarted, Required: true}}) + other := types.ServiceConfig{Name: "other"} + project := &types.Project{Name: "myproject", Services: types.Services{"db": db, "web": web, "other": other}} + observed := emptyObserved() + for _, service := range []types.ServiceConfig{db, web, other} { + hash, err := serviceHashWithResolvedRefs(service, nil) + assert.NilError(t, err) + state := container.StateExited + if service.Name == "db" { + state = container.StateRunning + } + observed.Containers[service.Name] = []ObservedContainer{observedServiceContainer(service.Name, 1, state, hash)} + } + options := startScopeOptions(ScopeCreateStart) + options.Services = []string{"db"} + + plan, err := reconcile(t.Context(), project, observed, options, noPrompt) + assert.NilError(t, err) + + assert.Equal(t, plan.String(), strings.TrimSpace(` +[] -> #1 service:db:2, CreateContainer, no existing container +[1] -> #2 service:db:2, StartContainer, start [start:db:2] {start} +[] -> #3 service:other:1, StartContainer, start [start:other:1] {start} +[2] -> #4 service:web:1, StartContainer, start [start:web:1] {start} +`)+"\n") +} From 561a1e9f0e65fb4e9512e29981b51f556f8635d2 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 27 Oct 2026 14:28:13 +0100 Subject: [PATCH 4/8] test(executor): lock the start-phase executor's guarantees Pin that every ContainerStart holds startMx, that the start, pre_start, post_start and hook-runner nodes do nothing once the context is canceled, that a failure relays untouched down a replica's chain, that the node bookkeeping records cancellations as failures, that the phase returns the originating error, that an invalid plan is refused, that a concurrency cap cannot deadlock the node goroutines, that Starting is reported when the container starts (not during pre_start), that the hook runner targets the lowest-numbered replica, and that stop timeouts reach the daemon in whole seconds. Part of #14081. Signed-off-by: Nicolas De Loof --- pkg/compose/executor_start_test.go | 471 +++++++++++++++++++++++++++++ 1 file changed, 471 insertions(+) diff --git a/pkg/compose/executor_start_test.go b/pkg/compose/executor_start_test.go index 81ca2b3c91..0d905f110d 100644 --- a/pkg/compose/executor_start_test.go +++ b/pkg/compose/executor_start_test.go @@ -2128,3 +2128,474 @@ func TestPlanRunOfAnEmptyOrSinglePhasePlan(t *testing.T) { assert.NilError(t, pr.runStart(t.Context(), nil)) }) } + +// TestExecutePlanStartHoldsTheStartMutex pins that every ContainerStart the +// plan issues goes through a call site holding startMx, the bare start of a +// paused or dead container as much as the start of a service container, which +// also injects its secrets and configs. startMx is a workaround for a flaw of +// engine 28.0 to 28.2 (see its doc comment), kept until Engine 28.x is out of +// support: the mutex is probed from inside ContainerStart, where it must be +// held, so that dropping it from either path is a decision, not an accident. +// The test is not parallel, so no other test holds startMx while it runs. +func TestExecutePlanStartHoldsTheStartMutex(t *testing.T) { + tests := []struct { + name string + service *types.ServiceConfig + }{ + {name: "a bare start of a paused or dead container"}, + {name: "the start of a service container", service: &types.ServiceConfig{Name: "web"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + ctr := container.Summary{ID: "web-id", Names: []string{"/test-web-1"}} + apiClient.EXPECT().ContainerStart(gomock.Any(), "web-id", gomock.Any()). + DoAndReturn(func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) { + if startMx.TryLock() { + startMx.Unlock() + t.Error("ContainerStart was called without holding startMx") + } + return client.ContainerStartResult{}, nil + }) + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpStartContainer, + ResourceID: "service:web:1", + Cause: "start", + Service: tt.service, + Container: &ctr, + }, "") + + project := &types.Project{Name: "test", Services: types.Services{"web": {Name: "web"}}} + exec := svc.newPlanExecutor(project, emptyObservedState("test")) + assert.NilError(t, exec.run(t.Context(), plan)) + }) + } +} + +// TestExecutePlanFailureRelaysDownALongChainUntouched pins that the error a +// plan fails with is the one of the node that failed, however long the chain +// of nodes held behind it: here the wait on a dependency that has no +// container, with the pre_start hooks, the start and the post_start hooks of +// the service that depends on it after it -- the shape of a replica's chain in +// the start phase. None of them runs, and the error is the wait's own, with +// nothing in front of it. How each hop of the chain relays the failure is +// TestRunNodeRelaysTheOriginalErrorDownALongChain's. +func TestExecutePlanFailureRelaysDownALongChainUntouched(t *testing.T) { + svc, _, _ := newStartPhaseTestService(t) + + service := types.ServiceConfig{ + Name: "web", + PreStart: []types.PreStartHook{{ContainerSpec: types.ContainerSpec{Command: types.ShellCommand{"init"}}}}, + PostStart: []types.ServiceHook{{Command: types.ShellCommand{"notify"}}}, + } + project := &types.Project{Name: "test", Services: types.Services{"web": service}} + ctr := container.Summary{ID: "web-id", Names: []string{"/test-web-1"}} + + // no expectation on the daemon at all: nothing but the wait may run + plan := &Plan{} + wait := plan.addNode(Operation{ + Type: OpWaitCondition, + ResourceID: "wait:db:service_healthy", + Name: "db", + Condition: types.ServiceConditionHealthy, + }, "") + wait.Phase = PhaseStart + preStart := plan.addNode(Operation{ + Type: OpRunPreStart, + ResourceID: "service:web:1", + Cause: "pre_start hooks", + Service: &service, + Container: &ctr, + }, "start:web:1", wait) + preStart.Phase = PhaseStart + start := plan.addNode(Operation{ + Type: OpStartContainer, + ResourceID: "service:web:1", + Cause: "start", + Service: &service, + Container: &ctr, + }, "start:web:1", preStart) + start.Phase = PhaseStart + post := plan.addNode(Operation{ + Type: OpRunPostStart, + ResourceID: "service:web:1", + Cause: "post_start hooks", + Service: &service, + Container: &ctr, + }, "start:web:1", start) + post.Phase = PhaseStart + + err := svc.executePlan(t.Context(), project, emptyObservedState("test"), plan) + + assert.Error(t, err, "missing dependency db") +} + +// TestPlanRunRefusesACreateNodeDependingOnAStartNode pins that a plan that +// could only deadlock is refused before anything runs: a Create-phase node +// waiting on a Start-phase one would wait forever, as the Start phase is only +// dispatched once the Create phase is over. +func TestPlanRunRefusesACreateNodeDependingOnAStartNode(t *testing.T) { + svc, _, _ := newStartPhaseTestService(t) + service := types.ServiceConfig{Name: "web"} + ctr := container.Summary{ID: "web-id", Names: []string{"/test-web-1"}} + + plan := &Plan{} + start := plan.addNode(Operation{Type: OpStartContainer, ResourceID: "service:web:1", Cause: "start", Service: &service, Container: &ctr}, "") + start.Phase = PhaseStart + plan.addNode(Operation{Type: OpCreateNetwork, ResourceID: "network:default", Name: "default"}, "", start) + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test")) + _, err := exec.begin(plan) + + assert.ErrorContains(t, err, "invalid plan: create-phase node 2 depends on start-phase node 1") +} + +// TestExecStartContainer_BailsOutWhenContextCanceled is the start node's side +// of TestExecRunPreStart_BailsOutWhenContextCanceled: once ctx is canceled (a +// sibling node failed, or the caller gave up), a start must neither inject the +// container's secrets and configs nor start it. No daemon expectation is +// registered: an unexpected call fails the test. +func TestExecStartContainer_BailsOutWhenContextCanceled(t *testing.T) { + svc, _, _ := newStartPhaseTestService(t) + service := types.ServiceConfig{Name: "web"} + ctr := container.Summary{ID: "web-id", Names: []string{"/test-web-1"}} + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + exec := svc.newPlanExecutor(&types.Project{Name: "test", Services: types.Services{"web": service}}, emptyObservedState("test")) + err := exec.execStartContainer(ctx, Operation{Type: OpStartContainer, Service: &service, Container: &ctr}) + + assert.ErrorIs(t, err, context.Canceled) +} + +// TestExecRunPostStart_BailsOutWhenContextCanceled is the same for the +// post_start hooks: none runs once ctx is canceled. +func TestExecRunPostStart_BailsOutWhenContextCanceled(t *testing.T) { + svc, _, _ := newStartPhaseTestService(t) + service := types.ServiceConfig{ + Name: "web", + PostStart: []types.ServiceHook{{Command: types.ShellCommand{"/notify.sh"}}}, + } + ctr := container.Summary{ID: "web-id", Names: []string{"/test-web-1"}} + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test")) + err := exec.execRunPostStart(ctx, Operation{Type: OpRunPostStart, Service: &service, Container: &ctr}) + + assert.ErrorIs(t, err, context.Canceled) +} + +// TestExecutePlanBoundedConcurrencyNeverBlocksAPrerequisite pins the executor +// constraint that a concurrency cap must not be put on the goroutines of the +// plan's nodes: every node waits on its prerequisites in a goroutine of its +// own, so a cap on the goroutines would let nodes waiting for a prerequisite +// hold every slot, and the prerequisite never run. Here --parallel is 1 and +// the nodes depend on nodes created after them (the first waits for the +// third, which waits for the second), so that no creation order saves them: +// the plan has to complete, in the order of its edges. +func TestExecutePlanBoundedConcurrencyNeverBlocksAPrerequisite(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + svc.maxConcurrency = 1 + + ctrs := map[string]container.Summary{} + plan := &Plan{} + nodes := map[string]*PlanNode{} + for _, name := range []string{"first", "second", "third"} { + ctr := container.Summary{ID: name + "-id", Names: []string{"/test-" + name + "-1"}} + ctrs[name] = ctr + nodes[name] = plan.addNode(Operation{Type: OpStartContainer, ResourceID: "service:" + name + ":1", Cause: "start", Container: &ctr}, "") + } + // the dependencies point at nodes created later + nodes["first"].DependsOn = []*PlanNode{nodes["third"]} + nodes["third"].DependsOn = []*PlanNode{nodes["second"]} + second := apiClient.EXPECT().ContainerStart(gomock.Any(), "second-id", gomock.Any()).Return(client.ContainerStartResult{}, nil) + third := apiClient.EXPECT().ContainerStart(gomock.Any(), "third-id", gomock.Any()).Return(client.ContainerStartResult{}, nil).After(second) + apiClient.EXPECT().ContainerStart(gomock.Any(), "first-id", gomock.Any()).Return(client.ContainerStartResult{}, nil).After(third) + + // the deadline only turns a deadlock into a prompt failure + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test")) + assert.NilError(t, exec.run(ctx, plan)) +} + +// TestExecutePlanStartingIsReportedWhenTheContainerStarts pins when the row of +// a replica reads Starting: when its container is started, not when the chain +// that leads to it begins. The pre_start hooks of the service run silently +// before it; the row's Working event is the start's own. +func TestExecutePlanStartingIsReportedWhenTheContainerStarts(t *testing.T) { + svc, apiClient, recorder := newStartPhaseTestService(t) + + service := types.ServiceConfig{ + Name: "web", + PreStart: []types.PreStartHook{{ContainerSpec: types.ContainerSpec{Command: types.ShellCommand{"init"}}}}, + } + ctr := container.Summary{ID: "web-id", Names: []string{"/test-web-1"}} + const row = "Container test-web-1" + rowNow := func() []string { + recorder.mu.Lock() + defer recorder.mu.Unlock() + return slices.Clone(recorder.byID[row]) + } + + // the pre_start node first asks the daemon whether a replica already runs, + // and finds one: it has nothing to run, and the row is still silent + var duringPreStart, atStart []string + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). + DoAndReturn(func(context.Context, client.ContainerListOptions) (client.ContainerListResult, error) { + duringPreStart = rowNow() + return client.ContainerListResult{Items: []container.Summary{ctr}}, nil + }) + apiClient.EXPECT().ContainerStart(gomock.Any(), "web-id", gomock.Any()). + DoAndReturn(func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) { + atStart = rowNow() + return client.ContainerStartResult{}, nil + }) + + plan := &Plan{} + preStart := plan.addNode(Operation{Type: OpRunPreStart, ResourceID: "service:web:1", Cause: "pre_start hooks", Service: &service, Container: &ctr}, "start:web:1") + preStart.Phase = PhaseStart + start := plan.addNode(Operation{Type: OpStartContainer, ResourceID: "service:web:1", Cause: "start", Service: &service, Container: &ctr}, "start:web:1", preStart) + start.Phase = PhaseStart + + exec := svc.newPlanExecutor(&types.Project{Name: "test", Services: types.Services{"web": service}}, emptyObservedState("test")) + assert.NilError(t, exec.run(t.Context(), plan)) + + assert.Equal(t, len(duringPreStart), 0, "the row must say nothing during pre_start: %v", duringPreStart) + assert.DeepEqual(t, atStart, []string{api.StatusStarting}) + assert.DeepEqual(t, recorder.byID[row], []string{api.StatusStarting, api.StatusStarted}) +} + +// TestRunNodeRelaysTheOriginalErrorDownALongChain pins what each node of a +// chain does with the failure of the node it depends on: it does not run, and +// it passes on the ORIGINAL error -- the one the failing node recorded, not +// the "dependency failed" error its neighbour returned -- so that the root +// cause stays legible however many hops down the chain a node is: a +// replica's chain (create, pre_start, start, post_start) is three hops long. +func TestRunNodeRelaysTheOriginalErrorDownALongChain(t *testing.T) { + svc, _, _ := newStartPhaseTestService(t) + service := types.ServiceConfig{Name: "web"} + ctr := container.Summary{ID: "web-id", Names: []string{"/test-web-1"}} + + plan := &Plan{} + failing := plan.addNode(Operation{Type: OpWaitCondition, ResourceID: "wait:db:service_healthy", Name: "db", Condition: types.ServiceConditionHealthy}, "") + failing.Phase = PhaseStart + chain := []*PlanNode{failing} + for _, op := range []OperationType{OpRunPreStart, OpStartContainer, OpRunPostStart} { + node := plan.addNode(Operation{Type: op, ResourceID: "service:web:1", Service: &service, Container: &ctr}, "start:web:1", chain[len(chain)-1]) + node.Phase = PhaseStart + chain = append(chain, node) + } + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test")) + run, err := exec.begin(plan) + assert.NilError(t, err) + + // the first node failed: what the executor does when its operation fails + original := errors.New("db is unhealthy") + run.rs.recordFailure(failing.ID, original) + close(run.rs.done[failing.ID]) + + phase := newPhaseCancel(t.Context()) + for hop, node := range chain[1:] { + err := exec.runNode(node, run.rs, phase) + + assert.Assert(t, errors.Is(err, original), "hop %d: %v", hop+1, err) + assert.Equal(t, err.Error(), "dependency failed: db is unhealthy", "hop %d wrapped the error once more", hop+1) + } +} + +// runNodeFixture is a planExecutor and the state of a planRun, to drive +// runNode on the nodes of plan one at a time: what the goroutines of a phase do +// concurrently, with an interleaving the test picks. +func runNodeFixture(t *testing.T, plan *Plan) (*planExecutor, *planRun) { + t.Helper() + svc, _, _ := newStartPhaseTestService(t) + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test")) + run, err := exec.begin(plan) + assert.NilError(t, err) + return exec, run +} + +func bareStartNode(plan *Plan, name string, after ...*PlanNode) *PlanNode { + ctr := container.Summary{ID: name + "-id", Names: []string{"/test-" + name + "-1"}} + node := plan.addNode(Operation{Type: OpStartContainer, ResourceID: "service:" + name + ":1", Cause: "start", Container: &ctr}, "", after...) + node.Phase = PhaseStart + return node +} + +// TestRunNodeRecordsACancellationWhileWaitingAsAFailure pins that a node +// given up while it waits for its prerequisites is recorded as failed, so that +// the nodes depending on it skip: their own wait can find its done channel +// closed and the phase's context canceled at once, and select picks either -- +// the one that sees only the closed channel must still find the failure, or it +// would go on to start with a canceled context, and report a Starting it never +// finishes. +func TestRunNodeRecordsACancellationWhileWaitingAsAFailure(t *testing.T) { + plan := &Plan{} + prerequisite := bareStartNode(plan, "db") + waiting := bareStartNode(plan, "web", prerequisite) + exec, run := runNodeFixture(t, plan) + phase := newPhaseCancel(t.Context()) + phase.cancel() // the prerequisite never completes: the phase is over + + err := exec.runNode(waiting, run.rs, phase) + + assert.ErrorIs(t, err, context.Canceled) + assert.ErrorIs(t, run.rs.failedDependency([]*PlanNode{waiting}), context.Canceled, "the dependents of the node must find it failed") +} + +// TestRunNodeRecordsAGroupGivenUpAsAFailure is the same for a node that does +// not start because the group of its resource ended meanwhile (another node of +// the group failed while this one was getting a slot). +func TestRunNodeRecordsAGroupGivenUpAsAFailure(t *testing.T) { + plan := &Plan{} + ctr := container.Summary{ID: "web-id", Names: []string{"/test-web-1"}} + node := plan.addNode(Operation{Type: OpStartContainer, ResourceID: "service:web:1", Cause: "start", Container: &ctr}, "start:web:1") + node.Phase = PhaseStart + exec, run := runNodeFixture(t, plan) + run.rs.groups.groups["start:web:1"].finished = true + + err := exec.runNode(node, run.rs, newPhaseCancel(t.Context())) + + assert.ErrorIs(t, err, context.Canceled) + assert.ErrorIs(t, run.rs.failedDependency([]*PlanNode{node}), context.Canceled, "the dependents of the node must find it failed") +} + +// TestRunNodeDoesNotCountAnInterruptedWaitAsASuccess pins that a wait ended +// by its caller going away -- it then ends quietly, with no error -- is not +// recorded as a success: its condition was never met, and what is reported for +// the nodes held behind it must not take it for satisfied. +func TestRunNodeDoesNotCountAnInterruptedWaitAsASuccess(t *testing.T) { + plan := &Plan{} + wait := plan.addNode(Operation{Type: OpWaitCondition, ResourceID: "wait:db:service_healthy", Name: "db", Condition: types.ServiceConditionHealthy}, "") + wait.Phase = PhaseStart + exec, run := runNodeFixture(t, plan) + exec.containersByService["db"] = Containers{{ID: "db-id", Names: []string{"/test-db-1"}, Labels: map[string]string{api.ServiceLabel: "db"}}} + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + err := exec.runNode(wait, run.rs, newPhaseCancel(ctx)) + + assert.NilError(t, err) + assert.Assert(t, !run.rs.succeeded(wait.ID), "the wait ended unmet, it is not a success") + assert.NilError(t, run.rs.failedDependency([]*PlanNode{wait})) +} + +// TestRunPhaseFailsWithTheErrorOfTheNodeThatFailedFirst pins what a phase +// returns once one of its nodes failed: that node's error, and not the first +// error the phase's goroutines happen to return, which may be the bare +// context.Canceled of a node that was given up because of it (errgroup keeps +// whichever comes first). +func TestRunPhaseFailsWithTheErrorOfTheNodeThatFailedFirst(t *testing.T) { + plan := &Plan{} + failed := bareStartNode(plan, "db") + casualty := bareStartNode(plan, "web", failed) + exec, run := runNodeFixture(t, plan) + origin := errors.New("no space left on device") + phase := newPhaseCancel(t.Context()) + phase.err = origin // what the failure of the prerequisite registered... + phase.cancel() // ...before its dependent, still waiting for it, was given up + + err := exec.runPhase([]*PlanNode{casualty}, run.rs, phase) + + assert.Equal(t, err, origin) +} + +// TestExecCreateHookContainer_TargetsTheLowestNumberedReplica pins which +// replica a pre_start hook shares the volumes of: the lowest-numbered one of +// the live view, the one the start phase hands the hooks, whatever order the +// replicas are in. +func TestExecCreateHookContainer_TargetsTheLowestNumberedReplica(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + service := types.ServiceConfig{ + Name: "web", + ContainerSpec: types.ContainerSpec{Image: "alpine"}, + PreStart: []types.PreStartHook{{ContainerSpec: types.ContainerSpec{Command: types.ShellCommand{"init"}}}}, + } + replica := func(number string) container.Summary { + return container.Summary{ID: "c" + number, Labels: map[string]string{api.ContainerNumberLabel: number}} + } + var created client.ContainerCreateOptions + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + created = opts + return client.ContainerCreateResult{ID: "hook-id"}, nil + }) + + plan := &Plan{} + hook := plan.addNode(Operation{ + Type: OpCreateHookContainer, + ResourceID: "hook:web:pre_start:0", + Cause: "pre_start hook", + Service: &service, + HookIndex: 0, + Name: getHookContainerName("test", "web", 0), + }, "") + exec := svc.newPlanExecutor(&types.Project{Name: "test", Services: types.Services{"web": service}}, emptyObservedState("test")) + // neither the first nor the last of the view is the lowest-numbered + exec.containersByService["web"] = Containers{replica("3"), replica("1"), replica("2")} + + assert.NilError(t, exec.execCreateHookContainer(t.Context(), hook)) + + assert.DeepEqual(t, created.HostConfig.VolumesFrom, []string{"c1"}) +} + +// TestExecCreateHookContainer_BailsOutWhenContextCanceled pins that a runner +// is not created once ctx is canceled -- a replica's creation that failed +// canceled the phase, and the live view this node resolves its target from is +// then incomplete: it ends on the cancellation, not on an internal error about +// a missing container, and calls nothing. +func TestExecCreateHookContainer_BailsOutWhenContextCanceled(t *testing.T) { + svc, _, _ := newStartPhaseTestService(t) + service := types.ServiceConfig{ + Name: "web", + PreStart: []types.PreStartHook{{ContainerSpec: types.ContainerSpec{Command: types.ShellCommand{"init"}}}}, + } + plan := &Plan{} + hook := plan.addNode(Operation{Type: OpCreateHookContainer, ResourceID: "hook:web:pre_start:0", Service: &service, HookIndex: 0, Name: "test-web-pre_start-0"}, "") + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test")) + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + err := exec.execCreateHookContainer(ctx, hook) + + assert.ErrorIs(t, err, context.Canceled) +} + +// TestExecStopContainer_PassesTheTimeoutInWholeSeconds pins how the timeout of +// a stop of the plan (the one that precedes a container's recreation, from +// up --timeout) reaches the daemon: in whole seconds, and not at all when +// none was given, so that the container's own stop timeout applies. +func TestExecStopContainer_PassesTheTimeoutInWholeSeconds(t *testing.T) { + seconds := func(d time.Duration) *time.Duration { return &d } + tests := []struct { + name string + timeout *time.Duration + want *int + }{ + {name: "none given"}, + {name: "whole seconds", timeout: seconds(7 * time.Second), want: intPtr(7)}, + {name: "a fraction of a second is dropped", timeout: seconds(1900 * time.Millisecond), want: intPtr(1)}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + ctr := container.Summary{ID: "web-id", Names: []string{"/test-web-1"}} + var got client.ContainerStopOptions + apiClient.EXPECT().ContainerStop(gomock.Any(), "web-id", gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, opts client.ContainerStopOptions) (client.ContainerStopResult, error) { + got = opts + return client.ContainerStopResult{}, nil + }) + exec := svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test")) + + err := exec.execStopContainer(t.Context(), Operation{Type: OpStopContainer, Container: &ctr, Timeout: tt.timeout}) + + assert.NilError(t, err) + assert.DeepEqual(t, got.Timeout, tt.want) + }) + } +} From 6fb10fbac92089252132ecb172b027b8649e6c5f Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 27 Oct 2026 14:28:53 +0100 Subject: [PATCH 5/8] test(up): lock what an interactive and a detached up leave to the plan Extend the interactive-up daemon fake and pin the behaviors mutation checks of the interactive-up switchover found unlocked: the monitor's Services scope, a Start-phase error while terminated, the cleanup when attaching fails, signal handling starting only after the Create phase, late starters stopped after the teardown sweep, hook logs reaching the session, the wait timeout translation, --exit-code-from and the Running events. Add the wiring of detached up (with its --wait verification), scale, the rebuild of watch and the dependencies of run onto the plan. Part of #14081. Signed-off-by: Nicolas De Loof --- pkg/compose/lifecycle_wiring_test.go | 236 +++++++++++++++ pkg/compose/up_phases_test.go | 414 +++++++++++++++++++++++++-- pkg/compose/up_signal_test.go | 144 ++++++++++ 3 files changed, 767 insertions(+), 27 deletions(-) create mode 100644 pkg/compose/lifecycle_wiring_test.go create mode 100644 pkg/compose/up_signal_test.go diff --git a/pkg/compose/lifecycle_wiring_test.go b/pkg/compose/lifecycle_wiring_test.go new file mode 100644 index 0000000000..e5db2c7dd9 --- /dev/null +++ b/pkg/compose/lifecycle_wiring_test.go @@ -0,0 +1,236 @@ +//go:build !windows + +/* + Copyright 2026 Docker Compose CLI authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package compose + +import ( + "context" + "strings" + "sync" + "testing" + "time" + + "github.com/compose-spec/compose-go/v2/types" + "github.com/moby/moby/api/types/container" + "github.com/moby/moby/api/types/image" + "github.com/moby/moby/client" + "go.uber.org/mock/gomock" + "gotest.tools/v3/assert" + + "github.com/docker/compose/v5/pkg/api" +) + +// callsOf counts the recorded engine calls named call. +func callsOf(d *interactiveUpDaemon, call string) int { + n := 0 + for _, c := range d.recorded() { + if c == call { + n++ + } + } + return n +} + +// assertCreatedThenStarted checks that the daemon was asked to create the +// container, then to start it. +func assertCreatedThenStarted(t *testing.T, d *interactiveUpDaemon) { + t.Helper() + create, start := d.indexOf("create"), d.indexOf("start") + assert.Assert(t, create >= 0, "the container was never created: %v", d.recorded()) + assert.Assert(t, start >= 0, "the container was never started: %v", d.recorded()) + assert.Assert(t, create < start, "the container must be created before it is started: %v", d.recorded()) +} + +// TestUpDetachedCreatesThenStartsTheContainers pins that a detached up leaves +// the application running: its plan covers both phases, so that what it creates +// it starts, with no session attached to it. +func TestUpDetachedCreatesThenStartsTheContainers(t *testing.T) { + h := newInteractiveUpHarness(t) + + assert.NilError(t, h.svc.Up(t.Context(), interactiveUpProject(), api.UpOptions{})) + + assertCreatedThenStarted(t, h.daemon) + assert.Equal(t, h.daemon.indexOf("attach"), -1, "a detached up attaches to nothing: %v", h.daemon.recorded()) +} + +// TestScaleCreatesThenStartsTheReplicas pins that scale starts the replicas it +// creates: it converges the project and starts it in a single plan, as a +// detached up does, and so does the rebuild of a service by watch, which goes +// through the same path. +func TestScaleCreatesThenStartsTheReplicas(t *testing.T) { + h := newInteractiveUpHarness(t) + + err := h.svc.Scale(t.Context(), interactiveUpProject(), api.ScaleOptions{Replicas: map[string]int{"web": 1}}) + + assert.NilError(t, err) + assertCreatedThenStarted(t, h.daemon) +} + +// TestRunStartsTheDependenciesOfTheOneOff pins that `compose run` brings up +// the dependencies of the service it runs -- created, then started -- and +// only those: the service itself is the one-off container's business. +func TestRunStartsTheDependenciesOfTheOneOff(t *testing.T) { + h := newInteractiveUpHarness(t) + project := interactiveUpProject() + app := types.ServiceConfig{Name: "app", ContainerSpec: types.ContainerSpec{Image: "alpine"}} + app.DependsOn = types.DependsOnConfig{"web": {Condition: types.ServiceConditionStarted, Required: true}} + project.Services["app"] = app + + err := h.svc.startDependencies(t.Context(), project, api.RunOptions{Project: project, Service: "app"}) + + assert.NilError(t, err) + assertCreatedThenStarted(t, h.daemon) + assert.Equal(t, callsOf(h.daemon, "create"), 1, "only the dependency is created, not the service run: %v", h.daemon.recorded()) + assert.Equal(t, callsOf(h.daemon, "start"), 1, "only the dependency is started: %v", h.daemon.recorded()) +} + +// TestUpDetachedWaitVerifiesTheApplicationIsReady pins what --wait adds to a +// detached up: once the containers are started, it checks that they are up -- +// an exited container fails the up, naming it, and one that does not become +// healthy in time fails it with the --wait-timeout that elapsed -- where +// without --wait the up returns as soon as the containers are started. +func TestUpDetachedWaitVerifiesTheApplicationIsReady(t *testing.T) { + healthcheck := &types.HealthCheckConfig{Test: []string{"CMD", "true"}} + projectWithHealthcheck := func() *types.Project { + project := interactiveUpProject() + web := project.Services["web"] + web.HealthCheck = healthcheck + project.Services["web"] = web + return project + } + + t.Run("a container that exited fails the up", func(t *testing.T) { + h := newInteractiveUpHarness(t) + + err := h.svc.Up(t.Context(), interactiveUpProject(), api.UpOptions{Start: api.StartOptions{Wait: true, WaitTimeout: 5 * time.Second}}) + + assert.ErrorContains(t, err, "container test-web-1 exited") + }) + + t.Run("a container that stays unhealthy fails the up at the --wait-timeout", func(t *testing.T) { + h := newInteractiveUpHarness(t) + h.daemon.inspectState = &container.State{Status: container.StateRunning, Health: &container.Health{Status: container.Starting}} + + err := h.svc.Up(t.Context(), projectWithHealthcheck(), api.UpOptions{Start: api.StartOptions{Wait: true, WaitTimeout: 100 * time.Millisecond}}) + + assert.Error(t, err, "application not healthy after 100ms") + }) + + t.Run("without --wait nothing is verified", func(t *testing.T) { + h := newInteractiveUpHarness(t) + + assert.NilError(t, h.svc.Up(t.Context(), interactiveUpProject(), api.UpOptions{})) + }) +} + +// TestWatchRebuildTouchesTheRebuiltServiceAndItsDependentsOnly pins which of +// the project's containers a rebuild by watch converges: those of the rebuilt +// service and of the services depending on it (their configuration may +// diverge from what the rebuilt service now is), and no others -- neither its +// dependencies, which are not rebuilt either, nor the services unrelated to +// it, whose stopped containers have no business being started by a rebuild. +// The images are not built here (no service has a build section); the project +// the rebuild works on is what is narrowed. +func TestWatchRebuildTouchesTheRebuiltServiceAndItsDependentsOnly(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + service := func(name string, dependsOn ...string) types.ServiceConfig { + s := types.ServiceConfig{Name: name, ContainerSpec: types.ContainerSpec{Image: "alpine"}} + if len(dependsOn) > 0 { + s.DependsOn = types.DependsOnConfig{} + } + for _, dep := range dependsOn { + s.DependsOn[dep] = types.ServiceDependency{Condition: types.ServiceConditionStarted, Required: true} + } + return s + } + project := &types.Project{ + Name: "prj", + Services: types.Services{ + "app": service("app", "database"), + "proxy": service("proxy", "app"), + "database": service("database"), + "unrelated": service("unrelated"), + }, + Networks: types.Networks{}, + Volumes: types.Volumes{}, + } + existing := []container.Summary{ + stoppedServiceContainer("proxy", 1, container.StateExited), + stoppedServiceContainer("database", 1, container.StateExited), + stoppedServiceContainer("unrelated", 1, container.StateExited), + } + + // what the daemon is asked to do, by container ID + var mu sync.Mutex + touched := map[string]bool{} + touch := func(id string) { + mu.Lock() + defer mu.Unlock() + touched[id] = true + } + apiClient.EXPECT().ImageInspect(gomock.Any(), "alpine", gomock.Any()).AnyTimes(). + Return(client.ImageInspectResult{InspectResponse: image.InspectResponse{ID: "sha256:alpine"}}, nil) + apiClient.EXPECT().NetworkList(gomock.Any(), gomock.Any()).AnyTimes().Return(client.NetworkListResult{}, nil) + apiClient.EXPECT().VolumeList(gomock.Any(), gomock.Any()).AnyTimes().Return(client.VolumeListResult{}, nil) + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).AnyTimes(). + Return(client.ContainerListResult{Items: existing}, nil) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(_ context.Context, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + touch("new:" + opts.Name) + return client.ContainerCreateResult{ID: "new-" + opts.Name}, nil + }) + apiClient.EXPECT().ContainerInspect(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(_ context.Context, id string, _ client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + return client.ContainerInspectResult{Container: container.InspectResponse{ + ID: id, Name: "/" + strings.TrimPrefix(id, "new-"), Config: &container.Config{}, NetworkSettings: &container.NetworkSettings{}, + }}, nil + }) + apiClient.EXPECT().ContainerStop(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(_ context.Context, id string, _ client.ContainerStopOptions) (client.ContainerStopResult, error) { + touch(id) + return client.ContainerStopResult{}, nil + }) + apiClient.EXPECT().ContainerRemove(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(_ context.Context, id string, _ client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + touch(id) + return client.ContainerRemoveResult{}, nil + }) + apiClient.EXPECT().ContainerRename(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes(). + Return(client.ContainerRenameResult{}, nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(_ context.Context, id string, _ client.ContainerStartOptions) (client.ContainerStartResult, error) { + touch(id) + return client.ContainerStartResult{}, nil + }) + + options := api.WatchOptions{Build: &api.BuildOptions{}, LogTo: &testLogConsumer{}} + assert.NilError(t, svc.rebuild(t.Context(), project, []string{"app"}, options)) + + touchesService := func(name string) bool { + for id := range touched { + if strings.Contains(id, "prj-"+name+"-1") { + return true + } + } + return false + } + assert.Assert(t, touchesService("app"), "the rebuilt service is created and started: %v", touched) + assert.Assert(t, touchesService("proxy"), "the container of the service depending on it is converged: %v", touched) + assert.Assert(t, !touchesService("database"), "a dependency of the rebuilt service is left alone: %v", touched) + assert.Assert(t, !touchesService("unrelated"), "a service unrelated to it is left alone: %v", touched) +} diff --git a/pkg/compose/up_phases_test.go b/pkg/compose/up_phases_test.go index 664357360d..78e8b83aee 100644 --- a/pkg/compose/up_phases_test.go +++ b/pkg/compose/up_phases_test.go @@ -23,6 +23,8 @@ import ( "context" "errors" "io" + "net" + "slices" "strings" "sync" "sync/atomic" @@ -30,6 +32,7 @@ import ( "time" "github.com/compose-spec/compose-go/v2/types" + dockercli "github.com/docker/cli/cli" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/streams" "github.com/moby/moby/api/types/container" @@ -74,12 +77,60 @@ type interactiveUpDaemon struct { slowSubscription bool started chan struct{} startedOnce sync.Once + // neverExits keeps the container running once started: no die event + neverExits bool + // startGate, when set, holds ContainerStart (after recording + // "start-requested") until it is closed, and then fails it with startErr + // if that is set + startGate chan struct{} + startErr error + // signals closes, the first time a call is recorded, the channel + // callSignal hands out for it + signals map[string]chan struct{} + // listErr, when set, fails the container listings made once the container + // is created + listErr error + // inspectState is the state the engine reports for the container; it + // reports it exited when unset + inspectState *container.State + // stopEndsStartedContainer makes the engine report the container dead when + // it is stopped once started; stopEndsContainer, whatever its state + stopEndsStartedContainer bool + stopEndsContainer bool + // exitCode is the exit code of the container when it exits, "0" if unset + exitCode string +} + +// callSignal returns a channel closed once call has been recorded, to wait for +// an engine call without polling. +func (d *interactiveUpDaemon) callSignal(call string) <-chan struct{} { + d.mu.Lock() + defer d.mu.Unlock() + return d.signalLocked(call) +} + +func (d *interactiveUpDaemon) signalLocked(call string) chan struct{} { + if d.signals == nil { + d.signals = map[string]chan struct{}{} + } + ch, ok := d.signals[call] + if !ok { + ch = make(chan struct{}) + d.signals[call] = ch + } + return ch } func (d *interactiveUpDaemon) record(call string) { d.mu.Lock() defer d.mu.Unlock() d.calls = append(d.calls, call) + ch := d.signalLocked(call) + select { + case <-ch: + default: + close(ch) + } } func (d *interactiveUpDaemon) recorded() []string { @@ -120,6 +171,9 @@ func expectInteractiveUpDaemon(t *testing.T, apiClient *mocks.MockAPIClient) *in if !d.created { return client.ContainerListResult{}, nil } + if d.listErr != nil { + return client.ContainerListResult{}, d.listErr + } return client.ContainerListResult{Items: []container.Summary{{ ID: "c1", Names: []string{"/test-web-1"}, Labels: labels, State: container.StateCreated, }}}, nil @@ -143,13 +197,19 @@ func expectInteractiveUpDaemon(t *testing.T, apiClient *mocks.MockAPIClient) *in return client.ContainerCreateResult{ID: "c1"}, nil }) apiClient.EXPECT().ContainerInspect(gomock.Any(), "c1", gomock.Any()).AnyTimes(). - Return(client.ContainerInspectResult{Container: container.InspectResponse{ - ID: "c1", - Name: "/test-web-1", - Config: &container.Config{Labels: labels}, - State: &container.State{Status: container.StateExited}, - NetworkSettings: &container.NetworkSettings{}, - }}, nil) + DoAndReturn(func(context.Context, string, client.ContainerInspectOptions) (client.ContainerInspectResult, error) { + state := d.inspectState + if state == nil { + state = &container.State{Status: container.StateExited} + } + return client.ContainerInspectResult{Container: container.InspectResponse{ + ID: "c1", + Name: "/test-web-1", + Config: &container.Config{Labels: labels}, + State: state, + NetworkSettings: &container.NetworkSettings{}, + }}, nil + }) // no attach endpoint: the log stream falls back to the logs API, which // the monitor-less assertions below don't read @@ -191,33 +251,68 @@ func expectInteractiveUpDaemon(t *testing.T, apiClient *mocks.MockAPIClient) *in }) // a teardown (the test giving up on a hung up) must not trip the mock - apiClient.EXPECT().ContainerStop(gomock.Any(), "c1", gomock.Any()).AnyTimes().Return(client.ContainerStopResult{}, nil) + apiClient.EXPECT().ContainerStop(gomock.Any(), "c1", gomock.Any()).AnyTimes(). + DoAndReturn(func(context.Context, string, client.ContainerStopOptions) (client.ContainerStopResult, error) { + d.record("stop") + if d.stopEndsContainer { + d.eventLog.emit(containerMessage(events.ActionDie, "c1", "test-web-1", "web", map[string]string{"exitCode": "143"})) + } else if d.stopEndsStartedContainer { + select { + case <-d.started: + d.eventLog.emit(containerMessage(events.ActionDie, "c1", "test-web-1", "web", map[string]string{"exitCode": "143"})) + default: + } + } + return client.ContainerStopResult{}, nil + }) apiClient.EXPECT().ContainerKill(gomock.Any(), "c1", gomock.Any()).AnyTimes().Return(client.ContainerKillResult{}, nil) // the container exits right after it starts apiClient.EXPECT().ContainerStart(gomock.Any(), "c1", gomock.Any()).AnyTimes(). DoAndReturn(func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) { - if d.startWaitsForEvents { - select { - case <-d.eventsSeen: - case <-time.After(5 * time.Second): - d.startedWithoutMonitor.Store(true) - } - } - d.record("start") - defer d.startedOnce.Do(func() { close(d.started) }) - d.eventLog.emit(containerMessage(events.ActionStart, "c1", "test-web-1", "web", labels)) - // The engine reports the exit some time after the start call, not - // within it. - timer := time.AfterFunc(containerExitDelay, func() { - d.eventLog.emit(containerMessage(events.ActionDie, "c1", "test-web-1", "web", map[string]string{"exitCode": "0"})) - }) - t.Cleanup(func() { timer.Stop() }) - return client.ContainerStartResult{}, nil + return client.ContainerStartResult{}, d.containerStart(t, labels) }) return d } +// containerStart is the engine starting the container: once the events +// subscription is open, if the test wants it so, and the start gate lifted, +// when it is set; it then emits the start event and, unless the container is +// to run for good, schedules its exit. +func (d *interactiveUpDaemon) containerStart(t *testing.T, labels map[string]string) error { + if d.startWaitsForEvents { + select { + case <-d.eventsSeen: + case <-time.After(5 * time.Second): + d.startedWithoutMonitor.Store(true) + } + } + if d.startGate != nil { + d.record("start-requested") + <-d.startGate + if d.startErr != nil { + return d.startErr + } + } + d.record("start") + defer d.startedOnce.Do(func() { close(d.started) }) + d.eventLog.emit(containerMessage(events.ActionStart, "c1", "test-web-1", "web", labels)) + if d.neverExits { + return nil + } + // The engine reports the exit some time after the start call, not + // within it. + exitCode := d.exitCode + if exitCode == "" { + exitCode = "0" + } + timer := time.AfterFunc(containerExitDelay, func() { + d.eventLog.emit(containerMessage(events.ActionDie, "c1", "test-web-1", "web", map[string]string{"exitCode": exitCode})) + }) + t.Cleanup(func() { timer.Stop() }) + return nil +} + // progressRecorder records the lifetime of the progress scopes ("up") in the // daemon's call log, to order them against the engine calls. type progressRecorder struct{ d *interactiveUpDaemon } @@ -234,6 +329,9 @@ type interactiveUpHarness struct { svc *composeService daemon *interactiveUpDaemon out *bytes.Buffer + // api is the engine's client, for the expectations a test adds to the + // daemon's + api *mocks.MockAPIClient } func newInteractiveUpHarness(t *testing.T) *interactiveUpHarness { @@ -254,7 +352,7 @@ func newInteractiveUpHarness(t *testing.T) *interactiveUpHarness { daemon := expectInteractiveUpDaemon(t, apiClient) svc, err := NewComposeService(cli, WithEventProcessor(progressRecorder{daemon})) assert.NilError(t, err) - return &interactiveUpHarness{svc: svc.(*composeService), daemon: daemon, out: out} + return &interactiveUpHarness{svc: svc.(*composeService), daemon: daemon, out: out, api: apiClient} } func interactiveUpProject() *types.Project { @@ -280,9 +378,15 @@ func (h *interactiveUpHarness) up(t *testing.T) error { // upWith is up on a context the test owns, to cancel it at a given point. func (h *interactiveUpHarness) upWith(t *testing.T, ctx context.Context, cancel context.CancelFunc) error { + t.Helper() + return h.upWithOptions(t, ctx, cancel, interactiveUpOptions()) +} + +// upWithOptions is upWith for given options. +func (h *interactiveUpHarness) upWithOptions(t *testing.T, ctx context.Context, cancel context.CancelFunc, options api.UpOptions) error { t.Helper() done := make(chan error, 1) - go func() { done <- h.svc.Up(ctx, interactiveUpProject(), interactiveUpOptions()) }() + go func() { done <- h.svc.Up(ctx, interactiveUpProject(), options) }() select { case err := <-done: return err @@ -496,3 +600,259 @@ func TestUpInteractiveCreatePhaseCarriesThePlanProject(t *testing.T) { _, caller := project.Configs["#apisocket"] assert.Assert(t, !caller, "the caller's project is not transformed") } + +// TestUpInteractiveFailsOnTheStartPhaseErrorOfAnUninterruptedUp pins that +// nothing excuses a Start phase failing in an up nobody interrupted: it is the +// up's error (see TestUpInteractiveIgnoresTheStartPhaseErrorOfAnInterruptedUp +// for the case it is not). +func TestUpInteractiveFailsOnTheStartPhaseErrorOfAnUninterruptedUp(t *testing.T) { + h := newInteractiveUpHarness(t) + h.daemon.startGate = make(chan struct{}) + h.daemon.startErr = errors.New("cannot start the container") + close(h.daemon.startGate) + + err := h.up(t) + assert.ErrorContains(t, err, "cannot start the container") +} + +// TestUpInteractiveWatchesTheServicesItTargets pins which containers the +// session follows, and so ends with: those of the services the up targets +// (Start.Services) when it has some, those it attaches to otherwise. The +// container here never exits, and what is watched is a service with no +// container: the session ends as soon as the Start phase is over. Were the +// monitor watching the container too, the up would hang with it. +func TestUpInteractiveWatchesTheServicesItTargets(t *testing.T) { + tests := []struct { + name string + services []string + attachTo []string + }{ + {name: "the targeted services, not the attached ones", services: []string{"other"}, attachTo: []string{"web"}}, + {name: "the attached services when none is targeted", attachTo: []string{"other"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h := newInteractiveUpHarness(t) + h.daemon.neverExits = true + options := interactiveUpOptions() + options.Start.Services = tt.services + options.Start.AttachTo = tt.attachTo + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + assert.NilError(t, h.upWithOptions(t, ctx, cancel, options)) + }) + } +} + +// TestUpReportsTheContainersItLeavesRunning pins that an up reports as Running +// the containers its plan won't touch, so that the progress display accounts +// for the whole project: the plan holds nothing for them. It is the +// preparation of the plan, which a detached and an interactive up share, that +// does it. +func TestUpReportsTheContainersItLeavesRunning(t *testing.T) { + svc, apiClient, recorder := newStartPhaseTestService(t) + apiClient.EXPECT().ImageInspect(gomock.Any(), "alpine", gomock.Any()).AnyTimes(). + Return(client.ImageInspectResult{InspectResponse: image.InspectResponse{ID: "sha256:alpine"}}, nil) + apiClient.EXPECT().NetworkList(gomock.Any(), gomock.Any()).AnyTimes().Return(client.NetworkListResult{}, nil) + apiClient.EXPECT().VolumeList(gomock.Any(), gomock.Any()).AnyTimes().Return(client.VolumeListResult{}, nil) + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).Return(client.ContainerListResult{Items: []container.Summary{{ + ID: "c1", + Names: []string{"/test-web-1"}, + State: container.StateRunning, + Labels: map[string]string{ + api.ProjectLabel: "test", + api.ServiceLabel: "web", + api.OneoffLabel: "False", + api.ContainerNumberLabel: "1", + api.ConfigHashLabel: "whatever", + }, + }}}, nil).AnyTimes() + + // never recreate: the running container stays whatever its configuration + options := api.UpOptions{Create: api.CreateOptions{Recreate: api.RecreateNever}} + _, _, plan, err := svc.prepareUp(t.Context(), interactiveUpProject(), options) + + assert.NilError(t, err) + assert.Equal(t, len(plan.Nodes), 0, "the plan leaves the running container alone:\n%s", plan) + assert.DeepEqual(t, recorder.byID["Container test-web-1"], []string{api.StatusRunning}) +} + +// createdUpOf builds by hand what an interactive up carries from its Create +// phase to its Start phase, for a plan the test writes itself: a Create phase +// with nothing to do, and the Start phase of plan. It also returns the +// executor, to give it what the daemon's listing would have. +func (h *interactiveUpHarness) createdUpOf(t *testing.T, project *types.Project, plan *Plan) (*createdUp, *planExecutor) { + t.Helper() + exec := h.svc.newPlanExecutor(project, emptyObservedState(project.Name)) + run, err := exec.begin(plan) + assert.NilError(t, err) + assert.NilError(t, run.runCreate(t.Context())) + return &createdUp{project: project, run: run}, exec +} + +// runInteractiveUp runs the session of created, failing the test instead of +// hanging when it doesn't return. +func (h *interactiveUpHarness) runInteractiveUp(t *testing.T, created *createdUp) error { + t.Helper() + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + done := make(chan error, 1) + go func() { done <- h.svc.runInteractiveUp(ctx, created, interactiveUpOptions()) }() + select { + case err := <-done: + return err + case <-time.After(10 * time.Second): + cancel() + t.Fatalf("the session did not return; calls so far: %v", h.daemon.recorded()) + return nil + } +} + +func webContainerSummary() container.Summary { + return container.Summary{ID: "c1", Names: []string{"/test-web-1"}, Labels: map[string]string{api.ServiceLabel: "web"}} +} + +// TestUpInteractiveShowsTheHookLogsInTheSession pins that what the post_start +// hook of a service prints during an interactive up's Start phase reaches the +// session's log consumer, as the output of a container does: the session's +// printer is the listener the Start phase is given. +func TestUpInteractiveShowsTheHookLogsInTheSession(t *testing.T) { + h := newInteractiveUpHarness(t) + project := interactiveUpProject() + web := project.Services["web"] + web.PostStart = []types.ServiceHook{{Command: types.ShellCommand{"/notify.sh"}}} + project.Services["web"] = web + ctr := webContainerSummary() + + serverConn, clientConn := net.Pipe() + go func() { + if err := writeStdcopyFrame(serverConn, 1, "hook says hello\n"); err != nil { + t.Error(err) + } + serverConn.Close() //nolint:errcheck + }() + h.api.EXPECT().ExecCreate(gomock.Any(), "c1", gomock.Any()).Return(client.ExecCreateResult{ID: "exec1"}, nil) + h.api.EXPECT().ExecAttach(gomock.Any(), "exec1", gomock.Any()). + Return(client.ExecAttachResult{HijackedResponse: client.NewHijackedResponse(clientConn, "")}, nil) + h.api.EXPECT().ExecInspect(gomock.Any(), "exec1", gomock.Any()).Return(client.ExecInspectResult{ExitCode: 0}, nil) + + plan := &Plan{} + start := plan.addNode(Operation{Type: OpStartContainer, ResourceID: "service:web:1", Cause: "start", Service: &web, Container: &ctr}, "start:web:1") + start.Phase = PhaseStart + post := plan.addNode(Operation{Type: OpRunPostStart, ResourceID: "service:web:1", Cause: "post_start hooks", Service: &web, Container: &ctr}, "start:web:1", start) + post.Phase = PhaseStart + created, _ := h.createdUpOf(t, project, plan) + + consumer := &testLogConsumer{} + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + options := interactiveUpOptions() + options.Start.Attach = consumer + done := make(chan error, 1) + go func() { done <- h.svc.runInteractiveUp(ctx, created, options) }() + select { + case err := <-done: + assert.NilError(t, err) + case <-time.After(10 * time.Second): + t.Fatalf("the session did not return; calls so far: %v", h.daemon.recorded()) + } + + var logged []string + for _, lines := range consumer.logs { + logged = append(logged, lines...) + } + assert.Assert(t, slices.Contains(logged, "hook says hello"), "the hook's output did not reach the session: %v", consumer.logs) +} + +// TestUpInteractiveTranslatesAWaitTimeout pins what a depends_on wait that +// outlasts --wait-timeout makes of an interactive up: the dependency timeout +// the other commands report, not the composite error the executor builds to +// tell that wait's own deadline from any other. +func TestUpInteractiveTranslatesAWaitTimeout(t *testing.T) { + h := newInteractiveUpHarness(t) + project := interactiveUpProject() + web := project.Services["web"] + ctr := webContainerSummary() + h.api.EXPECT().ContainerInspect(gomock.Any(), "db-id", gomock.Any()).AnyTimes(). + Return(client.ContainerInspectResult{Container: container.InspectResponse{ + ID: "db-id", + Name: "/test-db-1", + Config: &container.Config{Healthcheck: &container.HealthConfig{Test: []string{"CMD", "true"}}}, + State: &container.State{Status: container.StateRunning, Health: &container.Health{Status: container.Starting}}, + }}, nil) + + plan := &Plan{} + wait := plan.addNode(Operation{Type: OpWaitCondition, ResourceID: "wait:db:service_healthy", Name: "db", Condition: types.ServiceConditionHealthy}, "") + wait.Phase = PhaseStart + start := plan.addNode(Operation{Type: OpStartContainer, ResourceID: "service:web:1", Cause: "start", Service: &web, Container: &ctr}, "start:web:1", wait) + start.Phase = PhaseStart + created, exec := h.createdUpOf(t, project, plan) + exec.waitTimeout = 50 * time.Millisecond + exec.containersByService["db"] = Containers{{ID: "db-id", Names: []string{"/test-db-1"}, Labels: map[string]string{api.ServiceLabel: "db"}}} + + err := h.runInteractiveUp(t, created) + + assert.Error(t, err, "timeout waiting for dependencies") + assert.Equal(t, h.daemon.indexOf("start"), -1, "web must not start before its dependency is healthy: %v", h.daemon.recorded()) +} + +// TestUpInteractiveFailedAttachEndsTheSession pins that an interactive up whose +// session cannot attach to the created containers fails with that error, starts +// nothing, and returns rather than waiting on the session it had begun to set +// up. +func TestUpInteractiveFailedAttachEndsTheSession(t *testing.T) { + h := newInteractiveUpHarness(t) + h.daemon.listErr = errors.New("cannot list the containers to attach to") + + err := h.up(t) + + assert.ErrorContains(t, err, "cannot list the containers to attach to") + assert.Equal(t, h.daemon.indexOf("start"), -1, "nothing may start: %v", h.daemon.recorded()) +} + +// TestPrepareUpBoundsEveryWaitByTheWaitTimeout pins that --wait-timeout reaches +// the executor of an up, detached or interactive: it bounds each depends_on +// wait of the plan, even without --wait. +func TestPrepareUpBoundsEveryWaitByTheWaitTimeout(t *testing.T) { + svc, apiClient, _ := newStartPhaseTestService(t) + apiClient.EXPECT().ImageInspect(gomock.Any(), "alpine", gomock.Any()).AnyTimes(). + Return(client.ImageInspectResult{InspectResponse: image.InspectResponse{ID: "sha256:alpine"}}, nil) + apiClient.EXPECT().NetworkList(gomock.Any(), gomock.Any()).AnyTimes().Return(client.NetworkListResult{}, nil) + apiClient.EXPECT().VolumeList(gomock.Any(), gomock.Any()).AnyTimes().Return(client.VolumeListResult{}, nil) + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).AnyTimes().Return(client.ContainerListResult{}, nil) + + options := api.UpOptions{Start: api.StartOptions{WaitTimeout: 42 * time.Second}} + _, exec, _, err := svc.prepareUp(t.Context(), interactiveUpProject(), options) + + assert.NilError(t, err) + assert.Equal(t, exec.waitTimeout, 42*time.Second) +} + +// TestUpInteractiveExitsWithTheExitCodeOfTheServiceItIsToldTo pins what +// --exit-code-from does to an interactive up: the up ends with the exit code +// the container of the named service exited with, as a status error, where +// without it the up succeeds whatever the containers exited with. +func TestUpInteractiveExitsWithTheExitCodeOfTheServiceItIsToldTo(t *testing.T) { + t.Run("with --exit-code-from", func(t *testing.T) { + h := newInteractiveUpHarness(t) + h.daemon.exitCode = "3" + options := interactiveUpOptions() + options.Start.ExitCodeFrom = "web" + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + err := h.upWithOptions(t, ctx, cancel, options) + + var status dockercli.StatusError + assert.Assert(t, errors.As(err, &status), "err: %v", err) + assert.Equal(t, status.StatusCode, 3) + }) + + t.Run("without it", func(t *testing.T) { + h := newInteractiveUpHarness(t) + h.daemon.exitCode = "3" + + assert.NilError(t, h.up(t)) + }) +} diff --git a/pkg/compose/up_signal_test.go b/pkg/compose/up_signal_test.go new file mode 100644 index 0000000000..4c96b6ecb2 --- /dev/null +++ b/pkg/compose/up_signal_test.go @@ -0,0 +1,144 @@ +//go:build !windows + +/* + Copyright 2020 Docker Compose CLI authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package compose + +import ( + "errors" + "os" + "os/signal" + "syscall" + "testing" + "time" + + "gotest.tools/v3/assert" +) + +// TestUpInteractiveDoesNotHandleSignalsDuringTheCreatePhase pins when an +// interactive up starts to handle SIGINT and SIGTERM itself: once its Create +// phase is over, not before. A signal met during the Create phase belongs to +// its caller, which interrupts the phase through the context; were the +// session already registered, the signal would be waiting in its channel when +// it starts and tear the application down at once, a graceful stop nobody +// asked for after the interruption had been dealt with. +// +// The signal is real: the test takes it too, so that it doesn't end the test +// process, and waits for it to be delivered. +func TestUpInteractiveDoesNotHandleSignalsDuringTheCreatePhase(t *testing.T) { + mine := make(chan os.Signal, 1) + signal.Notify(mine, syscall.SIGINT) + defer signal.Stop(mine) + + h := newInteractiveUpHarness(t) + h.daemon.onCreate = func() { + if err := syscall.Kill(os.Getpid(), syscall.SIGINT); err != nil { + t.Error(err) + return + } + select { + case <-mine: + case <-time.After(10 * time.Second): + t.Error("the signal was never delivered") + } + } + + assert.NilError(t, h.up(t)) + assert.Equal(t, h.daemon.indexOf("stop"), -1, "the application was stopped after a signal met during the Create phase: %v", h.daemon.recorded()) +} + +// TestUpInteractiveStopsAContainerThatStartsAfterTheTeardownSweep pins what a +// graceful teardown does with a container whose start was already in flight +// when it stopped the application: the one-shot sweep of the teardown misses +// it, since it was not up yet, and it comes up right after. The session stops +// it as it appears, rather than staying up for as long as it runs. +// +// The interruption is a real signal, the one the session itself handles (a +// canceled context would end the session's monitor with it): the test takes it +// too, so that it doesn't end the test process. +func TestUpInteractiveStopsAContainerThatStartsAfterTheTeardownSweep(t *testing.T) { + mine := make(chan os.Signal, 1) + signal.Notify(mine, syscall.SIGINT) + defer signal.Stop(mine) + + h := newInteractiveUpHarness(t) + h.daemon.startGate = make(chan struct{}) + h.daemon.neverExits = true + h.daemon.stopEndsStartedContainer = true + go func() { + select { + case <-h.daemon.callSignal("start-requested"): + case <-t.Context().Done(): + return + } + if err := syscall.Kill(os.Getpid(), syscall.SIGINT); err != nil { + t.Error(err) + return + } + // the sweep of the teardown, which finds the container not up yet + select { + case <-h.daemon.callSignal("stop"): + case <-t.Context().Done(): + return + } + // the start goes through right after it + close(h.daemon.startGate) + }() + + assert.NilError(t, h.up(t)) + assert.Equal(t, callsOf(h.daemon, "stop"), 2, "the container that started after the sweep must be stopped too: %v", h.daemon.recorded()) +} + +// TestUpInteractiveIgnoresTheStartPhaseErrorOfAnInterruptedUp pins that a +// Start phase failing while the up is being torn down is not the up's error: +// the Start phase runs on a context that outlives the interruption, so the +// graceful stop the interruption triggers can land on a container whose start +// is still in flight, and the start then fails because of it. +// +// The interruption is a real signal (see +// TestUpInteractiveStopsAContainerThatStartsAfterTheTeardownSweep). +func TestUpInteractiveIgnoresTheStartPhaseErrorOfAnInterruptedUp(t *testing.T) { + mine := make(chan os.Signal, 1) + signal.Notify(mine, syscall.SIGINT) + defer signal.Stop(mine) + + h := newInteractiveUpHarness(t) + h.daemon.startGate = make(chan struct{}) + h.daemon.startErr = errors.New("container start interrupted by the stop") + h.daemon.stopEndsContainer = true + go func() { + // the container start is in flight when the interruption comes, and + // fails only once the graceful stop it triggered went through + select { + case <-h.daemon.callSignal("start-requested"): + case <-t.Context().Done(): + return + } + if err := syscall.Kill(os.Getpid(), syscall.SIGINT); err != nil { + t.Error(err) + return + } + select { + case <-h.daemon.callSignal("stop"): + case <-t.Context().Done(): + return + } + close(h.daemon.startGate) + }() + + assert.NilError(t, h.up(t)) +} From df9743565fbb9a32450ea715bf358b32039d976d Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 27 Oct 2026 14:29:35 +0100 Subject: [PATCH 6/8] test(restart): lock the dependency wait and the timeout unit Pin that restart waits for a restart:true dependency to hold its condition again before restarting its dependent, and that its timeout reaches the daemon in whole seconds. Part of #14081. Signed-off-by: Nicolas De Loof --- pkg/compose/restart_test.go | 65 +++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/pkg/compose/restart_test.go b/pkg/compose/restart_test.go index f74dd8f6ed..e2d69daca3 100644 --- a/pkg/compose/restart_test.go +++ b/pkg/compose/restart_test.go @@ -26,6 +26,7 @@ import ( "time" "github.com/compose-spec/compose-go/v2/types" + "github.com/moby/moby/api/types/container" "github.com/moby/moby/client" "go.uber.org/mock/gomock" "gotest.tools/v3/assert" @@ -210,3 +211,67 @@ func TestRestart_ConcurrencyIsBoundedAcrossServices(t *testing.T) { assert.NilError(t, err) assert.Equal(t, tracker.Peak(), 1, "restart must never run more than maxConcurrency ContainerRestart calls at once") } + +// TestRestart_WaitsForADependencyBeforeRestartingItsDependent pins the order +// restart gives a depends_on relation declaring restart:true: the dependency +// is restarted first, and its dependent waits for the condition to hold again +// -- here healthy -- before being restarted itself. +func TestRestart_WaitsForADependencyBeforeRestartingItsDependent(t *testing.T) { + svc, apiClient, _ := newStartTestService(t) + + db := types.ServiceConfig{Name: "db"} + web := types.ServiceConfig{Name: "web"} + web.DependsOn = types.DependsOnConfig{"db": {Condition: types.ServiceConditionHealthy, Restart: true, Required: true}} + project := &types.Project{Name: "prj", Services: types.Services{"db": db, "web": web}} + dbCtr := stoppedServiceContainer("db", 1, container.StateRunning) + webCtr := stoppedServiceContainer("web", 1, container.StateRunning) + + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). + Return(client.ContainerListResult{Items: []container.Summary{dbCtr, webCtr}}, nil) + dbRestart := apiClient.EXPECT().ContainerRestart(gomock.Any(), dbCtr.ID, gomock.Any()). + Return(client.ContainerRestartResult{}, nil) + dbHealthy := apiClient.EXPECT().ContainerInspect(gomock.Any(), dbCtr.ID, gomock.Any()). + Return(inspectResultOf(dbCtr.ID, "prj-db-1", container.StateRunning, 0, &container.Health{Status: container.Healthy}), nil). + After(dbRestart) + apiClient.EXPECT().ContainerRestart(gomock.Any(), webCtr.ID, gomock.Any()). + Return(client.ContainerRestartResult{}, nil).After(dbHealthy) + + err := svc.restart(t.Context(), "prj", api.RestartOptions{Project: project}) + + assert.NilError(t, err) +} + +// TestRestartContainer_PassesTheTimeoutInWholeSeconds pins how the timeout +// given to restart reaches the daemon: in whole seconds, the unit the engine's +// API knows, and not at all when none was given, so that the container's own +// stop timeout applies. +func TestRestartContainer_PassesTheTimeoutInWholeSeconds(t *testing.T) { + seconds := func(d time.Duration) *time.Duration { return &d } + tests := []struct { + name string + timeout *time.Duration + want *int + }{ + {name: "none given", timeout: nil, want: nil}, + {name: "whole seconds", timeout: seconds(5 * time.Second), want: intPtr(5)}, + {name: "zero is a timeout too", timeout: seconds(0), want: intPtr(0)}, + {name: "a fraction of a second is dropped", timeout: seconds(2500 * time.Millisecond), want: intPtr(2)}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc, apiClient, _ := newStartTestService(t) + ctr := serviceContainer("web") + var got client.ContainerRestartOptions + apiClient.EXPECT().ContainerRestart(gomock.Any(), ctr.ID, gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, opts client.ContainerRestartOptions) (client.ContainerRestartResult, error) { + got = opts + return client.ContainerRestartResult{}, nil + }) + + err := svc.restartContainer(t.Context(), types.ServiceConfig{Name: "web"}, ctr, api.RestartOptions{Timeout: tt.timeout}) + + assert.NilError(t, err) + assert.DeepEqual(t, got.Timeout, tt.want) + }) + } +} From 09d7231bfdd7cf72c24a2396b1a8ccf6de397313 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 27 Oct 2026 14:57:34 +0100 Subject: [PATCH 7/8] test(up): close the late-starter race and the pipe in the test harness Close the started channel before the start event is emitted, so that a late starter stopped in reaction to it always finds the container started, and close both ends of the pipe the hook logs test hands to the session. Signed-off-by: Nicolas De Loof --- pkg/compose/up_phases_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/compose/up_phases_test.go b/pkg/compose/up_phases_test.go index 78e8b83aee..83ff1ea4bd 100644 --- a/pkg/compose/up_phases_test.go +++ b/pkg/compose/up_phases_test.go @@ -295,7 +295,9 @@ func (d *interactiveUpDaemon) containerStart(t *testing.T, labels map[string]str } } d.record("start") - defer d.startedOnce.Do(func() { close(d.started) }) + // before the start event: whoever reacts to it (the session stopping a + // late starter) finds the container started + d.startedOnce.Do(func() { close(d.started) }) d.eventLog.emit(containerMessage(events.ActionStart, "c1", "test-web-1", "web", labels)) if d.neverExits { return nil @@ -726,6 +728,10 @@ func TestUpInteractiveShowsTheHookLogsInTheSession(t *testing.T) { ctr := webContainerSummary() serverConn, clientConn := net.Pipe() + t.Cleanup(func() { + serverConn.Close() //nolint:errcheck + clientConn.Close() //nolint:errcheck + }) go func() { if err := writeStdcopyFrame(serverConn, 1, "hook says hello\n"); err != nil { t.Error(err) From 953588bbaa7fc30d8257afe66f9c644b7351aa5c Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 27 Oct 2026 15:09:20 +0100 Subject: [PATCH 8/8] test(up): report the hook write error from the test goroutine Signed-off-by: Nicolas De Loof --- pkg/compose/up_phases_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/compose/up_phases_test.go b/pkg/compose/up_phases_test.go index 83ff1ea4bd..5c7503ebfd 100644 --- a/pkg/compose/up_phases_test.go +++ b/pkg/compose/up_phases_test.go @@ -732,10 +732,11 @@ func TestUpInteractiveShowsTheHookLogsInTheSession(t *testing.T) { serverConn.Close() //nolint:errcheck clientConn.Close() //nolint:errcheck }) + // The write error is handed back to the test goroutine: reporting it from + // here could hit a test that has already finished. + writeErr := make(chan error, 1) go func() { - if err := writeStdcopyFrame(serverConn, 1, "hook says hello\n"); err != nil { - t.Error(err) - } + writeErr <- writeStdcopyFrame(serverConn, 1, "hook says hello\n") serverConn.Close() //nolint:errcheck }() h.api.EXPECT().ExecCreate(gomock.Any(), "c1", gomock.Any()).Return(client.ExecCreateResult{ID: "exec1"}, nil) @@ -763,6 +764,7 @@ func TestUpInteractiveShowsTheHookLogsInTheSession(t *testing.T) { case <-time.After(10 * time.Second): t.Fatalf("the session did not return; calls so far: %v", h.daemon.recorded()) } + assert.NilError(t, <-writeErr) var logged []string for _, lines := range consumer.logs {