Container lifecycle transitions - #41140
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors WSLC container lifecycle handling to coordinate Start/Stop/Delete operations via a shared “transition” object, with event-driven completion, and renames the internal state update helper from Transition to CommitState.
Changes:
- Introduce a
StateTransitionmodel (with completion event/exception propagation) plusm_transitionLock/m_transitionto coordinate lifecycle operations and allow concurrentStop()callers to join an in-flight transition. - Refactor lifecycle flow so
Start(),Stop(), andDelete()publish a transition and wait for corresponding Docker events to complete it. - Extend Docker event tracking to include the
"restart"action (ContainerEvent::Restart).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/windows/wslcsession/WSLCContainer.h | Adds transition coordination primitives and renames Transition → CommitState; updates lifecycle method signatures. |
| src/windows/wslcsession/WSLCContainer.cpp | Implements transition creation/wait/completion and refactors event handling and Stop/Delete behavior around transitions. |
| src/windows/wslcsession/DockerEventTracker.h | Adds ContainerEvent::Restart. |
| src/windows/wslcsession/DockerEventTracker.cpp | Maps Docker "restart" events to ContainerEvent::Restart. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/windows/wslcsession/WSLCContainer.cpp:940
- OnEvent() is declared noexcept, but the Stop branch uses THROW_HR_IF when exitCode is missing. Any throw inside a noexcept function will terminate the process. Also, if this occurs while a Stop() transition is waiting, the transition would never be completed, causing a hang.
else if (event == ContainerEvent::Stop)
{
THROW_HR_IF(E_UNEXPECTED, !exitCode.has_value());
OnStopped(exitCode.value(), eventTime);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/windows/wslcsession/WSLCContainer.cpp:941
- WSLCContainerImpl::OnEvent() is declared
noexcept, but the Stop-path usesTHROW_HR_IF(...). Any exception escaping anoexceptfunction will callstd::terminate(), and DockerEventTracker invokes callbacks without a try/catch, so this would turn a malformed/partial Docker event (missing exitCode) into a process crash. Handle the missing exitCode without throwing (and still drive OnStopped so any waiting transition can complete).
else if (event == ContainerEvent::Stop)
{
THROW_HR_IF(E_UNEXPECTED, !exitCode.has_value());
OnStopped(exitCode.value(), eventTime);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/windows/wslcsession/WSLCContainer.cpp:941
- WSLCContainerImpl::OnEvent() is declared noexcept but still uses THROW_HR_IF when handling ContainerEvent::Stop. If Docker sends a "die" event without an exitCode attribute, this will throw inside a noexcept callback (invoked by DockerEventTracker without try/catch), causing std::terminate and potentially taking down the event thread/process. Handle missing exitCode without throwing (log and use a default exit code, or ignore the event safely).
else if (event == ContainerEvent::Stop)
{
THROW_HR_IF(E_UNEXPECTED, !exitCode.has_value());
OnStopped(exitCode.value(), eventTime);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/windows/wslcsession/WSLCContainer.cpp:941
- WSLCContainerImpl::OnEvent() is declared noexcept and is invoked from DockerEventTracker without any try/catch around the callback. Using THROW_HR_IF here will call std::terminate on a missing exitCode (or unwind out of the callback if noexcept is removed), potentially crashing the session/event thread and leaving waiters blocked. Handle the missing exitCode without throwing and complete any in-flight Stop transition with an error so WaitForTransition() doesn’t hang forever.
else if (event == ContainerEvent::Stop)
{
THROW_HR_IF(E_UNEXPECTED, !exitCode.has_value());
OnStopped(exitCode.value(), eventTime);
}
| void OnEvent(ContainerEvent event, std::optional<int> exitCode, std::uint64_t eventTime) noexcept; | ||
|
|
||
| // Returns with lock held when no transition is active or the active transition matches kind. | ||
| void WaitForConflictingTransitionToComplete(wil::rwlock_release_exclusive_scope_exit& lock, std::optional<TransitionKind> kind = std::nullopt); | ||
| void WaitForTransitionCompletion(const std::shared_ptr<StateTransition>& transition) const; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/windows/wslcsession/WSLCContainer.cpp:969
OnEvent()isnoexceptbut usesWI_ASSERT(exitCode.has_value())forContainerEvent::Stop. Docker events are external/untrusted input; if the event arrives without an exitCode attribute, this will fail-fast the process and may also leave a waiting Stop transition unresolved. Prefer handling this gracefully (log + complete an in-flight Stop transition with an error) instead of asserting.
else if (event == ContainerEvent::Stop)
{
WI_ASSERT(exitCode.has_value());
OnStopped(exitCode.value(), eventTime);
}
| // We must release m_lock and m_stopLock before the wrapper's destructor calls | ||
| // Disconnect(), so in-flight COM callers can drain from COMImplClass::m_callers. | ||
| unique_com_disconnect comWrapper; | ||
| THROW_HR_IF_MSG( |
There was a problem hiding this comment.
I think it would also be a good idea to plumb the caller's process here. The easiest way to do that would be to use WSLCSession::CreateIOContext() to setup the wait.
That way if a caller does wslc stop <id> and then ctrl-C, and the stop is stuck, the service thread doing the wait will be released
| WSLC Image created, session=*, id=sha256:*, name=wslc-registry:latest | ||
| WSLC Container started, session=*, id=*, name=*, image=wslc-registry:latest, state=running | ||
| WSLC Image created, session=*, id=sha256:*, name=127.0.0.1:5000/debian:latest | ||
| WSLC Container stopping, session=*, id=* |
There was a problem hiding this comment.
Interesting, I guess there was a race before and we didn't record the transition correctly before the session terminated ?
| VERIFY_ARE_EQUAL(m_defaultSession->OpenContainer(id.c_str(), &openedContainer), WSLC_E_CONTAINER_NOT_FOUND); | ||
| VERIFY_ARE_EQUAL(m_defaultSession->OpenContainer(name.c_str(), &openedContainer), WSLC_E_CONTAINER_NOT_FOUND); | ||
| } | ||
|
|
There was a problem hiding this comment.
I recommend also test coverage for the "stuck stop" case, where we do something like:
wslc run -it debian:latest sleep 9999999
then in thread A: wslc stop --timeout -1 # Should get stuck forever
And in thread B: wslc stop --timeout 0 # Should actually kill the container, and both threads should successfully exit
| } | ||
| } | ||
|
|
||
| transition = std::make_shared<StateTransition>(ContainerEvent::Start); |
There was a problem hiding this comment.
We're doing this (creating a new StateTransition + WI_ASSERT() + assigning to m_transition in 3 places so I think this should be worth extracting to a StartTransition() method (which would return the shared_ptr)
| { | ||
| THROW_HR_IF_MSG( | ||
| E_UNEXPECTED, | ||
| !m_wslcSession.WaitForEventOrSessionTerminating(transition->Completed.get(), 60s), |
There was a problem hiding this comment.
I'm not sure that we want a timeout here. On the Stop() path, if an "infinite stop" is running, and another stop arrives, we shouldn't time out when waiting for the state change
| WI_ASSERT(m_transition->Kind == TransitionKind::Stop); | ||
| transition = m_transition; | ||
| lock.reset(); | ||
| AttachToTransition(transition); |
There was a problem hiding this comment.
Unfortunately I don't think we can just attach to an existing stop in progress in this case.
If we have:
wslc stop <container> -t -1
Followed by:
wslc stop <container> -t 0
The second Stop() call should also a stop API call to docker as well
Summary of the Pull Request
Updates
WSLCContainerImplso requested COM lifecycle operations are represented by an activeStateTransitionand completed by the corresponding Docker event.OnEvent()becomes the source of truth for committing container state and releasing lifecycle resources, while concurrent Stop/Kill callers share the same in-flight transitionPR Checklist
Detailed Description of the Pull Request / Additional comments
Problem. Container lifecycle requests and observed Docker state were previously handled through separate paths.
Start()committed the Running state from the request path,Stop()coordinated withm_stopLockandm_stopNotification, andDelete()committed the Deleted state before separately waiting form_destroyEvent. This split state and resource management between COM callers and Docker event callbacks, while concurrent Stop/Kill callers could not share the result of the same requested transition.Change. Introduces
StateTransitionto represent an active state change requested through COM. A transition records the expected Docker event, a manual-reset completion event, any resulting exception, and deferred COM-wrapper cleanup. Lifecycle methods submit the Docker request while holdingm_lock, publish the transition, releasem_lock, and wait forOnEvent()to complete it.OnEvent()now commits state based on the Docker event stream:WslcContainerStateRunningand completes the matching Start transition.WslcContainerStateExited, and completes the matching Stop transition.Stopevent handling submits the delete request and updates the active transition to expect Destroy, allowing the original Stop caller to wait through deletion.WslcContainerStateDeleted, releases the remaining resources, disconnects the COM wrapper outside the container lock, and completes the matching Delete or auto-remove transition.m_transitionLockserializes lifecycle operations initiated through COM. Start and Delete hold it exclusively, while Stop/Kill hold it shared so concurrent callers can attach to the same active Stop transition.m_lockprotects the active transition and container state, while waits occur without holdingm_locksoOnEvent()can process the corresponding Docker event.This replaces the dedicated Stop and Destroy notification objects and their fixed 60-second waits with one transition-completion mechanism.
Validation Steps Performed
WSLCTests::ConcurrentContainerStopAndKillto verify that a concurrent Kill joins an active Stop transition instead of issuing a competing request after the container exits.WSLCTests::ForceDeleteAutoRemoveContainerto verify that force-deleting a running auto-remove container waits for Docker destruction, commits Deleted, disconnects the COM wrapper, and removes the container from lookup.PluginTests::WslcPullImageNotificationto account for the container-stopping notification produced during event-driven session cleanup.