Container lifecycle transitions

[Kevin Vega (kvega005)]

Summary of the Pull Request

WSLCContainerImpl lifecycle operations (Start, Stop, Delete) now coordinate through a shared StateTransition object published under m_lock, replacing the per-operation m_stopNotification/m_destroyEvent handshake serialzied by m_stopLock; a second lifecycle request is no longer blocked behind an in-flight Stop that is waiting indefinitely (WSLC_STOP_TIMEOUT_NONE), and the fixed 60-second waits in Stop() and Delete() are removed. DockerEventTracker now invokes callbacks outside its own lock, fixing a lock inversion between WSLCContainerImpl::Exec and event delivery that wedges the entire Docker event stream, and an iterator invalidation when a container stop unregisters its exec callbacks mid-dispatch.

PR Checklist

• Closes: Link to issue #xxx
• Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
• Tests: Added/updated if needed and all pass
• Localization: All end user facing strings can be localized
• Dev docs: Added/updated if needed
• Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Problem. Container lifecycle state was driven by two standalone events and a mutex. Stop() acquired m_stopLock then m_lock, issued the Docker request, and waited up to 60s on m_stopNotification.Event. OnEvent(Stop) acquired m_stopLock with std::try_to_lock and returned early when a Stop was already in flight, deferring cleanup to that caller. Ownership of stop cleanup was therefore decided by a try-lock race, and because m_stopLock is held for the entire duration of the wait, any second lifecycle request was blocked for as long as the first Stop waited — unbounded for WSLC_STOP_TIMEOUT_NONE. Delete() and the auto-remove path waited on m_destroyEvent under the same fixed 60s cap, and OnEvent(Destroy) asserted !m_destroyEvent.is_signaled(). OnEvent was not noexcept, and ContainerEvent::Start was not handled at all, so a container started outside WSLC went unrecorded.

DockerEventTracker::OnContainerEvent held m_lock across e.Callback(...), which produced two defects. First, a lock inversion: WSLCContainerImpl::Exec holds the container m_lock shared while constructing DockerExecProcessControl, whose constructor calls RegisterExecStateUpdates and takes the tracker lock; the event thread took those in the opposite order, holding the tracker lock while calling WSLCContainerImpl::OnEvent, which takes the container m_lock exclusive. Because the tracker is per-session, that deadlock stops delivery of all subsequent container and volume events. Second, iterator invalidation: the container's die callback reaches ReleaseProcesses(), which unregisters every live exec registration and erases from the same m_containerCallbacks vector the dispatch loop was iterating; the tracker's std::recursive_mutex allowed the erase to proceed and invalidate the loop's cached end iterator.

Change. Lifecycle requests now publish a StateTransition and wait on it:

struct StateTransition
{
    const TransitionKind Kind;               // Start | Stop | Delete
    ContainerEvent ExpectedEvent;
    wil::unique_event Completed{wil::EventOptions::ManualReset};
    std::exception_ptr Exception;
    unique_com_disconnect Wrapper;           // access under WSLCContainerImpl::m_lock
};

StartTransition publishes into _Guarded_by_(m_lock) std::shared_ptr<StateTransition> m_transition; OnEvent matches the arriving event against ExpectedEvent and calls CompleteTransition. WaitForConflictingTransitionToComplete serializes conflicting operations while allowing a caller of the same TransitionKind to attach to the in-flight transition rather than queue behind a mutex, which is what unblocks a concurrent Kill or shorter-timeout Stop. WaitForTransitionCompletion waits through WSLCSession::CreateIOContext() and an EventHandle, so the wait is bounded by session termination instead of a hard-coded 60s. Failures raised on the event thread are captured into StateTransition::Exception and rethrown by AttachToTransition on the requesting thread, so OnEvent is now noexcept.

