fix: detect externally stopped and removed containers in up monitor - #13990
Open
glours wants to merge 1 commit into
Open
fix: detect externally stopped and removed containers in up monitor#13990glours wants to merge 1 commit into
glours wants to merge 1 commit into
Conversation
Since 2.39.3 the monitor relies solely on die events to stop tracking containers, but a container stopped while in restart backoff emits only stop and destroy events, so an attached `up` hangs forever after an external `stop`/`down`. Handle destroy as terminal, and on stop inspect the container to distinguish a definitive stop from the transient one emitted during a ContainerRestart, which keeps watch sync+restart (docker#13161) working. Fixes docker#13985 Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Two medium-confidence findings introduced by this PR's new ActionStop handling — both relate to how the monitor responds when ContainerInspect does or doesn't find the container.
Lower-confidence findings (not posted inline)
- [low] pkg/compose/monitor.go:122 —
ActionStarthandler does not clear therestartingset (confidence: weak 10/100) — not a real bug given the current code flow; noted for future maintainability
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The fix is well-structured and correct. The new ActionStop handler correctly distinguishes a definitive stop from a transient one by inspecting the container state, and ActionDestroy unconditionally terminates tracking without requiring an inspect (appropriate since there is nothing left to inspect). The isRestarting helper cleanly handles the NotFound case (container already removed) by returning false, nil, which is the right terminal disposition. The test suite covers the four key scenarios: destroy-only, die+stop canceling a restart backoff (#13985 repro), die+stop+start across a ContainerRestart cycle (#13161 guard), and stop racing a removal (inspect returns NotFound).
This was referenced Aug 10, 2026
ndeloof
approved these changes
Aug 11, 2026
Contributor
|
Considering the impact event management had in the past 😓 I'd like this new corner case is covered by an e2e test, based on reproduction scenario documented by #13985 |
ndeloof
added a commit
that referenced
this pull request
Aug 11, 2026
Regression test for #13985: a container stopped while in restart backoff emits no die event (only stop then destroy), and since v2.39.3 the attached up waits forever for a die event that never comes. The test fails on main and passes with the monitor fix from #13990. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
added a commit
that referenced
this pull request
Aug 11, 2026
Regression tests for #13985: a container stopped while in restart backoff emits no die event, and since v2.39.3 the attached up waits forever for a die event that never comes. Two explicit scenarios: - external stop alone: containers are stopped but not removed (stop event, no destroy). Up to v2.39.2 the attached up exited a few seconds after the stop. - external stop+down: the container in backoff emits stop then destroy. Both tests fail on main and pass with the monitor fix from #13990. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
This was referenced Aug 11, 2026
ndeloof
added a commit
that referenced
this pull request
Aug 11, 2026
Map, per container lifecycle scenario, the exact event sequences the engine emits and the monitor may receive: natural exit, policy-driven restart (die/start only, no restart action, silence during backoff), external stop of a running vs restarting container, ContainerRestart, docker kill, OOM kill, rm -f and rm. Mapped from moby's daemon sources (LogContainerEvent call sites and handleContainerExit/containerStop/containerRestart/kill paths) and verified against a live engine v29.6.1 with 'docker events'. Notably: the relative order of stop and die on a container stop is not guaranteed, and no event at all is emitted while a container sits in restart backoff. Related to #13985 / #13990. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
added a commit
that referenced
this pull request
Aug 13, 2026
Regression tests for #13985: a container stopped while in restart backoff emits no die event, and since v2.39.3 the attached up waits forever for a die event that never comes. Two explicit scenarios: - external stop alone: containers are stopped but not removed (stop event, no destroy). Up to v2.39.2 the attached up exited a few seconds after the stop. - external stop+down: the container in backoff emits stop then destroy. Both tests fail on main and pass with the monitor fix from #13990. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
added a commit
that referenced
this pull request
Aug 13, 2026
Map, per container lifecycle scenario, the exact event sequences the engine emits and the monitor may receive: natural exit, policy-driven restart (die/start only, no restart action, silence during backoff), external stop of a running vs restarting container, ContainerRestart, docker kill, OOM kill, rm -f and rm. Mapped from moby's daemon sources (LogContainerEvent call sites and handleContainerExit/containerStop/containerRestart/kill paths) and verified against a live engine v29.6.1 with 'docker events'. Notably: the relative order of stop and die on a container stop is not guaranteed, and no event at all is emitted while a container sits in restart backoff. Related to #13985 / #13990. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
Since 2.39.3 the monitor relies solely on die events to stop tracking containers, but a container stopped while in restart backoff emits only stop and destroy events, so an attached
uphangs forever after an externalstop/down. Handle destroy as terminal, and on stop inspect the container to distinguish a definitive stop from the transient one emitted during a ContainerRestart, which keeps watch sync+restart (#13161) working.Related issue
Fixes #13985
(not mandatory) A picture of a cute animal, if possible in relation to what you did
