Skip to content

fix(pre_start): capture a hook's output by opening its log stream once started - #14353

Merged
glours merged 1 commit into
docker:mainfrom
ndeloof:fix-pre-start-logs
Oct 16, 2026
Merged

glours merged 1 commit into
docker:mainfrom
ndeloof:fix-pre-start-logs

Conversation

@ndeloof

@ndeloof ndeloof commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

What this PR does, in one sentence

Makes the output of a pre_start hook visible again: it is shown by a foreground up, and a failing hook reports what it printed.

Context

A pre_start hook runs in a short-lived runner container before the service starts. Its output is meant to be streamed to the user, and the tail of it is meant to be attached to the error when the hook fails. Neither ever happened: the log stream was opened on the runner while it was still in created state, before it was started, and the engine ends such a stream immediately, empty. A foreground up therefore printed nothing from the hook, and a failing hook produced an error with no output to diagnose it, even when running detached.

What the PR brings

The log stream is now opened once the hook has started. The runner is kept after it exits, so a hook that finishes very fast cannot make the stream hit a 404 — which is the race that opening it early was meant to avoid. Once the hook has exited, the stream is given a short, bounded delay (2 seconds at most) to deliver what the daemon had buffered, instead of being cut right away; a stream that stays open does not delay a finished hook beyond that.

Nothing else changes: ordering of the hook relative to the service, cleanup of the runner, and the error returned on failure are as before. An e2e scenario locks that a failing hook reports what it printed; it fails on main and passes with this change.

This is independent of the work in progress on the interactive up plan (#14305), which depends on it to stream hook output, so it is best merged first.

🤖 Generated with Claude Code

@ndeloof
ndeloof requested review from a team as code owners October 16, 2026 10:56

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

The fix correctly addresses the root cause: the log stream is now opened after ContainerStart (not before), preventing the empty-stream issue when the container is still in created state. The fast-exit race is correctly addressed by disabling AutoRemove and removing the container explicitly after the hook completes. The bounded 2-second drain is a sound design choice. Cleanup on ContainerStart failure is properly handled with an explicit ContainerRemove. The test suite additions are consistent with the new code flow.

Lower-confidence findings (not posted inline)

  • [low] pkg/compose/pre_start.go:241 — time.After timer not stopped on early exit via logsDone arm (confidence: unverified low; bounded 2s leak per hook run; idiomatic fix: time.NewTimer + defer t.Stop())
  • [low] pkg/compose/pre_start_test.go:750 — hookLogsDrainTimeout package-level var mutated in test without parallel-safety documentation (confidence: unverified low; safe today but fragile if t.Parallel() is added later)

@codecov

codecov Bot commented Oct 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@glours glours left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two should-fix, inline.

Comment thread pkg/compose/pre_start.go Outdated
Comment thread pkg/compose/pre_start_test.go
…e started

A pre_start hook's output never reached anyone. The follow log stream was
opened on the runner while it was still in "created" state, before
ContainerStart; the engine ends such a stream at once, empty. So a foreground
up never showed the hook's output, and the error of a failing hook carried no
tail, though both are built from that stream.

The stream is now opened once the hook is started. The runner is retained
after it exits (AutoRemove is off), so a fast-exiting hook cannot make the
stream hit a 404, which is what opening it first was meant to prevent. Once the
hook has exited, the stream gets a short, bounded delay to deliver what the
daemon buffered before it is cut, instead of being cut right away.

The unit tests that pinned the old call order now pin the new one, and a
scenario locks that a failing hook reports what it printed.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

The fix is correct: moving ContainerLogs to after ContainerStart resolves the root cause (follow streams on a container in created state end immediately, empty), and retaining the runner post-exit eliminates the 404 race. The two-second (actually 5-second default) bounded drain is the right pattern. One minor resource management nit on the new drain code.

Comment thread pkg/compose/pre_start.go
@ndeloof
ndeloof enabled auto-merge (rebase) October 16, 2026 12:43

@glours glours left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@glours
glours disabled auto-merge October 16, 2026 13:11
@glours
glours merged commit ea2284d into docker:main Oct 16, 2026
67 of 69 checks passed

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

@ndeloof
ndeloof deleted the fix-pre-start-logs branch October 16, 2026 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants