Repository navigation
[WSLC] (2/4) Hold multiple sessions in the lifecycle daemon - #1489
Soham Das (SohamDas2021) wants to merge 5 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
02597a8 to
9398655
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved teardown ownership and deadline defects can cause unsafe DLL access, premature store reuse, or stalled shutdown.
7 open findings
Processor count undercounts multi-group hosts · New Guard drop order can unload SDK before job cleanup · New GPU-enabled sessions omit GPU container passthrough flag · New SDK may unload while pending boot reports still own guards · New Unchecked SessionBooted drain drops lease and clears active VM slot · New Synchronous boot termination escapes shared release deadline · New Synchronous session termination can exceed shutdown deadline · New
What changed in this PR
Adds multi-session infrastructure to the WSLc lifecycle daemon as part of #1460. Public lifecycle requests still use the shared implicit session.
Changes:
- Tracks sessions, container placement, and per-session store leases.
- Adds session sizing, CPU validation, and asynchronous boot/release handling.
- Extends protocol messages, tests, and operational documentation.
| File | Description |
|---|---|
| src/mxc-sdk/tests/wslc_daemon_ipc.rs | Updates provision fixtures. |
| src/mxc-sdk/src/bin/wslc_daemon/session_manager.rs | Manages multiple sessions and their lifetimes. |
| src/mxc-sdk/src/bin/wslc_daemon/control_server.rs | Handles session requests and undelivered replies. |
| src/mxc-sdk/src/backends/wslc/common/wslc_bindings.rs | Adds checked session termination. |
| src/mxc-sdk/src/backends/wslc/common/wsl_container_runner.rs | Validates CPU sizing against host capacity. |
| src/mxc-sdk/src/backends/wslc/common/store_slots.rs | Updates session-creation tests. |
| src/mxc-sdk/src/backends/wslc/common/state_aware.rs | Preserves implicit-session routing. |
| src/mxc-sdk/src/backends/wslc/common/image.rs | Counts outstanding pulls per session. |
| src/mxc-sdk/src/backends/wslc/common/daemon_protocol.rs | Adds session messages and bumps protocol version. |
| src/mxc-sdk/src/backends/wslc/common/container_steps.rs | Applies sizing, boot timeout, and checked release. |
| scripts/setup-wslc.ps1 | Clarifies multi-session cache warming. |
| docs/schema.md | Clarifies current public configuration limits. |
| docs/backends/wslc/wslc-state-aware.md | Documents session ownership, sizing, and teardown. |
| docs/backends/wslc/wsl-container-getting-started.md | Clarifies session infrastructure versus public support. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Summary
Changes requested: The new multi-session reaper can fan out into unbounded release threads and, on thread-spawn failure, drop a session guard on the shared worker while forgetting its store lease. Shutdown can also discard a queued SessionBooted result and return its lease even if that guard's unchecked termination fails. The second issue is already described in the existing inline review thread; I am not duplicating its anchor. Please fix both ownership paths before merging. Additional inline comments identify the stale idle grace, nondeterministic Windows tests, abandoned named boot, cold-slot release race, quadratic poll, and undelivered-session coverage.
Verified: Base 34187f5 is the merge-base of head 9398655; the captured 14-file diff matches gh pr diff 1489. I followed SessionRelease::drop and WslcSessionGuard::drop, plus both shutdown drains. On a recently booted Windows host, cargo test -p mxc-sdk --features wslc --bin wxc-wslc-daemon -- session_manager::tests:: --test-threads=1 produced 81 passed, 7 failed, 13 ignored: seven newly added tests panic at Instant::now() - 24h. I have not run live WSL2 host tests. The owner-only daemon pipe, typed default refusals and deny-egress image path were inspected without a separate attributable security finding; existing bot threads on GPU and SDK lifetime remain independent of this review.
Other findings and prior inline threads
- Medium — Shutdown deadline does not bound a synchronous termination.
session_manager.rs:2458-2492—introduced_by_change: the multi-session loop checks the new release deadline before each SDK terminate, not while it runs. One hung terminate can exceed the whole budget and block other VM releases. Already raised inline; no duplicate comment. Run releases on bounded workers and retain unconfirmed sessions/leases after the deadline. - Medium — Host-wide CPU check reads one processor group.
container_steps.rs:95-117—introduced_by_change:GetSystemInfo().dwNumberOfProcessorsreports only the current group, yet both new validation surfaces call it the host total. On multi-group machines a valid host-wide CPU count can be refused. The host test copies the same oracle. Already raised inline; use a system-wide count such asGetActiveProcessorCount(ALL_PROCESSOR_GROUPS)or deliberately narrow the policy. - Low — Session admission intentionally has no aggregate or pending-boot cap.
session_manager.rs:1254-1307—introduced_by_change: each request can reserve a slot and spawn a VM boot; after a 600s client deadline a still-running boot retains both. The guide tells callers to enforce their own limits, so this is a documented operational risk, not an independent merge condition. Account for abandoned boots when phase 4 exposes session admission. - Low — VM/lease teardown rules are repeated.
session_manager.rs:1024-1070,2358-2492—introduced_by_change: normal reaping, a late boot and shutdown separately decide to return versus retain a lease after termination. Their drift already surfaces in the High shutdown finding; centralize the error-aware ownership decision while fixing it. - Low — Session sizing is applied in two copies.
container_steps.rs:866-915andwsl_container_runner.rs:534-576—introduced_by_change: CPU, memory, timeout and GPU setters agree now but can drift between daemon and one-shot surfaces. Share the settings helper without dropping early host validation. - Low — Boot scheduling has no deterministic thread/report test seam.
session_manager.rs:955-990—introduced_by_change: hermetic tests insertPendingSessionrather than checking the real ordering of queuedSessionBootedversus the in-flight count; host-gated tests exist. A controlled executor would make that boundary testable without WSL2. - Low — Successful session RPC replies lack hermetic coverage.
session_manager.rs:1394,1072—introduced_by_change: positiveProvisionSessionandDeprovisionSessionreplies have only host-dependent exercise, leaving a missingreply.sendhard to detect without WSL2. Use the existing fake guard/channel helpers for success and failure replies. - Low — New memory narrowing guard has no true overflow test.
container_steps.rs:881—introduced_by_change: the purported very large 6410241024 MB host value still fits inu32, so it does not testu32::try_fromfailure. Testu32::MAX + 1with a pure narrowing helper.
Claim-only and documentation notes (not merge conditions by themselves)
- Medium — Teardown duration claim.
wslc-state-aware.md:586-592—claim_mismatch: the new guide promises a single 60s total shutdown budget, but the same guide at:367-370andrelease_sessions_for_shutdowngive draining and VM release separate budgets. State the two phases; do not use this claim alone to block. - Low — Per-caller VM isolation wording.
wslc-state-aware.md:51-52—claim_mismatch: current public lifecycle requests still use the shared implicit session (state_aware.rs:579-583); the guide later acknowledges named sessions are not yet caller-reachable. Clarify that only the daemon's internal protocol can choose another VM at this phase. - Low — Ignored host-test count.
PR description—claim_mismatch: the unfiltered--ignoredcommand is reported as 11/11, but this revision has 13 ignored daemon tests, including measurement-only probes. Name which selectors ran rather than suggesting every ignored test passed.
Verified pre-existing — not attributed to this PR
- The create-and-run default store, its isolated-image refusal and the owner-only pipe existed at base. The 24-hour backdating, multi-session thread fan-out and shutdown drain command are new here, so they are not being charged to #1475. Existing Copilot review threads on COM release ordering and GPU propagation are not reproduced as our own findings.
9398655 to
2efd151
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved reply-channel leaks, GPU propagation, idle-grace handling, and processor-count test errors need correction.
11 open findings
Release pending reply senders when the worker unwinds · New Unchecked SessionBooted drain drops lease and clears active VM slot SDK may unload while pending boot reports still own guards GPU-enabled sessions omit GPU container passthrough flag Guard drop order can unload SDK before job cleanup Processor count undercounts multi-group hosts Forward GPU settings when creating provisioned containers · New Reset empty-session grace period when provisioning starts · New Correct host capacity measurement in processor tests · New Synchronous session termination can exceed shutdown deadline Synchronous boot termination escapes shared release deadline
🧠 Review effort: Balanced
2efd151 to
6dc3dde
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved cleanup, shutdown-budget, and test-thread lifetime issues can compromise reliable session management.
6 open findings
Unchecked VM cleanup can release a store with a mounted VHDX · New Rollback loses implicit session ID after release thread spawn failure · New Late boot termination bypasses the shared shutdown deadline · New Test can free SDK storage while a boot thread still uses it · New Synchronous session termination can exceed shutdown deadline Synchronous boot termination escapes shared release deadline
9 resolved since last review
Release pending reply senders when the worker unwinds Unchecked SessionBooted drain drops lease and clears active VM slot SDK may unload while pending boot reports still own guards GPU-enabled sessions omit GPU container passthrough flag Guard drop order can unload SDK before job cleanup Processor count undercounts multi-group hosts Correct host capacity measurement in processor tests Reset empty-session grace period when provisioning starts Forward GPU settings when creating provisioned containers
🧠 Review effort: Balanced
| if self.default_session.as_deref() == Some(session_id) { | ||
| self.default_session = None; | ||
| } |
6dc3dde to
3e303e0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Timed-out deferred sessions retain bookkeeping entries indefinitely in a running daemon.
7 open findings
Timed-out deferred sessions without stores are never removed · New Unchecked VM cleanup can release a store with a mounted VHDX Test can free SDK storage while a boot thread still uses it Late boot termination bypasses the shared shutdown deadline Rollback loses implicit session ID after release thread spawn failure Synchronous session termination can exceed shutdown deadline Synchronous boot termination escapes shared release deadline
🧠 Review effort: Balanced
3e303e0 to
794411e
Compare
794411e to
44254dd
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Release-failure recovery and shutdown-budget handling still have unresolved correctness issues.
7 open findings
Timed-out deferred sessions without stores are never removed Unchecked VM cleanup can release a store with a mounted VHDX Test can free SDK storage while a boot thread still uses it Late boot termination bypasses the shared shutdown deadline Rollback loses implicit session ID after release thread spawn failure Synchronous session termination can exceed shutdown deadline Synchronous boot termination escapes shared release deadline
🧠 Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Previously identified shutdown-deadline and implicit-session rollback issues remain unresolved in the supplied code.
9 open findings
Timed-out deferred sessions without stores are never removed Unchecked VM cleanup can release a store with a mounted VHDX Test can free SDK storage while a boot thread still uses it Late boot termination bypasses the shared shutdown deadline Rollback loses implicit session ID after release thread spawn failure Synchronous session termination can exceed shutdown deadline Synchronous boot termination escapes shared release deadline Public provisions incorrectly share the implicit VM · New Shutdown duration claim ignores synchronous termination · New
🧠 Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Shutdown termination can exceed its deadline indefinitely, and timed-out deferred sessions retain unreachable bookkeeping entries.
11 open findings
Bound synchronous termination calls during shutdown · New Timed-out deferred sessions without stores are never removed Unchecked VM cleanup can release a store with a mounted VHDX Remove expired deferred sessions after client responses · New Test can free SDK storage while a boot thread still uses it Late boot termination bypasses the shared shutdown deadline Rollback loses implicit session ID after release thread spawn failure Synchronous session termination can exceed shutdown deadline Synchronous boot termination escapes shared release deadline Shutdown duration claim ignores synchronous termination Public provisions incorrectly share the implicit VM
🧠 Review effort: Balanced
66e40cb to
18ad3a1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
A stalled session release can prevent unrelated session creation even when a warm store is available.
11 open findings
Resume deferred boots when individual slots become available · New Bound synchronous termination calls during shutdown Timed-out deferred sessions without stores are never removed Unchecked VM cleanup can release a store with a mounted VHDX Remove expired deferred sessions after client responses Test can free SDK storage while a boot thread still uses it Late boot termination bypasses the shared shutdown deadline Rollback loses implicit session ID after release thread spawn failure Synchronous session termination can exceed shutdown deadline Synchronous boot termination escapes shared release deadline Shutdown duration claim ignores synchronous termination
1 resolved since last review
🧠 Review effort: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9709ae67-94ac-4e0a-8f90-ce6072e5354a
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9709ae67-94ac-4e0a-8f90-ce6072e5354a
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9709ae67-94ac-4e0a-8f90-ce6072e5354a
…ion logs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9709ae67-94ac-4e0a-8f90-ce6072e5354a
18ad3a1 to
dfa3d04
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Release-thread startup failure loses implicit-session routing despite restoring the live session, causing unnecessary replacement boots and provisioning failures.
12 open findings
Resume deferred boots when individual slots become available Bound synchronous termination calls during shutdown Timed-out deferred sessions without stores are never removed Unchecked VM cleanup can release a store with a mounted VHDX Preserve implicit-session ID when release-thread creation fails · New Remove expired deferred sessions after client responses Test can free SDK storage while a boot thread still uses it Late boot termination bypasses the shared shutdown deadline Rollback loses implicit session ID after release thread spawn failure Synchronous session termination can exceed shutdown deadline Synchronous boot termination escapes shared release deadline Shutdown duration claim ignores synchronous termination
🧠 Review effort: Balanced
| if self.default_session.as_deref() == Some(session_ref) { | ||
| self.default_session = None; | ||
| } |
… still hold Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9709ae67-94ac-4e0a-8f90-ce6072e5354a
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved release handling can prevent idle shutdown and bypass warm-store reuse.
4 open findings
10 resolved since last review
Resume deferred boots when individual slots become available Bound synchronous termination calls during shutdown Timed-out deferred sessions without stores are never removed Unchecked VM cleanup can release a store with a mounted VHDX Remove expired deferred sessions after client responses Test can free SDK storage while a boot thread still uses it Late boot termination bypasses the shared shutdown deadline Synchronous session termination can exceed shutdown deadline Synchronous boot termination escapes shared release deadline Shutdown duration claim ignores synchronous termination
🧠 Review effort: Balanced
| ) -> Result<(), WorkerError> { | ||
| let (reply, rx) = oneshot::channel(); | ||
| self.send(WorkerCommand::DeprovisionSession { config, reply })?; | ||
| rx.await.map_err(worker_gone)? |
| let _ = start_session_release( | ||
| SessionRelease { | ||
| session: Some(booted), | ||
| store, | ||
| name, | ||
| }, | ||
| session_ref.to_string(), | ||
| &self.releases_in_flight, | ||
| None, | ||
| Some(worker.clone()), | ||
| ); |



📖 Description
The container-lifecycle daemon held exactly one WSLc session — one WSL2 utility VM — and created every sandbox's container inside it. That single session is why
cpuCount/memoryMb/gpuare absent from the lifecycleprovisioncontract: they configure the session rather than the container, so under one shared VM they could not mean anything per sandbox. No caller can supply them today, since the contract rejects unknown fields, andcreate_daemon_sessionset none of them either — so every lifecycle sandbox ran on the SDK defaults of 2 CPUs and 2000 MB, and a hung create had no bound at all.The daemon now holds as many sessions as it is asked for.
Option<WslcSessionGuard>became a map keyed by session id, each entry owning its VM handle, its leased store slot and its sizing. A container records the session it was placed in, and a session stays alive for exactly as long as something is using it. Sessions are sized on creation now, and a boot is bounded.cpuCountabove the host's logical processor count is refused on both surfaces, because the SDK does not oversubscribe processors.memoryMbis deliberately not checked: it is a ceiling the host backs on demand rather than a reservation, so a request past installed memory is legitimate and works.🔗 References
Part of #1460. Second of four PRs; stacked on #1475.
🔍 Validation
From
src/:cargo fmt --all -- --checkcargo clippy -p mxc-sdk -p wxc --features wslc --all-targets -- -D warningscargo test -p mxc-sdk --features wslc --libcargo test -p mxc-sdk --features wslc --binscargo test -p mxc-sdk --features wslc --test wslc_daemon_ipcOn a live WSL2 host, with
alpine:latestwarmed into two daemon store slots:cargo test -p mxc-sdk --features wslc --bin wxc-wslc-daemon -- --ignored --test-threads=1The 11 include two sessions coexisting in one daemon, and a new test that provisions and runs a container in each of two sessions, releases one session entirely, and proves the other still executes. That last one fails if session routing is disabled, which the single-session tests do not catch.
Every fix in the second commit was confirmed by reverting it and watching its test fail, rather than by the test merely passing. One example: with the queued-boot drain removed,
shutdown_releases_a_boot_that_landed_while_it_was_drainingreportsleft: 0, right: 1— the VM was never stopped.Not run: the
tests\scripts\run_wslc_*.ps1end-to-end harnesses.✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes📋 Issue Type
Microsoft Reviewers: Open in CodeFlow