Skip to content

Add streaming and state-aware lifecycle to the C ABI and .NET SDK#651

Open
shschaefer wants to merge 11 commits into
mainfrom
user/stscha/ffi-streaming-stateaware
Open

Add streaming and state-aware lifecycle to the C ABI and .NET SDK#651
shschaefer wants to merge 11 commits into
mainfrom
user/stscha/ffi-streaming-stateaware

Conversation

@shschaefer

@shschaefer shschaefer commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

📖 Description

Extends the mxc_ffi C ABI and the Microsoft.Mxc.Sdk .NET binding beyond run-to-completion to cover live streaming and the state-aware sandbox lifecycle — the deferred "Phase 8" of the Rust-SDK/FFI work (follow-up to #635 and #645). The lower layers (mxc-sdk streaming, mxc_engine state-aware dispatch) already existed; this surfaces them through the C ABI and the .NET SDK.

Streaming

  • mxc_ffi: opaque-handle C ABI — mxc_spawn returns an MxcSandbox*; separate MxcReadStream* / MxcWriteStream* handles (take_stdin/stdout/stderr) allow reading stdout, reading stderr, and writing stdin concurrently; mxc_stream_read/write/flush; mxc_sandbox_id/try_wait/wait/kill/free.
  • .NET: MxcSandbox.Spawn(policy, command)MxcSandboxProcess with Stream-based StandardInput/Output/Error, Wait/WaitAsync/WaitBlocking/WaitForExitWithOutputAsync, Kill.

State-aware lifecycle

  • mxc-sdk: run_state_aware_json (envelope phases) + exec_sandbox (live exec → Sandbox).
  • mxc_ffi: mxc_state_aware (provision/start/stop/deprovision) + mxc_state_aware_exec (live exec, reusing the streaming handle).
  • .NET: MxcLifecycleProvisionSandbox/StartSandbox/ExecInSandbox/ExecInSandboxAsync/StopSandbox/DeprovisionSandbox, with a typed SandboxId.
  • Backend-agnostic plumbing: wxc_common::state_aware_dispatch::dispatch_state_aware_exec returns the backend's ExecHandle instead of relaying to the executor's stdio, and exec_stream::ExecSandboxProcess adapts it to the streaming trait (duplicating pipe handles so it never shares ownership with the backend).

The generated C# P/Invoke (NativeMethods.g.cs) is regenerated from the three FFI source files; the MXC_STATUS_* space already reserved the state-aware phase codes, so no renumbering.

Scope / known limitations

  • The only in-tree state-aware backend, IsolationSession, is Windows-only, experimental, and needs its OS-side service. It still relays exec internally (returns null pipe handles), so live-exec streaming for that backend is a deliberate follow-up; the FFI/SDK/.NET plumbing is complete and backend-agnostic (validated with a fake backend over real OS pipes).
  • The C ABI remains not a stable external contract — native + binding are co-versioned and generated together.

🔗 References

🔍 Validation

  • Rust: cargo build --workspace, cargo fmt/clippy -D warnings; unit + integration tests (FFI, exec_stream incl. real std::io::pipe round trips, engine, mxc-sdk state-aware). Real streaming round-trips run via BaseContainer locally.
  • .NET: 30/30 tests pass (host-independent, and with MXC_E2E_HOST_PREPPED=1 for the real streaming/lifecycle paths — kill, stderr, WaitAsync cancellation, and a >64KB-both-streams deadlock guard, plus a dispose-during-read UAF regression).
  • All CI gates green: check-dotnet-bindings-codegen, check-dotnet-errorcode-parity, check-version-sync, check-schema-*, validate-configs.

⚠️ Real streaming/lifecycle behavior on Windows runs only on a host-prepped lane (MXC_E2E_HOST_PREPPED=1), matching the existing #[ignore] ProcessContainer tests; the macOS Seatbelt lane runs its streaming suite un-gated. CI YAML/MSBuild changes are verified by inspection until the first pipeline run.

✅ Checklist

📋 Issue Type

  • Bug fix
  • Feature
  • Task
Microsoft Reviewers: Open in CodeFlow

Stuart Schaefer added 10 commits July 14, 2026 12:13
Expose a live, handle-based streaming surface over mxc_sdk::spawn_sandbox
alongside the existing run-to-completion mxc_run:

- mxc_spawn returns an opaque MxcSandbox handle; separate MxcReadStream /
  MxcWriteStream handles (take_stdin/stdout/stderr) allow reading stdout,
  reading stderr, and writing stdin concurrently on different threads.
- Stream I/O: mxc_stream_read/write/flush. Process control:
  mxc_sandbox_id/try_wait/wait/kill. Destructors: mxc_sandbox_free (kills
  the child tree), mxc_read_stream_free, mxc_write_stream_free.
- Every entry point is catch_unwind-wrapped and null-argument guarded.
- Add src/streaming.rs to the csbindgen inputs so the C# P/Invoke layer
  (NativeMethods.g.cs) regenerates with the new externs.

Covered by null/error unit tests plus an ignored real echo streaming
round-trip; bindings-drift and errorcode-parity gates pass.
Expose MxcSandbox.Spawn(policy, command) returning a live
MxcSandboxProcess over the streaming C ABI, alongside the existing
run-to-completion Run/RunAsync:

- StandardInput/StandardOutput/StandardError are ordinary Streams (no
  pty); each wraps a separate native stream handle so stdout, stderr and
  stdin can be driven concurrently on different threads.
- Wait() drains any untaken standard stream (mirroring the Rust Sandbox)
  so a caller that ignores output cannot deadlock on a full pipe, and
  uses try_wait polling so Kill() stays responsive from another thread.
  WaitBlocking() reports a policy timeout distinctly; WaitAsync() awaits
  exit; WaitForExitWithOutputAsync() reads both streams while waiting.
- Kill() terminates the child tree; Dispose() frees handles (killing the
  child if still running) and is idempotent.
- Native opaque MxcSandbox struct is aliased to avoid colliding with the
  public MxcSandbox class.

Sample streams the command live; xUnit covers arg/error paths always and
a real streaming round-trip gated on MXC_E2E_HOST_PREPPED. errorcode
parity and version-sync gates pass.
Expose the state-aware lifecycle through mxc-sdk so language bindings can
drive it without the executor binary:

- mxc-sdk::run_state_aware_json(requestJson, dryRun) -> envelope JSON for
  the provision/start/stop/deprovision phases (and a dry run of any
  phase); a non-dry-run exec is rejected in favour of the streaming path.
- mxc-sdk::exec_sandbox(requestJson) -> a live streaming Sandbox for the
  exec phase, the same handle spawn_sandbox returns.

Plumbing added beneath it:
- wxc_common::state_aware_dispatch::dispatch_state_aware_exec returns the
  backend's ExecHandle for the exec phase instead of relaying it to the
  executor's stdio (the run-to-completion relay path is unchanged).
- wxc_common::exec_stream::ExecSandboxProcess adapts an ExecHandle to the
  SandboxProcess streaming trait: it wraps non-null agent pipe handles as
  owned readers/writers and runs the waiter on a background thread so exit
  is observable via both wait() and try_wait(); kill() drives the
  terminator. A null-pipe handle (the shape IsolationSession returns,
  since it relays internally) simply exposes no streams.
- mxc_engine gains exec_state_aware(_json) and run_state_aware_json, which
  parse the request JSON and route to the resolved backend.

Note: IsolationSession still relays exec internally (returns null pipes);
exposing real pipes for live exec is a separate follow-up. Host-
independent tests cover parse/routing/error mapping; the ExecHandle
adapter is unit-tested with fake waiters. fmt + clippy clean.
Expose the state-aware lifecycle over mxc_ffi, on top of the SDK's
run_state_aware_json / exec_sandbox:

- mxc_state_aware(requestJson, dryRun, out) fills an MxcStateAwareResult
  (status + owned response_json/error C strings) for the envelope phases
  (provision/start/stop/deprovision, and a dry run of any phase); paired
  with mxc_state_aware_result_free. Uses the phase status codes already
  reserved in the MXC_STATUS_* space.
- mxc_state_aware_exec(requestJson, out_handle, out_error) runs the exec
  phase as a live streaming process, returning the same opaque MxcSandbox
  handle as mxc_spawn so the caller reuses the mxc_stream_* / mxc_sandbox_*
  externs to read/write/wait/kill.

Add src/state_aware.rs to the csbindgen inputs so NativeMethods.g.cs
regenerates with the three new externs. Null/error-path unit tests cover
both entry points; bindings-drift and errorcode-parity gates pass.
Expose MxcLifecycle over the state-aware C ABI, mirroring the Node SDK:

- ProvisionSandbox / StartSandbox / StopSandbox / DeprovisionSandbox drive
  the envelope phases via mxc_state_aware; ProvisionSandbox returns a typed
  SandboxId (with optional filesystem policy and Entra user credentials).
- ExecInSandbox runs a command as a live streaming MxcSandboxProcess via
  mxc_state_aware_exec; ExecInSandboxAsync runs it to completion, draining
  stdout/stderr, and returns a RunResult.
- Requests are built as the wire envelope (version 0.6.0-alpha, phase,
  containment/sandboxId, cross-cutting filesystem lifted to top level,
  backend-specific config nested under experimental.isolation_session);
  responses parse the result envelope. Errors map to the typed
  MxcException/ErrorCode.

The sample now demonstrates the lifecycle (reporting gracefully when the
IsolationSession backend is unavailable). Host-independent xUnit tests
cover envelope routing, SandboxId, and error mapping through the real
native round trip; errorcode-parity and version-sync gates pass.
Update the docs to describe the streaming and state-aware lifecycle
additions to the C ABI and the SDKs:

- mxc_ffi crate docs: list the three surfaces (run-to-completion,
  streaming, state-aware) and drop the stale 'planned additions' note.
- mxc-sdk README: add a state-aware lifecycle section
  (run_state_aware_json / exec_sandbox).
- copilot-instructions: describe the mxc_ffi streaming + state-aware
  externs and the .NET SDK's Spawn / MxcLifecycle surface (no longer
  run-to-completion only).
