[WSLC] State-aware lifecycle (PR 1/3): persistent daemon + named-pipe IPC control plane - #745
[WSLC] State-aware lifecycle (PR 1/3): persistent daemon + named-pipe IPC control plane#745Soham Das (SohamDas2021) wants to merge 3 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Introduces the persistent per-user WSLc daemon and internal IPC foundation for the upcoming state-aware lifecycle.
Changes:
- Adds named-pipe protocol, daemon discovery, lifecycle worker, and client.
- Refactors WSLc operations into reusable container steps.
- Adds build integration and daemon IPC tests.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/Cargo.toml |
Registers the daemon crate. |
src/Cargo.lock |
Locks new crate dependencies. |
src/backends/wslc/daemon/tests/daemon_ipc.rs |
Tests pipe and lifecycle IPC. |
src/backends/wslc/daemon/src/session_manager.rs |
Owns WSLc sessions and containers. |
src/backends/wslc/daemon/src/main.rs |
Bootstraps and monitors the daemon. |
src/backends/wslc/daemon/src/control_server.rs |
Implements named-pipe request dispatch. |
src/backends/wslc/daemon/Cargo.toml |
Defines the daemon package. |
src/backends/wslc/daemon/build.rs |
Embeds executable version metadata. |
src/backends/wslc/common/src/wslc_bindings.rs |
Requires process-signalling support. |
src/backends/wslc/common/src/wsl_container_runner.rs |
Uses shared WSLc construction helpers. |
src/backends/wslc/common/src/lib.rs |
Exports daemon support modules. |
src/backends/wslc/common/src/daemon_record.rs |
Manages discovery and transition locking. |
src/backends/wslc/common/src/daemon_protocol.rs |
Defines IPC messages and framing. |
src/backends/wslc/common/src/daemon_client.rs |
Discovers and calls the daemon. |
src/backends/wslc/common/src/container_steps.rs |
Extracts reusable WSLc lifecycle operations. |
src/backends/wslc/common/Cargo.toml |
Adds protocol dependencies. |
build.bat |
Stages the daemon for the Node SDK. |
| let _ = reply.send(worker.start(config)); | ||
| } | ||
| WorkerCommand::Exec { config, reply } => { | ||
| let _ = reply.send(worker.exec(config)); |
There was a problem hiding this comment.
Will be addressed in PR2
| fn ok_or_err(result: Result<()>) -> DaemonResponse { | ||
| match result { | ||
| Ok(()) => DaemonResponse::Ok, | ||
| Err(e) => err_response(ErrKind::Backend, e), |
There was a problem hiding this comment.
Will be addressed in PR2
| write_frame(&mut pipe, &DaemonResponse::Ok).await?; | ||
| let terminal = match session.exec(config).await { |
There was a problem hiding this comment.
Will be addressed in PR2
| /// TODO(fill-in): bidirectional live stdio (client `Stdin` frames -> process, | ||
| /// process stdout/stderr -> `Stdout`/`Stderr` frames). The skeleton runs the | ||
| /// command to completion and emits only the terminal frame. |
There was a problem hiding this comment.
PR description updated. Doc change will be addressed in PR2
| if exist "!BIN_DIR!\wxc-wslc-daemon.exe" ( | ||
| copy /Y "!BIN_DIR!\wxc-wslc-daemon.exe" "sdk\node\bin\!SDK_ARCH!\" >nul | ||
| echo Copied !SDK_ARCH!\wxc-wslc-daemon.exe |
There was a problem hiding this comment.
Will be addressed in PR2
| "backends/lxc/common", | ||
| "backends/bubblewrap/common", | ||
| "backends/wslc/common", | ||
| "backends/wslc/daemon", |
There was a problem hiding this comment.
Will be addressed in PR2
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/backends/wslc/daemon/src/control_server.rs:260
- Exec is admitted before checking whether the sandbox exists or is started. Those failures consequently arrive as an untyped
StreamFrame::ErrorafterOk, rather than the documentedDaemonResponse::ErrwithNotProvisioned/NotStarted. Split admission/state validation from execution and sendOkonly after admission succeeds.
write_frame(&mut pipe, &DaemonResponse::Ok).await?;
let terminal = match session.exec(config).await {
Ok(code) => StreamFrame::Exit { code },
Err(e) => StreamFrame::Error {
src/backends/wslc/daemon/src/control_server.rs:273
- Every start/stop/deprovision failure is classified as
Backend, including the explicit unknown-sandbox and invalid-state paths. This makes the stableErrKind::NotProvisioned/NotStartedcontract unusable by the later state-aware adapter. Preserve a typed worker error and map each state failure to its declared protocol kind instead of erasing it intoanyhow.
fn ok_or_err(result: Result<()>) -> DaemonResponse {
match result {
Ok(()) => DaemonResponse::Ok,
Err(e) => err_response(ErrKind::Backend, e),
}
src/backends/wslc/daemon/src/main.rs:133
- The zero-client check is not coordinated with pipe acceptance, and the discovery record remains
readywhile shutdown begins. A phase can discover this daemon just after the check, then either lose its listening pipe or be accepted whilerun()shuts down the worker, causing a transient lifecycle failure. Make shutdown an atomic admission state (retire the record/listener under the transition protocol and drain admitted clients), or make the client rediscover/spawn after a stale-pipe open failure.
Ok(0) if active_clients.load(Ordering::SeqCst) == 0 => {
idle_for += IDLE_POLL;
if idle_for >= IDLE_TIMEOUT {
shutdown.notify_waiters();
return;
src/backends/wslc/daemon/src/session_manager.rs:331
ExecConfig::working_directoryis documented as an in-container path (and the protocol test uses/work), but this call reachesProcessSettings::build, which only accepts Windows drive paths viawindows_path_to_container_path. A normal container path therefore gets silently omitted. Add a daemon exec builder/path mode that passes the container cwd through verbatim while retaining Windows-path mapping for the one-shot runner.
&config.working_directory,
src/backends/wslc/daemon/src/session_manager.rs:344
- The PR describes an exec stdio bridge and the protocol promises stdout/stderr frames, but the captured buffers are discarded here, so
echo hireturns an emptyExecResult. Return the captured outcome through the worker and emitStdout/Stderrframes beforeExit; the IPC lifecycle test should assert the output rather than only the exit code.
// NOTE: outcome.stdout/stderr are captured but not yet forwarded — live
// stdio streaming over the control pipe is a later fill-in; the PR1
// contract returns the exit code only.
Ok(outcome.exit_code)
Address HIGH + MEDIUM findings from the multi-model review of the WSLc state-aware daemon (PR 1): - Fix a use-after-free of the SDK-owned Arc<IoContext>: reclaim the reference inside exit_callback (leak-on-kill rather than free while the SDK may still call back); drop IoCtxRawGuard. - Order Worker fields so the SDK/DLL drops last, after handle guards. - Rewrite deprovision to delete the container before removing bookkeeping, leaving the entry retryable on failure. - Add WslcSignalProcess to the required-symbol check. - Bind the control pipe before publishing a single ready record. - Handle pipe-accept errors locally instead of killing the daemon. - Gate the idle watchdog on no in-flight clients and no live containers. - Verify record + directory-chain ownership and validate the pipe name on the read path; harden every directory component, not just the leaf. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f3ae1af2-7b79-4340-a5ce-a5402e7ede3d
945d371 to
bd4b3a7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/backends/wslc/daemon/src/main.rs:132
- The zero-client check is not synchronized with the accept loop. A connection can be accepted immediately after this load, after which the watchdog still requests shutdown and
runtears down the worker while that provision/exec is in flight. Coordinate shutdown with the server: stop accepting, drain/recheck active requests and container count, then commit teardown atomically.
Ok(0) if active_clients.load(Ordering::SeqCst) == 0 => {
idle_for += IDLE_POLL;
if idle_for >= IDLE_TIMEOUT {
shutdown.notify_waiters();
src/backends/wslc/common/src/daemon_protocol.rs:106
- This contract says callers provide an in-container path, but
ProcessSettings::build_innerpasses the value throughwindows_path_to_container_path;/workis therefore ignored and the process runs in its default directory. The state-aware request also carries the existing host-sideworking_directory, so document the actual Windows-host-path contract.
/// Working directory inside the container (empty = container default).
#[serde(default)]
pub working_directory: String,
| if !*exited { | ||
| let _ = writeln!( | ||
| logger, | ||
| "[WSLC][daemon] Warning: exit callback did not fire within 30s" | ||
| ); |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
| // --------------------------------------------------------------------------- | ||
| // Shared error helper | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
thought: probably would do away with these kinds of comments since they take up space, no clue why the agent always does them. They do it to me too.
| } else { | ||
| format!("{}: {} (HRESULT 0x{:08X})", context, sdk_msg, hr as u32) | ||
| }; | ||
| ScriptResponse::error(&msg) |
There was a problem hiding this comment.
question: I thought for ScriptResponse errors now we try to encapsulate the error in an enum error type since this would help the rust sdk. Could be wrong though but that seemed like a good error pattern.
| /// SDK guarantees no I/O callback fires after exit, and the owning | ||
| /// [`ProcessSettings`] holds an independent reference, so the pointer stays valid | ||
| /// for every callback. The SDK guarantees `data` is valid for `data_size` bytes. | ||
| unsafe extern "C" fn io_callback( |
There was a problem hiding this comment.
issue: can we move all the unsafe functions to a wslc_sys.rs file under src/backends/wslc/common/src/ ? That would be helpful so we have all the unsafe functions in one place that we can review.
| _sh: Vec<u8>, | ||
| _dash_c: Vec<u8>, | ||
| _script_cstr: Vec<u8>, | ||
| _argv: Vec<PCSTR>, | ||
| _env_cstrings: Vec<Vec<u8>>, | ||
| _env_ptrs: Vec<PCSTR>, | ||
| _cwd_cstr: Option<Vec<u8>>, |
There was a problem hiding this comment.
question: why the need for underscores here? Is it because these fields won't be used in the PR but another one?
| // creation fails before a live process adopts it. Disarmed by | ||
| // `mark_process_created` once a process is created, after which | ||
| // `exit_callback` (or a deliberate leak on kill) owns reclamation. | ||
| sdk_io_ref: SdkIoRef, |
There was a problem hiding this comment.
thought: haven't read through the rest of this yet but seeing deliberate leak on kill makes me think we need a simple design doc (mostly diagrams, headings and brief text, nothing long form). That said maybe it'll make sense further down.
Branden Bonaby (bbonaby)
left a comment
There was a problem hiding this comment.
issue (blocking): wxc-wslc-daemon.exe is required beside wxc-exec.exe, but it is absent from the ADO signing/copy pattern, GitHub artifact upload, and playground resources. Could we add it to all three manifests so release builds sign and ship the daemon?
| /// to be: a process with its PID is currently running AND its creation time | ||
| /// matches the recorded one. | ||
| pub fn daemon_alive(record: &DaemonRecord) -> bool { | ||
| running_process_creation_time(record.pid) == Some(record.pid_creation_time) |
There was a problem hiding this comment.
praise: Pairing PID liveness with creation time and validating the record owner and pipe prefix is a strong fail-closed discovery design.
| name = "wxc-wslc-daemon" | ||
| path = "src/main.rs" | ||
|
|
||
| [dependencies] |
There was a problem hiding this comment.
issue: This workspace member imports Windows-only APIs unconditionally, so documented non-Windows workspace commands fail. We should target-gate the dependencies/code and provide a non-Windows stub main.
| match io_handle { | ||
| WslcProcessIOHandle::WSLC_PROCESS_IO_HANDLE_STDOUT => { | ||
| let mut buf = ctx.stdout.lock().unwrap_or_else(|e| e.into_inner()); | ||
| buf.extend_from_slice(bytes); |
There was a problem hiding this comment.
issue (blocking): stdout/stderr comes from untrusted code and grows these buffers without limit, so one exec can OOM the daemon and terminate every owned sandbox. Let's cap each stream or use bounded streaming/backpressure.
| windows::Win32::Foundation::HANDLE(exit_event), | ||
| wait_ms, | ||
| ); | ||
| if wait_result == windows::Win32::Foundation::WAIT_TIMEOUT { |
There was a problem hiding this comment.
issue (blocking): A timeout is reported even if SIGKILL fails, and unexpected waits or a missing exit callback still fall through to WslcGetProcessExitCode (possibly STILL_ACTIVE). Could we require positive exit confirmation and retire the warm container when termination cannot be confirmed?
| // Working directory (mapped Windows -> container path; skip if unmapped). | ||
| let mut cwd_cstr: Option<Vec<u8>> = None; | ||
| if !working_directory.is_empty() { | ||
| if let Some(container_cwd) = |
There was a problem hiding this comment.
issue: working_directory is an in-container path, but /work is treated as a Windows path and silently ignored. We can avoid that fallback by validating an absolute container path and passing it directly to the SDK.
| let session = session.clone(); | ||
| let active = active_clients.clone(); | ||
| active.fetch_add(1, Ordering::SeqCst); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
issue: These handlers are detached, so shutdown can tear down the worker while an accepted request is still running. We should retain the tasks, stop admission atomically, and drain them before worker shutdown.
|
|
||
| /// Service exactly one request on a freshly-connected pipe instance. | ||
| async fn handle_client(mut pipe: NamedPipeServer, session: SessionHandle) -> Result<()> { | ||
| let request: DaemonRequest = read_frame(&mut pipe).await?; |
There was a problem hiding this comment.
issue: The first frame has no deadline and each connection creates an unbounded task, so an abandoned same-user client can retain daemon resources indefinitely. Could we add a short request timeout and a bounded client semaphore?
| // Ensure the watchdog and worker are torn down. | ||
| shutdown.notify_waiters(); | ||
| watchdog.abort(); | ||
| let _ = session.shutdown().await; |
There was a problem hiding this comment.
issue: The record remains ready while the server stops and is removed outside the transition lock, so a client can select a daemon that is already disappearing. Let's mark it not-ready and serialize admission shutdown, draining, worker teardown, and record removal with the transition lock.
| let _ = self.child.wait(); | ||
| // The daemon was killed before it could clean up; drop the stale record | ||
| // so it does not confuse a later run. | ||
| let _ = remove_daemon_record(); |
There was a problem hiding this comment.
issue: This test uses and unconditionally deletes the real per-user daemon record, which can orphan a live daemon or race another test run. Could we use an isolated test state root and remove only the record owned by this child?
| use crate::daemon_record::{live_daemon, DaemonRecord, TransitionLock}; | ||
|
|
||
| /// Max wait to acquire the daemon spawn/teardown transition lock. | ||
| const SPAWN_LOCK_TIMEOUT: Duration = Duration::from_secs(30); |
There was a problem hiding this comment.
issue: This 30-second lock can expire before the 60-second readiness wait it protects, so a second client can fail while the first daemon is still within its valid startup window. We should make the lock timeout longer than READY_TIMEOUT.
📖 Description
First of three PRs delivering the state-aware WSLc lifecycle. The WSLc SDK (2.9.3) has no cross-process re-attach —
WslcSession/WslcContainer/WslcProcesshandles are in-process only — so a persistent, per-Windows-user daemonmust hold the handles across the separate
provision/start/exec/stop/deprovisionphase processes. This PR lands that daemon and its IPC control plane. It has no product-visible surface (nowire.rs/schema changes; nothing invokes the daemon yet), so it merges independently.What's included
wxc-wslc-daemoncrate (src/backends/wslc/daemon/):main.rs— bootstrap, bind-before-publish daemon record, dedicated WSLc worker thread/COM apartment, idle watchdog (teardown on no live containers + no in-flight clients).control_server.rs— owner-only-SDDL named pipe, length-prefixed JSON frame dispatch(
PROVISION/START/EXEC/STOP/DEPROVISION/PING).session_manager.rs— realWslcSession/container/process lifecycle overcontainer_steps, holding the refcountedsandbox_id -> WslcContainermap on a thread-affine worker.wslc_commonadditions:daemon_protocol.rs(control frames + internal per-phase config structs, deliberately separate from the public wire schema),daemon_record.rs(discovery record, pid+creation-time liveness, owner-only DACL hardening, transition lock),daemon_client.rs(spawn/poll, connect, exec result accumulation — live bidirectional stdio bridge deferred to PR 2).container_steps.rs: daemon-side WSLc step primitives (create session/container, exec, stop, delete) adapted for a warm keepalive-init session. The one-shotwsl_container_runner.rsis unchanged except two visibility relaxations so the daemon steps can reuse itsimport_image_from_tar+wslc_prerequisite_error.build.batstages the daemon exe next towxc-exec+ SDK bin.#[ignore]d (requires a WSL2 host withalpine:latestpre-pulled). Unit tests for framing, refcount, idle-teardown, transition lock, and record trust run by default.Coming in the pipeline
wslc/common/state_aware.rs(StatefulSandboxBackend, prefixwslc) translating the publicexperimental.wslc.*wire schema into daemon protocol frames;mxc_enginestate-aware arm + config-parser wiring; regenerated dev schema + generated TS wire types; multi-invocation E2E script with warm-reuse + idle-teardown assertions;Wslc*Config/*Resulttypes + brandedSandboxId<'wslc'>and helper prefix wiring, mirroring the LXC state-aware SDK surface.🔗 References
🔍 Validation
✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 Issue Type
GitHub Actions runs the PR validation build automatically. The ADO pipeline
(
MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity with the GitHubActions build; it runs on merge to
main, and Microsoft reviewers with write access can trigger iton a PR with
/azp run. See docs/pull-requests.md.If the
dependency-feed-checkcheck fails on a new dependency, the crate must be added tothe feed before the PR can pass. See docs/pull-requests.md
for the steps.
Microsoft Reviewers: Open in CodeFlow