feat(hooks): capture and surface lifecycle hook output by glours · Pull Request #14091 · docker/compose

glours · 2026-08-19T08:51:24Z

What this PR delivers

Builds on #14088 (merged), which added the hook output tail and error reporting foundation.

Context cancellation fix: runHook previously blocked indefinitely on the exec-attach reader. The copy goroutine held the connection open, so the first Ctrl+C had no effect — only the OS kill on the second signal interrupted the process, skipping cleanup. Fix: run the copy in a background goroutine and select on ctx.Done() vs completion; on cancellation, close the connection to unblock the goroutine and return ctx.Err() directly (no ExecInspect on the cancel path). Restores the behaviour of the old runWaitExec.

outputTail robustness:

Write() rewritten to block-scan with bytes.IndexByte (O(n) vs O(n²) byte-by-byte loop); partial line byte-capped via appendToPartial.
strings.ToValidUTF8 applied at both truncation sites so a cut multi-byte rune does not corrupt the error message.
pushLine handles mid-line \r (progress bars from curl/apt/pip): strip trailing \r, then keep the segment after the last bare \r.

pre_start parity (from #14088 base): log stream always opened even without a listener so the tail is populated in detached mode; stderr-biased error reporting.

What was intentionally excluded

The client-side NDJSON hook-log store and compose logs --hooks flag were prototyped in an earlier revision of this branch but have been dropped. Persisting hook output belongs in the engine (see moby/moby#32047, moby/moby#9527), not as a client-side workaround. That work is deferred until an engine-native solution exists.

Testing

TestRunHook_ContextCancellation: net.Pipe that never writes; asserts context.Canceled within 5 s of cancel().
TestRunHook_FailureIncludesOutput/with_listener: asserts listener receives all output lines on failure.
TestRunHook_StderrBias: uses a capturing listener; asserts both stream lines are delivered.
TestOutputTail: CRLF stripping, progress-bar \r, UTF-8-safe byte truncation.

docker-agent

Assessment: 🟡 NEEDS ATTENTION

Two real bugs introduced by this PR — one memory-management issue and one output-correctness issue — plus a usability gap in --hooks --follow. See inline comments for details.

ndeloof · 2026-08-19T09:33:56Z

Some notes:

the first two commits are the right fix, and they're @dennislapchenko's work from #14088: I'd rather we merge his PR as-is and rebase the rest on top, so authorship lands where it belongs.

On the third commit, I'm going to push back on the NDJSON store.

First, scoping the actual problem: in attached mode hook output already streams to the console (HookEventLog), and with the tail-in-error fix a failing hook is now diagnosable in every mode — detached, restart, run, and library usage through Start. What remains uncovered is only post-hoc inspection of hook output, and it's worth being precise about why that gap exists: post_start/pre_stop are execs, and the engine's logging driver never sees exec output. There is genuinely nothing for compose logs to read today.

But a client-side store is the wrong place to fill that gap. Compose's contract is that project state lives in the engine: any client, from any machine, can operate a project it didn't create. With logs under the client's config dir, logs --hooks shows nothing from another workstation or CI, a remote docker context writes them on the wrong machine, cleanup depends on down running (a crash leaves orphans), and we now own a second log pipeline — rotation, concurrency, platform paths — duplicating what the engine's logging driver is for.

I'd rather we go engine-native:

pre_start hooks are containers — their output does go through the logging driver, and we only lose it because we run them with AutoRemove: true. Keeping the container around on failure (removed by the next up/down, like any other project container, and/or after successful completion) makes compose logs and docker logs work with zero new machinery. That covers the case where post-hoc inspection matters most.
For post_start/pre_stop, the structural fix is upstream: an exec-create option to tee output into the container's logging driver. That would give us logs --hooks for free, stateless, for every client. I'll open that discussion on moby; until then the bounded tail on failure covers the diagnosis need, which is what fix(hooks): include hook output in the error when a hook fails #14088 set out to solve.

So: merge #14088, add failed-pre_start retention here, drop the store. WDYT?

ndeloof · 2026-08-19T09:35:49Z

see moby/moby#32047 / moby/moby#9527

codecov · 2026-08-19T14:10:33Z

Codecov Report

❌ Patch coverage is 90.38462% with 15 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/compose/pre_start.go	84.28%	9 Missing and 2 partials ⚠️
pkg/compose/down.go	85.71%	2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

ndeloof

Scope is exactly right: pure client-side, bounded tail in the error + retained container for post-mortem, with persistence deferred to the engine — and the test coverage is outstanding (31 new tests including every cleanup-failure path). Nice work on the Ctrl-C fix and the \r/UTF-8 handling too.

Inline comments below. I'd consider the first three worth fixing in this PR (anonymous-volume leak in the orphan sweep, retention on user cancellation, stale AutoRemove comments); the rest is fine as follow-ups:

down <svc> cleans only the named services' hook containers while the traversal removes the service and its dependents — the dependents' retained hooks survive a targeted down.
a ContainerList failure aborts the teardown mid-way, while the doc comment promises removal failures don't; warn-and-continue would match.
the two near-identical sweep helpers (removeOrphanPreStartContainers / removePreStartHookContainers) could merge into one, which would structurally prevent the RemoveVolumes divergence.
ordering: retained hooks are removed after the service containers, yet they reference the service's volumes via VolumesFrom — worth checking down -v still reclaims the service's anonymous volumes in that order (sweeping hooks first would be safer).
"intentionally excluded" could also point at the concrete engine-native path in flight: moby/moby#53406 / moby/moby#53407 (opt-in exec output capture through the logging driver) — that's the API a future logs --hooks would consume.

ndeloof · 2026-08-20T13:28:43Z

+		return err
+	}
+	for _, ctr := range res.Items {
+		if _, removeErr := s.apiClient().ContainerRemove(ctx, ctr.ID, client.ContainerRemoveOptions{Force: true}); removeErr != nil {


This sweep is missing RemoveVolumes: true: the success path and the down sweep both pass it, so a hook whose image declares VOLUMEs leaks its anonymous volumes when this path does the cleanup. (Merging this helper with removePreStartHookContainers in down.go would prevent the divergence structurally.)

ndeloof · 2026-08-20T13:28:43Z

 	cancelLogs()
 	<-logsDone
-	return waitErr
+	if waitErr != nil {


Two things on the retention branch:

waitPreStart returns ctx.Err() on cancellation, so a plain Ctrl-C lands here too: the container is retained and the tail gets wrapped around context.Canceled. A user cancellation isn't a post-mortem — at minimum don't decorate context.Canceled with the tail, and arguably don't retain in that case.

The docs say the operator can run docker logs <id>, but neither the ID nor a name appears in the error (hook containers are created unnamed). Including the short ID here — or better, naming the container (<project>-<service>-pre_start-<i>) — would make the retention actually actionable.

ndeloof · 2026-08-20T13:28:43Z

+	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


This comment (and its sibling in the connectPreStartExtraNetworks failure path) still justifies the explicit removal with "AutoRemove only fires after a successful start" — but AutoRemove is now false unconditionally. The behavior is right, the rationale is stale.

ndeloof · 2026-08-20T13:28:43Z

+		f.Add("label", hookFilter(preStartHookType))
+		filters = []client.Filters{f}
+	} else {
+		for _, service := range services {


Follow-up: scoping by options.Services diverges from what the teardown actually removes — WithRootNodesAndDown takes down the named services plus their dependents, so a targeted down web leaves the dependents' retained hook containers behind.

ndeloof · 2026-08-20T13:28:43Z

+			Filters: f,
+		})
+		if err != nil {
+			return err


Follow-up: this return err aborts the teardown (networks/volumes/images not yet processed) on a listing failure, while the doc comment above promises removal failures don't abort. Warn-and-continue would be consistent.

ndeloof · 2026-08-20T13:28:43Z

 		}
 	}

+	if err := s.removePreStartHookContainers(ctx, projectName, options.Services); err != nil {


Follow-up / to verify: the retained hook containers reference the service containers' volumes via VolumesFrom, and they are removed after the service containers — so the service's rm -v may fail to reclaim anonymous volumes still referenced by a retained hook (they'd only go away with the hook's own RemoveVolumes afterwards). Sweeping hooks before the service containers would be the safer order.

ndeloof · 2026-08-20T13:28:43Z

-	if partial := strings.TrimSpace(t.partial.String()); partial != "" {
-		lines = append(append([]string{}, lines...), partial)
+	if partial := strings.TrimSpace(strings.ToValidUTF8(t.partial.String(), "")); partial != "" {
+		lines = append(lines, partial)


Nit: this dropped the defensive copy (append(append([]string{}, lines...), partial)) — append(lines, partial) can write into t.lines' backing array. Harmless with the current single-goroutine write / read-after-done usage, but fragile for the one line it saves.

ndeloof

Thanks — the three must-fix items from my previous review are all properly addressed: RemoveVolumes: true on every sweep/cleanup path (with the mocks updated to pin it), the cancellation branch no longer retains nor decorates context.Canceled, the retained-container short ID makes the error actionable, and the stale AutoRemove comments now state the actual behavior. The new TestPreStart_CancellationRemovesContainer and the short-ID assertion are exactly the locks I hoped for, and the start_test.go adaptation is correct and well-explained.

Two unrelated regressions slipped into the last commit (rebase leftovers, I assume) that must be dropped before merge — see inline: the validate-mocks CI check removal and the codeql-action downgrade.

One real bug remains in the new cancellation path (inline): the cleanup ContainerRemove runs on the already-cancelled context, so in production it fails immediately and the container is left behind anyway (the orphan sweep saves the day on the next run, but the code's promise is dead on arrival). context.WithoutCancel(ctx) fixes it.

The agreed follow-ups (down scoping vs dependents, list-failure aborting teardown, merging the two sweep helpers, hook-before-service removal order, moby#53406/#53407 pointer) remain open — fine by me to land them separately.

ndeloof · 2026-08-20T14:53:49Z

          - lint
          - validate-go-mod
          - validate-headers
          - validate-docs


This commit removes validate-mocks from the validate matrix — the check just introduced on main (#14102), and nothing in this PR justifies it (the commit message doesn't mention it either). Looks like a rebase leftover; please restore it. If the check is red on this branch, that's a mocks drift to fix with make mocks, not a check to drop.

ndeloof · 2026-08-20T14:53:49Z

      # Upload the results to GitHub's code scanning dashboard.
      - name: "Upload to code-scanning"
-        uses: github/codeql-action/upload-sarif@ff2f1c621b7f889edc0d3c761ac2e6a3f8cdb0dd # v4.37.7
+        uses: github/codeql-action/upload-sarif@5595ccaf912efad79be6eef63a5619ff05969be3 # v4.37.6


Same commit downgrades codeql-action v4.37.7 → v4.37.6, reverting the dependabot bump merged on main. Rebase artifact — please drop this hunk.

ndeloof · 2026-08-20T14:53:49Z

+		// 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 {


This ContainerRemove runs on the already-cancelled ctx, so in production it fails immediately with context.Canceled and lands in the Warnf below — the container is retained despite this branch's intent (the mock in TestPreStart_CancellationRemovesContainer doesn't honor ctx, which is why the test passes). Use context.WithoutCancel(ctx) for the cleanup call — same pattern up uses for its teardown.

On top of the output-tail infrastructure from the previous commit: Stderr bias: split the combined output tail into separate stdout and stderr buffers. stdcopy.StdCopy(wOut, wErr, reader) now routes each stream to its own outputTail. The error message prefers stderr (where hooks write their actual error) and falls back to stdout when stderr is empty: db hook exited with status 1: Table 'service.sites' doesn't exist A new hookExitError helper encapsulates this preference. pre_start parity: streamPreStartLogs previously short-circuited and returned immediately when listener was nil (detached mode / compose up -d), leaving the tail buffers empty and failure errors without context. The log stream is now always opened. The demultiplexed streams fill separate tailOut / tailErr buffers. On non-zero exit the stderr-biased tail is appended to the error (same behaviour as exec hooks). Tests: - TestRunHook_StderrBias: stdout noise stays out of the error when stderr has content. - TestPreStart_DetachedModeAttachesLogs: ContainerLogs is called even with a nil listener. - TestPreStart_FailureIncludesTail: a failing pre_start hook carries its stderr output in the returned error. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>

ndeloof

The rebase regressions are gone (validate-mocks back in the matrix, codeql-action back on v4.37.7) and the outputTail.String() defensive copy is restored — thanks, and the squashed history reads well.

One item from my previous review survived the rewrite though (the inline thread got marked outdated by the force-push, so re-anchoring it): the cancellation-path cleanup still runs ContainerRemove on the already-cancelled context, so in production it can't succeed. One-line fix, and with it I have nothing else blocking — LGTM once addressed.

ndeloof · 2026-08-20T16:19:22Z

+		// 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(ctx, created.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil {


Still passing the already-cancelled ctx here: the moby client aborts the request immediately with context.Canceled, so this removal always lands in the Warnf and the container is left behind despite the branch's intent (the gomock in TestPreStart_CancellationRemovesContainer doesn't honor ctx, which is why the test stays green). context.WithoutCancel(ctx) on this call fixes it.

A failing pre_start hook container is now retained (AutoRemove: false) so operators can run 'docker logs <id>' 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 <glours@users.noreply.github.com>

A pre_start hook container retained after failure (introduced in the previous commit) is created without ConfigHashLabel so getContainers() and every downstream compose operation (down, ps, stop, rm) is blind to it. Without explicit cleanup it would survive compose down. This commit adds removePreStartHookContainers(), called from down() after regular container teardown and before network removal (so hook containers are gone before their networks are removed). The cleanup is project-wide on a full compose down; scoped to the requested services when compose down <svc...> is used. Individual ContainerRemove errors are logged at warn level and do not abort the teardown. Changes: - pkg/compose/down.go: removePreStartHookContainers helper + call in down() - pkg/compose/filters.go: add //nolint:unparam to hookFilter (param is architecturally correct; today only preStartHookType is in use) - pkg/compose/down_test.go: update all existing down tests to expect the new ContainerList call; add TestDownRemovesRetainedPreStartHookContainers and TestDownHookContainerRemovalFailureIsNonFatal Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>

- compose_down.md: note that retained pre_start hook containers (those kept after a hook failure for post-mortem) are automatically removed by compose down, with the label filter to list them beforehand. - compose_up.md: note the retention behavior on pre_start hook failure, how to inspect the retained container, and that it is cleaned up on the next compose up or compose down. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>

ndeloof

The cancellation-path removal now survives the cancelled context — that was my last blocking point. LGTM.

One optional polish inline (fine as-is or in a follow-up): context.WithoutCancel(ctx) instead of context.Background() keeps the context values (tracing spans notably) while still dropping cancellation, and matches the idiom runInteractiveUp already uses for its teardown.

ndeloof · 2026-08-20T16:56:22Z

+		// 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 {


Non-blocking:

Suggested change

if _, removeErr := s.apiClient().ContainerRemove(context.Background(), created.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil {

if _, removeErr := s.apiClient().ContainerRemove(context.WithoutCancel(ctx), created.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil {

WithoutCancel keeps the tracing/values of the parent context while dropping its cancellation — same effect as Background() for the API call, better trace correlation, and consistent with runInteractiveUp's teardown.

glours requested review from a team as code owners August 19, 2026 08:51

glours requested a review from ndeloof August 19, 2026 08:51

glours self-assigned this Aug 19, 2026

docker-agent reviewed Aug 19, 2026

View reviewed changes

Comment thread pkg/compose/hook_logs.go Outdated

Comment thread pkg/compose/logs.go Outdated

Comment thread pkg/compose/hook.go Outdated

glours force-pushed the hooks-logging branch from 928dc37 to d67feda Compare August 19, 2026 13:39

kimdre mentioned this pull request Aug 20, 2026

fix(deploy): retry deployments that failed after containers were created kimdre/doco-cd#1711

Merged

ndeloof reviewed Aug 20, 2026

View reviewed changes

glours force-pushed the hooks-logging branch from 2ea99f8 to 8e04e4c Compare August 20, 2026 13:54

ndeloof reviewed Aug 20, 2026

View reviewed changes

glours force-pushed the hooks-logging branch from 8e04e4c to 9762d95 Compare August 20, 2026 15:47

ndeloof reviewed Aug 20, 2026

View reviewed changes

glours added 3 commits August 20, 2026 18:23

glours force-pushed the hooks-logging branch from 9762d95 to 9eeb69a Compare August 20, 2026 16:23

ndeloof approved these changes Aug 20, 2026

View reviewed changes

ndeloof merged commit f729794 into docker:main Aug 20, 2026
51 checks passed

ndeloof deleted the hooks-logging branch August 20, 2026 16:57

	if _, removeErr := s.apiClient().ContainerRemove(context.Background(), created.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil {
	if _, removeErr := s.apiClient().ContainerRemove(context.WithoutCancel(ctx), created.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil {

Uh oh!

Conversation

glours commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR delivers

What was intentionally excluded

Testing

Uh oh!

docker-agent left a comment

Choose a reason for hiding this comment

Assessment: 🟡 NEEDS ATTENTION

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ndeloof commented Aug 19, 2026

Uh oh!

ndeloof commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ndeloof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndeloof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndeloof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndeloof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

glours commented Aug 19, 2026 •

edited

Loading

ndeloof commented Aug 19, 2026 •

edited

Loading

codecov Bot commented Aug 19, 2026 •

edited

Loading