The stream wrappers freed their native handle without synchronising with
in-flight reads/writes, and MxcSandboxProcess.Dispose freed the streams
without awaiting the background drain tasks reading them, so a dispose
racing a read (directly reachable via WaitForExitWithOutputAsync on
cancellation, or via the internal drain tasks) hit a use-after-free /
double-free at the FFI boundary.

- Back each native pointer with a SafeHandle (MxcSandboxHandle,
  MxcReadStreamHandle, MxcWriteStreamHandle). Stream Read/Write/Flush hold
  a DangerousAddRef reference across the native call, so the free is
  deferred until every in-flight I/O has returned — no lock held across
  the blocking read, so no deadlock.
- Dispose now frees the sandbox handle first (killing the child, which
  EOFs any blocked reader), then awaits the drain tasks, then releases the
  stream handles.
- SafeHandle finalizers also reclaim the native handles (and kill the
  child) if Dispose is skipped.
- Document the free-during-read UB contract in the mxc_ffi streaming docs.

Adds a gated regression test that disposes the process while a background
read is parked in the native read. All 21 C# tests pass (host-independent
and with MXC_E2E_HOST_PREPPED=1).
Address the medium-severity review findings:

- ExecSandboxProcess now duplicates each non-null pipe handle
  (try_clone_to_owned) and wraps the duplicate, so it never shares
  ownership with the backend's process object — no double-close even for a
  future backend that keeps and closes its own pipe ends.
