Repository navigation
Conversation
There was a problem hiding this comment.
🟡 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.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
There was a problem hiding this comment.
🟡 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.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
There was a problem hiding this comment.
🟡 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.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
f6dd7bd to
13004be
Compare
docker-agent
left a comment
There was a problem hiding this comment.
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 —
readDaemonTimereads/writesc.sincewithout 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 async.Once, would make the invariant explicit. Not a correctness bug today. -
[low] pkg/compose/monitor_test.go:592 —
fakeEventLog.subscribecallsparseEventsSince(which may invoket.Errorf) while holdingl.mu(confidence: not scored — low severity, not verified).t.Errorfdoes 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
left a comment
There was a problem hiding this comment.
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 —readDaemonTimeusestime.RFC3339Nanoto parse the daemon'sSystemTime, 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 wheret.Unix() < 0and nanoseconds are non-zero, but this is theoretical given daemon system times are always post-epoch
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>
13004be to
9ceae52
Compare
docker-agent
left a comment
There was a problem hiding this comment.


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.Startlists the project's containers, then opens the event stream. Both are round trips, and the stream carried nosince: an event emitted in between is delivered to nobody. For a container that starts and exits right then (echo hi), thedieis lost, the monitor keeps tracking a container that is gone, and--abort-on-container-exit/--exit-code-fromnever fire. The exposure grows as the lifecycle converges on one plan (#14081): with Create and Start run back to back, the firstContainerStartfollows the monitor's attachment much more closely than it used to.What the PR brings
The subscription is requested
sincethe 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 ofStart, 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 withsince, 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 throughlogs --follow, and the live subscription with its single warning for an engine that can't be asked, at the monitor and throughlogs --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 withContainerStart: 0 missed exits out of 60 runs, with and withoutsince; 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 --followshares 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
upswitchover 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