You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[WSLC] (1/4) Move the lifecycle daemon off the create-and-run image store - #1475
While any container-lifecycle sandbox was provisioned, every create-and-run WSLc execution on the same host failed with WslcListSessionImages failed: HRESULT 0x80070020, and kept failing for five minutes after the sandbox was gone. The lifecycle daemon and the create-and-run runner had independently defaulted to the same image store, %TEMP%\mxc-wslc-sessions. That store is a VHDX, which admits one live session, and the daemon holds its session through the idle window.
The daemon now leases its store from a pool instead:
It takes the lowest free slot when it boots a VM and returns it when the VM is released. Create-and-run execution keeps the store it always used, so neither surface can block the other. %TEMP%\mxc\wslc-store-slots is the fallback when %LOCALAPPDATA% is unresolvable — deliberately not the create-and-run default, so even the fallback cannot re-collide.
Slots are leased rather than minted per session because a released store keeps its images while a fresh directory has none, and an uncached image under the isolated posture is refused rather than pulled. A per-session directory would leave every session cold and fail isolated provisioning outright. A slot directory is created only when first leased, so an unused index costs nothing.
🔗 References
Part of #1460. First of four PRs; fixes the store-sharing symptom on its own.
PR
Title
1
[WSLC] (1/4) Move the lifecycle daemon off the create-and-run image store — this PR
2
[WSLC] (2/4) Hold multiple sessions in the lifecycle daemon
3
[WSLC] (3/4) Add the session lifecycle phases and dispatch routing
4
[WSLC] (4/4) Expose provisionSession through the contract, SDKs and CLI
A multi-axis review of this branch raised 26 findings. Ten that nothing downstream addresses are fixed here; the rest are multi-session concerns that PRs 2–4 absorb.
The reason will be displayed to describe this comment to others. Learn more.
Summary
Changes requested: Make daemon-slot prewarming independent of the create-and-run store. The newly added --wslc-daemon-slots path first opens the create-and-run store and returns on failure, so a live create-and-run execution prevents warming an otherwise free daemon slot. Please also correct the new claim that disk usage cannot exceed peak concurrent sessions: explicitly prewarmed slots persist even if never leased. That documentation issue is already covered by the existing Copilot comment, so I have not opened a duplicate inline thread.
The inline comments distinguish these requested changes from non-blocking performance, maintenance and test-coverage suggestions. Findings below are also non-blocking; they were verified against base 1d10040 and head cfc9fe6.
Findings outside the diff and other non-blocking context
Low (documentation) — cross-reference the separate daemon cache.docs/schema.md:570-573 — newly_exposed_by_change: this file is byte-identical in base and head, and storagePath was already create-and-run-only. The PR's new separate daemon store makes the reference's grouping of storagePath with session-sizing controls misleading for readers deciding which cache to warm. Link to the new daemon slot and isolated-provision warming instructions; this is a documentation gap in the new behavior, not a newly changed schema rule.
Low (testability) — isolate the daemon store used by integration tests.src/mxc-sdk/src/backends/wslc/common/store_slots.rs:53-59 — newly_exposed_by_change: the new root uses process LOCALAPPDATA, while the spawned-daemon tests override only MXC_WSLC_STATE_ROOT. Set a temporary LOCALAPPDATA for the child daemon and setup command so a live user's slot cache cannot influence the test. The environment can be overridden, so this is a test-fixture gap, not an untestable production design.
Low (performance) — avoid materializing the same inventory twice on cold start.src/mxc-sdk/src/backends/wslc/common/container_steps.rs:767-769 — introduced_by_change: the added access probe builds and discards an image list before image.rs:687-688 lists it again during first resolution. A non-allocating probe or reuse of the list would avoid one bounded cache traversal; no hot-path impact was measured.
Low (documentation) — distinguish an unset from an unusable application-data root.docs/backends/wslc/wslc-state-aware.md:149-150 — introduced_by_change: the new guide says fallback occurs if %LOCALAPPDATA% is 'unresolvable', but store_slots.rs:47-59 falls back only when the variable is absent or empty. The added banner in scripts/setup-wslc.ps1:123-124 always prints the %LOCALAPPDATA% root even when %TEMP% is actually chosen. Say 'unset or empty' and report the resolved root.
Low (staged design, not a required change).store_slots.rs:119-176 and src/tools/wxc/src/main.rs:139-150 — introduced_by_change: each present worker holds one session and therefore uses slot 0, while the pool and public 0..64 warming count prepare later multi-session PRs. The CLI already warns of per-slot disk cost. Consider whether this future-facing interface needs to ship in phase 1; I am not treating the choice itself as a blocker.
Verified clean, with receipts
The local 1d10040...cfc9fe6 diff matches gh pr diff 1475 after newline normalization (14 files, +1265/-144). The new create_daemon_session store probe rejects a held VHDX before returning a session. The new lifecycle test S provisions a daemon sandbox and checks create-and-run succeeds while it is live (tests/scripts/run_wslc_state_aware_tests.ps1:1866-1899). Pool unit tests and ignored host probes cover slot bookkeeping. I did not rerun the WSLc host tests for this review.
Verified pre-existing — not charged to this PR
docs/schema.md:570-573 is byte-identical across the range; its older wording is not itself an introduced edit. The requested cross-reference above is limited to the PR's new separate-store behavior.
The harness's warning-and-continue behavior predates this PR; the inline preflight comment concerns the newly added -DaemonSlots 1 requirement for isolated lifecycle fixtures.
The short 'lowest free' wording is qualified by the PR's new guide, which expressly limits lease tracking to one process and says an outside holder fails provisioning. I withdrew that original claim-mismatch finding. The existing Copilot comment already asks for better holder guidance, so it is not duplicated here.
The reason will be displayed to describe this comment to others. Learn more.
Medium (test coverage) — the new session/lease lifetime has no worker-level regression.
Attribution:newly_exposed_by_change — the worker's pre-existing direct SDK construction now must retain the new lease whenever it retains a session. The worker test count remains 54 across this PR. Pool tests cover bookkeeping, and store_access_failure has phase tests, but neither checks this worker assignment, Refused versus backend mapping, the three lease-forget branches (:1404, :1506, :1543), or normal release (:1550).
Fix: Add an injectable session opener or observable session-plus-lease test seam, then assert failure mapping, retention during abandonment and reuse after normal shutdown.
The reason will be displayed to describe this comment to others. Learn more.
Will be addressed in 2/4, which adds the seam you describe — worker_with_scratch_pool() points the worker at a scratch pool root — and takes the worker tests from 54 to 92. Lease retention versus release is asserted directly by a_shutdown_that_gives_up_on_a_boot_keeps_its_slot (slot stays held, not reissued) and a_shutdown_with_no_boot_running_returns_a_pending_slot (same slot re-leasable).
One part of your list deliberately has no test there: Refused versus backend mapping. 2/4 decouples leasing from session creation, so the lease closure becomes Infallible and that arm stops existing rather than going untested.
Use the configured fallback path in store diagnostics
scripts/setup-wslc.ps1:126
This diagnostic always names %LOCALAPPDATA%, but the new pool intentionally falls back to %TEMP%\mxc\wslc-store-slots when LOCALAPPDATA is unavailable. On that supported path the setup output points users at the wrong store; either resolve the same fallback here or describe the default slot pool without hard-coding a root.
This issue also appears on line 162 of the same file.
Spike PR 0 for AB#64133563. Four ignore-gated host probes that answer the
questions the multi-session store design rests on, so the design is settled
against a live WSL2 host rather than against assumptions.
Findings on the vendored SDK 2.9.9:
- Two sessions with separate stores coexist in one process on one MTA thread;
each stays usable while the other is live, and one survives the other's
release.
- A second session on a store another session already holds is NOT refused at
create time. The create succeeds and the violation surfaces only on first
use, as HRESULT 0x80070020.
- A released store keeps its cache: a store warmed by a real pull still lists
the image after the session is dropped and the store reopened.
- A freshly minted store starts empty, so a per-session store under the
default no-egress posture cannot resolve an uncached image.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 02c25756-75fe-4b3f-84be-6e5c972ad967
With the daemon binary staged but WSLc unavailable, setup returns nonzero and this exit prevents the backend_unavailable skip at lines 689–693. For example, an executor built without wslc now makes the harness fail instead of reporting the documented prerequisite skip. Check backend availability without opening a session before warming, or classify backend-unavailable setup failures separately. Keep actual cache-warming failures fatal.
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed the updated head 34187f5 against my requested changes. The create-and-run store no longer prevents independently warming daemon slots: warm_each attempts every target and reports failures rather than returning before the slot loop. Setup now opens each store once for all images, derives the path from StoreHint, shares StoredImage, and fails the isolated lifecycle test preflight promptly. The focused image unit tests passed locally (55 passed). I did not run live WSL2 host tests. The persistent-disk wording and worker lease tests are explicitly carried into stacked #1489, which I will review separately; approval here does not approve #1489 or its current red CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gudge (MGudgin)
left a comment
microsoft-github-policy-service
Bot
added
Needs-Author-Feedback
📖 Description
While any container-lifecycle sandbox was provisioned, every create-and-run WSLc execution on the same host failed with
WslcListSessionImages failed: HRESULT 0x80070020, and kept failing for five minutes after the sandbox was gone. The lifecycle daemon and the create-and-run runner had independently defaulted to the same image store,%TEMP%\mxc-wslc-sessions. That store is a VHDX, which admits one live session, and the daemon holds its session through the idle window.The daemon now leases its store from a pool instead:
It takes the lowest free slot when it boots a VM and returns it when the VM is released. Create-and-run execution keeps the store it always used, so neither surface can block the other.
%TEMP%\mxc\wslc-store-slotsis the fallback when%LOCALAPPDATA%is unresolvable — deliberately not the create-and-run default, so even the fallback cannot re-collide.Slots are leased rather than minted per session because a released store keeps its images while a fresh directory has none, and an uncached image under the isolated posture is refused rather than pulled. A per-session directory would leave every session cold and fail isolated provisioning outright. A slot directory is created only when first leased, so an unused index costs nothing.
🔗 References
Part of #1460. First of four PRs; fixes the store-sharing symptom on its own.
A multi-axis review of this branch raised 26 findings. Ten that nothing downstream addresses are fixed here; the rest are multi-session concerns that PRs 2–4 absorb.
🔍 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 --binsOn a live WSL2 host (Ubuntu + alpine-build, both v2):
cargo test -p mxc-sdk --features wslc --lib store_slots -- --ignored --test-threads=1tests\scripts\run_wslc_all_tests.ps1✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes📋 Issue Type
Microsoft Reviewers: Open in CodeFlow