- ExecInSandboxAsync offloads the (possibly blocking) exec-start P/Invoke
  to the thread pool so it never blocks the caller's thread.
- Fix a real wire-format bug: MxcLifecycle's JSON options lacked
  PropertyNamingPolicy=CamelCase, so the provision user bundle serialized
  as Upn/WamToken instead of upn/wamToken.
- Fix the GenerateNativeBindings target Inputs to include streaming.rs and
  state_aware.rs so a change there re-triggers binding regeneration.

Adds tests: real-pipe stdout/stdin round trips through ExecSandboxProcess
(via std::io::pipe), a C# stdin write-streaming round trip, and
MxcLifecycle envelope-JSON builder tests (exposed via InternalsVisibleTo).
All 25 C# tests pass (default and gated); fmt + clippy + gates clean.
- Add wxc_common::config_parser::load_mxc_request_from_json and use it in
  the engine's state-aware parse, dropping the base64 round-trip that only
  existed to dodge the file-path branch.
- Back off the .NET wait poll interval (1ms doubling to a 50ms cap) instead
  of a fixed 5ms, so a long-running child does not spin a core.
- Document that MxcSandboxProcess.Id is 0 for a state-aware exec handle.
- Extract the shared out-handle/out-error spawn tail into
  streaming::finish_spawn, used by both mxc_spawn and mxc_state_aware_exec.

