[WSLC] (1/2) Run the state-aware daemon's exec off the single worker thread

[Soham Das (SohamDas2021)]

📖 Description

The WSLc daemon ran every SDK call on one apartment-affine worker thread, and exec held that thread for the entire run. While one container was executing, nothing else could be serviced — provisioning, starting, stopping or deprovisioning any other sandbox waited for it to finish.

Exec now runs on a thread of its own and reports back when it completes, so the worker stays responsive. A command naming a container whose run is still in flight waits for that run, because deleting the container would free a handle the run is using. Everything else proceeds immediately.

Nothing changes for a caller except ordering. The daemon still admits one exec at a time, an unknown or unstarted sandbox still fails before admission, and a timeout or cancellation still kills the process rather than the container. The one observable difference: commands are no longer globally ordered across sandboxes, so work finishing on one container no longer implies anything about another.

This is 1 of 2 changes needed to resolve the work item. It deliberately keeps the single-exec admission limit, so the new concurrency is not yet reachable by callers. The second raises that bound, reports a busy container to the caller, and proves the resulting concurrency end to end.

🔗 References

• ADO work item 63911810
• docs/wsl/wslc-state-aware.md updated: the daemon no longer runs all SDK calls on the worker, and the serialized-exec limitation is replaced with what actually remains.

🔍 Validation

• cargo test --workspace — 4861 passed, 0 failed
• cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --all -- --check — clean
• WSL2 host (not reachable from CI): the three #[ignore] daemon tests, both daemon_ipc pipe tests, and the full E2E suite — one-shot 35/35, state-aware 83/83
• New coverage for the teardown and failure paths this introduces: parking, parked-work release, cancellation before a run starts, quarantine on an unconfirmed run, shutdown delivering a finished exec's real exit code, and the in-flight drain. A WSL2-host test overlaps a deprovision with a live exec; disabling the parking makes it fail with the container deleted mid-run.

✅ Checklist

• Signed the Contributor License Agreement
• Linked to an issue
• Updated documentation (if applicable)
• Updated Copilot instructions (if build, architecture, or conventions changed)
• If this PR changes Cargo.lock, the dependency-feed-check check passes (see docs/pull-requests.md)

📋 Issue Type

• Bug fix
• Feature
• Task

Microsoft Reviewers: Open in CodeFlow

[Copilot]

Copilot review overview

🟡 Changes recommended

Shutdown can discard queued non-exec commands and lose a completed exec’s real result.

Review effort: Balanced
Findings: 1

Open (1)

• Drain loop discards queued commands before ExecFinished reports · New

What changed in this PR

Moves WSLc state-aware exec execution off the apartment worker while preserving container-handle safety.

Changes:

• Runs execs on dedicated MTA threads.
• Parks same-container lifecycle work until exec completion.
• Adds shutdown handling, tests, and updated documentation.

File	Description
session_manager.rs	Adds threaded exec lifecycle and parking.
control_server.rs	Updates admission documentation.
wslc-state-aware.md	Documents concurrency and ordering changes.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Shutdown can discard queued commands and lose a completed exec’s real result.

Review effort: Balanced
Findings: 1

Open (1)

• Drain loop discards queued commands before ExecFinished reports

[Copilot]

Copilot review overview

🟡 Changes recommended

Panic cleanup leaks reply senders, potentially leaving callers blocked indefinitely.

Review effort: Balanced
Findings: 1

Open (1)

• Worker panic leaks pending replies and stalls callers · New

Resolved since last review (1)

• Drain loop discards queued commands before ExecFinished reports

[Copilot]

Copilot review overview

🔵 Needs a closer look

The new unsafe cross-thread FFI handle sharing and teardown lifetime guarantees warrant final human review despite comprehensive tests.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Worker panic leaks pending replies and stalls callers

[Copilot]

Copilot review overview

🔵 Needs a closer look

Cross-thread raw FFI handle ownership and timeout teardown require final human validation.

Review effort: Balanced
Findings: None

