wslc: idle-terminate inactive per-user session VMs

[benhillis]

Idle-terminates per-user WSLC session VMs after a period of inactivity, so an idle container session releases its utility VM instead of holding memory indefinitely. Includes the supporting refactor that makes the VM lifecycle safe to reason about.

What this does

• Adds an idle timer to each per-user WSLC session VM. When no container operation has run for the configured grace period, the VM is torn down; the next operation transparently restarts it.
• Container operations take an activity lease (Session::BeginContainerOperation) so the VM cannot be idle-terminated mid-operation.
• Adds OnVmStarted / OnVmStopping plugin hooks, paired correctly across start/stop and VM-restart.

VM lifecycle hardening

• Extracts VM state management into a dedicated WSLCSessionRuntime state machine (start / stop / teardown / spontaneous-exit), replacing ad-hoc state spread across WSLCSession.
• Fixes several concurrency issues found during review:
  • reentrant deadlock in start/stop notifications,
  • idle-teardown vs. activity race,
  • stopping-only hook pairing,
  • two VM-lifecycle races found via multi-model review (dead-VM re-pair in the idle-timer abort path; terminating-gate ordering in EnsureVmRunning).
• Skips guest volume unmount when the VM has already exited.

Encapsulation cleanup

• Narrows the WSLCSession / WSLCSessionRuntime seam: replaces ~9 accessors that handed out mutable internals with intent-revealing methods (TryLockExclusive, SetContainerdProcess, Set/Reset/Signal/IsDockerdReady, SetSwapVhdPath), and drops dead accessors.
• Marks the per-operation activity leases [[maybe_unused]] to document that they are held for their scope lifetime.

Testing

Full build (x64 + arm64) + package, then locally: idle-termination (5/5), WslcVmRestart, VmKillTerminatesSession, StuckVmTermination, reentrant-mount plugin hook, and PluginTests::WslcSuccess all pass.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/windows/wslcsession/WSLCSessionRuntime.cpp:443

• In TearDownVmLockHeld, the bool parameter is named CaptureTerminationReason but it is also forwarded as the permanent flag to RuntimeHooks::TearDownSessionState. That dual meaning makes the teardown semantics hard to read and easy to misuse later. Consider introducing a clearly-named local (or renaming the parameter) when invoking the hook so it’s obvious the hook is receiving a “permanent shutdown” signal, not a “capture termination reason” signal.

    if (m_hooks.TearDownSessionState)
    {
        m_hooks.TearDownSessionState(CaptureTerminationReason);
    }

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

src/windows/wslcsession/WSLCContainer.cpp:2596

• vmLease is only used for its RAII side effect (keeping the VM alive), but the variable itself is otherwise unused. This commonly triggers MSVC warning C4189 (unused local variable) and can break builds when warnings are treated as errors. Consider marking this (and the other similar vmLease locals in this file) as [[maybe_unused]] to make the intent explicit and keep builds warning-clean.

    auto vmLease = m_session.Runtime().AcquireVmLease();
    return CallImpl(&WSLCContainerImpl::Attach, DetachKeys, Stdin, Stdout, Stderr);

src/windows/wslcsession/WSLCIdleState.h:22

• WSLCIdleState.h uses WIL result/diagnostics macros (THROW_LAST_ERROR_IF, FAIL_FAST_IF, CATCH_LOG), but the header only includes <wil/resource.h>. This makes the header non-self-contained and can break compilation if it’s included from a TU that doesn’t pull in <wil/result.h> first (e.g. outside the precompiled header). Include <wil/result.h> directly here to ensure the required macros are always available.

#include <atomic>
#include <chrono>
#include <functional>
#include <memory>
#include <utility>
#include <wil/resource.h>

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/windows/wslcsession/WSLCSessionRuntime.h:169

• TearDownVmLockHeld’s parameter name CaptureTerminationReason doesn’t match its documented/actual semantics: it’s passed through as the permanent argument to RuntimeHooks::TearDownSessionState. This is easy to misread as “only capture reason” and accidentally call it with the wrong value, which could clear/retain container wrappers incorrectly on future refactors. Consider renaming this parameter (and corresponding uses) to reflect permanence (e.g. permanentShutdown).

    _Requires_exclusive_lock_held_(m_lock)
    void StartVmLockHeld();
    _Requires_exclusive_lock_held_(m_lock)
    void StopVmLockHeld();
    _Requires_exclusive_lock_held_(m_lock)
    void TearDownVmLockHeld(bool CaptureTerminationReason = false);
    void EnsureVmRunning();

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

test/windows/testplugin/Plugin.cpp:659

• WSLCProcessGetExitEvent returns a duplicated handle (caller-owned). The current code stores it in g_leakedProcessExitEvent and waits on it, but never closes it, leaking a kernel handle each run of this test path.
  test/windows/testplugin/Plugin.cpp:654