fmt + clippy + all gates clean; 186 config_parser, 6 SDK state-aware, and
25 .NET tests pass.
… cancellation)

Fill the streaming-pathway gaps the review surfaced — previously the FFI
and C# streaming paths tested read+wait and stdin write, but not kill,
stderr, output-heavy deadlock avoidance, or WaitAsync cancellation.

C# (gated on MXC_E2E_HOST_PREPPED):
- Kill_TerminatesRunningChild / Kill_DuringWaitAsync_Unblocks: kill a
  genuinely-running child (blocked on stdin) and confirm Wait returns,
  including a kill from another thread during WaitAsync.
- WaitAsync_Cancellation_AbandonsWithoutKilling: cancelling the wait
  throws without killing (the path that previously triggered the UAF).
- StandardError_IsReadable: stderr is captured distinctly from stdout.
- OutputHeavyChild_DoesNotDeadlock: >64KB on BOTH stdout and stderr is
  returned in full, proving the concurrent-drain deadlock contract.

FFI Rust (ignored, real-host):
- real_stdin_write_stdout_read_roundtrip proves mxc_stream_write directly.
- real_kill_terminates_blocked_child proves mxc_sandbox_kill +
  try_wait/wait directly.

Uses cmd builtins (set /p, for /L, echo 1>&2) so the commands run under
the sandbox's invalid cwd on an un-prepped host. All 30 C# tests pass; the
new FFI real tests pass via BaseContainer; fmt + clippy clean.
Copilot AI review requested due to automatic review settings July 15, 2026 07:47
@shschaefer
shschaefer requested a review from a team as a code owner July 15, 2026 07:47
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends MXC’s language-binding surfaces by adding a streaming (handle-based) C ABI and exposing the state-aware sandbox lifecycle through mxc_ffi and the .NET SDK, building on existing mxc-sdk streaming and mxc_engine lifecycle dispatch.

Changes:

  • Adds a new mxc_ffi streaming handle API (mxc_spawn + mxc_stream_* + mxc_sandbox_*) and state-aware lifecycle entry points (mxc_state_aware, mxc_state_aware_exec).
  • Adds a wxc_common adapter (exec_stream::ExecSandboxProcess) and a dispatch helper to return ExecHandle for live exec streaming without stdio relaying.
  • Adds .NET SDK support for streaming (MxcSandbox.SpawnMxcSandboxProcess) and lifecycle (MxcLifecycle + typed SandboxId), plus tests/docs updates.