A second reader/writer lock, m_lifecycleLock, is held shared by lifecycle requests until their transition is published and exclusive by event delivery, closing the window between issuing the Docker request and publishing the transition. StateTransition::Wrapper carries the deferred unique_com_disconnect so Disconnect() runs on the COM caller after it leaves OnEvent's critical section rather than on the event thread, where it would block draining in-flight COM callers. OnStopped now takes the exit code and, for WSLCContainerFlagsRm, upgrades the active transition's ExpectedEvent to ContainerEvent::Destroy so auto-remove completes under the same transition instead of a second wait. StopNotification, m_destroyEvent, and m_stopLock are removed, and DeleteExclusiveLockHeld becomes RequestDeleteExclusiveLockHeld since resource release now happens on the Destroy event.

Event tracker locking notes. OnContainerEvent and OnVolumeEvent copy the matching registrations into a local vector under m_lock, release it, and invoke through InvokeCallbacks. Registrations are stored as std::shared_ptr<ContainerCallback> / std::shared_ptr<VolumeCallback> so a snapshot entry stays alive if it is unregistered mid-dispatch. Both derive from a new CallbackRegistration base holding a std::recursive_mutex InvokeLock and a _Guarded_by_(InvokeLock) bool Unregistered; the dispatch loop holds InvokeLock while the callback runs and skips unregistered entries, and UnregisterCallback erases under m_lock, releases it, then acquires InvokeLock to block until any in-flight invocation completes. That preserves the previous guarantee that a callback never runs after its EventTrackingReference is reset — required because DockerExecProcessControl declares m_eventTrackingReference last and so unregisters before its other members are destroyed. InvokeLock is recursive so a running callback can unregister itself. With no callback running under m_lock, it is now a leaf lock and is downgraded from std::recursive_mutex to std::mutex. ContainerEvent::Restart is added for the Docker restart action.

Lifecycle / state-machine impact. Start() publishes a Start transition and commits WslcContainerStateRunning only when the Docker start event arrives; an unmatched start is logged as UnexpectedContainerStart instead of being ignored. Stop() skips creating a transition once WslcContainerStateExited is already observed, and does not create one when the signal is not SIGKILL and cannot be expected to terminate the container. Delete() publishes a Delete transition expecting ContainerEvent::Destroy, which is also where ReleaseResources() and the COM disconnect now run, so a Stop with auto-remove and an explicit Delete converge on the same path.

Validation Steps Performed

• WSLCTests::ConcurrentContainerStopAndKill — guards against a second lifecycle request being blocked behind m_stopLock while an indefinite Stop(WSLC_STOP_TIMEOUT_NONE) is waiting; the Kill must reach Docker and return while the Stop is still outstanding.
• WSLCTests::ConcurrentContainerStopTimeoutOverride — guards against a shorter Stop(..., 0) being unable to override an in-flight indefinite Stop; both calls must complete.
• WSLCTests::ConcurrentContainerStopAndStart — asserts Start issued during an in-flight Stop fails with WSLC_E_CONTAINER_IS_RUNNING rather than racing the transition.
• WSLCTests::ConcurrentContainerStopAndForceDelete — asserts a force Delete during an in-flight Stop completes and both callers observe WslcContainerStateDeleted.
• WSLCTests::ForceDeleteAutoRemoveContainer — covers force-delete of a WSLCContainerFlagsRm container, where the stop transition is upgraded to expect Destroy.
• WSLCTests::ExecContainerStopManyExecs — guards against the mid-dispatch iterator invalidation. Uses eight concurrent execs, because the pre-existing ExecContainerDelete uses a single exec and leaves nothing after the erased entry for the dispatch loop to walk into.
• WSLCTests::ExecContainerEventStress — guards against the Exec/event-delivery lock inversion, and against a registration being destroyed while its own callback is in flight. Four threads run short-lived execs and release each process without waiting, so unregistration overlaps exec_die dispatch. A regression manifests as a hang rather than an assertion failure, since the deadlock is service-side and blocks the COM call itself.
• The ConcurrentContainer* tests use a LaunchContainerWithBlockingStopHandler helper whose init process traps the stop signal and blocks on stdin, so the in-flight Stop window is deterministic rather than timing-dependent.
• PluginTests.cpp expected log output adds WSLC Container stopping, session=*, id=*, reflecting that OnContainerStopping is now raised from OnStopped.

