Repository navigation
Record Docker die and stop events with parity - #41770
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is coherent across event tracking, recovery, metadata compatibility, and automated coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Separates Docker die and stop events while preserving action metadata and requested image names across recovery.
Changes:
- Records
killandstopat session scope and reports exits asdie. - Persists requested image names in container metadata.
- Adds unit and end-to-end coverage for event parity, cleanup, filtering, and recovery.
| File | Description |
|---|---|
test/windows/WSLCTests.cpp |
Expands event and recovery tests. |
test/windows/wslc/e2e/WSLCE2EEventsTests.cpp |
Updates CLI event expectations. |
src/windows/wslcsession/WSLCSession.h |
Declares session-level action tracking. |
src/windows/wslcsession/WSLCSession.cpp |
Records container actions in the event store. |
src/windows/wslcsession/WSLCContainerMetadata.h |
Persists the requested image name. |
src/windows/wslcsession/WSLCContainer.h |
Exposes event-attribute normalization. |
src/windows/wslcsession/WSLCContainer.cpp |
Reports die and restores requested images. |
src/windows/wslcsession/DockerEventTracker.h |
Adds container-action callback support. |
src/windows/wslcsession/DockerEventTracker.cpp |
Routes kill and stop independently. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
David Bennett (dkbennett)
left a comment
David Bennett (dkbennett)
left a comment
There was a problem hiding this comment.
Looks good but one parity gap remains:
Docker's die event includes execDuration, but WSLC drops it: DockerEventTracker forwards only the exit code, and RecordEvent reconstructs the attributes. The new kill/stop path preserves Docker attributes. Consider sharing attribute normalization across these paths and asserting execDuration against the underlying Docker event.
If goal of this PR is parity should fix that.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
| const auto execDuration = | ||
| lines[2].substr(execDurationValueStart, lines[2].find(L',', execDurationValueStart) - execDurationValueStart); |
| // Docker can report 'stop' before 'die', so it's published with the exit transition instead. | ||
| if (m_state == WslcContainerStateRunning) | ||
| { | ||
| m_pendingStopAttributes = attributes; |
There was a problem hiding this comment.
Doing this could unfortunately create a race if we receive stop that actually mapped to a previously running instance.
We could in theory have a Start() arrive after a kill, and if the stop notification from docker arrives after the start(), we could end up in a situation where we'll emit a "broken" stop.
Interestingly, this also seems to be a race inside docker, so for now I think we're OK to merge as-is.
I did some thinking and I haven't found an "easy" solution that wouldn't require a bigger change. Let's revisit if this turns out to be an issue later
Summary of the Pull Request
Make wslc events report Docker container exit and stop events separately. Keep kill and stop events available after container cleanup, and preserve the requested image name for -P containers and after recovery.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Docker's die event is now reported as die, with its exit code. Docker's separate stop event is recorded without changing container state. kill and stop are recorded at session level, so they remain available even if the container is auto-removed. Docker attributes such as signal are preserved, while WSLC's private metadata label is removed.
The requested image name is saved in the container metadata. This keeps image information correct for -P containers and after recovery. Older containers without the new field continue to use Docker's image value.
Validation Steps Performed