Show a summary per file
File Description
src/ffi/mxc_ffi/src/streaming.rs New streaming/handle-based C ABI for spawn + stream I/O + process control.
src/ffi/mxc_ffi/src/state_aware.rs New state-aware lifecycle C ABI (envelope + streaming exec).
src/ffi/mxc_ffi/src/lib.rs Wires streaming + state-aware modules into the crate’s public surface.
src/ffi/mxc_ffi/build.rs Adds new FFI modules to csbindgen inputs + rerun-if-changed.
src/core/wxc_common/src/state_aware_dispatch.rs Adds dispatch_state_aware_exec that returns an ExecHandle for streaming.
src/core/wxc_common/src/state_aware_backend.rs Adds cross-platform null pipe-handle sentinel helper.
src/core/wxc_common/src/lib.rs Exposes the new exec_stream module.
src/core/wxc_common/src/exec_stream.rs New adapter that wraps a state-aware ExecHandle as a streaming SandboxProcess.
src/core/wxc_common/src/config_parser.rs Adds raw-JSON parse entry point and refactors shared parsing core.
src/core/mxc-sdk/tests/state_aware.rs Host-independent tests for run_state_aware_json / exec_sandbox routing + errors.
src/core/mxc-sdk/src/lib.rs Exposes run_state_aware_json and exec_sandbox from the public Rust SDK.
src/core/mxc-sdk/README.md Documents state-aware lifecycle usage from mxc-sdk.
src/core/mxc_engine/src/state_aware.rs Adds JSON-based state-aware parsing + streaming exec entry point.
src/core/mxc_engine/src/lib.rs Re-exports run_state_aware_json + exec_state_aware_json.
sdk/dotnet/README.md Documents new .NET streaming + lifecycle surfaces.
sdk/dotnet/Microsoft.Mxc.Sdk/StateAwareTypes.cs Adds lifecycle option/result types (user creds, provision/start options).
sdk/dotnet/Microsoft.Mxc.Sdk/SandboxWaitResult.cs Adds wait result type for streaming waits.
sdk/dotnet/Microsoft.Mxc.Sdk/SandboxId.cs Adds typed sandbox identifier wrapper for lifecycle APIs.
sdk/dotnet/Microsoft.Mxc.Sdk/Native/SafeHandles.cs Adds SafeHandle wrappers for sandbox + stream native handles.
sdk/dotnet/Microsoft.Mxc.Sdk/MxcSandboxProcess.cs Implements streaming process wrapper with drain/wait/kill semantics.
sdk/dotnet/Microsoft.Mxc.Sdk/MxcSandbox.cs Adds Spawn() API to create MxcSandboxProcess via FFI.
sdk/dotnet/Microsoft.Mxc.Sdk/MxcLifecycle.cs Adds lifecycle APIs (provision/start/exec/stop/deprovision) over FFI.
sdk/dotnet/Microsoft.Mxc.Sdk/Microsoft.Mxc.Sdk.csproj Updates binding-generation inputs; exposes internals to test project.
sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcSandboxProcessTests.cs Adds streaming process tests (host-gated where needed).
sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcLifecycleTests.cs Adds lifecycle facade contract tests (routing + envelope shape).
sdk/dotnet/Microsoft.Mxc.Sdk.Sample/Program.cs Demonstrates run-to-completion + streaming + lifecycle in the sample.
.github/copilot-instructions.md Updates repo instructions to reflect new FFI/.NET streaming + lifecycle capabilities.

Review details

  • Files reviewed: 27/27 changed files
  • Comments generated: 3
  • Review effort level: Low

Comment on lines +53 to +56
var result = RunEnvelopePhase(BuildProvisionEnvelope(options));
var sandboxId = result?["sandboxId"]?.GetValue<string>()
?? throw new MxcException(ErrorCode.BackendError, "provision response carried no sandboxId");
var metadata = result["metadata"];
Comment on lines +426 to +429
if (offset < 0 || count < 0 || offset + count > buffer.Length)
{
throw new ArgumentOutOfRangeException(nameof(count));
}
Comment on lines +500 to +503
if (offset < 0 || count < 0 || offset + count > buffer.Length)
{
throw new ArgumentOutOfRangeException(nameof(count));
}
The Windows CI build tests the workspace with --features isolation_session,
which via cargo feature unification enables mxc_engine/isolation_session for
mxc-sdk and mxc_ffi too. The provision-without-backend tests hard-coded
UnsupportedPhase (the feature-off fallback), but with the feature on the real
IsolationSession backend is reached: on the CI runner it returns
BackendUnavailable (no OS-side service), and on a host that HAS the service it
actually succeeds and provisions a real sandbox as a side effect.