[Copilot]

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 StateTransition model (with completion event/exception propagation) plus m_transitionLock/m_transition to coordinate lifecycle operations and allow concurrent Stop() callers to join an in-flight transition.
• Refactor lifecycle flow so Start(), Stop(), and Delete() 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.

[Pooja Trivedi (ptrivedi)]

would be useful if we could test that terminating the waiting client returns E_ABORT but doesn't invalidate the published transition... that another caller or event can still finish it safely

[Kevin Vega (kvega005)]

I think we can do this with an E2E test, using the CLI. I'll look into it.

[Blue (OneBlue)]

exitCode being absent there would be a symptom of a deeper problem, since we'd essentially be a in broken state whatever we do.

If we wanted to be ultra safe here, we have a LOG_HR + default to something like 255

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/windows/wslcsession/WSLCContainer.cpp:1373

• The comment above waitForStop doesn’t match the actual logic: for Stop() calls (Kill == false) the expression forces waitForStop to be true regardless of whether SIGKILL is used. This is confusing for future maintenance and makes it look like Stop() may skip waiting when it cannot.

            // Don't wait for the container to stop if we're not sending SIGKILL, since it may not stop the container.
            // N.B. If the signal was SIGTERM for instance, we'll receive the stop notification via OnEvent().
            bool waitForStop = !Kill || (SignalArg.value_or(WSLCSignalSIGKILL) == WSLCSignalSIGKILL);

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/windows/wslcsession/WSLCContainer.cpp:1287

• OnEvent(ContainerEvent::Stop) assumes Docker always provides an exit code. If Docker omits the exitCode attribute (or parsing failed upstream), exitCode.value() will throw and because OnEvent is noexcept this will terminate the process. Please handle a missing exitCode explicitly (log + use a sentinel) so event delivery can’t bring down the session.

        else if (event == ContainerEvent::Stop)
        {
            WI_ASSERT(exitCode.has_value());
            OnStopped(exitCode.value(), eventTime);
        }

[Blue (OneBlue)]

If we have thread A doing something like:

wslc stop

And in parallel thread B does:

wslc kill -s SIGUSR1 ,

thread will have waitForStop = false, but will have transition set to the pending stop transition, so when we reach this block, thread B will attach to thread A's transition, which is not what we want.

The easiest fix I can think of would be switching to:

if (waitForStop)
{
    lock = m_lock.lock_exclusive();
    [...]
}
else
{
    transition.reset();
}

It might also be a good idea to add a test case covering that specific potential hang. We could have a purposefully hung stop, followed by a kill with a dummy signal like USR1 in a separate thread, to validate that the second thread doesn't get stuck

[Blue (OneBlue)]

LGTM, thank you for doing this. This change will give us a much stronger foundation than the previous implementation and will unblock things like parallel stops & wslc restart

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/windows/wslcsession/DockerEventTracker.h:143

• InvokeCallbacks() does not guard against exceptions thrown by callback implementations. Some registered callbacks can throw (e.g., WSLCVolumes::OnVolumeEvent() calls THROW_HR_IF_MSG at WSLCVolumes.cpp:52-68), so a single bad volume/container callback would abort dispatch of the remaining callbacks for that event and get reported as a DockerEventParseError by DockerEventTracker::Connect's outer catch, which is misleading.

Consider catching exceptions per-callback inside InvokeCallbacks() (or inside the per-event Invoke lambda) and logging them, while continuing dispatch to the rest of the snapshot.

    // Invokes a snapshot of callbacks taken under m_lock, skipping registrations that have since been unregistered.
    template <typename TCallback, typename TInvoke>
    static void InvokeCallbacks(const std::vector<std::shared_ptr<TCallback>>& Callbacks, const TInvoke& Invoke)
    {
        for (const auto& e : Callbacks)
        {
            std::lock_guard invokeLock{e->InvokeLock};
            if (!e->Unregistered)
            {
                Invoke(*e);
            }
        }