Make WSLC an available backend on the Rust SDK - #687
Conversation
Expose the WSL Container backend through `mxc-sdk` for both run-to-completion (`run`) and streaming (`spawn_sandbox`), mirroring how the TypeScript SDK selects it with `createConfigFromPolicy(policy, 'wslc')`. `WSLContainerRunner` now implements both `ScriptRunner` and `SandboxBackend` over one shared `start_container` lifecycle, matching the Seatbelt / Bubblewrap / ProcessContainer backends. The two models differ only in where the WSLC SDK's output callbacks send their bytes, selected by a new `IoSink` (buffer, stream, or host stdio). The run-to-completion path used by `wxc-exec` is unchanged. Because the WSLC SDK delivers output via callbacks on its own threads rather than exposing pipe ends, streaming needs somewhere to park bytes until the caller reads them. `stream_buffer` provides that: an in-memory queue whose writer never blocks. Backpressure is deliberately absent — stalling a callback thread would also stall the exit callback that teardown waits on, deadlocking shutdown. Backend selection is explicit via `build_request_with_containment` and `Containment::Wslc(WslcSection)`; `build_request` still resolves the host's native backend. WSLC additionally requires `set_experimental(true)`, the library-side equivalent of `--experimental`. `platform_support` reports `wslc` only when the host can actually run it. Two limits follow from the WSLC SDK's surface: the container has no stdin (`take_stdin` returns `None`) and its process has no host pid (`id()` is `0`), so `kill()` stops the whole container. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98f4724a-151d-4e38-9367-52048ffcdb5d Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Exposes experimental WSLC through the Rust SDK for explicit run-to-completion and streaming execution.
Changes:
- Adds explicit containment selection and WSLC configuration APIs.
- Implements callback-backed streaming, lifecycle handling, and capability discovery.
- Adds tests and documentation for WSLC SDK usage.
Show a summary per file
| File | Description |
|---|---|
.github/copilot-instructions.md |
Documents the expanded architecture. |
docs/wsl/wsl-container-getting-started.md |
Adds Rust SDK guidance. |
src/backends/wslc/common/src/lib.rs |
Exports streaming and availability APIs. |
src/backends/wslc/common/src/sandbox.rs |
Implements streaming WSLC processes. |
src/backends/wslc/common/src/stream_buffer.rs |
Adds callback-to-reader buffering. |
src/backends/wslc/common/src/wsl_container_runner.rs |
Shares WSLC lifecycle across execution modes. |
src/backends/wslc/common/src/wslc_bindings.rs |
Adds runtime capability probing. |
src/core/mxc-sdk/Cargo.toml |
Adds the optional WSLC feature. |
src/core/mxc-sdk/README.md |
Documents WSLC support and limitations. |
src/core/mxc-sdk/src/lib.rs |
Re-exports WSLC APIs. |
src/core/mxc_engine/src/dispatch.rs |
Dispatches streaming WSLC requests. |
src/core/mxc_engine/src/lib.rs |
Exports containment configuration types. |
src/core/mxc_engine/src/platform.rs |
Reports available WSLC runtimes. |
src/core/mxc_engine/src/policy.rs |
Builds and validates WSLC requests. |
Review details
Comments suppressed due to low confidence (1)
src/backends/wslc/common/src/sandbox.rs:164
- After
try_wait()caches an exit code, this fast path returns without callingstarted.destroy()or settingtorn_down. Consequently a completed WSLC container/session remains allocated until theSandboxis dropped, even thoughwait()has reported completion. Only use the cached fast path after teardown; otherwise continue through the normal wait/cleanup path.
if let Some(code) = self.exit {
return Ok(code);
}
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Medium
…rtment Three fixes from CI and PR review on the WSLC Rust SDK change. `platform_support` only mutated `available_methods` under `#[cfg(feature = "wslc")]`, so every build without that feature tripped `unused_mut` — fatal under the `-D warnings` the Windows targets set, which broke the Hyperlight and MicroVM jobs. The WSLC probe now goes through a cfg'd `wslc_available()` that is simply `false` when the backend isn't compiled in, so the local is genuinely mutable in every configuration. `cancel_unblocks_a_parked_read_and_writes_never_block` was racy and failed on windows/arm64: the 256 KB write legitimately wakes the parked reader with data, so whether the reader observed bytes or the cancellation came down to which thread reacquired the mutex first. Split into two tests that each have exactly one wake source — a write-never-blocks test with no reader, and a cancellation test with no write. Behaviour under test is unchanged; returning available data to a parked reader was always correct. The streaming handle is `Send`, but `CoInitializeEx` is per-thread, so `wait`/`kill`/`Drop` could call the SDK from a thread that never entered the apartment `init_and_load_sdk` established. Every `StartedContainer` entry point that calls the SDK now joins the MTA for the duration via a `ComApartment` guard, mirroring `appcontainer_common`'s, and the `unsafe impl Send` comment no longer overstates what the callback threads prove. Also documents that `allowedHosts`/`blockedHosts` cannot be enforced on WSLC today (the container lacks `CAP_NET_ADMIN`, so the run fails at spawn), in both the `WslcSection` docs and the getting-started guide. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98f4724a-151d-4e38-9367-52048ffcdb5d Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
`try_wait` cached the exit code in `self.exit`, which `wait` treats as proof that it already drained the untaken streams, destroyed the container, and set `torn_down`. Polling `try_wait` to completion therefore made the subsequent `wait` return immediately and skip teardown entirely (only `Drop` still cleaned up), and it broke timeout idempotence: after a timed-out `wait`, a `try_wait` would populate `exit` so the next `wait` reported a normal exit instead of `TimedOut`. `try_wait` now re-queries the SDK instead of caching, leaving `exit` to be set only by `wait` once teardown has actually run, and `wait` checks the sticky `timed_out` flag before the cached code so a timeout can never be masked. Also widens `platform_support_windows_is_processcontainer`, which asserted `available_methods` was exactly `["processcontainer"]` and would fail on a WSLC-capable host once the backend is compiled in. It now pins ProcessContainer as the first entry and allows WSLC as the only other member, with a companion test asserting WSLC is never advertised without the feature. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98f4724a-151d-4e38-9367-52048ffcdb5d Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Dropping a caller-taken stdout/stderr reader left the stream with no consumer: `wait` only drains streams the caller did *not* take, so the SDK callback side kept appending to the deliberately unbounded queue for the life of the container, which an output-heavy sandbox could ride into host memory exhaustion. `StreamReader` now abandons the stream on drop — the same cancel-and-release the explicit `StreamCanceller` performs — so the writer stops queueing as soon as the last consumer is gone. `ComApartment::enter` treated every `CoInitializeEx` failure as though it were `RPC_E_CHANGED_MODE` and carried on, which contradicts the apartment guarantee the `unsafe impl Send` leans on. It now mirrors `appcontainer_common`'s guard: `S_OK`/`S_FALSE` are owned initializations, `RPC_E_CHANGED_MODE` reuses the thread's existing apartment, and any other HRESULT is an error. Call sites propagate it, `is_available` fails closed, and `destroy` — which also runs from `Drop`, where there is no error channel and leaking a live container is worse than a doomed SDK call — logs and attempts teardown regardless. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98f4724a-151d-4e38-9367-52048ffcdb5d Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
src/backends/wslc/common/src/wslc_bindings.rs:552
- This probe can advertise WSLC on an STA thread because
ComApartment::enteracceptsRPC_E_CHANGED_MODE, but the actual startup path still callsinit_and_load_sdk, which treats that same result as fatal. Consequentlyplatform_support()can report"wslc"and a subsequentrun/spawn_sandboxon the same thread immediately fails COM initialization. Make startup use the same apartment handling (with a guard spanning all startup SDK calls), or make the probe match the runner's real preflight.
let Ok(_com) = ComApartment::enter() else {
return false;
};
probe_components()
src/backends/wslc/common/src/sandbox.rs:215
- A successful terminal wait does not actually release the WSLC session:
destroy()only stops/deletes the container, whileWslcSessionGuard, the process/container handles, and the loaded DLL remain insideself.starteduntil the caller drops theSandbox. Applications that retain completed handles therefore retain the heavyweight WSL session indefinitely despitewait()reporting completion. Consume/releaseStartedContaineron the first terminal wait and keep only the cached outcome needed for repeated waits.
self.started.destroy(&mut self.logger);
self.torn_down = true;
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Medium
`SandboxBackend::spawn` bypassed `validate_common`, which every other backend's `spawn` applies and which WSLC's own run-to-completion path gets through `ScriptRunner::run`. So `spawn_sandbox` accepted a request the executor would reject — an empty command line — and, more importantly, skipped the central testing-features gate, letting `network.proxy.builtinTestServer` through without `--allow-testing-features` on that path alone. Applies `validate_common` and the backend's own `validate` hook before starting the container, matching Seatbelt, Bubblewrap, and ProcessContainer. The run-to-completion path is unaffected: it validates in `ScriptRunner::run` before reaching the shared `start_container`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98f4724a-151d-4e38-9367-52048ffcdb5d Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
`platform_support()` probes through `ComApartment`, which reuses an existing apartment on `RPC_E_CHANGED_MODE`, while `init_and_load_sdk` treated that same HRESULT as fatal. An STA caller was therefore told WSLC was available and then refused at spawn. The loader now goes through `ComApartment` too, so probe and spawn accept exactly the same thread states. The initialization stays deliberately unbalanced, now explicitly rather than by omission: the SDK and its callback threads outlive this call, so releasing the apartment on return could tear the MTA down under a running container. Tying it to `StartedContainer`'s cross-thread lifetime is the separate ownership follow-up. Also corrects the `image_tar_path` docs, which claimed the tar is imported instead of consulting the store. `resolve_image` is cache-first and skips the import when the image name is already present, so an updated tar under an existing name silently runs stale content — callers need a new name or a cleared store. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98f4724a-151d-4e38-9367-52048ffcdb5d Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Once `WslcStartContainer` succeeds the container is live and the SDK is delivering callbacks into the `Arc<IoContext>` handed over as a raw pointer. The two failure paths after that point returned directly, dropping the function's locals in reverse declaration order — freeing the callback context before the session was terminated and the DLL unloaded — so a late callback could dereference freed memory. This is a routine path rather than a corner case: `apply_iptables_rules` fails for any host-rule policy today, because the container is not granted `CAP_NET_ADMIN`. The post-start work moves into `attach_init_process`, so both failures funnel through one cleanup path, and `quiesce_started_container` stops the container, closes the streams, and blocks on the exit callback — the SDK's guarantee that no further callback can arrive — before the caller drops anything. Separately, `wait` closed both SDK-side streams before checking the wait result. `wait_for_exit` can fail while the process is still running (a failed apartment, or `WslcGetProcessExitEvent`), and closing then made caller-taken readers see EOF and discarded every later callback for a live process. Stream closing now happens only after a successful wait; the internally owned drain readers are still cancelled and joined on both paths, which cannot truncate a caller's stream because `cancel_and_join_discard` only fires for a stream this handle drained itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98f4724a-151d-4e38-9367-52048ffcdb5d Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
src/backends/wslc/common/src/wsl_container_runner.rs:1492
- If
wait_for_exitfails (for example,WslcGetProcessExitEventreturns an error), this returns while the container and its callbacks may still be live.StartedContainerhas noDrophandshake, so its fields are released without waiting for the exit callback—the condition this file identifies as necessary before reclaimingIoContextand unloading the DLL. Quiesce the started container on this error path before returning, as the post-start failures above already do.
let (exit_code, timed_out) = match started.wait_for_exit(logger) {
Ok(r) => r,
Err(e) => return e,
};
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Medium
`run_internal` returned directly when `wait_for_exit` failed, dropping `StartedContainer` while the container and its callbacks could still be live. Its fields free the `IoContext` the SDK writes through and then unload `wslcsdk.dll`, so a late callback could dereference freed memory — the same use-after-free class already fixed on the post-start error paths, on the remaining uncovered path. Consolidates that teardown into `StartedContainer::quiesce`: force-stop, close the streams, block on the exit callback (the SDK's guarantee that no further callback will arrive), then destroy. `run_internal`'s error path and `WslcSandboxProcess::drop` — which had the same sequence inline — now share it, so the invariant lives in one place. The now-redundant `StartedContainer::wait_for_exit_callback` wrapper is removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98f4724a-151d-4e38-9367-52048ffcdb5d Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
`mxc_sdk::Sandbox::wait` maps every `ErrorKind::TimedOut` onto `WaitOutcome::TimedOut` -- a *success* value whose contract is that the process and its whole tree were killed -- and discards the error message doing so. `try_wait`'s unconfirmed-kill error therefore arrived at the public API as exactly the claim it was written to avoid, with the "may still be running" wording dropped. `wait_with_output` had the same hole. Being unable to establish whether the sandbox is still running is a genuine failure rather than a timeout outcome, so it now uses `io::Error::other` and reaches the caller as `Err`. That needs no change to the public `WaitOutcome`, which keeps meaning precisely what it documents. The confirmed path still uses `ErrorKind::TimedOut`, where the promise holds. The test now asserts the kinds differ, and says why. Addresses review feedback on the wslc sandbox. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1cfac031-7051-4559-8c4e-9f76fa8605c2 Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Two conflicts, both resolved by keeping each side's intent: * src/backends/wslc/common/src/wsl_container_runner.rs -- both sides appended to `mod tests`, so both blocks are kept: this branch's teardown/pump tests and main's `wslc_prerequisite_error` tests (microsoft#656). * docs/wsl/wsl-container-getting-started.md -- takes main's clarified WSL runtime and WSLC SDK prerequisite wording, with this branch's note that the DLL sits next to *the running executable* (which is now the caller's own binary when using the Rust SDK), not only wxc-exec.exe. One deliberate deviation in that table: microsoft#656 landed the `Windows 11` row twice, and the merge offered to bring the duplicate along. The resolution keeps a single row -- adjacent identical rows are plainly accidental, and restating one here would make it look intentional. Flagging it since it is technically an upstream line this merge does not reproduce. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1cfac031-7051-4559-8c4e-9f76fa8605c2 Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
Callback waiting can leak captured output, and failed waits may be misreported as normal exits.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/backends/wslc/common/src/wsl_container_runner.rs:1098
- This interim wait already leaks an
Arcwhen the first 30-second callback wait expires, but the code then retries afterSIGKILL. If the callback arrives during that second wait, teardown is confirmed yet the first leaked reference permanently retains theIoContextand any captured stdout/stderr. Use the non-leaking predicate wait for this first probe and reserveawait_callbacks_quiescedfor the final failed attempt.
let mut confirmed = await_callbacks_quiesced(io_ctx);
src/core/mxc-sdk/src/lib.rs:57
- The public WSLC limitations should also disclose that stdout/stderr are backed by unbounded in-memory queues. A caller that takes a reader but keeps it alive without draining or dropping it can grow the host process until OOM; this behavior is currently documented only in the private
stream_buffermodule, so SDK consumers cannot discover the requirement.
//! WSLC is **experimental**: build with the crate's `wslc` feature, and call
//! [`SandboxRequest::set_experimental(true)`](SandboxRequest::set_experimental)
//! on the request. Its container has no stdin (the WSLC SDK exposes no
//! process-input API), so [`Sandbox::take_stdin`] returns `None` for it.
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
`wait_for_process` only special-cased `WAIT_TIMEOUT`, so every other `WaitForSingleObject` result fell through as a normal exit. `WAIT_FAILED` and `WAIT_ABANDONED` therefore reported `WaitOutcome::Exited` with whatever `WslcGetProcessExitCode` returned -- `STILL_ACTIVE` for a container that is still running. The same held when the SDK handed back no exit event at all: nothing was waited on, and the result was still read as an exit. The three wait results are now handled explicitly: `WAIT_OBJECT_0` records that the exit was signalled, `WAIT_TIMEOUT` takes the timeout path, and anything else fails with the wait result and `GetLastError`. Reporting an exit now also requires positive evidence that one happened -- the exit event signalling, or the SDK's exit callback confirming it. With neither (a null exit event and no callback) the run fails rather than inventing an exit code. Addresses review feedback on the wslc container runner. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1cfac031-7051-4559-8c4e-9f76fa8605c2 Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
Callback retry paths can permanently retain complete output buffers through premature repeated Arc leaks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
src/backends/wslc/common/src/wsl_container_runner.rs:270
- This helper leaks an
Arcas soon as any 30-second wait misses the callback, butwait_for_processdeliberately calls it once before SIGKILL and again afterward. If the callback arrives only after escalation, the first leaked reference remains forever; in capture mode it retains the complete stdout/stderr buffers, and repeated failed waits leak additional references. Separate the non-leaking predicate wait from the final “about to release the callback context” fallback, and onlyforgetone reference after the last confirmation attempt fails.
fn await_callbacks_quiesced(io_ctx: &Arc<IoContext>) -> bool {
if io_ctx.wait_for_exit_callback() {
return true;
}
std::mem::forget(Arc::clone(io_ctx));
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
One conflict, in .github/copilot-instructions.md: both sides rewrote the same four architecture bullets. Resolved per bullet so neither side's documentation is lost: * `mxc_engine` -- only this branch changed it (adding `build_request_with_containment`), so that wins. * `mxc-sdk` -- both changed it. Keeps this branch's `build_request_with_containment` / `Containment` / `WslcSection` re-exports and the WSLC streaming entry, plus main's `Output` warnings + structured output metadata and `output_metadata()`. * `wxc_common::sandbox_process` -- both changed it. Keeps this branch's WSLC `SandboxBackend` entry and main's `SandboxProcess::output_metadata()` sentence. * `mxc_ffi` -- only main changed it (output-metadata C string and `mxc_sandbox_output_metadata_json`), so that wins. No code change was needed for main's new `SandboxProcess::output_metadata`: it defaults to `None`, which is correct for WSLC -- captureDenials is a Windows ProcessContainer / Learning Mode feature and WSLC produces no structured outputs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1cfac031-7051-4559-8c4e-9f76fa8605c2 Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Human review recommended
The Windows-only FFI lifecycle remains untested on a WSLC host and contains an unresolved pump-thread leak on setup failures.
Review details
Suppressed comments (1)
src/backends/wslc/common/src/stream_buffer.rs:57
StreamWriterdoes not close the shared state when its producer is dropped. InOutputMode::Stream(StdioMode::Inherit),IoContext::newstarts two pump threads before several fallible SDK setup calls; any pre-start error drops these writers and detaches theJoinHandles, but each pump remains parked on the condvar forever because no one setsclosedor notifies it. Close the stream fromStreamWriter::dropso producer teardown always wakes its reader.
pub(crate) struct StreamWriter(Arc<Shared>);
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Human review recommended
The unexecuted Windows lifecycle still has unresolved COM-drop and callback-lifetime concerns.
Review details
Suppressed comments (4)
src/backends/wslc/common/src/wslc_bindings.rs:408
- This also proceeds with
WslcReleaseContainerwhenComApartment::enter()failed, so cross-thread drop can still invoke the SDK outside any COM apartment. Fail closed here (or centralize releases on an initialized worker) rather than violating the teardown precondition.
let _com = ComApartment::enter();
src/backends/wslc/common/src/wslc_bindings.rs:444
- As with the other guards, ignoring an
Errhere meansWslcReleaseProcessstill runs apartment-less, contradicting the invariant used to justify moving this handle across threads. Skip the release when COM cannot be entered, or perform it on a guaranteed initialized teardown thread.
let _com = ComApartment::enter();
src/backends/wslc/common/src/wsl_container_runner.rs:270
- This first failed callback wait leaks an
Arcimmediately, butwait_for_processthen waits again (and may observe the callback on that retry). In that successful-retry case the leaked reference is never recoverable, so every delayed callback permanently retains theIoContextand its potentially large capture/stream-buffer capacity. Separate “wait” from “make immortal”: only leak once the final confirmation attempt has failed and teardown is actually about to release the callback owner.
std::mem::forget(Arc::clone(io_ctx));
src/backends/wslc/common/src/wsl_container_runner.rs:1888
- This safety documentation is stale:
WslcSdk::shared()deliberately keeps the DLL loaded for the process lifetime, so teardown no longer unloads it. Reword this to focus on preventing callbacks from reaching a freedIoContext; otherwise the documented ownership argument contradicts the implementation.
/// The exit callback is the SDK's guarantee that no further callback will
/// arrive, which must hold before this container's fields drop: they free
/// the `IoContext` those callbacks write through and then unload the DLL.
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The handle guards' `Drop` impls entered the apartment but ignored the result, so a failed `CoInitializeEx` still ran the release calls apartment-less -- the exact condition the `Send` soundness argument and issue microsoft#721 exist to rule out. `StartedContainer::destroy` did the same, logging "attempting teardown anyway". Both now decline the SDK call instead: * The guards leak the handle. There is no error channel in `Drop`, and a leaked in-process handle is the strictly safer of the two failures. * `destroy` returns the failure, so a caller learns the container leaked rather than believing it was destroyed. Its previous reasoning -- that leaking a live VM beats a doomed SDK call -- assumed the apartment-less call would still work, which is exactly what cannot be relied on. The `Send` safety comment now states the invariant with no exception, since every path that reaches the SDK either holds an apartment or declines. Addresses review feedback on wslc_bindings.rs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1cfac031-7051-4559-8c4e-9f76fa8605c2 Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
Intermediate callback-state and pre-start pump lifecycle paths can leak memory and threads.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/backends/wslc/common/src/wsl_container_runner.rs:271
- This leaks the callback
Arcon an intermediate timeout, before teardown has actually given up. On the timeout path, the first 30-second wait can expire,SIGKILLcan then make the second wait succeed, but the first forgotten reference still keeps the entireIoContext(including captured stdout/stderr) alive for the rest of the process. Split “wait for callback” from “leak on final failure,” and only forget a reference after the last confirmation attempt has failed.
fn await_callbacks_quiesced(io_ctx: &Arc<IoContext>) -> bool {
if io_ctx.wait_for_exit_callback() {
return true;
}
std::mem::forget(Arc::clone(io_ctx));
false
src/backends/wslc/common/src/stream_buffer.rs:57
- Dropping the producer does not close the stream.
IoContext::new(Stream(Inherit))starts pump threads before the many fallible setup calls instart_container; if one of those calls fails before the container starts, the context and writer are dropped but each pump remains blocked inread()forever because neitherclosednorcancelledis set. Make writer drop signal EOF so pre-start failures cannot leak two threads.
pub(crate) struct StreamWriter(Arc<Shared>);
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Ran all the existing wslc config jsons to ensure that there is no regression. |
Integrates microsoft#687 (WSLC as an available backend on the Rust SDK), which landed on the same surface this branch touches. `policy.rs` conflicted in two places. Both were pure additions on each side inserted at the same point, so both were kept: - `CaptureDenialsMode`/`CaptureDenialsSection` (ours) alongside `Containment`/`WslcSection` (theirs). - The `captureDenials` tests (ours) alongside the WSLC containment tests (theirs). Each side's last item had lost its closing brace to a brace shared with the other side's, so those were restored during the union. Adding `SandboxPolicy::capture_denials` made three of microsoft#687's new exhaustive initializers incomplete; each gained `capture_denials: None`: the `minimal_policy` test helper, the `build_request_with_containment` doctest, and the `mxc-sdk` crate-level doctest. `captureDenials` is emitted only from `apply_host_process_backend`, so `Containment::Wslc` drops it the same way Linux and macOS do, rather than emitting a `processContainer` block the WSLC path would not read. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 18beca1b-3235-4884-bbaf-c0ee2371c8e4 Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
…equest PR #687 added a test asserting build_request_with_containment accepts WSLc allowedHosts without allowOutbound, believing WSLc enforces host rules container-side. This PR establishes that WSLc cannot enforce per-host egress filtering (no CAP_NET_ADMIN) and rejects it at parse time, so that premise is now false. Update the test to assert rejection and correct the stale comment in build_request_with_containment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f3ae1af2-7b79-4340-a5ce-a5402e7ede3d
📖 Description
Makes the WSL Container (WSLC) backend available through the public Rust SDK
(
mxc-sdk), for both run-to-completion (run) and streaming (spawn_sandbox).Previously WSLC was reachable only by driving the
wxc-execbinary; selecting itfrom the library returned
ErrorCode::UnsupportedContainment.The API mirrors how the TypeScript SDK selects the backend
(
createConfigFromPolicy(policy, 'wslc')+{ experimental: true }, then tweakconfig.experimental.wslc):How it works
WSLContainerRunnernow implements bothScriptRunnerandSandboxBackendover one shared
start_containerlifecycle — the same shape as the Seatbelt,Bubblewrap, and ProcessContainer backends. The two execution models differ only
in where the WSLC SDK's output callbacks send their bytes, selected by an
IoSink(an in-memory buffer for capture, or a non-blocking stream for thecaller — with host stdio being that same stream drained by a pump thread, so the
SDK's callback thread never performs the host's blocking I/O). The
run-to-completion path used by
wxc-execis unchanged.Because the WSLC SDK delivers output through callbacks on its own threads rather
than exposing the container's pipe ends, streaming needs somewhere to park bytes
until the caller reads them. The
stream_buffermodule provides that: anin-memory queue whose writer never blocks. Backpressure is deliberately absent
and documented — stalling a callback thread would also stall the exit callback
that teardown waits on, deadlocking shutdown. The queue is abandoned (and
released) as soon as its reader is cancelled or dropped, so an unread stream
cannot grow without bound.
Backend selection is explicit:
build_requeststill resolves the host'snative backend, and
build_request_with_containmenttakes aContainment.Auto-selecting WSLC would silently boot a VM and resolve a container image. WSLC
additionally requires
set_experimental(true)— the library-side equivalent of--experimental, kept fail-closed.platform_support()reports"wslc"onlywhen the host can actually run it.
Limitations
Two limits follow directly from the WSLC SDK's surface, documented in the crate
docs and the getting-started guide:
take_stdin()returnsNone.id()is0;kill()stops the whole container (the process-tree kill the trait requires).Separately,
allowedHosts/blockedHostscannot be enforced on WSLC today: thebackend applies them with in-container
iptablesand the container is notgranted
CAP_NET_ADMIN, so such a policy fails the run at spawn. That ispre-existing behaviour, now documented on
WslcSectionand in the guide ratherthan left as a surprise.
WSLC remains Windows-only and experimental, behind the crate's
wslcfeature.🔗 References
docs/wsl/wsl-container-getting-started.md(new Rust SDKsection),
src/core/mxc-sdk/README.md, and themxc-sdkcrate docs.🔍 Validation
Automated, all green:
cargo fmt --all -- --checkcargo clippy --target x86_64-pc-windows-msvcclean both with and withoutthe
wslcfeature (the feature-off configuration is what theWXC-Exec Hyperlight/MicroVMjobs build).cargo test -p mxc_engine -p mxc-sdk -p wxc_common— 471 tests pass, including7 new
mxc_enginetests (WSLC config mapping, defaults matching the TypeScriptSDK, the fail-closed experimental gate, parser rejection of invalid port
mappings, and the off-Windows / no-experimental dispatch rejections) and 7
stream_buffertests (EOF-on-close, partial reads, parked reads, cancellationreleasing buffered bytes, reader-drop abandoning the stream, and a large write
to a never-drained stream completing without blocking).
wslc_commontests added for the teardown hardening below: theSettledstate machine that keepswaitandtry_waitfrom disagreeing abouta timeout, both timeout error kinds, the confirmed vs. unconfirmed timeout
wording, the host-stdio pump loop, the per-mode I/O wiring, and the regression
the pump exists for — callback-path writes not blocking on a stalled host sink.
Teardown hardening (review follow-ups)
Review of the streaming lifecycle surfaced several related ways the backend
could report more than it had proven, all fixed here:
try_waitignored the sticky timeout. A timeout kills the container, sohas_exited()is true afterwards andtry_waitreturned the kill's exit code— contradicting the
Err(TimedOut)the same handle'swaithad just returned.Both now read one
Settledoutcome.WslcStopContainer'sHRESULT says the call was accepted, not that the process died.
wait_for_processnow escalates
SIGTERM→SIGKILLand reports a three-stateWaitOutcome, soan unconfirmed timeout says the container may still be running instead of
asserting it was killed.
StartedContainerhad noDrop. A panic unwinding past a startedcontainer freed the callback context while the SDK could still be calling
through it; it now quiesces unless teardown already ran.
Dropimpls didnot join the MTA, which the
Sendimpl's soundness argument assumed (reportedas [WSLC] RAII guard Drop impls call SDK teardown outside a COM apartment #721).
mxc_sdk::Sandbox::waitmaps everyErrorKind::TimedOutontoWaitOutcome::TimedOut— a success value documented as "the process tree waskilled" — discarding the message with it. The unconfirmed case now uses
io::Error::other, so it surfaces asErrand the publicWaitOutcomekeepsmeaning exactly what it documents.
WAIT_TIMEOUTwas special-cased.WAIT_FAILED,WAIT_ABANDONEDanda null exit event all fell through as a normal exit, reporting
STILL_ACTIVEfor a container that was still running. Every wait result is now handled
explicitly, and reporting an exit requires positive evidence of one — the exit
event signalling, or the SDK's exit callback.
mainhas been merged in, including the bindgen-generated WSLC FFI bindings(#669). That refactor and this branch both rewrote
wslc_bindings.rsandwsl_container_runner.rs, so the resolution keeps upstream's generated-bindingsfacade and re-applies this branch's additions on top, porting every SDK call site
to the generated API shape. One consequence is worth calling out: bindgen's
WslcSdkowns itsLibrary, so the "never unload the module" property now livesin
WslcSdk::shared()— a process-wideOnceLockinstance handed out as&'static. The DLL is loaded and symbol-checked once, never unloaded, andStartedContainerborrows it rather than owning it, which removes the SDK fromthat struct's drop-order reasoning entirely.
the WSLC runtime, and this was developed on macOS, where only cross-target
cargo check/clippyis possible. It needs a run on a WSLC-capable hostbefore merge. The existing
tests/configs/wslc_*.jsoncorpus exercises therun-to-completion path this change refactors, so that corpus is the natural
regression check.
That caveat is not theoretical. Because the Windows-only code could not be run,
it went through several rounds of independent review, which found ten defects —
all fixed here, and all but one in the streaming/teardown state machine:
WslcStartContainersucceeds, thetwo error returns dropped locals in reverse declaration order, freeing the
SDK's callback context before the session was terminated and the DLL unloaded.
Reached by any host-rules policy, since
apply_iptables_rulesalways failswithout
CAP_NET_ADMIN. Failures now quiesce the container and wait for theexit callback first — as does the run-to-completion wait-failure path, which
had the same hole.
waited for the callback but proceeded regardless, freeing the context and
unloading the DLL even though the "SDK has stopped calling back" guarantee had
not held. The module is now never unloaded, and a timed-out wait leaks one
Arcreference, so a late callback can touch neither freed memory norunmapped code.
WriteFile, so a full pipe with an undrained reader hung every teardown path.Replaced with the non-blocking in-memory stream.
Dropracing live callbacks intoFreeLibrary, without theexit-callback handshake.
wait_timeout, resultdiscarded) that silently reintroduced 2 and 3.
try_waitshort-circuited teardown by caching the exit codewaittreatsas proof it already ran, and masked a prior timeout.
waitonlydrains streams the caller did not take.
validate_commonwas skipped on the streaming path — which also bypassedthe central
--allow-testing-featuresgate onnetwork.proxy.builtinTestServer.RPC_E_CHANGED_MODE, so an STA caller wastold WSLC was available and then refused at spawn.
assertion a mutex coin flip) that failed on
windows / arm64.Known follow-up (pre-existing)
COM initialization is deliberately not balanced:
init_and_load_sdkenters theMTA and intentionally leaks that initialization, because the SDK and its callback
threads outlive the call, so releasing the apartment on return could tear the MTA
down under a running container. Correctly balancing it — and covering the
handles' own
Drop, which releases them outside an apartment — means tying theapartment to
StartedContainer's cross-thread lifetime viaManuallyDropor anowning worker thread. The same exposure exists on
maintoday (the previousrun_internalinitialized COM and never uninitialized), so this is left as afocused follow-up rather than widening this change.
Note this is now a refcount/apartment concern only: the memory-safety half — a
late callback reaching a freed context or an unloaded module — is closed above,
by never unloading
wslcsdk.dlland by leaking the callback context whenever theexit callback cannot be positively observed.
✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md) — n/a, no dependency changes📋 Issue Type