Replace the isolation_session provision assertions with a deterministic,
feature- and host-independent routing check: a non-provision phase with an
unregistered sandbox-id prefix always resolves to unsupported_containment
before any feature-gated backend arm, with no backend side effects. Provision
envelope construction is already covered separately by the BuildProvisionEnvelope
tests (no native call).

Verified both mxc-sdk and mxc_ffi tests pass with isolation_session on and off;
C# 30/30 pass; fmt + clippy clean.
let status = catch_unwind(AssertUnwindSafe(|| {
// SAFETY: `stream` non-null live handle; `buf`/`cap` describe a valid
// writable region per the caller contract.
let s = unsafe { &mut *stream };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let s = unsafe { &mut *stream };

This line takes an exclusive Rust borrow of the stream (&mut *stream), and the
same pattern is in mxc_stream_write (line 348) and mxc_stream_flush (line 373).
Rust requires a &mut to be exclusive for its whole lifetime, so if two calls into
this function run against the same stream handle at once, there are two live &mut
to one MxcReadStream, which is undefined behaviour (not merely interleaved bytes).

The C# binding does not enforce that exclusivity. MxcReadPipeStream.Read
(MxcSandboxProcess.cs:447) and MxcStdinStream.Write (MxcSandboxProcess.cs:518)
guard the native call only with SafeHandle.DangerousAddRef / DangerousRelease.
DangerousAddRef is a refcount that stops the handle from being freed while a call
is in flight; it is not a mutex and gives no mutual exclusion. Two threads can
both AddRef and both be inside mxc_stream_read on the same handle simultaneously.
The earlier use-after-free fix closed the free-vs-read race but left the
read-vs-read / write-vs-write race open.

This is reachable two ways:

  1. A caller reads the same stdout/stderr stream from two threads. Here that is undefined behaviour, not just a logic bug.
  2. Without any caller mistake: Wait()/WaitAsync starts an internal drain task on
    stdout/stderr and stores that stream, and a later StandardOutput / StandardError
    access hands the same stream back to the caller. The internal drain and the
    caller's read then run on one handle concurrently.

Also note the module doc above claims the C# binding enforces the single-owner
contract by refcounting the handle via SafeHandle. That is true for free ordering
only, not for concurrent read/write, so the documented safety contract is not
actually upheld.

Suggested fix:

  • Add a per-handle lock around the native read/write/flush call on the C# side,
    held only across the P/Invoke (no lock held during unrelated work, so no
    deadlock). This makes the boundary sound even under caller misuse.
  • Fix the stream state machine so a stream being drained internally by
    Wait()/WaitAsync is never handed back out via StandardOutput / StandardError
    (track untaken vs caller-owned vs internally-draining). This removes the
    SDK-induced concurrent-access path.
  • Update the streaming.rs module doc so the stated single-owner contract matches
    what the binding actually guarantees.

Refs: streaming.rs:311/348/373 (the &mut *stream borrows);
MxcSandboxProcess.cs:447 (MxcReadPipeStream.Read), :518 (MxcStdinStream.Write),
and the Wait()/WaitAsync drain-then-hand-out path.

ThrowIfDisposed();
unsafe
{
status = NativeMethods.mxc_sandbox_try_wait(_handle.Ptr, &exit, &running);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on lines 199-203 assumes the policy timeout is enforced natively
("killing the tree, which try_wait then sees as an exit"). That isn't true for
the spawn_sandbox path: nothing kills the child at the deadline unless
mxc_sandbox_wait is called, and WaitCore only ever calls mxc_sandbox_try_wait
(line 186). So Wait()/WaitAsync()/WaitForExitWithOutputAsync() never enforce
SandboxPolicy.TimeoutMs and can hang indefinitely.

The deadline+kill live only in SandboxProcess::wait (WaitForSingleObject(process,
timeout_ms) then kill on WAIT_TIMEOUT: appcontainer_runner.rs:1405-1416,
base_container_runner.rs:1422-1433; wait_with_timeout on bwrap/seatbelt:
bwrap_runner.rs:445). try_wait is a pure exited/still-running check with no
deadline (appcontainer_runner.rs:1378, base_container_runner.rs:1389), and
spawn_sandbox starts no watchdog (mxc-sdk/src/lib.rs:96) -- so on the polling
path the deadline is never consulted.

Worked example:
var policy = new SandboxPolicy { TimeoutMs = 5000 };
using var proc = MxcSandbox.Spawn(policy, "ping -n 61 127.0.0.1"); // ~60s
var result = proc.Wait(); // caller expects a return in ~5s
WaitCore loops on mxc_sandbox_try_wait, which reports "still running" the whole
time; at t=5s nothing kills the child, so Wait() returns only when the command
exits on its own (~60s) with TimedOut=false, or never returns if it never exits.
The sandbox tree stays alive the entire time.

The same policy is honored by WaitBlocking() (mxc_sandbox_wait -> kill at 5s ->
TimedOut=true), but that holds _controlLock for the whole blocking wait so it
can't be interrupted by a concurrent Kill() (lines 213-215). So no wait API today
gives both timeout enforcement and kill-responsiveness, and Wait() contradicts
its own summary on lines 153-155 ("honouring the policy's TimeoutMs").

Suggested fix: keep the try_wait poll loop for kill-responsiveness, but enforce
the deadline in managed code -- thread TimeoutMs into MxcSandboxProcess and, once
the deadline passes in the WaitCore loop, call mxc_sandbox_kill and return. That
stays lock-free across the native call, so it doesn't reintroduce the
WaitBlocking kill-deadlock. Update the comment on lines 199-203 and the Wait()
summary on lines 153-155 accordingly.

Refs: MxcSandboxProcess.cs:159 (Wait) / :166 (WaitAsync) / :169 (WaitCore) / :186
(try_wait poll) / :196 (TimedOut=false) / :199-203 / :217-236 (WaitBlocking);
backends appcontainer_runner.rs:1378,1405-1416,
base_container_runner.rs:1389,1422-1433, bwrap_runner.rs:445; spawn_sandbox
mxc-sdk/src/lib.rs:96.

// not run detached. The stream handles are refcounted, so this is
// safe regardless of when the caller disposes.
await Task.WhenAll(SwallowAsync(stdoutTask), SwallowAsync(stderrTask))
.ConfigureAwait(false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On cancellation this catch awaits the two reader tasks before rethrowing, but
those reads can never complete, so the wait deadlocks and the child is never
killed.

The readers (stdoutTask/stderrTask, started at lines 253-254) are parked in the
blocking mxc_stream_read on the child's stdout/stderr. MxcReadPipeStream has no
cancellable read -- ReadToEndAsync uses CopyToAsync(ms, ct) (line 288), but the
token only gates between reads; a read already in flight in native code is not
interruptible. For a still-running child that hasn't closed its pipes, those
reads never return, so Task.WhenAll on line 267 blocks forever.

The only thing that would unblock them is disposing/killing the child (frees the
handle -> kills the tree -> EOFs the pipes). But in ExecInSandboxAsync the dispose
is in the finally (MxcLifecycle.cs:173), which is unreachable because this catch
never returns. So the code blocks awaiting the readers before it can run the one
step that would release them -- a permanent deadlock. Net effect:
ExecInSandboxAsync hangs, and the native sandbox container + handles leak (never
disposed or deprovisioned).

The comment on lines 264-266 ("the stream handles are refcounted, so this is
safe regardless of when the caller disposes") is about a different property:
refcounting prevents a use-after-free crash, not the deadlock. The code is
memory-safe and permanently hung at the same time.

Worked example:
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(5));
var r = await MxcLifecycle.ExecInSandboxAsync(id, "ping -n 600 127.0.0.1",
cts.Token);
// t=5s: WaitAsync throws OCE -> this catch awaits the parked reads -> hangs
// forever; ExecInSandboxAsync's finally/Dispose never runs; sandbox leaks.

Suggested fix: on cancellation/failure, kill (or dispose) the child before
awaiting the readers, so the pipes EOF and the reads return. e.g. in this catch,
call Kill() (mxc_sandbox_kill) first, then await Task.WhenAll(...), then throw;
or restructure ExecInSandboxAsync to Kill/Dispose on cancellation before the
await unwinds. Optionally also give MxcReadPipeStream a genuinely cancellable
read so the token can abandon a parked read without killing. Update the comment
on lines 264-266 to speak to liveness, not just refcount safety.

Refs: MxcSandboxProcess.cs:244 (WaitForExitWithOutputAsync) / :253-254 (reader
tasks) / :262-269 (catch) / :285-290 (ReadToEndAsync, non-cancellable native
read); MxcLifecycle.cs:148 (ExecInSandboxAsync) / :160 (await) / :171-174
(finally/Dispose).

envelope["sandboxId"] = id.Value;
if (options?.Size is { } size)
{
SetBackendConfig(envelope, "start", "size", size);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetBackendConfig(envelope, "start", "size", size);

This emits the size under experimental.isolationSession.start.size, but the Rust
wire model never reads that key, so the value is silently dropped.

The start phase deserializes into IsolationSessionPhase (wire.rs), whose only
fields are configuration_id ("configurationId") and user -- there is no "size"
field and no #[serde(alias = "size")]. The experimental block is intentionally
permissive (no deny_unknown_fields), so "size" is an unknown field that serde
silently ignores; configuration_id stays None and the backend falls back to its
default (Composable). The one-shot path and every other phase use configurationId,
so "start" is the only place using the wrong key.

Note the field name is misleading: configurationId is not an opaque id -- it is
typed as the sizing-profile enum IsolationConfigurationId
(small/medium/large/composable), the same value space StartSandboxOptions.Size
carries. So value-wise Size maps directly onto configurationId; only the wire key
differs. The configurationId-vs-Size naming mismatch is almost certainly what led
to emitting "size" here.

Effect: every StartSandboxOptions.Size value, including valid ones like "medium"
or "large", is discarded. The size profile is never applied and no error or
warning is raised.

Also, the XML doc on StartSandboxOptions.Size (StateAwareTypes.cs:37-40) says
"Unknown values are warned and downgraded to composable on the native side."
That does not happen: the value never reaches the native validator (wrong key),
and configurationId is a typed enum, so an unknown value under the correct key
would be a deserialization error, not a warn-and-downgrade. The doc describes
behavior the code doesn't implement.

Fix: emit "configurationId" instead of "size" (SetBackendConfig(envelope,
"start", "configurationId", size)), and add a test asserting a non-default size
reaches IsolationSessionConfig.configuration_id (e.g. start with Size="large" and
assert the parsed phase config is Large, not the Composable default). To make the
intuitive key also work and prevent a recurrence, consider adding
#[serde(alias = "size")] to configuration_id, or renaming the wire field to
size/sizeProfile so the C# Size and the wire key line up. Either way, reconcile
the StartSandboxOptions.Size doc with the actual enum-validation behavior.

Refs: MxcLifecycle.cs:88-90 (emits "size"); wire.rs IsolationSessionPhase
(configuration_id / "configurationId", typed as IsolationConfigurationId
size enum; no size field; experimental block has no deny_unknown_fields);
StateAwareTypes.cs:37-41 (Size doc + string type).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants