From 674010661a02c545e6424c682d6f8c3ff9bc6a72 Mon Sep 17 00:00:00 2001 From: Guillaume Lours Date: Wed, 19 Aug 2026 14:48:34 +0000 Subject: [PATCH] feat(hooks): retain failed pre_start containers for post-mortem A failing pre_start hook container is now retained (AutoRemove: false) so operators can run 'docker logs ' and 'docker ps -a' to diagnose the failure. On success the container is removed explicitly, mirroring the old AutoRemove behaviour including anonymous volumes. Before each pre_start run, stale hook containers from a previous failed run are detected via project+service+HookLabel filters and force-removed so they do not accumulate. Changes: - pkg/api/labels.go: add HookLabel (com.docker.compose.hook) - pkg/compose/filters.go: add hookFilter helper - pkg/compose/pre_start.go: - AutoRemove: false in createPreStartContainer - HookLabel added to container labels - runPreStartHook: explicit ContainerRemove on success; retain on failure - removeOrphanPreStartContainers: new helper called in runPreStart - Tests: update all existing pre_start tests for the new flow; add feature tests (success removes, failure retains, orphan cleanup) and coverage-gap tests (lowestNumberedContainer, waitPreStart cancel, preStartResultErr, streamPreStartLogs error paths, old-API network paths, ExecCreate/Attach/Inspect errors, hookExitError branches) Coverage after: hook.go 100%, pre_start.go most functions 100% (was 89% and 64% respectively per Codecov delta). Signed-off-by: Guillaume Lours --- pkg/api/labels.go | 5 + pkg/compose/filters.go | 4 + pkg/compose/hook_test.go | 163 +++++++ pkg/compose/pre_start.go | 105 ++++- pkg/compose/pre_start_test.go | 775 +++++++++++++++++++++++++++++++++- 5 files changed, 1007 insertions(+), 45 deletions(-) diff --git a/pkg/api/labels.go b/pkg/api/labels.go index 793973813..9122fb5a8 100644 --- a/pkg/api/labels.go +++ b/pkg/api/labels.go @@ -62,6 +62,11 @@ const ( ContainerReplaceLabel = "com.docker.compose.replace" // ContainerEngineLabel stores the name of the engine that runs the container ContainerEngineLabel = "com.docker.compose.engine" + // HookLabel identifies an ephemeral lifecycle-hook container and stores the + // hook type, e.g. "pre_start". It is applied to every container created by + // runPreStartHook so orphan hook containers from a previous failed run can + // be found and removed by project+service+hook label filters. + HookLabel = "com.docker.compose.hook" ) // ComposeVersion is the compose tool version as declared by label VersionLabel diff --git a/pkg/compose/filters.go b/pkg/compose/filters.go index 5a4e245d3..26b507a43 100644 --- a/pkg/compose/filters.go +++ b/pkg/compose/filters.go @@ -53,3 +53,7 @@ func oneOffFilter(b bool) string { func containerNumberFilter(index int) string { return labelFilter(api.ContainerNumberLabel, strconv.Itoa(index)) } + +func hookFilter(hookType string) string { + return labelFilter(api.HookLabel, hookType) +} diff --git a/pkg/compose/hook_test.go b/pkg/compose/hook_test.go index 5072ead03..5a4ad1d8e 100644 --- a/pkg/compose/hook_test.go +++ b/pkg/compose/hook_test.go @@ -19,8 +19,10 @@ package compose import ( + "bufio" "context" "encoding/binary" + "fmt" "io" "net" "os" @@ -434,3 +436,164 @@ func TestRunHook_ContextCancellation(t *testing.T) { t.Fatal("runHook did not return within 5 s after ctx cancellation") } } + +// TestRunHook_CopyError covers the branch in runHook where the copy goroutine +// itself returns an error (e.g. a mid-stream I/O failure on the exec reader). +// The error must be propagated immediately without blocking. +func TestRunHook_CopyError(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mockAPI := mocks.NewMockAPIClient(mockCtrl) + mockCli := mocks.NewMockCli(mockCtrl) + mockCli.EXPECT().Client().Return(mockAPI).AnyTimes() + mockCli.EXPECT().Err().Return(streams.NewOut(os.Stderr)).AnyTimes() + mockCli.EXPECT().Out().Return(streams.NewOut(os.Stdout)).AnyTimes() + + ctr := container.Summary{ID: "ctr-1"} + service := types.ServiceConfig{Name: "svc", Tty: false} + hook := types.ServiceHook{Command: types.ShellCommand{"true"}} + + mockAPI.EXPECT(). + ExecCreate(gomock.Any(), "ctr-1", gomock.Any()). + Return(client.ExecCreateResult{ID: "exec-1"}, nil) + + // Build a pipe whose read side immediately returns a hard error, simulating + // an I/O failure mid-stream rather than a clean EOF. + pr, pw := io.Pipe() + _ = pw.CloseWithError(fmt.Errorf("simulated I/O failure")) + + // Use net.Pipe() only for the Conn field (Close); reads come from pr. + serverConn, clientConn := net.Pipe() + defer serverConn.Close() //nolint:errcheck + defer clientConn.Close() //nolint:errcheck + + mockAPI.EXPECT(). + ExecAttach(gomock.Any(), "exec-1", gomock.Any()). + Return(client.ExecAttachResult{ + HijackedResponse: client.HijackedResponse{ + Conn: clientConn, + Reader: bufio.NewReader(pr), + }, + }, nil) + // ExecInspect must NOT be called: runHook must return the copy error first. + + s, err := NewComposeService(mockCli) + assert.NilError(t, err) + + err = s.(*composeService).runHook(t.Context(), ctr, service, hook, nil) + assert.ErrorContains(t, err, "simulated I/O failure") +} + +// TestHookExitError_NoOutput covers the branch in hookExitError where both +// stdout and stderr tails are empty — the error must not include an output suffix. +func TestHookExitError_NoOutput(t *testing.T) { + empty1 := newOutputTail(10, 512) + empty2 := newOutputTail(10, 512) + err := hookExitError("mysvc", 127, empty1, empty2) + assert.ErrorContains(t, err, "mysvc") + assert.ErrorContains(t, err, "127") + // No colon separator: the no-output format has no ': ' suffix. + assert.Assert(t, !strings.Contains(err.Error(), ": "), "unexpected output suffix in: %q", err.Error()) +} + +// TestHookExitError_StdoutFallback covers the branch in hookExitError where +// stderr is empty but stdout has content: stdout is used as the fallback output. +func TestHookExitError_StdoutFallback(t *testing.T) { + stdoutTail := newOutputTail(10, 512) + _, _ = stdoutTail.Write([]byte("stdout only line\n")) + stderrTail := newOutputTail(10, 512) // empty + + err := hookExitError("mysvc", 1, stdoutTail, stderrTail) + assert.ErrorContains(t, err, "stdout only line") +} + +// TestRunHook_ExecCreateError covers the branch where ExecCreate returns an +// error — runHook must propagate it immediately. +func TestRunHook_ExecCreateError(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mockAPI := mocks.NewMockAPIClient(mockCtrl) + mockCli := mocks.NewMockCli(mockCtrl) + mockCli.EXPECT().Client().Return(mockAPI).AnyTimes() + mockCli.EXPECT().Err().Return(streams.NewOut(os.Stderr)).AnyTimes() + mockCli.EXPECT().Out().Return(streams.NewOut(os.Stdout)).AnyTimes() + + ctr := container.Summary{ID: "ctr-1"} + service := types.ServiceConfig{Name: "svc"} + hook := types.ServiceHook{Command: types.ShellCommand{"true"}} + + mockAPI.EXPECT(). + ExecCreate(gomock.Any(), "ctr-1", gomock.Any()). + Return(client.ExecCreateResult{}, fmt.Errorf("exec create failed")) + + s, err := NewComposeService(mockCli) + assert.NilError(t, err) + err = s.(*composeService).runHook(t.Context(), ctr, service, hook, nil) + assert.ErrorContains(t, err, "exec create failed") +} + +// TestRunHook_ExecAttachError covers the branch where ExecAttach returns an +// error — runHook must propagate it immediately. +func TestRunHook_ExecAttachError(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mockAPI := mocks.NewMockAPIClient(mockCtrl) + mockCli := mocks.NewMockCli(mockCtrl) + mockCli.EXPECT().Client().Return(mockAPI).AnyTimes() + mockCli.EXPECT().Err().Return(streams.NewOut(os.Stderr)).AnyTimes() + mockCli.EXPECT().Out().Return(streams.NewOut(os.Stdout)).AnyTimes() + + ctr := container.Summary{ID: "ctr-1"} + service := types.ServiceConfig{Name: "svc"} + hook := types.ServiceHook{Command: types.ShellCommand{"true"}} + + mockAPI.EXPECT(). + ExecCreate(gomock.Any(), "ctr-1", gomock.Any()). + Return(client.ExecCreateResult{ID: "exec-1"}, nil) + mockAPI.EXPECT(). + ExecAttach(gomock.Any(), "exec-1", gomock.Any()). + Return(client.ExecAttachResult{}, fmt.Errorf("exec attach failed")) + + s, err := NewComposeService(mockCli) + assert.NilError(t, err) + err = s.(*composeService).runHook(t.Context(), ctr, service, hook, nil) + assert.ErrorContains(t, err, "exec attach failed") +} + +// TestRunHook_ExecInspectError covers the branch where ExecInspect returns an +// error — runHook must propagate it (copy finished successfully beforehand). +func TestRunHook_ExecInspectError(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mockAPI := mocks.NewMockAPIClient(mockCtrl) + mockCli := mocks.NewMockCli(mockCtrl) + mockCli.EXPECT().Client().Return(mockAPI).AnyTimes() + mockCli.EXPECT().Err().Return(streams.NewOut(os.Stderr)).AnyTimes() + mockCli.EXPECT().Out().Return(streams.NewOut(os.Stdout)).AnyTimes() + + ctr := container.Summary{ID: "ctr-1"} + service := types.ServiceConfig{Name: "svc", Tty: false} + hook := types.ServiceHook{Command: types.ShellCommand{"true"}} + + mockAPI.EXPECT(). + ExecCreate(gomock.Any(), "ctr-1", gomock.Any()). + Return(client.ExecCreateResult{ID: "exec-1"}, nil) + + // Pipe that closes immediately (EOF) so StdCopy returns with no error. + serverConn, clientConn := net.Pipe() + serverConn.Close() //nolint:errcheck + mockAPI.EXPECT(). + ExecAttach(gomock.Any(), "exec-1", gomock.Any()). + Return(client.ExecAttachResult{ + HijackedResponse: client.NewHijackedResponse(clientConn, ""), + }, nil) + + mockAPI.EXPECT(). + ExecInspect(gomock.Any(), "exec-1", gomock.Any()). + Return(client.ExecInspectResult{}, fmt.Errorf("inspect failed")) + + s, err := NewComposeService(mockCli) + assert.NilError(t, err) + err = s.(*composeService).runHook(t.Context(), ctr, service, hook, nil) + assert.ErrorContains(t, err, "inspect failed") +} diff --git a/pkg/compose/pre_start.go b/pkg/compose/pre_start.go index 4d9c56830..4d70c54bb 100644 --- a/pkg/compose/pre_start.go +++ b/pkg/compose/pre_start.go @@ -33,6 +33,11 @@ import ( "github.com/docker/compose/v5/pkg/utils" ) +// preStartHookType is stored in the HookLabel on every pre_start hook container +// so orphan containers from a previous failed run can be identified and removed +// by a project+service+hook label filter. +const preStartHookType = "pre_start" + // lowestNumberedContainer returns the container with the lowest // com.docker.compose.container-number label, so pre_start always targets the // same replica regardless of the order the daemon returned them in. @@ -65,6 +70,14 @@ func (s *composeService) runPreStart(ctx context.Context, project *types.Project return fmt.Errorf("service %q pre_start[%d]: per_replica is not yet supported; remove per_replica or set it to false", service.Name, i) } } + // Remove any hook containers left behind by a previous failed run so they do + // not accumulate. Only one orphan can exist per service (failure gates the + // remaining hooks), but we clean the whole set in case the service definition + // changed between runs. Removal failures are non-fatal: they are logged so + // the operator can identify the stale container. + if err := s.removeOrphanPreStartContainers(ctx, project.Name, service.Name); err != nil { + logrus.Warnf("service %q: failed to remove stale pre_start hook containers: %v", service.Name, err) + } for i, hook := range service.PreStart { if err := s.runPreStartHook(ctx, project, service, ctr, i, hook, listener); err != nil { return err @@ -89,20 +102,18 @@ func (s *composeService) runPreStartHook( Condition: container.WaitConditionNextExit, }) - // Open the log stream before ContainerStart so AutoRemove cannot race us - // to a 404 on a fast-exiting hook. The dedicated logCtx lets us force the - // follow stream closed once the hook has exited, so a daemon that keeps - // the connection open cannot deadlock `<-logsDone`. + // Open the log stream before ContainerStart so a fast-exiting hook cannot + // race us to a 404. The dedicated logCtx lets us force the follow stream + // closed once the hook has exited, so a daemon that keeps the connection + // open cannot deadlock `<-logsDone`. logCtx, cancelLogs := context.WithCancel(ctx) defer cancelLogs() logsDone, getTail := s.streamPreStartLogs(logCtx, created.ID, service, index, listener) if _, err := s.apiClient().ContainerStart(ctx, created.ID, client.ContainerStartOptions{}); err != nil { - // AutoRemove only fires after a successful start, so the never-started - // container has to be dropped explicitly. A failed removal is logged - // at warn level — without that hint the orphan is only discoverable - // via the project/service labels. - if _, removeErr := s.apiClient().ContainerRemove(ctx, created.ID, client.ContainerRemoveOptions{Force: true}); removeErr != nil { + // AutoRemove is false, so we must remove the never-started container + // explicitly. A failed removal is logged so the orphan is visible. + if _, removeErr := s.apiClient().ContainerRemove(ctx, created.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil { logrus.Warnf("service %q pre_start[%d]: failed to remove orphan hook container %s: %v", service.Name, index, created.ID, removeErr) } // Drain waitRes so the client's wait goroutine exits without having to @@ -121,11 +132,34 @@ func (s *composeService) runPreStartHook( cancelLogs() <-logsDone if waitErr != nil { - if tail := getTail(); tail != "" { - return fmt.Errorf("%w: %s", waitErr, tail) + // Ctrl-C is a user cancellation, not a hook failure: remove the container + // and return the raw context error without decorating it with the tail or + // retaining the container for post-mortem inspection. + if ctx.Err() != nil { + if _, removeErr := s.apiClient().ContainerRemove(context.Background(), created.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil { + logrus.Warnf("service %q pre_start[%d]: failed to remove hook container %s after cancellation: %v", service.Name, index, created.ID, removeErr) + } + return waitErr } + // Genuine hook failure: retain the container so the operator can run + // `docker logs ` and `docker inspect ` to diagnose the failure. + // Include the short container ID in the error to make it actionable. + shortID := created.ID + if len(shortID) > 12 { + shortID = shortID[:12] + } + if tail := getTail(); tail != "" { + return fmt.Errorf("%w: %s (hook container %s retained for inspection)", waitErr, tail, shortID) + } + return fmt.Errorf("%w (hook container %s retained for inspection)", waitErr, shortID) } - return waitErr + // Success: remove the hook container, mirroring the old AutoRemove behaviour + // (including its anonymous volumes). A removal failure is logged but does not + // gate service start — the hook already succeeded. + if _, removeErr := s.apiClient().ContainerRemove(ctx, created.ID, client.ContainerRemoveOptions{RemoveVolumes: true}); removeErr != nil { + logrus.Warnf("service %q pre_start[%d]: failed to remove hook container %s: %v", service.Name, index, created.ID, removeErr) + } + return nil } func (s *composeService) createPreStartContainer( @@ -144,16 +178,21 @@ func (s *composeService) createPreStartContainer( WorkingDir: hook.WorkingDir, Env: append(ToMobyEnv(service.Environment), ToMobyEnv(hook.Environment)...), // Tag the ephemeral hook container with the project/service it belongs - // to so a failed AutoRemove leaves something that `compose down` (and - // other label-scoped tooling) can still find. + // to so it can be found by `compose down` and label-scoped tooling. + // HookLabel also distinguishes hook containers from the real service + // container (which shares ProjectLabel and ServiceLabel). Labels: map[string]string{ api.ProjectLabel: project.Name, api.ServiceLabel: service.Name, api.VersionLabel: api.ComposeVersion, + api.HookLabel: preStartHookType, }, } hostCfg := &container.HostConfig{ - AutoRemove: true, + // AutoRemove is intentionally false: a failed hook container is retained + // so the operator can inspect its logs. On success runPreStartHook + // removes the container explicitly (see the success path below). + AutoRemove: false, Privileged: hook.Privileged, VolumesFrom: []string{ctr.ID}, } @@ -180,10 +219,9 @@ func (s *composeService) createPreStartContainer( if versions.LessThan(apiVersion, apiVersion144) { if err := s.connectPreStartExtraNetworks(ctx, project, service, created.ID, networkMode); err != nil { - // Same reason as the ContainerStart-failure cleanup: AutoRemove never - // fires on a container that was created but not started. Surface - // any cleanup failure so the orphan is at least visible in logs. - if _, removeErr := s.apiClient().ContainerRemove(ctx, created.ID, client.ContainerRemoveOptions{Force: true}); removeErr != nil { + // AutoRemove is false; remove the container explicitly since it was + // never started. Log failures so the orphan is at least visible. + if _, removeErr := s.apiClient().ContainerRemove(ctx, created.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil { logrus.Warnf("service %q pre_start: failed to remove orphan hook container %s: %v", service.Name, created.ID, removeErr) } return client.ContainerCreateResult{}, err @@ -215,6 +253,32 @@ func (s *composeService) connectPreStartExtraNetworks(ctx context.Context, proje return nil } +// removeOrphanPreStartContainers finds and force-removes any hook containers +// left behind by a previous failed run of this service's pre_start hooks. +// Containers are identified by project + service + HookLabel=pre_start. +// Removal failures are logged at warn level and do not abort the run. +func (s *composeService) removeOrphanPreStartContainers(ctx context.Context, projectName, serviceName string) error { + f := projectFilter(projectName) + f.Add("label", serviceFilter(serviceName)) + f.Add("label", hookFilter(preStartHookType)) + res, err := s.apiClient().ContainerList(ctx, client.ContainerListOptions{ + All: true, + Filters: f, + }) + if err != nil { + return err + } + for _, ctr := range res.Items { + if _, removeErr := s.apiClient().ContainerRemove(ctx, ctr.ID, client.ContainerRemoveOptions{ + Force: true, + RemoveVolumes: true, + }); removeErr != nil { + logrus.Warnf("failed to remove stale pre_start hook container %s: %v", ctr.ID, removeErr) + } + } + return nil +} + func waitPreStart(ctx context.Context, serviceName string, index int, waitRes client.ContainerWaitResult) error { // ContainerWait can deliver on Result and Error at the same instant. Two // races have to be closed deterministically here: @@ -262,9 +326,6 @@ func preStartResultErr(serviceName string, index int, res container.WaitResponse return nil } -// streamPreStartLogs returns a channel that is closed once the hook log stream -// has been fully drained (or never opened). Callers must wait on it before -// returning so the goroutine cannot outlive the hook. // streamPreStartLogs opens the hook container's log stream in a background // goroutine, tees output into the listener (when non-nil) and into per-stream // tail buffers. It returns: diff --git a/pkg/compose/pre_start_test.go b/pkg/compose/pre_start_test.go index c4277bc61..97e6cbc62 100644 --- a/pkg/compose/pre_start_test.go +++ b/pkg/compose/pre_start_test.go @@ -18,8 +18,10 @@ package compose import ( "bytes" + "context" "fmt" "io" + "strings" "testing" "github.com/compose-spec/compose-go/v2/types" @@ -34,11 +36,11 @@ import ( ) func newPreStartTestService(t *testing.T) (*composeService, *mocks.MockAPIClient) { + return newPreStartTestServiceWithVersion(t, "1.44") +} + +func newPreStartTestServiceWithVersion(t *testing.T, apiVersion string) (*composeService, *mocks.MockAPIClient) { t.Helper() - // Register the goroutine-leak check first so it runs last (t.Cleanup is - // LIFO), after gomock's own cleanup has drained any internal goroutines. - // Any goroutine spawned by the code under test that outlives the test will - // fail this assertion. ignoreExisting := goleak.IgnoreCurrent() t.Cleanup(func() { goleak.VerifyNone(t, ignoreExisting) @@ -49,8 +51,8 @@ func newPreStartTestService(t *testing.T) (*composeService, *mocks.MockAPIClient cli := mocks.NewMockCli(mockCtrl) cli.EXPECT().Client().Return(apiClient).AnyTimes() apiClient.EXPECT().Ping(gomock.Any(), client.PingOptions{NegotiateAPIVersion: true}). - Return(client.PingResult{APIVersion: "1.44"}, nil).AnyTimes() - apiClient.EXPECT().ClientVersion().Return("1.44").AnyTimes() + Return(client.PingResult{APIVersion: apiVersion}, nil).AnyTimes() + apiClient.EXPECT().ClientVersion().Return(apiVersion).AnyTimes() tested, err := NewComposeService(cli) assert.NilError(t, err) return tested.(*composeService), apiClient @@ -67,6 +69,23 @@ func emptyLogs() client.ContainerLogsResult { return io.NopCloser(bytes.NewReader(nil)) } +// expectEmptyOrphanScan sets up the ContainerList expectation for the orphan +// pre_start cleanup that happens once at the start of every runPreStart call +// (after the per_replica validation loop). It returns the empty list. +func expectEmptyOrphanScan(apiClient *mocks.MockAPIClient) *gomock.Call { + return apiClient.EXPECT(). + ContainerList(gomock.Any(), gomock.Any()). + Return(client.ContainerListResult{}, nil) +} + +// expectSuccessRemove sets up the ContainerRemove call that runPreStartHook +// makes after a successful hook run (mirrors old AutoRemove behaviour). +func expectSuccessRemove(apiClient *mocks.MockAPIClient, hookID string) *gomock.Call { + return apiClient.EXPECT(). + ContainerRemove(gomock.Any(), hookID, client.ContainerRemoveOptions{RemoveVolumes: true}). + Return(client.ContainerRemoveResult{}, nil) +} + func TestPreStart_SuccessTwoHooksInOrder(t *testing.T) { tested, apiClient := newPreStartTestService(t) @@ -81,25 +100,30 @@ func TestPreStart_SuccessTwoHooksInOrder(t *testing.T) { } ctr := container.Summary{ID: "service-ctr-id"} - // Hook 1: create → wait (subscribe) → logs (subscribe) → start. + // Orphan cleanup runs once before the first hook. + scan := expectEmptyOrphanScan(apiClient) + + // Hook 1: create → wait (subscribe) → logs (subscribe) → start → remove. create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil) + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). Return(waitResultExit(0)).After(create1) logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil).After(wait1) start1 := apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil).After(logs1) + remove1 := expectSuccessRemove(apiClient, "hook-1").After(start1) - // Hook 2 is only created after hook 1 has been started (and waited on). + // Hook 2 is only created after hook 1 has been removed. create2 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-2"}, nil).After(start1) + Return(client.ContainerCreateResult{ID: "hook-2"}, nil).After(remove1) wait2 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-2", gomock.Any()). Return(waitResultExit(0)).After(create2) logs2 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-2", gomock.Any()). Return(emptyLogs(), nil).After(wait2) - apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-2", gomock.Any()). + start2 := apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-2", gomock.Any()). Return(client.ContainerStartResult{}, nil).After(logs2) + expectSuccessRemove(apiClient, "hook-2").After(start2) err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) assert.NilError(t, err) @@ -119,14 +143,16 @@ func TestPreStart_FirstHookFailsStopsExecution(t *testing.T) { } ctr := container.Summary{ID: "service-ctr-id"} + scan := expectEmptyOrphanScan(apiClient) create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil) + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). Return(waitResultExit(42)).After(create1) logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil).After(wait1) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil).After(logs1) + // Hook container is retained on failure — no ContainerRemove expected. err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) assert.ErrorContains(t, err, `service "web" pre_start[0]`) @@ -165,17 +191,19 @@ func TestPreStart_ImageFallsBackToBuiltImage(t *testing.T) { ctr := container.Summary{ID: "service-ctr-id"} var gotImage string + scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). DoAndReturn(func(_ any, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { gotImage = opts.Config.Image return client.ContainerCreateResult{ID: "hook-1"}, nil - }) + }).After(scan) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). Return(waitResultExit(0)) + expectSuccessRemove(apiClient, "hook-1") err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) assert.NilError(t, err) @@ -196,17 +224,19 @@ func TestPreStart_ExplicitHookImageUsed(t *testing.T) { ctr := container.Summary{ID: "service-ctr-id"} var gotImage string + scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). DoAndReturn(func(_ any, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { gotImage = opts.Config.Image return client.ContainerCreateResult{ID: "hook-1"}, nil - }) + }).After(scan) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). Return(waitResultExit(0)) + expectSuccessRemove(apiClient, "hook-1") err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) assert.NilError(t, err) @@ -228,23 +258,31 @@ func TestPreStart_VolumesFromServiceContainer(t *testing.T) { var gotVolumesFrom []string var gotAutoRemove bool + var gotLabels map[string]string + scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). DoAndReturn(func(_ any, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { gotVolumesFrom = opts.HostConfig.VolumesFrom gotAutoRemove = opts.HostConfig.AutoRemove + gotLabels = opts.Config.Labels return client.ContainerCreateResult{ID: "hook-1"}, nil - }) + }).After(scan) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). Return(waitResultExit(0)) + expectSuccessRemove(apiClient, "hook-1") err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) assert.NilError(t, err) assert.DeepEqual(t, gotVolumesFrom, []string{"service-ctr-id"}) - assert.Assert(t, gotAutoRemove) + // AutoRemove must be false: the hook container is retained on failure for + // post-mortem and explicitly removed on success by runPreStartHook. + assert.Assert(t, !gotAutoRemove, "AutoRemove must be false") + // HookLabel must be set so orphan cleanup can identify the container. + assert.Equal(t, gotLabels[api.HookLabel], preStartHookType) } func TestPreStart_ContainerCreateFailurePropagates(t *testing.T) { @@ -261,8 +299,9 @@ func TestPreStart_ContainerCreateFailurePropagates(t *testing.T) { } ctr := container.Summary{ID: "service-ctr-id"} + scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{}, fmt.Errorf("no such image: missing:latest")) + Return(client.ContainerCreateResult{}, fmt.Errorf("no such image: missing:latest")).After(scan) err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) assert.ErrorContains(t, err, "no such image") @@ -281,8 +320,9 @@ func TestPreStart_ContainerStartFailurePropagates(t *testing.T) { } ctr := container.Summary{ID: "service-ctr-id"} + scan := expectEmptyOrphanScan(apiClient) create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil) + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). Return(waitResultExit(0)).After(create1) logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). @@ -290,8 +330,9 @@ func TestPreStart_ContainerStartFailurePropagates(t *testing.T) { start1 := apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, fmt.Errorf("daemon: container start failed")).After(logs1) // AutoRemove never fires when start fails, so the hook must drop the ghost - // container explicitly. - apiClient.EXPECT().ContainerRemove(gomock.Any(), "hook-1", gomock.Any()). + // container explicitly. This is distinct from the success-path removal + // (RemoveVolumes:true) — the never-started container has no logs to preserve. + apiClient.EXPECT().ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). Return(client.ContainerRemoveResult{}, nil).After(start1) err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) @@ -324,14 +365,16 @@ func TestPreStart_WaitResultPreferredOverNilError(t *testing.T) { resultC <- container.WaitResponse{StatusCode: 0} errC <- nil + scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil) + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerWaitResult{Result: resultC, Error: errC}) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil) + expectSuccessRemove(apiClient, "hook-1") err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) assert.NilError(t, err) @@ -399,8 +442,9 @@ func TestPreStart_DetachedModeAttachesLogs(t *testing.T) { } ctr := container.Summary{ID: "service-ctr-id"} + scan := expectEmptyOrphanScan(apiClient) create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil) + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). Return(waitResultExit(0)).After(create1) // ContainerLogs MUST be called even with a nil listener. @@ -408,6 +452,7 @@ func TestPreStart_DetachedModeAttachesLogs(t *testing.T) { Return(emptyLogs(), nil).After(wait1) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil).After(logs1) + expectSuccessRemove(apiClient, "hook-1") err := tested.runPreStart(t.Context(), project, service, ctr, nil) assert.NilError(t, err) @@ -435,14 +480,16 @@ func TestPreStart_FailureIncludesTail(t *testing.T) { stdcopyFrame(2, "table 'sites' doesn't exist\n")..., // stderr error ) + scan := expectEmptyOrphanScan(apiClient) create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil) + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). Return(waitResultExit(1)).After(create1) logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(io.NopCloser(bytes.NewReader(logContent)), nil).After(wait1) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil).After(logs1) + // Hook container is retained on failure — no ContainerRemove expected. err := tested.runPreStart(t.Context(), project, service, ctr, nil) assert.Assert(t, err != nil) @@ -450,3 +497,685 @@ func TestPreStart_FailureIncludesTail(t *testing.T) { // Stderr content must be in the error (stderr bias). assert.ErrorContains(t, err, "doesn't exist") } + +// --------------------------------------------------------------------------- +// Feature tests: pre_start container retention on failure +// --------------------------------------------------------------------------- + +// TestPreStart_SuccessRemovesContainer verifies that a successful pre_start hook +// triggers an explicit ContainerRemove (with RemoveVolumes: true to mirror the +// old AutoRemove behaviour) and that AutoRemove is false at create time. +func TestPreStart_SuccessRemovesContainer(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + var gotAutoRemove bool + scan := expectEmptyOrphanScan(apiClient) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ any, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + gotAutoRemove = opts.HostConfig.AutoRemove + return client.ContainerCreateResult{ID: "hook-1"}, nil + }).After(scan) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(0)) + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(emptyLogs(), nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + // Success path: explicit remove with RemoveVolumes: true. + apiClient.EXPECT(). + ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{RemoveVolumes: true}). + Return(client.ContainerRemoveResult{}, nil) + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.NilError(t, err) + assert.Assert(t, !gotAutoRemove, "AutoRemove must be false; retention is managed explicitly") +} + +// TestPreStart_FailureRetainsContainer verifies that a pre_start hook that exits +// with a non-zero code does NOT trigger ContainerRemove — the container is kept +// for post-mortem inspection. +func TestPreStart_FailureRetainsContainer(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"migrate"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + logContent := stdcopyFrame(2, "migration failed: table missing\n") + + scan := expectEmptyOrphanScan(apiClient) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(1)) + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(io.NopCloser(bytes.NewReader(logContent)), nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + // No ContainerRemove expectation: gomock fails on unexpected calls. + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.ErrorContains(t, err, "pre_start[0]") + assert.ErrorContains(t, err, "table missing") + // Short container ID must appear in the error so the operator can run + // `docker logs hook-1` immediately. + assert.ErrorContains(t, err, "hook-1") +} + +// TestPreStart_CancellationRemovesContainer verifies that when the context is +// cancelled while a hook is running the container is removed (not retained) and +// the error is exactly ctx.Err() with no tail decoration. +func TestPreStart_CancellationRemovesContainer(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + ctx, cancel := context.WithCancel(t.Context()) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"long-running-op"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + scan := expectEmptyOrphanScan(apiClient) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-cancel-123"}, nil).After(scan) + // ContainerWait channel: cancel the context before delivering any result so + // waitPreStart returns ctx.Err(). + resultC := make(chan container.WaitResponse) + errC := make(chan error) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-cancel-123", gomock.Any()). + Return(client.ContainerWaitResult{Result: resultC, Error: errC}) + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-cancel-123", gomock.Any()). + Return(emptyLogs(), nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-cancel-123", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + // On cancellation the container must be removed, not retained. + apiClient.EXPECT(). + ContainerRemove(gomock.Any(), "hook-cancel-123", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). + Return(client.ContainerRemoveResult{}, nil) + + errCh := make(chan error, 1) + go func() { + errCh <- tested.runPreStart(ctx, project, service, ctr, nil) + }() + // Cancel after the hook container has started. + cancel() + + err := <-errCh + assert.ErrorIs(t, err, context.Canceled, "must return ctx.Err() on cancellation") + // The error must NOT include "retained" — it should be the raw context error. + assert.Assert(t, !strings.Contains(err.Error(), "retained"), "cancelled hook must not be retained; got: %s", err) +} + +// TestPreStart_RemovesOrphanBeforeRun verifies that a hook container left behind +// by a previous failed run is force-removed before the new container is created. +func TestPreStart_RemovesOrphanBeforeRun(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"migrate"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + // ContainerList returns the stale orphan from a previous run. + orphan := container.Summary{ID: "orphan-hook-123", State: "exited"} + scan := apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). + Return(client.ContainerListResult{Items: []container.Summary{orphan}}, nil) + // Orphan must be removed before the new hook container is created. + removeOrphan := apiClient.EXPECT(). + ContainerRemove(gomock.Any(), "orphan-hook-123", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). + Return(client.ContainerRemoveResult{}, nil).After(scan) + create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(removeOrphan) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(0)).After(create1) + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(emptyLogs(), nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + expectSuccessRemove(apiClient, "hook-1") + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.NilError(t, err) +} + +// TestPreStart_SuccessRemoveFailureIsNonFatal verifies that a ContainerRemove +// failure on the success path is logged but does not fail runPreStart. +func TestPreStart_SuccessRemoveFailureIsNonFatal(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + scan := expectEmptyOrphanScan(apiClient) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(0)) + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(emptyLogs(), nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + // Simulate a removal failure. + apiClient.EXPECT(). + ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{RemoveVolumes: true}). + Return(client.ContainerRemoveResult{}, fmt.Errorf("already removed")) + + // The hook succeeded; the service must start even if removal failed. + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.NilError(t, err) +} + +// --------------------------------------------------------------------------- +// Coverage-gap tests: previously uncovered branches +// --------------------------------------------------------------------------- + +// TestLowestNumberedContainer verifies that lowestNumberedContainer always +// picks the replica with the smallest ContainerNumberLabel value. +func TestLowestNumberedContainer(t *testing.T) { + makeCtr := func(id, num string) container.Summary { + return container.Summary{ID: id, Labels: map[string]string{ + api.ContainerNumberLabel: num, + }} + } + tests := []struct { + name string + containers Containers + wantID string + }{ + {"single", Containers{makeCtr("a", "1")}, "a"}, + {"ascending", Containers{makeCtr("a", "1"), makeCtr("b", "2"), makeCtr("c", "3")}, "a"}, + {"descending", Containers{makeCtr("a", "3"), makeCtr("b", "2"), makeCtr("c", "1")}, "c"}, + {"unordered", Containers{makeCtr("a", "5"), makeCtr("b", "1"), makeCtr("c", "3")}, "b"}, + {"missing_label", Containers{makeCtr("x", ""), makeCtr("y", "1")}, "x"}, // strconv.Atoi("") = 0 = lowest + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := lowestNumberedContainer(tc.containers) + assert.Equal(t, got.ID, tc.wantID) + }) + } +} + +// TestPreStart_WaitCancelled covers the ctx.Done() branch in waitPreStart: +// when the context is already canceled the function must return ctx.Err() +// without blocking on the result or error channels. +func TestPreStart_WaitCancelled(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() // already canceled + + resultC := make(chan container.WaitResponse) // never sends + errC := make(chan error) // never sends + waitRes := client.ContainerWaitResult{Result: resultC, Error: errC} + + err := waitPreStart(ctx, "web", 0, waitRes) + assert.ErrorIs(t, err, context.Canceled) +} + +// TestPreStart_ResultErrWaitError covers the preStartResultErr branch where +// the WaitResponse carries a daemon-reported error message. +func TestPreStart_ResultErrWaitError(t *testing.T) { + res := container.WaitResponse{ + Error: &container.WaitExitError{Message: "oom killed"}, + } + err := preStartResultErr("web", 0, res) + assert.ErrorContains(t, err, "wait error") + assert.ErrorContains(t, err, "oom killed") +} + +// TestPreStart_StreamLogsError_NilListener covers the ContainerLogs-failure +// branch in streamPreStartLogs when the caller passes no listener. +// The hook must still start and succeed; the missing log stream is non-fatal. +func TestPreStart_StreamLogsError_NilListener(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + scan := expectEmptyOrphanScan(apiClient) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(0)) + // ContainerLogs fails; nil listener → no warning event, done closed immediately. + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(nil, fmt.Errorf("logs: connection refused")) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + expectSuccessRemove(apiClient, "hook-1") + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.NilError(t, err) +} + +// TestPreStart_StreamLogsError_WithListener covers the ContainerLogs-failure +// branch in streamPreStartLogs when a listener IS present: a warning event +// must be emitted on the listener. +func TestPreStart_StreamLogsError_WithListener(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + scan := expectEmptyOrphanScan(apiClient) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(0)) + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(nil, fmt.Errorf("logs: daemon unavailable")) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + expectSuccessRemove(apiClient, "hook-1") + + var gotWarning string + listener := func(ev api.ContainerEvent) { + gotWarning = ev.Line + } + err := tested.runPreStart(t.Context(), project, service, ctr, listener) + assert.NilError(t, err) + assert.Assert(t, gotWarning != "", "listener must receive a warning when ContainerLogs fails") + assert.Assert(t, bytes.Contains([]byte(gotWarning), []byte("warning")), "expected 'warning' in: %q", gotWarning) +} + +// TestPreStart_OldAPIVersion covers the versions.LessThan(apiVersion, "1.44") +// branch in createPreStartContainer: on a pre-1.44 daemon the extra-networks +// path runs via connectPreStartExtraNetworks. With only one (primary) network +// no NetworkConnect call is issued and the hook succeeds normally. +func TestPreStart_OldAPIVersion(t *testing.T) { + tested, apiClient := newPreStartTestServiceWithVersion(t, "1.43") + + project := &types.Project{ + Name: "proj", + Networks: types.Networks{ + "default": {Name: "proj_default"}, + }, + } + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + Networks: map[string]*types.ServiceNetworkConfig{ + "default": nil, + }, + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + scan := expectEmptyOrphanScan(apiClient) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(0)) + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(emptyLogs(), nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + // Single network = primary only; no NetworkConnect expected. + expectSuccessRemove(apiClient, "hook-1") + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.NilError(t, err) +} + +// TestPreStart_ConnectExtraNetworksSuccess covers connectPreStartExtraNetworks +// when a secondary network is present: NetworkConnect must be called for it. +func TestPreStart_ConnectExtraNetworksSuccess(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{ + Name: "proj", + Networks: types.Networks{ + "default": {Name: "proj_default"}, + "extra": {Name: "proj_extra"}, + }, + } + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + Networks: map[string]*types.ServiceNetworkConfig{ + "default": nil, + "extra": nil, + }, + } + + apiClient.EXPECT(). + NetworkConnect(gomock.Any(), "proj_extra", gomock.Any()). + Return(client.NetworkConnectResult{}, nil) + + err := tested.connectPreStartExtraNetworks(t.Context(), project, service, "ctr-id", "proj_default") + assert.NilError(t, err) +} + +// TestPreStart_ConnectExtraNetworksFails covers the NetworkConnect error branch +// in connectPreStartExtraNetworks. +func TestPreStart_ConnectExtraNetworksFails(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{ + Name: "proj", + Networks: types.Networks{ + "default": {Name: "proj_default"}, + "extra": {Name: "proj_extra"}, + }, + } + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + Networks: map[string]*types.ServiceNetworkConfig{ + "default": nil, + "extra": nil, + }, + } + + apiClient.EXPECT(). + NetworkConnect(gomock.Any(), "proj_extra", gomock.Any()). + Return(client.NetworkConnectResult{}, fmt.Errorf("network not found")) + + err := tested.connectPreStartExtraNetworks(t.Context(), project, service, "ctr-id", "proj_default") + assert.ErrorContains(t, err, "network not found") +} + +// TestPreStart_ContainerStartFailureAndRemoveFails covers the Warnf path in +// runPreStartHook when ContainerStart fails AND the subsequent ContainerRemove +// also fails (the orphan is unremovable but the caller still gets the start error). +func TestPreStart_ContainerStartFailureAndRemoveFails(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + scan := expectEmptyOrphanScan(apiClient) + create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(0)).After(create1) + logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(emptyLogs(), nil) + start1 := apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, fmt.Errorf("start failed")).After(logs1) + // Both the start AND the cleanup removal fail. + apiClient.EXPECT().ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). + Return(client.ContainerRemoveResult{}, fmt.Errorf("removal failed")).After(start1) + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + // The original start error must be returned, not the removal error. + assert.ErrorContains(t, err, "start failed") +} + +// TestPreStart_OrphanScanFails verifies that when ContainerList fails during +// orphan cleanup the Warnf path in runPreStart is hit, but execution continues +// and the hook runs successfully. +func TestPreStart_OrphanScanFails(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + // ContainerList fails → Warnf in runPreStart; hook still proceeds. + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). + Return(client.ContainerListResult{}, fmt.Errorf("daemon unavailable")) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(0)) + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(emptyLogs(), nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + expectSuccessRemove(apiClient, "hook-1") + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.NilError(t, err) +} + +// TestPreStart_OrphanRemovalFails verifies the Warnf path in +// removeOrphanPreStartContainers when an individual stale container cannot be +// removed. The hook must still proceed normally. +func TestPreStart_OrphanRemovalFails(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + orphan := container.Summary{ID: "orphan-123"} + // ContainerList finds the orphan; its removal fails → Warnf then continue. + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). + Return(client.ContainerListResult{Items: []container.Summary{orphan}}, nil) + apiClient.EXPECT(). + ContainerRemove(gomock.Any(), "orphan-123", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). + Return(client.ContainerRemoveResult{}, fmt.Errorf("permission denied")) + // Hook still runs and succeeds. + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(0)) + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(emptyLogs(), nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + expectSuccessRemove(apiClient, "hook-1") + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.NilError(t, err) +} + +// TestPreStart_OldAPINetworkConnectFails covers the createPreStartContainer path +// for API < 1.44 with a secondary network: when NetworkConnect fails the function +// must clean up the created-but-never-started container and return the error. +func TestPreStart_OldAPINetworkConnectFails(t *testing.T) { + tested, apiClient := newPreStartTestServiceWithVersion(t, "1.43") + + project := &types.Project{ + Name: "proj", + Networks: types.Networks{ + "default": {Name: "proj_default"}, + "extra": {Name: "proj_extra"}, + }, + } + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + Networks: map[string]*types.ServiceNetworkConfig{ + "default": nil, + "extra": nil, + }, + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + scan := expectEmptyOrphanScan(apiClient) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + // NetworkConnect for the secondary network fails. + apiClient.EXPECT().NetworkConnect(gomock.Any(), "proj_extra", gomock.Any()). + Return(client.NetworkConnectResult{}, fmt.Errorf("network not found")) + // The never-started container is cleaned up (Force:true). + apiClient.EXPECT(). + ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). + Return(client.ContainerRemoveResult{}, nil) + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.ErrorContains(t, err, "network not found") +} + +// TestPreStart_OldAPINetworkConnectAndRemoveFails covers the Warnf path inside +// createPreStartContainer when both NetworkConnect and the cleanup ContainerRemove fail. +func TestPreStart_OldAPINetworkConnectAndRemoveFails(t *testing.T) { + tested, apiClient := newPreStartTestServiceWithVersion(t, "1.43") + + project := &types.Project{ + Name: "proj", + Networks: types.Networks{ + "default": {Name: "proj_default"}, + "extra": {Name: "proj_extra"}, + }, + } + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + Networks: map[string]*types.ServiceNetworkConfig{ + "default": nil, + "extra": nil, + }, + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + scan := expectEmptyOrphanScan(apiClient) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + apiClient.EXPECT().NetworkConnect(gomock.Any(), "proj_extra", gomock.Any()). + Return(client.NetworkConnectResult{}, fmt.Errorf("network not found")) + // Cleanup removal also fails → Warnf; original error is still returned. + apiClient.EXPECT(). + ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). + Return(client.ContainerRemoveResult{}, fmt.Errorf("removal also failed")) + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.ErrorContains(t, err, "network not found") +} + +// TestPreStart_RuntimeAPIVersionError covers the early-return in +// createPreStartContainer when RuntimeAPIVersion fails (Ping returns an error). +func TestPreStart_RuntimeAPIVersionError(t *testing.T) { + ignoreExisting := goleak.IgnoreCurrent() + t.Cleanup(func() { goleak.VerifyNone(t, ignoreExisting) }) + + mockCtrl := gomock.NewController(t) + t.Cleanup(mockCtrl.Finish) + apiClient := mocks.NewMockAPIClient(mockCtrl) + cli := mocks.NewMockCli(mockCtrl) + cli.EXPECT().Client().Return(apiClient).AnyTimes() + tested, err := NewComposeService(cli) + assert.NilError(t, err) + s := tested.(*composeService) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + // Orphan scan succeeds (ContainerList does not need Ping). + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). + Return(client.ContainerListResult{}, nil) + // createPreStartContainer calls RuntimeAPIVersion → Ping fails. + apiClient.EXPECT(). + Ping(gomock.Any(), client.PingOptions{NegotiateAPIVersion: true}). + Return(client.PingResult{}, fmt.Errorf("daemon unreachable")) + + err = s.runPreStart(t.Context(), project, service, ctr, nil) + assert.ErrorContains(t, err, "daemon unreachable") +} + +// TestPreStart_FailureStdoutOnlyTail covers the stdout-fallback branch in the +// getTail closure inside streamPreStartLogs: when stderr is empty but stdout has +// content, getTail must return the stdout content. +func TestPreStart_FailureStdoutOnlyTail(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "db", + Image: "postgres", + PreStart: []types.ServiceHook{ + {Image: "postgres", Command: types.ShellCommand{"migrate"}}, + }, + } + ctr := container.Summary{ID: "svc-ctr"} + + // stdout-only content: stderr frame absent so getTail falls back to stdout. + logContent := stdcopyFrame(1, "migration failed: schema mismatch\n") + + scan := expectEmptyOrphanScan(apiClient) + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(1)) + apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(io.NopCloser(bytes.NewReader(logContent)), nil) + apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil) + + err := tested.runPreStart(t.Context(), project, service, ctr, nil) + assert.ErrorContains(t, err, "pre_start[0]") + // Stdout fallback: no stderr → stdout content appears in the error. + assert.ErrorContains(t, err, "schema mismatch") +}