Skip to content

fix(monitor): subscribe to events since before the initial listing - #14304

Open
ndeloof wants to merge 1 commit into
docker:mainfrom
ndeloof:monitor-since
Open

ndeloof wants to merge 1 commit into
docker:mainfrom
ndeloof:monitor-since

Conversation

@ndeloof

@ndeloof ndeloof commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Follows #14296 (merged). Independent of #14300 and #14302.

What this PR does, in one sentence

The container monitor now asks the engine for events since the daemon's time read before it lists the project's containers, so a container that starts and exits between the listing and the event subscription can no longer go unseen.

Context

monitor.Start lists the project's containers, then opens the event stream. Both are round trips, and the stream carried no since: an event emitted in between is delivered to nobody. For a container that starts and exits right then (echo hi), the die is lost, the monitor keeps tracking a container that is gone, and --abort-on-container-exit / --exit-code-from never fire. The exposure grows as the lifecycle converges on one plan (#14081): with Create and Start run back to back, the first ContainerStart follows the monitor's attachment much more closely than it used to.

What the PR brings

The subscription is requested since the daemon's own time, read from its system info (one extra round trip) before the listing, so the engine replays anything that happened after it. Events are stamped by the daemon, so its clock is the one that has to be used: a client clock that differs from it, as against a remote daemon, would miss part of the window or replay events from before the monitor started. There is deliberately no falling back on the client's clock. A daemon that answers with a time that is empty or unparsable makes the monitor fail with an explicit error (reading the daemon time to subscribe to container events: …), and so the command, rather than following with no sound starting point; compose logs --follow, which shares the monitor, returns that error. A daemon that can't be asked at all (a read-only socket proxy that doesn't expose the info endpoint, for instance) does not fail the command, which worked without it: the monitor warns that events emitted while it attaches may be missed, and subscribes to live events as it did before this change. A caller that wants to know before it starts any container can read the time ahead of Start, which then neither asks nor warns again. The replay is idempotent by construction: the listing notifies nothing, so each replayed event is reported once, and the tracked set only ever gets an ID added again or removed again. Unit tests pin this against a fake event log that, like the engine, only hands unsubscribed events to a subscription asking for them with since, and stamps events with a clock of its own (set hours apart from the test's to prove the daemon's time is the one used, as reported and whatever its offset). The failure is covered for an empty or unparsable time, at the monitor and through logs --follow, and the live subscription with its single warning for an engine that can't be asked, at the monitor and through logs --follow. The replay cases: a die emitted between listing and subscription, and replays of a die for a container listed as exited, a start for one listed as running, the whole life of a container the listing doesn't know, and a restart policy already in action.

I could not reproduce the race against a real engine (Docker Desktop 29.7.2, busybox echo hi, monitor launched concurrently with ContainerStart: 0 missed exits out of 60 runs, with and without since; the exit lands about 60 ms after the start call, far beyond the listing plus subscription round trips). The change is therefore a cheap guard for a narrow window rather than the fix of an observed failure; it is meant to hold when a later step of the series removes the slack that hides it today.

Caveats, deliberately not worked around here: compose logs --follow shares the monitor and gets the same replay (and the extra system-info call). An empty initial listing still ends the monitor before it reads any event, as before.

Why this is the right next brick

It is the last prerequisite the interactive up switchover needs from the monitor, and it is inert for anything that does not race. It touches nothing else.

Part of #14081.

🤖 Generated with Claude Code

@ndeloof
ndeloof requested review from docker-agent and a balanced review from Copilot October 8, 2026 10:58

Copilot AI 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.

🟡 Changes recommended

The timestamp parser tests pass errors directly to assert.Check, causing panic instead of validation.

1 open finding
What changed in this PR

Guards monitor startup against missed container events. The stacked diff also migrates lifecycle operations to the phased plan executor.

Changes:

  • Subscribes to events from before the initial container listing.
  • Unifies create/start planning for detached up, scale, watch, and start.
  • Expands reconciler, executor, timeout, provider, and regression coverage.
File Description
pkg/​compose/​watch.go Uses unified create/start planning for rebuilds.
pkg/​compose/​up.go Adds plan-based detached up execution.
pkg/​compose/​start.go Migrates start to start-only plans.
pkg/​compose/​start_test.go Removes obsolete imperative-start tests.
pkg/​compose/​service_containers.go Refines dependency timeout handling and removes legacy start logic.
pkg/​compose/​service_containers_test.go Tests optional dependency timeout behavior.
pkg/​compose/​scale.go Uses unified create/start execution.
pkg/​compose/​restart_test.go Updates shared container fixtures.
pkg/​compose/​reconcile.go Extends start planning and provider handling.
pkg/​compose/​reconcile_start_test.go Adds start-plan regression coverage.
pkg/​compose/​observed_state.go Scopes observed resources by lifecycle phase.
pkg/​compose/​observed_state_test.go Tests start-only observation behavior.
pkg/​compose/​monitor.go Replays events since before initial listing.
pkg/​compose/​monitor_test.go Adds monitor replay tests and fake event log.
pkg/​compose/​executor.go Adds phased execution and bounded waits.
pkg/​compose/​executor_start_test.go Tests phase barriers, timeouts, and recreation.
pkg/​compose/​executor_ops.go Updates wait and rename execution state.
pkg/​compose/​down_test.go Updates shared container fixture usage.
pkg/​compose/​create.go Extracts plan preparation and combined execution.
pkg/​compose/​containers.go Removes an obsolete predicate.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread pkg/compose/monitor_test.go
@codecov