[Copilot]

Copilot review overview

🔵 Needs a closer look

The unsafe cross-thread SDK handle lifetime and shutdown behavior warrant final human review despite comprehensive coverage.

Review effort: Balanced
Findings: None

[Gudge (MGudgin)]

Review summary

Requesting changes for the one High finding: after a client disconnects, its handler releases the sole exec permit while the newly off-worker run can continue. Another exec can then start on a different container, contrary to the intended one-active-exec limit. Please bind capacity to confirmed run termination, handle premature client exit, and test that sequence. The other findings below are non-blocking individually.

What I checked: The PR base 13d899c is the merge-base of head 7ce1c8e, and the local three-file diff matches gh pr diff after line-ending normalization. The module grows from 25 to 42 #[test]/#[tokio::test] markers. Same-container parking, finished-run replies, and a WSL2-only same-container deprovision test are present. The permit acquisition and handler ownership in control_server.rs are unchanged; it is the new off-worker exec path that removes the old serialization backstop. This was a source-level review; I did not rerun the WSL2 suites.

Claim-only finding (non-blocking)

Medium (performance) — Same-container waiters can exhaust the client reserve. claim_mismatch: the PR description says other work proceeds immediately, but the new parked same-container requests keep their handlers waiting. With one attached exec and eight such waiters, all nine pre-existing client permits are occupied, so another sandbox's request (or cancellation) cannot get a handler until capacity frees. This qualifies the claim under saturation; it is not a new unbounded queue or a merge condition. Consider bounding parked waiters, reserving a cancellation lane, or qualifying the description.

Verified pre-existing — not attributed to this PR

• The pipe-handler-scoped exec permit existed in the base; the High finding is attributed to the PR only because moving exec off the worker makes its early release capable of admitting another active run.
• The global client capacity and the transition-lock teardown order predate the PR. I am not treating the previously suggested lock-held shutdown delay or pull-on-worker-panic concern as a new regression.

The accompanying inline comments contain the remaining verified findings, including the two safety-test gaps and a non-blocking documentation clarification.

[Gudge (MGudgin)]

Medium (test coverage) — Test exec overlapping another container's real SDK lifecycle.

Attribution: newly_exposed_by_change — this PR moves exec to an MTA thread while the worker can concurrently provision/start/stop/deprovision another container against the shared WSLc session.

The comment describes a manual two-exec measurement. The committed ignored WSL2 test deprovision_during_an_exec_waits_for_the_run checks only same-container parking, not the cross-container SDK calls made live by this PR. Admission is sent before the run thread starts, so an admission reply alone does not prove an overlap occurred.

Fix: Add an ignored WSL2 test that waits for a ready signal from exec on A, completes B's lifecycle before A exits, then verifies A's output and exit status. Multi-exec admission can remain deferred.

[Soham Das (SohamDas2021)]

Agreed - deferring to PR 2, which builds the concurrency E2E harness this needs anyway.

[Gudge (MGudgin)]

Low (test coverage) — Multiple parked commands' replay order lacks a test.

Attribution: introduced_by_change — this new loop redispatches every command parked behind the finished run, but the existing tests park one command at a time.

For a parked [Exec, Stop], the next Exec must take the container claim and the Stop must re-park behind it, rather than stop the newly running process.

Fix: Exercise that sequence through a controllable exec runner or in an ignored WSL2 test; assert the Stop remains pending until the second Exec finishes.

[Soham Das (SohamDas2021)]

Agreed - deferring to PR 2; needs the injectable exec runner that work introduces.

[Copilot]

🔵 Needs a closer look

The unsafe cross-thread SDK and raw-handle lifetime changes warrant final human validation despite strong coverage.

0 open findings

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Cancellation, permit lifetime, and panic cleanup contain unresolved concurrency and resource-safety defects.

2 open findings

• Cancel admitted execs when handler futures are dropped · New
• Prevent SDK/session release while image pull is still active · New

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[Gudge (MGudgin)]

Meant to approve #1396 :-(