• This comment block is duplicated verbatim (the same 5 lines appear twice), which makes the intent harder to read and looks accidental. Please remove the duplicate copy.

This issue also appears on line 655 of the same file.
test/windows/testplugin/Plugin.cpp:644

• The stop-window worker thread writes to g_logfile (e.g. "WSLC stop-window caller" / "WSLC leaked process died") while the callback thread is also writing to the same std::ofstream. Concurrent access to the same iostream without synchronization is a data race (UB) and can lead to flaky/corrupted logs. Prefer having the worker thread only compute/store results and emit the log lines after join (e.g., in OnWslcSessionStopping), or add a mutex that is consistently taken by all g_logfile writes.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

test/windows/wslc/e2e/WSLCE2EWarningTests.cpp:114

• This test now silently passes if RunWslc fails to capture stderr (Stderr == nullopt). Since the assertion here is specifically that the recovery warning is not emitted, the test should still require that stderr capture is present and then assert the warning substring is absent; otherwise a broken capture path could mask a regression.
  test/windows/testplugin/Plugin.cpp:667
• The stop-window test caches a raw Win32 HANDLE (g_leakedProcessExitEvent) but never closes it. Even in test code, leaking handles can accumulate across test runs and cause unrelated failures. Consider closing the handle once the waiting thread is done with it (and clearing the atomic) to keep resource usage bounded.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

test/windows/testplugin/Plugin.cpp:523

• g_leakedProcess / g_leakedProcessExitEvent are intentionally retained for the stop-committed scenario, but they are never cleaned up afterwards, which leaks the service-side process wrapper (and can also leak the duplicated exit-event handle if the stop-window thread doesn’t reach the exchange/CloseHandle path). Consider releasing the wrapper and closing any remaining exit event once the stop-window thread is drained at session teardown.
  test/windows/wslc/e2e/WSLCE2EWarningTests.cpp:116
• The new stderr check can still pass vacuously if stderr capture breaks: value_or(std::wstring{}) yields an empty string when result.Stderr is nullopt, and an empty string will never contain the warning. If the intent is to ensure capture is working while asserting the warning is not printed, require result.Stderr.has_value() before searching it.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (4)

src/windows/wslcsession/WSLCVirtualMachine.cpp:1285

• Same exception-safety issue as TryAllocatePort(): if std::make_shared<VmPortAllocation> throws after inserting port into m_reservations->Ports, the port remains reserved forever. Roll back the reservation in the failure path to keep the reservation table consistent.
  test/windows/testplugin/Plugin.cpp:666
• g_logfile is written from both the plugin callback thread and g_stopWindowCaller's worker thread (e.g. this line runs in the worker), but there is no synchronization around the shared std::ofstream. Concurrent operator<< on the same stream is a data race and can corrupt the log or make the expected-output matching flaky.

Consider adding a shared synchronization mechanism (e.g. std::mutex + helper that serializes all g_logfile writes, or std::osyncstream for line-at-a-time atomic writes) and using it for all log output that can come from multiple threads (this worker thread, the callback thread, and the existing async thread in OnVmStarted).
test/windows/testplugin/Plugin.cpp:587

• This comment is misleading: the WslcVmNeverStarted test expects OnWslcVmStarted to never fire, but this branch logs a "VM started" line if it does fire (which is how the test would detect the regression). Consider rewording to reflect that this is an assertion/diagnostic path, not expected behavior.
  src/windows/wslcsession/WSLCVirtualMachine.cpp:1271
• If std::make_shared<VmPortAllocation> throws after the port is inserted into m_reservations->Ports, the reservation is leaked (the port stays marked in-use), which can cause spurious ERROR_ALREADY_EXISTS / allocation failures later in the VM lifetime. The reservation update should be exception-safe by rolling back the set insert on failure.

This issue also appears on line 1281 of the same file.

[OneBlue]

nit: I'd rename to AcquireVmLease or similar to describe what this flag does instead of when it's set by the caller

[OneBlue]

I think we should reject the caller if a plugin tries to mount a folder / execute a process while the VM is not running (regardless of whether the VM is terminating or not).

The easiest way I could think of doing that would be to have flags in that call to determine if the caller should obtain a VM lease or not, and in the case of plugins, that flag should always be set to false

[OneBlue]

nit: I think we can probably remove these methods, they don't feel like they add much value

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

test/windows/testplugin/Plugin.cpp:696

• Avoid calling CloseHandle directly here as well; wrap the duplicated event in wil::unique_handle and wait on the wrapper. This keeps the close reliably paired even if additional early-returns/throws are introduced later in the thread body.
  test/windows/testplugin/Plugin.cpp:539
• Avoid calling CloseHandle directly; use a WIL RAII wrapper so the handle is always closed even if this function later changes to return/throw early. This aligns with the rest of the codebase’s handle-management pattern and keeps teardown logic exception-safe.

This issue also appears on line 692 of the same file.