codecov Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@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

Copilot AI 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.

🟡 Changes recommended

Monitor startup remains unordered with container startup, so the targeted missed-exit race can still occur.

2 open findings

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread pkg/compose/monitor.go Outdated

@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 requested review from docker-agent and a balanced review from Copilot October 8, 2026 12:14

Copilot AI 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.

🟡 Changes recommended

Foreground up can still miss fast exits, and narrowed watch rebuilds can misclassify unrelated containers as orphans.

4 open findings

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread pkg/compose/monitor.go
Comment thread pkg/compose/watch.go

@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

Comment thread pkg/compose/monitor.go

@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 race-fix logic is correct: reading the daemon's clock via Info before the container listing, using it as the since parameter in the event subscription, and failing fast with an explicit error when the clock is unavailable are all sound. The since format (%d.%09d with Unix()/Nanosecond()) round-trips correctly through parseEventsSince. The idempotency of replayed events (listing notifies nothing, so each replayed event is reported once) is valid. Error-handling and context-cancellation semantics are correct.

Lower-confidence findings (not posted inline)

  • [low] pkg/compose/monitor.go:213 — readDaemonTime reads/writes c.since without synchronization (confidence: not scored — low severity, not verified). The documented call contract is sequential, so there is no real race under current usage; a comment asserting the sequential contract, or a sync.Once, would make the invariant explicit. Not a correctness bug today.

  • [low] pkg/compose/monitor_test.go:592 — fakeEventLog.subscribe calls parseEventsSince (which may invoke t.Errorf) while holding l.mu (confidence: not scored — low severity, not verified). t.Errorf does not re-enter the event log, so deadlock is not possible; but holding a mutex during test-framework calls is fragile. In practice this is harmless.

@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

No bugs were introduced by the changes in this PR. The since-based event replay logic in readDaemonTime is sound: the daemon time is fetched before the container listing, formatted correctly as <seconds>.<nanoseconds>, and used to filter the event stream so events emitted during the listing–subscription gap are replayed.

Lower-confidence findings (not posted inline)

  • [medium] pkg/compose/monitor.go:229 — readDaemonTime uses time.RFC3339Nano to parse the daemon's SystemTime, with no format fallback (not verified; low-risk: moby consistently returns RFC3339Nano, and failing fast on unexpected formats is the declared design intent)
  • [low] pkg/compose/monitor.go:233 — fmt.Sprintf(\"%d.%09d\", t.Unix(), t.Nanosecond()) is incorrect for pre-epoch times where t.Unix() < 0 and nanoseconds are non-zero, but this is theoretical given daemon system times are always post-epoch

Comment thread pkg/compose/monitor.go
monitor.Start lists the project's containers, then opens the event
stream. Both are round trips: a container that starts and terminates
in between has its die emitted to nobody, the monitor tracks it
forever, and --abort-on-container-exit / --exit-code-from never fire.
The window is narrow, but a plan running Create and Start back to back
makes the first start follow the monitor's attachment closely.

Ask the engine for events since a point taken before the listing, and
take that point from the daemon (its system time, one extra round trip),
not from the client: events are stamped by the daemon, so a client clock
that differs from it, as against a remote daemon, would either miss part
of the window or replay events from before the monitor was started.
There is no falling back on the client's clock. A daemon that answers
with a time that is empty or can't be parsed makes Start fail with an
explicit error, and so the command. A daemon that can't be asked at all
(a read-only socket proxy that doesn't expose the info endpoint) doesn't:
`logs --follow` never needed that endpoint, and failing it would be a
regression. The monitor then warns that the window can't be closed and
subscribes to live events, as it did before the replay. A caller that
wants to know before it starts any container can read the time ahead of
Start (readDaemonTime), which then doesn't ask, or warn, again.

Replayed events already reflected in the listing are harmless: the
listing notifies nothing, so each is reported once, and the tracked set
only gets an ID added or removed again. Unit tests drive the monitor
against a fake event log that, like the engine, only delivers
unsubscribed events to a subscription asking for them with since, and
stamps events with a clock of its own. They pin that replay for a die, a
start, a destroy and a restart policy, the daemon's time being used
whatever its offset from the client's, the failure on an unusable time
(also through `logs --follow`), and the live subscription, with a single
warning, when the engine can't be asked.

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

This branch has not been deployed

No deployments
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.

4 participants