diff --git a/docs/isolation-session/state-aware-rust.md b/docs/isolation-session/state-aware-rust.md index a2aeab68a..598c9ac95 100644 --- a/docs/isolation-session/state-aware-rust.md +++ b/docs/isolation-session/state-aware-rust.md @@ -448,7 +448,13 @@ State-aware exec (and other phases) use OS-level cancellation in v1: - The SDK kills the `wxc-exec` process via process termination. - The agent process's pipes EOF, the relay threads exit. - The OS-side service's per-process timer (set from - `process.timeout`) reaps the agent if the runner does not. + `process.timeout`) reaps the agent if the runner does not. On the + streaming path that timer is armed with a **margin** past the caller's + deadline, so it acts purely as a watchdog: the service kills with an + ordinary exit code (the host suite pins it as exit code 1), so if it + fired first a genuine timeout would be indistinguishable from a normal + exit and could not be reported as one. The run-to-completion path arms + it unchanged — it has no timeout channel to report through. - The runner's existing 3-tier shutdown (`CloseStandardInput` → `SendCtrlClose` → `Terminate`) handles the timeout case from inside the agent process before returning. @@ -461,14 +467,50 @@ without waiting and returns a terminator that calls `IsoSessionProcess::Terminate()`, alongside the real pipe handles and a waiter that blocks on exit. -That terminator reports **nothing**: `ExecHandle.terminator` is an -infallible `FnOnce()`, so the backend requests termination and discards -whether the platform even accepted it. The underlying -`StartedProcess::terminate` does distinguish an accepted request from a -rejected one — it is the handle type that has nowhere to carry the -answer. Teardown is also not bounded: the streaming adapter's `Drop` -joins the waiter unconditionally, and nothing in that path caps the -wait. A process that survives the kill can park that waiter by either of +That terminator now reports whether the platform **accepted** the kill: +`ExecHandle.terminator` returns a `Result`, and the answer +`StartedProcess::terminate` already computed reaches the caller through +`SandboxProcess::kill`. What it still cannot say is whether the process +actually died — that would need the bounded post-kill wait's result, +which the handle type does not carry. + +The waiter likewise distinguishes a timeout from an exit, returning +`ExecOutcome::TimedOut` when the deadline elapsed while the process was +running. Neither available signal proves that on its own: `WaitForExit` +answers `-1` on timeout and `ExitCode()` reads `STILL_ACTIVE` (259), and +both are legal exit codes for untrusted code. Their conjunction +establishes whether the process was still running when it was sampled, +which is what the ladder decision needs. + +A spent deadline is **sticky**. The two reads are not atomic, so a +process can exit in the window between them; that still reports +`TimedOut`, because the sentinel proves the deadline elapsed while the +process ran and a later-observed exit code does not un-spend it. +Reporting that code would hide a missed deadline from a caller who asked +for one. The sibling WSLc backend draws the same line, tracking +`deadline_elapsed` separately from `timed_out`. The one irreducible case +is `-1`/`-1`, where the sentinel and the exit code collide and nothing +distinguishes them, so it is read as the exit. + +`TimedOut` promises the process is **gone**, not that MXC killed it, and +it reaches only the foreground process: `IsoSessionProcess` exposes +`Terminate` and `ExitCode` and no tree primitive, so a descendant the +workload backgrounded outlives a reported timeout on either path and is +reclaimed when the session is stopped and deprovisioned. + +An exited process is never routed through the shutdown ladder, which reads only `ExitCode()` +and so cannot tell a `259` exit from a live process. The adapter maps +`TimedOut` onto `ErrorKind::TimedOut`, which is what +`mxc_sdk::Sandbox::wait` reads as `WaitOutcome::TimedOut`. That outcome +is reachable only for an in-process consumer: the executor has no +timeout field in `ScriptResponse` to report one through, so the executor +arm keeps reporting `Exited`. + +Teardown is bounded only insofar as the kill is: the streaming adapter's +`Drop` joins the waiter when the kill was accepted, and abandons the +thread when it was refused rather than blocking forever in a `Drop` the +caller cannot opt out of. A process that survives an *accepted* kill can +still park that waiter by either of two routes — in its leading `WaitForExit` (INFINITE when the caller supplied no timeout), or, when a timeout was supplied, in the graceful ladder's tier 3, which is `Terminate` followed by an INFINITE @@ -476,8 +518,13 @@ ladder's tier 3, which is `Terminate` followed by an INFINITE exits on stdin EOF once the caller has dropped its own duplicate of the write end, and tier 3's `Terminate` is a fresh attempt that may land where the first did not. The narrow claim is that nothing bounds the -join if the process does survive. Making the terminator fallible end to -end, and bounding that join, is future work. +join if the process does survive. The terminator is now fallible end to +end, so a *refused* kill is reported and the join is skipped; bounding +the join after an *accepted* kill that never took effect is future work, +and needs the backend to confirm the process actually died. A **confirmed +exit** retires the terminator entirely: once the waiter has reported an +outcome there is nothing to kill, so `kill` succeeds without running the +terminator and an earlier refusal no longer applies. ## Known issues diff --git a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md index 4ef623850..7310d333f 100644 --- a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md +++ b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md @@ -1228,6 +1228,15 @@ pub trait StatefulSandboxBackend { /// stderr may therefore arrive merged into stdout, leaving /// `ExecHandle::stderr` null. A backend that probes the host to decide how /// to wire stdio must confine that probe to the `Executor` case. + /// + /// A backend that cannot serve `Library` at all — because it relays the + /// workload's output to the *host process's* own stdio rather than + /// returning streams — must refuse **before running anything**. The + /// workload is arbitrary and may not be idempotent, so a refusal issued + /// after the fact reports "unsupported" for something that has already + /// taken effect and whose output has already gone somewhere the caller + /// never asked for. `wxc_common::state_aware_backend::unsupported_library_exec` + /// is the shared refusal. fn exec( &mut self, sandbox_id: &str, @@ -1331,10 +1340,25 @@ pub struct ExecHandle { /// Stdin pipe handle. Not consumed by the executor relay, which forwards /// no input; the streaming path hands it to an in-process caller. pub stdin: PipeHandle, - /// Function to wait for exit; returns the exit code. - pub waiter: Box Result + Send>, - /// Function to terminate the process (called on AbortSignal). - pub terminator: Box, + /// Function to wait for exit; returns how the exec finished. + pub waiter: Box Result + Send>, + /// Function to terminate the process (called on AbortSignal). Fallible: a + /// platform that refuses the request must be able to say so, because a + /// caller that assumes a refused kill succeeded can block forever waiting + /// on a process that is still running. + pub terminator: Box Result<(), MxcError> + Send>, +} + +/// How an exec finished, as distinct from why a wait failed. A timeout is an +/// outcome — the deadline was spent while the process ran, and the process is +/// no longer running — whereas `Err` means the exit could not be determined. +/// Deliberately not "the backend killed it": a workload that overruns its +/// deadline and then exits on its own has still missed it, and how far the +/// termination reaches is the backend's to state. Only `ExecConsumer::Library` +/// can observe `TimedOut`; the executor relay has no field to carry it. +pub enum ExecOutcome { + Exited(i32), + TimedOut, } ``` diff --git a/src/backends/isolation_session/common/src/manager.rs b/src/backends/isolation_session/common/src/manager.rs index 26888141d..641116780 100644 --- a/src/backends/isolation_session/common/src/manager.rs +++ b/src/backends/isolation_session/common/src/manager.rs @@ -7,6 +7,7 @@ //! against the local console. use wxc_common::process_util::OwnedHandle; +use wxc_common::state_aware_backend::ExecOutcome; use isolation_session_bindings::bindings::{ IsoSessionOps, IsoSessionProcess, IsoSessionProcessResult, IsoSessionUserResult, @@ -560,15 +561,126 @@ impl StartedProcess { /// degrades to the full 5s + 3s stall followed by tier 3's hard terminate. /// Making tier 1 work unconditionally needs an owned or transferable stdin /// on the handle type, which is tracked separately. - pub(super) fn wait(&self, timeout_ms: u32) -> Result { + /// + /// # Telling a timeout from an exit + /// + /// Reported as [`ExecOutcome::TimedOut`] when the deadline elapsed with the + /// process still running, and [`ExecOutcome::Exited`] otherwise. + /// + /// **Neither available signal is sound alone**, because each is also a legal + /// exit code: `WaitForExit` answers `-1` on timeout, and a timed-out + /// process still reads [`STILL_ACTIVE`] (259) from `ExitCode()`. A workload + /// is untrusted code and can return either value at will. + /// + /// Their *conjunction* establishes **whether the process was still running + /// when it was sampled**, which is what the ladder decision needs. A process + /// that exited cannot have exited with both values at once, so requiring + /// both pins the live case: + /// + /// | Case | `WaitForExit` | `ExitCode()` | Verdict | + /// |---|---|---|---| + /// | still running | `-1` | `259` | `TimedOut` (after the ladder confirms it died) | + /// | exited after the deadline | `-1` | `7` | `TimedOut` (already gone; no ladder) | + /// | ambiguous | `-1` | `-1` | `Exited(-1)` | + /// | exited `259` | `259` | `259` | `Exited(259)` | + /// + /// # A spent deadline is sticky + /// + /// The two reads are not atomic, so a process can exit in the window between + /// them. That still reports [`ExecOutcome::TimedOut`]: the sentinel proves + /// the deadline elapsed while the process was running, and a later-observed + /// exit code does not un-spend it. Reporting that code instead would hide a + /// missed deadline from a caller who asked for one. + /// + /// `TimedOut` promises the process is **gone**, not that this code killed + /// it — a process that died on its own satisfies that just as a killed one + /// does. The sibling WSLc backend draws the same line, tracking + /// `deadline_elapsed` separately from `timed_out` so a spent deadline is + /// reported "stickily rather than a later-observed exit code", and treating + /// an already-confirmed exit as satisfying its termination check. + /// + /// The one irreducible case is `-1`/`-1`, where the sentinel and the exit + /// code collide: nothing distinguishes a timeout from a workload that + /// exited with `-1`, so it is read as the exit. + /// + /// # How far "gone" reaches here + /// + /// Both timeout paths confirm **the foreground process only**, because that + /// is the only thing this API exposes: `IsoSessionProcess` has `Terminate` + /// and `ExitCode` for the process it represents and no tree primitive. A + /// descendant the workload backgrounded can therefore outlive a reported + /// timeout on either path — the live one, which checks `ExitCode()` after + /// the ladder, and the late-exit one, which has nothing left to terminate. + /// + /// That is not a gap this layer can close: descendants live in the isolation + /// session's own user context and are reclaimed when the session is stopped + /// and deprovisioned. Stated rather than implied, because + /// [`ExecOutcome::TimedOut`] leaves the reach to the backend and the SDK's + /// `WaitOutcome::TimedOut` describes process-tree backends where the kill + /// does cover the tree. + /// + /// A zero `timeout_ms` short-circuits the whole test: it means INFINITE, and + /// a wait with no deadline cannot have missed one. This is the default, so + /// without the guard the common configuration would be the one at risk. + /// + /// # Why the ladder does not run for an exit + /// + /// The ladder exists to kill a **survivor**. Running it for a process that + /// already exited reintroduces the very ambiguity this function avoids: its + /// first `ExitCode()` reads 259 for a workload that exited with 259, so it + /// walks all three tiers and cannot then tell that exit from a live + /// process. An exited process therefore returns its code directly. + pub(super) fn wait(&self, timeout_ms: u32) -> Result { // `WaitForExit` is a Win32 `WaitForSingleObject` on a kernel handle — // no COM round-trip. On timeout it returns -1; the ladder below then // decides what to do about a process that is still running. - let _ = self + let waited = self .process .WaitForExit(timeout_ms) .map_err(|e| transport_err(op::RUN_PROCESS, "WaitForExit failed", &e))?; - wait_with_graceful_shutdown(&self.process) + + // Sampled before the ladder, which kills a survivor and so destroys the + // evidence. `?`-propagated for the same reason the ladder propagates its + // first query: a failure here means the kernel handle is broken, and + // guessing "it exited" would report a fabricated outcome. + let plan = plan_wait(timeout_ms, waited, || { + self.process + .ExitCode() + .map_err(|e| transport_err(op::RUN_PROCESS, "get ExitCode failed", &e)) + })?; + + // An exited process never reaches the ladder: the ladder cannot tell a + // 259 exit from a live process, which is the ambiguity `plan_wait` + // exists to resolve while the evidence is still intact. + match plan { + WaitPlan::Exited(exit_code) => return Ok(ExecOutcome::Exited(exit_code)), + // The deadline was spent and the process is already gone, so there + // is nothing to kill and nothing to confirm — the sample that + // produced this plan is the confirmation. + WaitPlan::TimedOutAfterDeadline => return Ok(ExecOutcome::TimedOut), + WaitPlan::KillThenReportTimeout => {} + } + + // Timed out: the process was still running, so it must be dead before + // this reports `TimedOut`, which promises exactly that. + wait_with_graceful_shutdown(&self.process)?; + let after = self + .process + .ExitCode() + .map_err(|e| transport_err(op::RUN_PROCESS, "get ExitCode failed", &e))?; + if after == STILL_ACTIVE { + // Unlike the general case, 259 is not ambiguous here: the sample + // above established the process was running, so this is the same + // process still running rather than a stale exit code. (The one + // exception is a workload that exits with 259 inside the ladder's + // own window, which yields a conservative "could not determine" + // rather than a false claim that it was killed.) + return Err(lifecycle_err( + "the sandboxed process was still running after close-stdin, Ctrl-Close and \ + terminate; it timed out but could not be confirmed killed", + )); + } + Ok(ExecOutcome::TimedOut) } /// Kill the process now, reporting whether the kill request was *accepted*. @@ -584,10 +696,12 @@ impl StartedProcess { /// call forever if it ever failed against a live process. /// /// That bound covers *this call only*, and does not make teardown as a whole - /// bounded. The streaming adapter's `Drop` runs the terminator and then - /// joins the waiter thread unconditionally, and a process that survives the - /// kill can park that thread by either of two routes — so supplying a - /// timeout does not bound it: + /// bounded. The streaming adapter's `Drop` joins the waiter thread whenever + /// it believes the process is dead — which includes the case where this + /// function returned `Ok(())` for a `Terminate` the platform accepted but + /// that never took effect, since the bounded wait's result is discarded. A + /// process that survives the kill can then park that join by either of two + /// routes, so supplying a timeout does not bound it: /// /// - With no timeout, the waiter is still sitting in its leading /// `WaitForExit(timeout_ms)`, which is INFINITE for `0`. @@ -604,10 +718,11 @@ impl StartedProcess { /// /// **What this does not tell you.** The bounded wait's result is discarded, /// so a `Terminate` the platform accepted but that left the process running - /// still yields `Ok(())`. Surfacing that needs somewhere to surface it to: - /// `ExecHandle::terminator` is an infallible `FnOnce()`, so even the `Err` - /// this *does* return is dropped at that boundary. Making the path fallible - /// end to end is tracked separately. + /// still yields `Ok(())`. What *is* now reported is the distinction between + /// an accepted request and a refused one: the `Err` this returns reaches the + /// caller through `ExecHandle::terminator` and `SandboxProcess::kill`. + /// Confirming the process actually died would need the bounded wait's result + /// as well, which this type does not carry. pub(super) fn terminate(&self) -> Result<(), IsolationSessionError> { self.process .Terminate() @@ -658,11 +773,91 @@ impl Drop for ClosingProcess { } } +/// `STILL_ACTIVE` (0x103) is exposed by the `windows` crate as +/// `STATUS_PENDING: NTSTATUS` — same numeric value, different name. A process +/// whose `ExitCode()` reads this has not exited. +/// +/// **Not sufficient on its own to prove a process is running.** 259 is a legal +/// exit code, so a workload that exits with it is indistinguishable here from +/// one that never exited. Every liveness decision in this file therefore pairs +/// this with a second, independent signal; see [`StartedProcess::wait`]. +const STILL_ACTIVE: i32 = windows::Win32::Foundation::STATUS_PENDING.0; + +/// What `WaitForExit` returns when the deadline elapses before the process +/// exits. Like [`STILL_ACTIVE`] this is a legal exit code in its own right, so +/// it is never used alone to conclude a timeout. +const WAIT_FOR_EXIT_TIMEOUT: i32 = -1; + /// How long [`StartedProcess::terminate`] waits for a kill to land before /// reporting success anyway. Bounded so a failed `Terminate` cannot wedge that /// call; generous enough that a normal kill is observed synchronously. const TERMINATE_WAIT_MS: u32 = 5_000; +/// What [`StartedProcess::wait`] should do once its wait has returned. +/// +/// A value rather than a branch inside `wait`, because `wait` needs a live COM +/// process object and cannot be exercised on a host without the OS-side +/// service — so the decision it hinges on would otherwise be untestable, and a +/// regression in it silently undetectable. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum WaitPlan { + /// The process exited within its deadline, or there was no deadline. This + /// is its exit code. **The shutdown ladder must not run** — it cannot + /// distinguish a 259 exit from a live process. + Exited(i32), + /// The deadline elapsed and the process is **still running**. It must be + /// killed and confirmed dead before a timeout is reported. + KillThenReportTimeout, + /// The deadline elapsed, and the process then exited on its own before it + /// could be sampled. There is nothing left to kill — the exit-code read is + /// itself the confirmation the process is gone — but the outcome is still a + /// timeout, because the caller's deadline was spent while it ran. + /// + /// Reporting the later-observed exit code here instead would hide a missed + /// deadline. The sibling WSLc backend makes the same distinction, tracking + /// `deadline_elapsed` separately from `timed_out` so a spent deadline is + /// "reported stickily rather than a later-observed exit code". + TimedOutAfterDeadline, +} + +/// Decides what a completed `WaitForExit` means. +/// +/// Split out from [`StartedProcess::wait`] so the rule is exercisable without an +/// OS-side isolation session: every input here is a plain integer, while the +/// call it guards needs a live process object. +/// +/// `exit_code` is a closure rather than a value so the ordinary case costs no +/// extra COM round-trip — it is consulted only for the one `waited` value that +/// could mean a timeout. See [`StartedProcess::wait`] for why one signal alone +/// is not enough. +fn plan_wait( + timeout_ms: u32, + waited: i32, + exit_code: impl FnOnce() -> Result, +) -> Result { + // INFINITE: there was no deadline to miss, so the wait returned because the + // process exited, and `waited` is its code. + if timeout_ms == 0 || waited != WAIT_FOR_EXIT_TIMEOUT { + return Ok(WaitPlan::Exited(waited)); + } + // The sentinel alone proves nothing — a workload may exit with -1 — so the + // exit code is the second, independent signal. + let code = exit_code()?; + match code { + // Still running: the deadline is spent and the process must be killed. + STILL_ACTIVE => Ok(WaitPlan::KillThenReportTimeout), + // Genuinely ambiguous, and the one case that cannot be resolved: the + // wait returned -1 and the process's code *is* -1, so the sentinel is + // indistinguishable from a natural exit. Read as the exit. + WAIT_FOR_EXIT_TIMEOUT => Ok(WaitPlan::Exited(WAIT_FOR_EXIT_TIMEOUT)), + // The process is gone, and its code is not -1 — so the -1 the wait + // returned cannot have been that code, and was therefore the timeout + // sentinel. The deadline provably elapsed while the process ran, and it + // exited in the window before this sample. + _ => Ok(WaitPlan::TimedOutAfterDeadline), + } +} + /// Three-tier graceful shutdown for an `IsoSessionProcess` that's still /// running after `WaitForExit(timeout_ms)` returns. Tier 1: close stdin — /// many REPLs exit on EOF alone. Tier 2: `SendCtrlClose` — ConPTY-only; @@ -675,11 +870,15 @@ const TERMINATE_WAIT_MS: u32 = 5_000; /// than to fire blind. Per-tier subsequent queries fall back to /// `STILL_ACTIVE` so a transient read failure does not short-circuit the /// escalation. +/// +/// # What this does not tell you +/// +/// The returned code cannot distinguish a process that exited with 259 from +/// one that is still running, because `ExitCode()` reports 259 for both. A +/// caller that needs that distinction must establish it separately — +/// [`StartedProcess::wait`] does, which is why it only calls this once it has +/// independently determined the process was still running. fn wait_with_graceful_shutdown(process: &IsoSessionProcess) -> Result { - // `STILL_ACTIVE` (0x103) is exposed by the `windows` crate as - // `STATUS_PENDING: NTSTATUS` — same numeric value, different name. - use windows::Win32::Foundation::STATUS_PENDING; - const STILL_ACTIVE: i32 = STATUS_PENDING.0; let mut exit_code = process .ExitCode() .map_err(|e| transport_err(op::RUN_PROCESS, "get ExitCode failed", &e))?; @@ -703,6 +902,10 @@ fn wait_with_graceful_shutdown(process: &IsoSessionProcess) -> Result Result impl FnOnce() -> Result { + move || Ok(value) + } + + #[test] + fn timeout_needs_both_signals_not_either() { + // The real timeout: both signals present, and the ladder must run. + assert_eq!( + plan_wait(5_000, WAIT_FOR_EXIT_TIMEOUT, probe(STILL_ACTIVE)).unwrap(), + WaitPlan::KillThenReportTimeout + ); + + // A workload that exits with the wait sentinel's value. `WaitForExit` + // reports the code rather than -1, so this is an exit. + assert_eq!( + plan_wait(5_000, -1, probe(-1)).unwrap(), + WaitPlan::Exited(-1) + ); + + // A workload that exits 259 — the value `STILL_ACTIVE` also has. The + // wait returned the code, not the sentinel, so this is an exit too. + // Critically it must NOT be planned for the ladder: the ladder reads + // 259 from `ExitCode()` and cannot tell it from a live process, which + // is how a clean exit-259 became a backend error. + assert_eq!( + plan_wait(5_000, STILL_ACTIVE, probe(STILL_ACTIVE)).unwrap(), + WaitPlan::Exited(STILL_ACTIVE) + ); + + // An ordinary exit. + assert_eq!(plan_wait(5_000, 0, probe(0)).unwrap(), WaitPlan::Exited(0)); + } + + /// The sentinel with a non-`STILL_ACTIVE`, non-`-1` code means the deadline + /// was spent and the process then exited on its own. + /// + /// The regression this pins: reporting that later-observed exit code hides + /// a missed deadline. The `-1` the wait returned cannot have been the + /// process's code (its code is 7), so it was the timeout sentinel and the + /// deadline provably elapsed while the process ran. + #[test] + fn a_deadline_spent_before_a_late_exit_is_still_a_timeout() { + assert_eq!( + plan_wait(5_000, WAIT_FOR_EXIT_TIMEOUT, probe(7)).unwrap(), + WaitPlan::TimedOutAfterDeadline + ); + assert_eq!( + plan_wait(5_000, WAIT_FOR_EXIT_TIMEOUT, probe(0)).unwrap(), + WaitPlan::TimedOutAfterDeadline, + "a clean exit past the deadline is still a missed deadline" + ); + } + + /// The one irreducible collision: sentinel and exit code are both `-1`, so + /// nothing distinguishes a timeout from a workload that exited with `-1`. + #[test] + fn the_sentinel_colliding_with_a_real_minus_one_exit_is_read_as_the_exit() { + assert_eq!( + plan_wait(5_000, WAIT_FOR_EXIT_TIMEOUT, probe(-1)).unwrap(), + WaitPlan::Exited(-1) + ); + } + + #[test] + fn infinite_timeout_can_never_be_a_timeout() { + // 0 means INFINITE. Even with the sentinel apparently present, a wait + // with no deadline did not miss one. This is the default, so the guard + // covers the common configuration rather than an exotic one. + assert_eq!( + plan_wait(0, WAIT_FOR_EXIT_TIMEOUT, probe(STILL_ACTIVE)).unwrap(), + WaitPlan::Exited(WAIT_FOR_EXIT_TIMEOUT) + ); + } + + #[test] + fn exit_code_is_not_queried_unless_it_could_change_the_answer() { + let calls = std::cell::Cell::new(0); + let counting = || { + calls.set(calls.get() + 1); + Ok(STILL_ACTIVE) + }; + assert_eq!(plan_wait(5_000, 0, counting).unwrap(), WaitPlan::Exited(0)); + assert_eq!(calls.get(), 0, "no COM round-trip for a plain exit"); + } + + #[test] + fn unreadable_exit_code_propagates_rather_than_guessing() { + let err = plan_wait(5_000, WAIT_FOR_EXIT_TIMEOUT, || { + Err(lifecycle_err("handle is broken")) + }); + assert!( + err.is_err(), + "a failed probe must not be reported as an exit" + ); + } + #[test] fn feature_unavailable_returns_clean_error() { // Initialize COM (required for WinRT activation). diff --git a/src/backends/isolation_session/common/src/process_options.rs b/src/backends/isolation_session/common/src/process_options.rs index 84fc4ada2..a1f3050c7 100644 --- a/src/backends/isolation_session/common/src/process_options.rs +++ b/src/backends/isolation_session/common/src/process_options.rs @@ -90,6 +90,60 @@ pub(super) fn build_process_options( } } +/// How much longer than the caller's deadline the **service-side** timer is +/// armed for on the streaming path. +/// +/// The service arms its timer when the process is created and kills the process +/// with an ordinary exit code (the host suite pins this as exit code 1). Our own +/// `WaitForExit` starts later and, with an equal duration, therefore always +/// loses the race — so a genuine timeout arrives looking exactly like a normal +/// exit and cannot be reported as one. +/// +/// Giving the service timer a margin lets the local deadline fire first, so a +/// timeout is observed as the wait sentinel plus a still-running process. The +/// service timer stays armed as a watchdog for the case this process dies +/// before it can enforce anything. +/// +/// **This is a margin, not a guarantee.** It orders the two timers only while +/// the gap between arming the service timer and entering our own wait — a few +/// COM calls and a thread spawn — stays under the margin; a host stall longer +/// than that could still let the service win. No timeout *signal* is available +/// to key off instead: the process interface exposes lifetime and console +/// members (`ExitCode`, `WaitForExit`, `Terminate`, `CloseStandardInput`, +/// `SendCtrlClose`, the stdio handles, `ResizeConsole`) and nothing that +/// distinguishes a service-enforced kill from an ordinary exit. Five seconds is +/// chosen to dwarf that startup gap rather than to bound the OS. +pub(super) const SERVICE_TIMEOUT_GRACE_MS: u32 = 5_000; + +/// Relaxes the service-side timer so the caller's deadline is enforced locally. +/// +/// Only for the streaming path, which has somewhere to report a timeout. The +/// run-to-completion path keeps the service timer exactly as it was: it has no +/// timeout channel, and its observable behaviour (exit code 1 on an OS-side +/// timeout) is pinned by the host suite. +/// +/// Two deadlines are left alone rather than shifted: +/// +/// - **Zero** means INFINITE. There is no deadline to move behind, and adding a +/// grace would invent one the caller never asked for. +/// - **A deadline too large to move** (within `SERVICE_TIMEOUT_GRACE_MS` of +/// `u32::MAX`) disarms the service timer instead. Saturating would silently +/// shrink the margin to nothing and re-equalize the timers, restoring the +/// exact misclassification this exists to prevent. At that magnitude — over +/// forty-nine days — a watchdog is meaningless anyway, so the local deadline +/// becomes the only one. +pub(super) fn with_service_timeout_grace(mut options: ProcessOptions) -> ProcessOptions { + const NO_SERVICE_TIMEOUT: u32 = 0; + if options.timeout_ms == 0 { + return options; + } + options.timeout_ms = match options.timeout_ms.checked_add(SERVICE_TIMEOUT_GRACE_MS) { + Some(armed) => armed, + None => NO_SERVICE_TIMEOUT, + }; + options +} + /// Translates the MXC-internal `ProcessOptions` into a fresh /// `IsoSessionProcessOptions` ready for `RunProcessWithOptionsAsync`. pub(super) fn build_iso_process_options( @@ -175,6 +229,87 @@ mod tests { assert_eq!(opts.timeout_ms, 30000); } + /// The service-side timer must not be able to fire before the caller's own + /// deadline on the streaming path. + /// + /// The regression this pins: both timers were armed with the same duration, + /// but the service arms its own at process creation while our wait starts + /// later, so the service always won the race. A genuine timeout then + /// arrived looking like an ordinary exit — the host suite pins the + /// OS-side timeout as exit code 1 — so it could not be reported as a + /// timeout at all, and the streaming path's timeout reporting never fired. + #[test] + fn the_service_timer_is_armed_behind_the_local_deadline() { + let request = ExecutionRequest { + script_code: "echo hi".to_string(), + script_timeout: 30000, + ..Default::default() + }; + let local = build_process_options(&request, false); + let relaxed = with_service_timeout_grace(local); + assert!( + relaxed.timeout_ms > 30000, + "the service timer must outlast the caller's deadline, got {}", + relaxed.timeout_ms + ); + } + + /// INFINITE stays INFINITE: there is no deadline to move behind, and + /// giving it a grace would invent a deadline the caller never asked for. + #[test] + fn an_infinite_timeout_is_not_given_a_service_grace() { + let request = ExecutionRequest { + script_code: "echo hi".to_string(), + script_timeout: 0, + ..Default::default() + }; + let relaxed = with_service_timeout_grace(build_process_options(&request, false)); + assert_eq!(relaxed.timeout_ms, 0); + } + + /// A deadline too large to move disarms the service timer rather than + /// silently re-equalizing the two. + /// + /// The regression this pins: saturating arithmetic clamps at `u32::MAX`, so + /// a deadline within the grace of the maximum would come back with a margin + /// shrunk to nothing — the timers equal again, and a genuine timeout once + /// more indistinguishable from an ordinary exit. `timeout` is an + /// unconstrained `u32` on the wire, so this is reachable input. + #[test] + fn a_deadline_too_large_to_move_disarms_the_service_timer() { + for script_timeout in [ + u32::MAX, + u32::MAX - 1, + u32::MAX - SERVICE_TIMEOUT_GRACE_MS + 1, + ] { + let request = ExecutionRequest { + script_code: "echo hi".to_string(), + script_timeout, + ..Default::default() + }; + let relaxed = with_service_timeout_grace(build_process_options(&request, false)); + assert_eq!( + relaxed.timeout_ms, 0, + "a deadline of {script_timeout} must disarm the service timer rather than \ + arm it equal to the caller's deadline" + ); + } + } + + /// The largest deadline that can still be moved keeps a full margin. + #[test] + fn the_largest_movable_deadline_keeps_its_full_margin() { + let script_timeout = u32::MAX - SERVICE_TIMEOUT_GRACE_MS; + let request = ExecutionRequest { + script_code: "echo hi".to_string(), + script_timeout, + ..Default::default() + }; + let relaxed = with_service_timeout_grace(build_process_options(&request, false)); + assert_eq!(relaxed.timeout_ms, u32::MAX); + assert!(relaxed.timeout_ms > script_timeout); + } + #[test] fn options_maps_working_directory() { let request = ExecutionRequest { diff --git a/src/backends/isolation_session/common/src/state_aware.rs b/src/backends/isolation_session/common/src/state_aware.rs index c92bd640a..57efa8ee0 100644 --- a/src/backends/isolation_session/common/src/state_aware.rs +++ b/src/backends/isolation_session/common/src/state_aware.rs @@ -14,7 +14,7 @@ use serde::Serialize; use wxc_common::models::{ExecutionRequest, IsolationSessionProvisionConfig}; use wxc_common::mxc_error::MxcError; use wxc_common::state_aware_backend::{ - DeprovisionResult, ExecConsumer, ExecHandle, ProvisionResult, StartResult, + DeprovisionResult, ExecConsumer, ExecHandle, ExecOutcome, ProvisionResult, StartResult, StatefulSandboxBackend, StopResult, }; @@ -23,7 +23,7 @@ use windows::Win32::Foundation::HANDLE; use super::error::map_lifecycle_error; use super::manager::{ClosingProcess, IsolationSessionManager}; use super::policy::{validate_post_provision_policy, validate_provision_policy}; -use super::process_options::build_process_options; +use super::process_options::{build_process_options, with_service_timeout_grace}; use super::sandbox_id::{self, SandboxIdPayload}; use super::IsolationSessionRunner; @@ -310,8 +310,16 @@ impl StatefulSandboxBackend for IsolationSessionRunner { stdout: null, stderr: null, stdin: null, - waiter: Box::new(move || Ok(exit_code)), - terminator: Box::new(|| {}), + // `Exited`, never `TimedOut`, and not for lack of knowing: + // `create_process` ran the ladder inline and a timed-out + // workload is already dead by now. The executor has no + // timeout channel to report it through — `ScriptResponse` + // carries an exit code and nothing else — so reporting one + // here would be information the CLI must then discard. + waiter: Box::new(move || Ok(ExecOutcome::Exited(exit_code))), + // Nothing to terminate: `create_process` returned only once + // the process was gone. + terminator: Box::new(|| Ok(())), }) } ExecConsumer::Library => { @@ -319,7 +327,11 @@ impl StatefulSandboxBackend for IsolationSessionRunner { request, wants_interactive_console(consumer, || std::io::stdout().is_terminal()), ); + // The caller's deadline, enforced by our own wait below. The + // service timer is armed with a margin so it cannot fire first + // and turn a timeout into an ordinary exit we could not report. let timeout_ms = options.timeout_ms; + let options = with_service_timeout_grace(options); let started = Arc::new(ClosingProcess::new( manager @@ -343,18 +355,19 @@ impl StatefulSandboxBackend for IsolationSessionRunner { stdout, stderr, stdin, + // Reports `TimedOut` when the deadline elapsed with the + // process still running — see `StartedProcess::wait`, which + // samples that before the shutdown ladder destroys the + // evidence by killing the survivor. waiter: Box::new(move || { waiter_process.wait(timeout_ms).map_err(map_lifecycle_error) }), - // `ExecHandle::terminator` cannot report failure, so a - // failed kill is logged-by-discard here. The fallible - // signature still earns its keep: it is what stops - // `terminate` from waiting INFINITE on a `Terminate` that - // never landed, and it lets a future handle type surface - // the error without changing the backend. - terminator: Box::new(move || { - let _ = started.terminate(); - }), + // The result now reaches the caller instead of being + // discarded here. It reports whether the platform *accepted* + // the kill: `terminate`'s bounded post-kill wait is not + // consulted, so a `Terminate` that was accepted and then did + // not take effect still reports success. + terminator: Box::new(move || started.terminate().map_err(map_lifecycle_error)), }) } } diff --git a/src/backends/windows_sandbox/lifecycle/src/state_aware.rs b/src/backends/windows_sandbox/lifecycle/src/state_aware.rs index 4f46c771d..3f8dc6c01 100644 --- a/src/backends/windows_sandbox/lifecycle/src/state_aware.rs +++ b/src/backends/windows_sandbox/lifecycle/src/state_aware.rs @@ -18,7 +18,7 @@ use wxc_common::mxc_error::MxcError; use wxc_common::process_util::resolve_sibling_binary; use wxc_common::script_runner::get_timeout_milliseconds; use wxc_common::state_aware_backend::{ - DeprovisionResult, ExecConsumer, ExecHandle, ProvisionResult, StartResult, + DeprovisionResult, ExecConsumer, ExecHandle, ExecOutcome, ProvisionResult, StartResult, StatefulSandboxBackend, StopResult, }; @@ -756,8 +756,17 @@ impl StatefulSandboxBackend for WindowsSandboxRunner { sandbox_id: &str, request: &ExecutionRequest, _config: Option<()>, - _consumer: ExecConsumer, + consumer: ExecConsumer, ) -> Result { + // Before any work: this backend relays to the executor's stdio, so it + // cannot serve an in-process caller, and running the workload first + // would make the refusal a lie about what has already happened. + if consumer == ExecConsumer::Library { + return Err(wxc_common::state_aware_backend::unsupported_library_exec( + "Windows Sandbox", + )); + } + extract_token(sandbox_id)?; // Locate the live daemon holding this sandbox and confirm it is ready @@ -794,8 +803,13 @@ impl StatefulSandboxBackend for WindowsSandboxRunner { stdout: null, stderr: null, stdin: null, - waiter: Box::new(move || Ok(exit_code)), - terminator: Box::new(|| {}), + // `Exited`, not `TimedOut`: this backend runs the workload to + // completion inside `exec` and reports what the guest returned, so + // there is no live process left to have timed out. A `Library` path + // that could distinguish the two does not exist here yet. + waiter: Box::new(move || Ok(ExecOutcome::Exited(exit_code))), + // Nothing to terminate: the workload is already gone. + terminator: Box::new(|| Ok(())), }) } @@ -1035,6 +1049,35 @@ mod tests { use wxc_common::models::{ContainerPolicy, NetworkPolicy}; use wxc_common::mxc_error::MxcErrorCode; + /// A `Library` exec is refused before the backend looks for the daemon. + /// + /// This backend relays the workload's output to *this process's* stdio, so + /// it cannot serve an in-process caller. The refusal has to precede any + /// work: refusing after the workload ran would report "unsupported" for + /// something that already took effect. + /// + /// `extract_token` would reject this id, and there is no live daemon behind + /// it either — so any error other than the refusal means the consumer check + /// came too late. + #[test] + fn a_library_exec_is_refused_before_the_workload_runs() { + let mut runner = WindowsSandboxRunner::new(); + let err = runner + .exec( + "not-a-valid-sandbox-id", + &ExecutionRequest::default(), + None, + ExecConsumer::Library, + ) + .expect_err("an in-process caller must be refused"); + assert!( + err.message + .contains("does not support exec for an in-process caller"), + "expected the shared refusal ahead of id and daemon checks, got: {}", + err.message + ); + } + #[test] fn backend_key_matches_wire_format() { assert_eq!( diff --git a/src/backends/wslc/common/src/state_aware.rs b/src/backends/wslc/common/src/state_aware.rs index c4fafd646..cac8715f0 100644 --- a/src/backends/wslc/common/src/state_aware.rs +++ b/src/backends/wslc/common/src/state_aware.rs @@ -19,8 +19,8 @@ use wxc_common::logger::{Logger, Mode}; use wxc_common::models::{ContainerPolicy, ExecutionRequest, NetworkPolicy}; use wxc_common::mxc_error::MxcError; use wxc_common::state_aware_backend::{ - null_pipe_handle, DeprovisionResult, ExecConsumer, ExecHandle, ProvisionResult, StartResult, - StatefulSandboxBackend, StopResult, + null_pipe_handle, DeprovisionResult, ExecConsumer, ExecHandle, ExecOutcome, ProvisionResult, + StartResult, StatefulSandboxBackend, StopResult, }; use wxc_common::wire::WslcProvisionPhase; @@ -137,8 +137,17 @@ impl StatefulSandboxBackend for WslcStateAwareRunner { sandbox_id: &str, request: &ExecutionRequest, _config: Option<()>, - _consumer: ExecConsumer, + consumer: ExecConsumer, ) -> Result { + // Before any work: this backend relays to the executor's stdio, so it + // cannot serve an in-process caller, and running the workload first + // would make the refusal a lie about what has already happened. + if consumer == ExecConsumer::Library { + return Err(wxc_common::state_aware_backend::unsupported_library_exec( + "WSLc", + )); + } + // Cooperative proxy: inject HTTP(S)_PROXY (and scrub caller-supplied // proxy vars). `exec_proxy_url` yields the routable URL only when the // proxy is enabled *and* in the required `url` form — `validate_exec` @@ -202,8 +211,15 @@ impl StatefulSandboxBackend for WslcStateAwareRunner { stdout: null_pipe_handle(), stderr: null_pipe_handle(), stdin: null_pipe_handle(), - waiter: Box::new(move || Ok(exit_code)), - terminator: Box::new(|| {}), + // `Exited`, not `TimedOut`: this backend relays internally and has + // already run the workload to completion by the time it returns, so + // `exit_code` is whatever the container reported — including for a + // workload the daemon timed out. Reporting a timeout as such needs + // the `Library` path this backend does not have yet. + waiter: Box::new(move || Ok(ExecOutcome::Exited(exit_code))), + // Nothing to terminate: the workload is already gone. `Ok(())` is + // the truthful answer here, not a placeholder. + terminator: Box::new(|| Ok(())), }) } @@ -413,6 +429,41 @@ mod tests { use super::*; use wxc_common::models::ContainerPolicy; + /// A `Library` exec is refused before the backend touches the daemon. + /// + /// This backend writes the workload's output to *this process's* stdout and + /// stderr, so it cannot serve an in-process caller. The refusal has to come + /// first: the workload is arbitrary and may not be idempotent, so refusing + /// after running it would report "unsupported" for something that already + /// happened, with its output delivered somewhere the caller never asked for. + /// + /// The sandbox id is well-formed but names nothing, and there is no daemon + /// to connect to. Any error other than the refusal means the guard ran too + /// late — the code reached the daemon before checking who was asking. + #[test] + fn a_library_exec_is_refused_before_the_workload_runs() { + let mut runner = WslcStateAwareRunner::new(); + let err = runner + .exec( + "wslc:0123456789abcdef0123456789abcdef", + &ExecutionRequest::default(), + None, + ExecConsumer::Library, + ) + .expect_err("an in-process caller must be refused"); + assert!( + err.message + .contains("does not support exec for an in-process caller"), + "expected the shared refusal before any daemon work, got: {}", + err.message + ); + assert!( + err.message.contains("Nothing has been run"), + "the refusal must state that no workload ran: {}", + err.message + ); + } + #[test] fn backend_key_matches_wire_format() { assert_eq!( diff --git a/src/core/mxc-sdk/src/sandbox.rs b/src/core/mxc-sdk/src/sandbox.rs index e479a57ef..065190506 100644 --- a/src/core/mxc-sdk/src/sandbox.rs +++ b/src/core/mxc-sdk/src/sandbox.rs @@ -22,8 +22,25 @@ pub enum WaitOutcome { /// The process exited with this code. On Unix a process terminated by a /// signal (rather than exiting normally) surfaces as `Exited(-1)`. Exited(i32), - /// The request's `scriptTimeout` elapsed before the process exited; the - /// process and its whole tree were killed. + /// The request's `scriptTimeout` elapsed while the process was running, and + /// the process is no longer running. + /// + /// **Deadline spent, and the process is gone.** Whether it was killed or + /// exited on its own a moment past the deadline is not distinguished: both + /// missed the deadline the caller asked for, and reporting the exit code + /// one of them happened to produce would hide that. + /// + /// How far "gone" reaches depends on the backend. A process-spawning + /// backend kills the whole tree. A backend whose only primitive is the + /// foreground process — the state-aware `exec` path over IsolationSession — + /// confirms that process, and a descendant the workload backgrounded is + /// reclaimed when the sandbox is stopped and deprovisioned rather than here. + /// + /// That state-aware route is not reachable from this crate yet: + /// [`exec_sandbox`](crate::exec_sandbox) parses without the experimental + /// opt-in, so an experimental backend is refused before dispatch. The + /// distinction is documented here because it is what implementors build + /// against, and because it becomes observable the moment that opt-in lands. TimedOut, } @@ -114,8 +131,9 @@ impl Sandbox { /// /// Returns [`WaitOutcome::Exited`] with the exit code, or /// [`WaitOutcome::TimedOut`] if the request's `scriptTimeout` elapsed (the - /// process and its tree are killed first). `Err` is reserved for an actual - /// OS / wait failure. + /// workload is terminated first; see [`WaitOutcome::TimedOut`] for how far + /// that reaches on each backend). `Err` is reserved for an actual OS / wait + /// failure. pub fn wait(&mut self) -> std::io::Result { match self.inner.wait() { Ok(code) => Ok(WaitOutcome::Exited(code)), @@ -133,7 +151,7 @@ impl Sandbox { /// /// `Err` is reserved for an actual OS / wait failure; a timeout is reported /// as [`Output`] with `outcome: WaitOutcome::TimedOut` and whatever each - /// stream produced before the tree was killed. + /// stream produced before the workload was terminated. pub fn wait_with_output(mut self) -> std::io::Result { fn capture(stream: Option>) -> std::thread::JoinHandle> { std::thread::spawn(move || { diff --git a/src/core/wxc_common/src/exec_stream.rs b/src/core/wxc_common/src/exec_stream.rs index e9b24a2eb..a86ff7368 100644 --- a/src/core/wxc_common/src/exec_stream.rs +++ b/src/core/wxc_common/src/exec_stream.rs @@ -28,8 +28,9 @@ //! real pipe handles, and it closes its own ends when its process object drops. //! Under [`ExecConsumer::Executor`] it relays internally and returns null //! handles, and the other state-aware backends (Windows Sandbox, WSLc) return -//! null handles on every path — in those cases the streams are simply absent -//! here. +//! null handles on every path. A handle with *all three* streams null is +//! refused here rather than wrapped: it means the backend has not implemented +//! the `Library` contract, and a stream-less `SandboxProcess` would hide that. //! //! Each readable duplicate is wrapped as a **cancellable** reader, so a read on //! it can be made to return EOF on demand. That is what lets @@ -48,7 +49,7 @@ use std::thread::JoinHandle; use crate::mxc_error::MxcError; use crate::sandbox_process::{boxed_closer, cancel_and_join_discard, SandboxProcess, StreamCloser}; -use crate::state_aware_backend::{ExecHandle, PipeHandle}; +use crate::state_aware_backend::{ExecHandle, ExecOutcome, PipeHandle}; /// The platform's closer for a cancellable read — fired to make an in-flight /// read on a not-taken stream return EOF, so its discard thread ends and can be @@ -83,13 +84,18 @@ pub struct ExecSandboxProcess { /// The background thread running the handle's `waiter`. Taken and joined by /// the first [`wait`](SandboxProcess::wait) / successful /// [`try_wait`](SandboxProcess::try_wait). - waiter: Option>>, + waiter: Option>>, /// Kills the process tree. Taken by the first [`kill`](SandboxProcess::kill) /// or by `Drop`. - terminator: Option>, - /// Cached exit code once the waiter has been joined, so repeat waits are - /// idempotent. - exit: Option, + terminator: Option Result<(), MxcError> + Send>>, + /// The waiter's outcome once joined, so repeat waits are idempotent. Holds + /// the outcome rather than a code because a timeout has no code. + exit: Option, + /// Whether a termination request was **refused**. Recorded separately + /// because `terminator` is consumed on both outcomes, so its absence alone + /// cannot distinguish "killed" from "the kill was rejected" — and those + /// demand opposite behaviour in [`Drop`]. + kill_refused: bool, } impl ExecSandboxProcess { @@ -110,6 +116,39 @@ impl ExecSandboxProcess { terminator, } = handle; + // A handle with nothing on any stream is a backend that has not + // implemented the `Library` contract — it relayed internally and ran the + // workload to completion, which is the `Executor` shape. Wrapping it + // would hand the caller a `SandboxProcess` whose `take_stdout` is `None` + // and whose `wait` returns an exit code the backend already had, while + // the sandbox's output went wherever the backend chose — for the + // internally-relaying backends, the *host application's* own stdout. + // + // This is a **backstop, not the guard that matters**. The backends that + // cannot serve `Library` refuse it up front instead (see + // `unsupported_library_exec`), so reaching this branch means a backend + // returned an all-null handle for a `Library` exec anyway — hence the + // distinct wording, so the two are told apart in a log. + // + // **The process is not assumed to have finished.** The comment above + // describes the shape this usually means, not a guarantee: the + // IsolationSession `Library` path starts a process and passes through + // whatever handles the service gave it, so an all-zero set can name a + // process that is very much alive. Reaping is therefore conditional on + // the kill being accepted, exactly as on every other teardown path — a + // refused kill here would otherwise park construction forever, and the + // caller has no handle yet to interrupt it with. + if is_null_pipe(stdout) && is_null_pipe(stderr) && is_null_pipe(stdin) { + terminate_then_reap_if_safe(terminator, || { + let _ = waiter(); + }); + return Err(MxcError::backend_error( + "this backend returned no stdout, stderr or stdin for an in-process exec, \ + which means it relayed the output itself. It should have refused before \ + running the command; the command may have run", + )); + } + let streams = wrap_cancellable_read_checked(stdout, "stdout").and_then(|out| { let err = wrap_cancellable_read_checked(stderr, "stderr")?; let input = wrap_write_checked(stdin, "stdin")?; @@ -131,8 +170,8 @@ impl ExecSandboxProcess { /// being terminated and then reaped. fn from_prepared_streams( streams: Result, - waiter: Box Result + Send>, - terminator: Box, + waiter: Box Result + Send>, + terminator: Box Result<(), MxcError> + Send>, ) -> Result { let PreparedStreams { stdout, @@ -143,9 +182,13 @@ impl ExecSandboxProcess { Err(error) => { // Terminating is a request, not a reaping: run the waiter so // the backend's completion work happens and no zombie is left - // behind. Its outcome is discarded so the setup error survives. - terminator(); - let _ = waiter(); + // behind -- but only once the kill was accepted, since a + // refused kill can leave a waiter that never returns. Both + // outcomes are otherwise discarded so the setup error, which is + // the one the caller asked about, survives to be returned. + terminate_then_reap_if_safe(terminator, || { + let _ = waiter(); + }); return Err(error); } }; @@ -173,10 +216,16 @@ impl ExecSandboxProcess { let waiter_thread = match spawned { Ok(thread) => thread, Err(error) => { - terminator(); - if let Some(waiter) = waiter_slot.lock().ok().and_then(|mut w| w.take()) { - let _ = waiter(); - } + // Discarded for the same reason as above: the spawn failure is + // what the caller needs to see. Reaped only if the kill was + // accepted -- the waiter is still in the slot precisely because + // no thread took it, so calling it here would run it inline on + // this thread and a refused kill would park us indefinitely. + terminate_then_reap_if_safe(terminator, || { + if let Some(waiter) = waiter_slot.lock().ok().and_then(|mut w| w.take()) { + let _ = waiter(); + } + }); return Err(MxcError::backend_error(format!( "failed to start the exec waiter thread: {error}" ))); @@ -192,24 +241,28 @@ impl ExecSandboxProcess { waiter: Some(waiter_thread), terminator: Some(terminator), exit: None, + kill_refused: false, }) } - /// Join the waiter thread, caching and returning its exit code. + /// Join the waiter thread, caching and returning its outcome. + /// + /// The outcome is cached rather than the exit code, because a timeout has + /// no code to cache and repeat calls must still be idempotent. fn join_waiter(&mut self) -> std::io::Result { - if let Some(code) = self.exit { - return Ok(code); + if let Some(outcome) = self.exit { + return outcome_to_io(outcome); } let handle = self .waiter .take() .ok_or_else(|| std::io::Error::other("exec waiter already consumed"))?; - let code = handle + let outcome = handle .join() .map_err(|_| std::io::Error::other("exec waiter thread panicked"))? .map_err(|e: MxcError| std::io::Error::other(e.message))?; - self.exit = Some(code); - Ok(code) + self.exit = Some(outcome); + outcome_to_io(outcome) } /// Drain whatever output the caller did not take, concurrently with the @@ -275,20 +328,57 @@ impl ExecSandboxProcess { /// discard did start — only stdout's can have, since stderr's failure is /// what brought us here. Returns the original error so the caller reports /// the cause rather than the cleanup. + /// + /// **The reap is conditional on the kill being accepted.** Joining after a + /// refused kill would reintroduce exactly the stall this function exists to + /// avoid — stuck inside `wait`, with the caller unable to reach `kill` — + /// only on the teardown path instead of the drain path. A refusal is + /// recorded so a later `Drop` makes the same choice. fn abort_after_drain_failure( &mut self, stdout_drain: Option>, error: std::io::Error, ) -> std::io::Error { if let Some(terminator) = self.terminator.take() { - terminator(); + // Discarded: the drain-spawn failure is the error being returned. + let accepted = terminate_then_reap_if_safe(terminator, || {}); + if accepted { + let _ = self.join_waiter(); + } else { + self.kill_refused = true; + } } - let _ = self.join_waiter(); cancel_and_join_discard(stdout_drain, &self.stdout_canceller); error } } +/// Terminate an exec that is being abandoned, and reap it **only if that is +/// safe**. +/// +/// Every teardown path faces the same choice as [`Drop`](ExecSandboxProcess::drop): +/// the backend's completion work should run, but waiting for it is only bounded +/// once the process is actually dying. A terminator that reported a refusal +/// leaves a process that may still be running, and a waiter with no deadline +/// (`script_timeout` defaults to `0`, meaning INFINITE) then never returns — so +/// reaping would block forever on a path the caller cannot interrupt. +/// +/// Returns whether the kill was accepted, so a caller holding +/// [`kill_refused`](ExecSandboxProcess::kill_refused) can record it. +/// +/// Shared rather than repeated so the rule has one home. It was previously +/// open-coded at three sites, each of which reaped unconditionally. +fn terminate_then_reap_if_safe( + terminator: Box Result<(), MxcError> + Send>, + reap: impl FnOnce(), +) -> bool { + let accepted = terminator().is_ok(); + if accepted { + reap(); + } + accepted +} + /// Split a prepared stream into the stream itself and its closer. /// /// The stream may later be taken by the caller; the closer is retained either @@ -352,13 +442,13 @@ impl SandboxProcess for ExecSandboxProcess { } fn try_wait(&mut self) -> std::io::Result> { - if let Some(code) = self.exit { - return Ok(Some(code)); + if let Some(outcome) = self.exit { + return outcome_to_io(outcome).map(Some); } match &self.waiter { Some(handle) if handle.is_finished() => self.join_waiter().map(Some), Some(_) => Ok(None), - None => Ok(self.exit), + None => Ok(None), } } @@ -369,10 +459,35 @@ impl SandboxProcess for ExecSandboxProcess { } fn kill(&mut self) -> std::io::Result<()> { - if let Some(terminator) = self.terminator.take() { - terminator(); + // A confirmed exit retires the terminator. The process is gone, so + // there is nothing to kill, and an earlier refusal no longer describes + // reality — reporting it would fail a `kill()` for a process that is + // demonstrably dead. Running the terminator here would be equally + // wrong: it would surface a failure to terminate something that has + // already terminated. + if self.exit.is_some() { + self.terminator = None; + self.kill_refused = false; + return Ok(()); + } + match self.terminator.take() { + // A refusal now reaches the caller instead of being swallowed, and + // is remembered: the terminator is consumed either way, so this flag + // is the only surviving evidence of which outcome it produced. + Some(terminator) => terminator().map_err(|e| { + self.kill_refused = true; + std::io::Error::other(e.message) + }), + // A kill that was refused stays refused, until an exit is + // confirmed. Reporting `Ok` here would tell a caller retrying after + // a failure that the process is now dead, when nothing has changed + // since the refusal. + None if self.kill_refused => Err(std::io::Error::other( + "the sandbox refused an earlier request to terminate this exec", + )), + // Already killed: killing twice is not an error. + None => Ok(()), } - Ok(()) } fn wait(&mut self) -> std::io::Result { @@ -382,18 +497,60 @@ impl SandboxProcess for ExecSandboxProcess { impl Drop for ExecSandboxProcess { fn drop(&mut self) { - // Kill the process (if not already) so the waiter thread cannot block - // forever, then join it to avoid detaching a thread that borrows the - // backend's process object. - if let Some(terminator) = self.terminator.take() { - terminator(); - } + // Kill the process (if not already) so the waiter thread can finish, + // then join it to avoid detaching a thread that borrows the backend's + // process object. + // + // The join is conditional on the kill having been **accepted**. If the + // platform refused it, the process may still be running and its waiter + // parked in a wait with no deadline — joining would then block here + // forever, in a `Drop` a caller cannot opt out of. Abandoning the thread + // instead is memory-safe (it owns what it borrows through an `Arc`) and + // costs a leaked thread, its `Arc` share of the process object, and that + // object's COM reference — in a case that already indicates the sandbox + // is not responding to termination. + // + // This bounds the `Drop` only against a *reported* refusal. A terminate + // the platform accepted but that never took effect still reports `Ok`, + // and parks the join exactly as before; bounding that too needs the + // backend to confirm the process died, which its handle cannot yet do. + let kill_accepted = match self.terminator.take() { + // Nothing to terminate once the exit is known; running the + // terminator over a dead process only invites a spurious failure. + _ if self.exit.is_some() => true, + Some(terminator) => terminator().is_ok(), + // Already killed via `kill()`, whose result the caller has seen — + // and whose refusal, if it was one, is precisely the case this + // guard exists for. + None => !self.kill_refused, + }; if let Some(handle) = self.waiter.take() { - let _ = handle.join(); + if kill_accepted { + let _ = handle.join(); + } } } } +/// Map an [`ExecOutcome`] onto the [`SandboxProcess`] convention, where a +/// timeout is an `Err` carrying [`io::ErrorKind::TimedOut`](std::io::ErrorKind) +/// rather than an exit code. +/// +/// That convention is not invented here: five one-shot backends already report +/// a timeout this way, and `mxc_sdk::Sandbox::wait` maps exactly this kind onto +/// its public `WaitOutcome::TimedOut`. Translating at this boundary is what +/// lets the state-aware path reach that outcome without changing anything above +/// it. +fn outcome_to_io(outcome: ExecOutcome) -> std::io::Result { + match outcome { + ExecOutcome::Exited(code) => Ok(code), + ExecOutcome::TimedOut => Err(std::io::Error::new( + std::io::ErrorKind::TimedOut, + "the exec timed out and the sandbox terminated it", + )), + } +} + // --------------------------------------------------------------------------- // Platform pipe-handle → std stream wrapping // --------------------------------------------------------------------------- @@ -607,10 +764,11 @@ mod tests { Err(MxcError::backend_error("stdout could not be duplicated")), Box::new(move || { let _ = tx.send("waiter"); - Ok(0) + Ok(ExecOutcome::Exited(0)) }), Box::new(move || { let _ = terminator_tx.send("terminator"); + Ok(()) }), ); @@ -648,40 +806,68 @@ mod tests { ); } - /// An ExecHandle with null pipes (the IsolationSession shape) exposes no - /// streams and yields the waiter's exit code. + /// A handle with nothing on any stream is refused rather than wrapped. + /// + /// This test previously asserted the opposite — that the all-null shape + /// yielded a stream-less process carrying the waiter's exit code. That was + /// the contract when no backend surfaced real pipes; it is now the signature + /// of a backend that has not implemented the `Library` path, and handing the + /// caller a `SandboxProcess` with no streams hides that behind an object + /// that looks live. The exec is reaped on the way out, so refusing does not + /// orphan it. #[test] - fn null_pipes_expose_no_streams_and_return_exit_code() { + fn an_all_null_handle_is_refused_and_reaped() { + let (waiter_tx, waiter_rx) = mpsc::channel(); + let (term_tx, term_rx) = mpsc::channel(); let handle = ExecHandle { stdout: null_pipe_handle(), stderr: null_pipe_handle(), stdin: null_pipe_handle(), - waiter: Box::new(|| Ok(7)), - terminator: Box::new(|| {}), + waiter: Box::new(move || { + let _ = waiter_tx.send(()); + Ok(ExecOutcome::Exited(7)) + }), + terminator: Box::new(move || { + let _ = term_tx.send(()); + Ok(()) + }), }; - let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); - assert!(proc.take_stdout().is_none()); - assert!(proc.take_stderr().is_none()); - assert!(proc.take_stdin().is_none()); - assert_eq!(proc.id(), 0); - assert_eq!(proc.wait().unwrap(), 7); - // Idempotent. - assert_eq!(proc.wait().unwrap(), 7); - assert_eq!(proc.try_wait().unwrap(), Some(7)); + + let err = match ExecSandboxProcess::from_exec_handle(handle) { + Ok(_) => panic!("a backend exposing no streams cannot serve a library caller"), + Err(err) => err, + }; + assert!( + err.message.contains("relayed the output itself"), + "the refusal should say why: {}", + err.message + ); + + // Reaped, not orphaned: both closures ran. + assert!( + term_rx.try_recv().is_ok(), + "the exec must be terminated on refusal" + ); + assert!( + waiter_rx.try_recv().is_ok(), + "the exec must be reaped on refusal" + ); } - /// `kill` invokes the terminator exactly once. + /// `kill` invokes the terminator exactly once, and reports what it said. #[test] fn kill_invokes_terminator_once() { + // A real stdout, because an all-null handle is now refused outright. + let (stdout_r, stdout_w) = std::io::pipe().expect("pipe"); let (tx, rx) = mpsc::channel(); let handle = ExecHandle { - stdout: null_pipe_handle(), + stdout: reader_handle(&stdout_r), stderr: null_pipe_handle(), stdin: null_pipe_handle(), - // Block the waiter until killed, so kill drives the outcome. - waiter: Box::new(|| Ok(0)), + waiter: Box::new(|| Ok(ExecOutcome::Exited(0))), terminator: Box::new(move || { let _ = tx.send(()); + Ok(()) }), }; let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); @@ -690,6 +876,366 @@ mod tests { // Exactly one terminator signal. assert!(rx.recv().is_ok()); assert!(rx.try_recv().is_err()); + + drop(stdout_w); + drop(stdout_r); + } + + /// A terminator that reports a refusal reaches the caller through `kill`. + /// + /// The regression this pins: the terminator used to be infallible, so a + /// kill the platform rejected surfaced as `Ok(())` and a caller had no way + /// to learn the workload was still running. + #[test] + fn kill_reports_a_refused_termination() { + let (stdout_r, stdout_w) = std::io::pipe().expect("pipe"); + let handle = ExecHandle { + stdout: reader_handle(&stdout_r), + stderr: null_pipe_handle(), + stdin: null_pipe_handle(), + waiter: Box::new(|| Ok(ExecOutcome::Exited(0))), + terminator: Box::new(|| Err(MxcError::backend_error("Terminate was refused"))), + }; + let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); + + let err = proc + .kill() + .expect_err("a refused kill must reach the caller"); + assert!( + err.to_string().contains("Terminate was refused"), + "the backend's reason should survive: {err}" + ); + + drop(stdout_w); + drop(stdout_r); + } + + /// A refused kill stays refused when the caller retries. + /// + /// The regression this pins: `kill` consumes the terminator on both + /// outcomes, so without a separate record of the refusal the second call + /// reads "already killed" and reports `Ok(())` — telling a caller retrying + /// after a failure that the process is now dead, when nothing has changed. + #[test] + fn a_refused_kill_stays_refused_on_retry() { + let (stdout_r, stdout_w) = std::io::pipe().expect("pipe"); + let handle = ExecHandle { + stdout: reader_handle(&stdout_r), + stderr: null_pipe_handle(), + stdin: null_pipe_handle(), + waiter: Box::new(|| Ok(ExecOutcome::Exited(0))), + terminator: Box::new(|| Err(MxcError::backend_error("Terminate was refused"))), + }; + let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); + + proc.kill().expect_err("first kill is refused"); + proc.kill() + .expect_err("a retry must not claim the refusal succeeded"); + + drop(stdout_w); + drop(stdout_r); + } + + /// `Drop` must not join the waiter after a kill the platform refused. + /// + /// The regression this pins: a refused `kill()` leaves no terminator, and + /// `Drop` used to read that absence as "already killed" and join. With the + /// process still running its waiter is parked in a wait with no deadline, so + /// the join never returns — a permanent hang inside a `Drop` the caller + /// cannot opt out of. + /// + /// The waiter here blocks until the test releases it, standing in for that + /// parked wait. `kill_reports_a_refused_termination` cannot catch this: its + /// waiter has already finished, so joining it costs nothing. + /// + /// The stand-in wait is **bounded**. An unbounded `recv()` deadlocks the + /// whole suite if an assertion above panics: unwinding drops locals in + /// reverse declaration order, so `proc` is dropped — and its `Drop` joins — + /// before the sender that would release the waiter. That is not + /// hypothetical; it hung a full test run. + #[test] + fn drop_abandons_the_waiter_after_a_refused_kill() { + let (stdout_r, stdout_w) = std::io::pipe().expect("pipe"); + // Held by the waiter; never sent to until the assertion is done. + let (release_tx, release_rx) = mpsc::channel::<()>(); + let handle = ExecHandle { + stdout: reader_handle(&stdout_r), + stderr: null_pipe_handle(), + stdin: null_pipe_handle(), + waiter: Box::new(move || { + let _ = release_rx.recv_timeout(std::time::Duration::from_secs(60)); + Ok(ExecOutcome::Exited(0)) + }), + terminator: Box::new(|| Err(MxcError::backend_error("Terminate was refused"))), + }; + let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); + proc.kill().expect_err("the kill is refused"); + + // Drop on a helper thread so a regression fails the test instead of + // hanging the run. + let (done_tx, done_rx) = mpsc::channel(); + std::thread::spawn(move || { + drop(proc); + let _ = done_tx.send(()); + }); + + assert!( + done_rx + .recv_timeout(std::time::Duration::from_secs(10)) + .is_ok(), + "Drop joined a waiter that cannot finish, after a refused kill" + ); + + let _ = release_tx.send(()); + drop(stdout_w); + drop(stdout_r); + } + + /// A confirmed exit retires an earlier refusal. + /// + /// The regression this pins: `kill_refused` was permanent, so a process + /// that refused a kill and then exited on its own still reported the old + /// refusal from every later `kill()` — telling the caller the sandbox would + /// not terminate a process that is demonstrably gone. `kill()` after a + /// completed `wait()` is required to be a no-op. + #[test] + fn a_confirmed_exit_retires_an_earlier_kill_refusal() { + let (stdout_r, stdout_w) = std::io::pipe().expect("pipe"); + drop(stdout_w); // EOF so the drain finishes + let handle = ExecHandle { + stdout: reader_handle(&stdout_r), + stderr: null_pipe_handle(), + stdin: null_pipe_handle(), + waiter: Box::new(|| Ok(ExecOutcome::Exited(0))), + terminator: Box::new(|| Err(MxcError::backend_error("Terminate was refused"))), + }; + let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); + + proc.kill().expect_err("the kill is refused while it runs"); + assert_eq!(proc.wait().expect("the process exited on its own"), 0); + proc.kill() + .expect("a process known to have exited cannot still be refusing to die"); + + drop(stdout_r); + } + + /// A natural exit must not run the terminator and surface its failure. + /// + /// The regression this pins: making the terminator fallible meant a + /// `kill()` after a completed `wait()` ran it against an already-dead + /// process and reported whatever that failed with, turning a no-op into an + /// error. + #[test] + fn kill_after_a_completed_wait_does_not_run_the_terminator() { + let (stdout_r, stdout_w) = std::io::pipe().expect("pipe"); + drop(stdout_w); + let (tx, rx) = mpsc::channel(); + let handle = ExecHandle { + stdout: reader_handle(&stdout_r), + stderr: null_pipe_handle(), + stdin: null_pipe_handle(), + waiter: Box::new(|| Ok(ExecOutcome::Exited(3))), + terminator: Box::new(move || { + let _ = tx.send(()); + Err(MxcError::backend_error("no such process")) + }), + }; + let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); + + assert_eq!(proc.wait().expect("exited"), 3); + proc.kill().expect("kill after wait is a no-op"); + assert!( + rx.try_recv().is_err(), + "the terminator must not run against a process already known to be gone" + ); + + drop(stdout_r); + } + + /// Construction must not block when a refused kill leaves the waiter live. + /// + /// The regression this pins: the stream-prep failure path ran the + /// terminator, discarded its result, and then called the waiter *inline*. + /// With a refused kill the process may still be running, and with no + /// deadline (`script_timeout` defaults to 0 = INFINITE) that waiter never + /// returns — hanging the constructor, which no caller can interrupt because + /// it has not been handed a handle yet. + /// + /// The waiter here blocks until released, standing in for that parked wait. + #[test] + fn a_refused_kill_does_not_block_construction_on_a_stream_failure() { + let (release_tx, release_rx) = mpsc::channel::<()>(); + let (done_tx, done_rx) = mpsc::channel(); + std::thread::spawn(move || { + let result = ExecSandboxProcess::from_prepared_streams( + Err(MxcError::backend_error("stream setup failed")), + Box::new(move || { + let _ = release_rx.recv_timeout(std::time::Duration::from_secs(60)); + Ok(ExecOutcome::Exited(0)) + }), + Box::new(|| Err(MxcError::backend_error("Terminate was refused"))), + ); + let _ = done_tx.send(result.is_err()); + }); + + let finished = done_rx.recv_timeout(std::time::Duration::from_secs(10)); + assert!( + finished.is_ok(), + "construction reaped a waiter that cannot finish, after a refused kill" + ); + assert!(finished.unwrap(), "the setup error must still be returned"); + let _ = release_tx.send(()); + } + + /// The same rule on the accepted path: the waiter IS reaped when the kill + /// was accepted, so the backend's completion work still runs. + /// + /// Paired with the test above so "does not block" cannot be satisfied by + /// never reaping at all. + #[test] + fn an_accepted_kill_still_reaps_on_a_stream_failure() { + let (tx, rx) = mpsc::channel(); + let result = ExecSandboxProcess::from_prepared_streams( + Err(MxcError::backend_error("stream setup failed")), + Box::new(move || { + let _ = tx.send(()); + Ok(ExecOutcome::Exited(0)) + }), + Box::new(|| Ok(())), + ); + assert!(result.is_err(), "the setup error must be returned"); + assert!( + rx.try_recv().is_ok(), + "an accepted kill must still reap, or the backend leaks its completion work" + ); + } + + /// `wait()` must not block when a discard thread fails to start and the + /// kill is then refused. + /// + /// The regression this pins: `abort_after_drain_failure` joined the waiter + /// unconditionally. That reintroduces the exact stall the function exists to + /// avoid — stuck inside `wait` with the caller unable to reach `kill` — just + /// on the teardown path rather than the drain path. + /// + /// Driven through `abort_after_drain_failure` directly: making a real + /// thread spawn fail on demand is not something a unit test can arrange. + #[test] + fn a_refused_kill_does_not_block_the_drain_failure_teardown() { + let (stdout_r, stdout_w) = std::io::pipe().expect("pipe"); + drop(stdout_w); + let (release_tx, release_rx) = mpsc::channel::<()>(); + let handle = ExecHandle { + stdout: reader_handle(&stdout_r), + stderr: null_pipe_handle(), + stdin: null_pipe_handle(), + waiter: Box::new(move || { + let _ = release_rx.recv_timeout(std::time::Duration::from_secs(60)); + Ok(ExecOutcome::Exited(0)) + }), + terminator: Box::new(|| Err(MxcError::backend_error("Terminate was refused"))), + }; + let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); + + let (done_tx, done_rx) = mpsc::channel(); + std::thread::spawn(move || { + let err = proc.abort_after_drain_failure( + None, + std::io::Error::other("discard thread could not start"), + ); + // Report the refusal state alongside completion, so the test can + // assert both without racing the thread's own teardown. + let _ = done_tx.send((err.to_string(), proc.kill_refused)); + drop(proc); + }); + + let finished = done_rx.recv_timeout(std::time::Duration::from_secs(10)); + assert!( + finished.is_ok(), + "teardown joined a waiter that cannot finish, after a refused kill" + ); + let (msg, refused) = finished.unwrap(); + assert!( + msg.contains("discard thread could not start"), + "the original error must survive the cleanup: {msg}" + ); + assert!( + refused, + "the refusal must be recorded so a later Drop makes the same choice" + ); + + let _ = release_tx.send(()); + drop(stdout_r); + } + + /// An all-null handle whose kill is refused must not park construction. + /// + /// The regression this pins: the backstop reaped unconditionally on the + /// assumption that an all-null handle always means a finished process. It + /// does not — the IsolationSession `Library` path starts a process and + /// passes through whatever handles the service returned, so an all-zero set + /// can name a live one. With a refused kill and no deadline, reaping it + /// hangs `from_exec_handle`, which the caller cannot interrupt because it + /// has not been handed anything yet. + #[test] + fn an_all_null_handle_with_a_refused_kill_does_not_block() { + let (release_tx, release_rx) = mpsc::channel::<()>(); + let (done_tx, done_rx) = mpsc::channel(); + std::thread::spawn(move || { + let handle = ExecHandle { + stdout: null_pipe_handle(), + stderr: null_pipe_handle(), + stdin: null_pipe_handle(), + waiter: Box::new(move || { + let _ = release_rx.recv_timeout(std::time::Duration::from_secs(60)); + Ok(ExecOutcome::Exited(0)) + }), + terminator: Box::new(|| Err(MxcError::backend_error("Terminate was refused"))), + }; + let _ = done_tx.send(ExecSandboxProcess::from_exec_handle(handle).is_err()); + }); + + let finished = done_rx.recv_timeout(std::time::Duration::from_secs(10)); + assert!( + finished.is_ok(), + "the all-null backstop reaped a waiter that cannot finish, after a refused kill" + ); + assert!(finished.unwrap(), "the refusal must still be returned"); + let _ = release_tx.send(()); + } + + /// A waiter reporting a timeout surfaces as `ErrorKind::TimedOut`, which is + /// what `mxc_sdk::Sandbox::wait` maps onto `WaitOutcome::TimedOut`. + /// + /// The regression this pins: the waiter used to return a bare exit code, so + /// a timed-out exec was indistinguishable from one that exited with the code + /// its killed process happened to produce. + #[test] + fn a_timeout_surfaces_as_the_timed_out_error_kind() { + let (stdout_r, stdout_w) = std::io::pipe().expect("pipe"); + drop(stdout_w); // EOF, so the drain finishes promptly + let handle = ExecHandle { + stdout: reader_handle(&stdout_r), + stderr: null_pipe_handle(), + stdin: null_pipe_handle(), + waiter: Box::new(|| Ok(ExecOutcome::TimedOut)), + terminator: Box::new(|| Ok(())), + }; + let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); + + let err = proc.wait().expect_err("a timeout is not an exit code"); + assert_eq!( + err.kind(), + std::io::ErrorKind::TimedOut, + "the kind is the contract the SDK reads: {err}" + ); + + // Idempotent: the cached outcome maps the same way, rather than the + // second call reporting a missing waiter. + let again = proc.wait().expect_err("still a timeout"); + assert_eq!(again.kind(), std::io::ErrorKind::TimedOut); + + drop(stdout_r); } /// `wait()` drains a stream the caller never took. @@ -732,12 +1278,12 @@ mod tests { .recv_timeout(std::time::Duration::from_secs(30)) .map_err(|_| MxcError::backend_error("the child never finished writing"))?; if wrote_everything { - Ok(0) + Ok(ExecOutcome::Exited(0)) } else { Err(MxcError::backend_error("the child's writes failed")) } }), - terminator: Box::new(|| {}), + terminator: Box::new(|| Ok(())), }; let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); @@ -775,7 +1321,7 @@ mod tests { stderr: null_pipe_handle(), stdin: null_pipe_handle(), waiter: Box::new(|| Err(MxcError::backend_error("could not determine the exit"))), - terminator: Box::new(|| {}), + terminator: Box::new(|| Ok(())), }; let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); @@ -825,7 +1371,7 @@ mod tests { // Fails, so the child cannot be assumed gone — the case where the // old design gave up on the join and detached the thread. waiter: Box::new(|| Err(MxcError::backend_error("could not determine the exit"))), - terminator: Box::new(|| {}), + terminator: Box::new(|| Ok(())), }; let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); @@ -861,8 +1407,8 @@ mod tests { stdout: reader_handle(&stdout_r), stderr: null_pipe_handle(), stdin: null_pipe_handle(), - waiter: Box::new(|| Ok(0)), - terminator: Box::new(|| {}), + waiter: Box::new(|| Ok(ExecOutcome::Exited(0))), + terminator: Box::new(|| Ok(())), }; let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); @@ -919,8 +1465,8 @@ mod tests { stdout: reader_handle(&stdout_r), stderr: null_pipe_handle(), stdin: null_pipe_handle(), - waiter: Box::new(|| Ok(0)), - terminator: Box::new(|| {}), + waiter: Box::new(|| Ok(ExecOutcome::Exited(0))), + terminator: Box::new(|| Ok(())), }; let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); @@ -990,8 +1536,8 @@ mod tests { stdout: reader_handle(&reader), stderr: null_pipe_handle(), stdin: null_pipe_handle(), - waiter: Box::new(|| Ok(0)), - terminator: Box::new(|| {}), + waiter: Box::new(|| Ok(ExecOutcome::Exited(0))), + terminator: Box::new(|| Ok(())), }; let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); // The adapter duplicated the handle; the test's original can now drop. @@ -1015,8 +1561,8 @@ mod tests { stdout: null_pipe_handle(), stderr: null_pipe_handle(), stdin: writer_handle(&writer), - waiter: Box::new(|| Ok(0)), - terminator: Box::new(|| {}), + waiter: Box::new(|| Ok(ExecOutcome::Exited(0))), + terminator: Box::new(|| Ok(())), }; let mut proc = ExecSandboxProcess::from_exec_handle(handle).unwrap(); drop(writer); // original write end closed; adapter owns a duplicate diff --git a/src/core/wxc_common/src/sandbox_process.rs b/src/core/wxc_common/src/sandbox_process.rs index 058911774..12ef814ca 100644 --- a/src/core/wxc_common/src/sandbox_process.rs +++ b/src/core/wxc_common/src/sandbox_process.rs @@ -119,9 +119,18 @@ pub trait SandboxProcess: Send { /// /// Any stdout/stderr the caller did not `take_*` is drained and discarded /// while waiting so the child can never block on a full pipe. If the - /// timeout elapses, the child and its tree are killed and + /// timeout elapses, the workload is terminated and /// [`ErrorKind::TimedOut`](std::io::ErrorKind::TimedOut) is returned. /// + /// **How far that termination reaches is the implementation's to state.** + /// The process-spawning implementations kill the whole tree — the child + /// leads its own process group on Unix and is assigned a job object on + /// Windows. An implementation whose only primitive is the foreground + /// process (the state-aware exec adapter, over a backend with no tree API) + /// confirms that process and leaves descendants to whatever owns the + /// sandbox's lifetime. Do not read `TimedOut` as a tree kill without + /// checking the backend. + /// /// Implementors must drain the not-taken stdout and stderr **concurrently** /// (not one then the other) — see the type-level pipe-deadlock contract. fn wait(&mut self) -> std::io::Result; diff --git a/src/core/wxc_common/src/state_aware_backend.rs b/src/core/wxc_common/src/state_aware_backend.rs index 814ab0b59..b22826990 100644 --- a/src/core/wxc_common/src/state_aware_backend.rs +++ b/src/core/wxc_common/src/state_aware_backend.rs @@ -114,6 +114,69 @@ pub enum ExecConsumer { Library, } +/// The error a backend returns when it is asked to serve +/// [`ExecConsumer::Library`] and cannot. +/// +/// A backend that relays the workload's output internally — to the *host +/// process's* own stdout and stderr — has no streams to hand back. It must +/// refuse **before running anything**: the workload is arbitrary and may not be +/// idempotent, so a refusal issued after the fact has already caused the +/// side effects the caller is being told did not happen, and has already +/// written its output somewhere the caller never asked for. +/// +/// Shared so the refusal reads identically whichever backend raises it, and so +/// the check cannot drift into a per-backend spelling. +pub fn unsupported_library_exec(backend: &str) -> MxcError { + MxcError::backend_error(format!( + "the {backend} backend does not support exec for an in-process caller: it relays the \ + sandbox's output to this process's own stdout and stderr rather than returning streams. \ + Nothing has been run." + )) +} + +/// How an exec finished — as distinct from *why a wait failed*. +/// +/// A timeout is an **outcome**, not an error: the backend observed the deadline +/// and the workload is no longer running. Reserving `Err` for a genuine +/// inability to determine the exit is what lets a caller tell "it ran too long" +/// apart from "I could not find out what happened", which are different problems +/// with different responses. +/// +/// # Which consumers can see `TimedOut` +/// +/// Only [`ExecConsumer::Library`]. The executor path has nowhere to put it: +/// `ScriptResponse` carries an `exit_code` and no timeout field, so `wxc-exec` +/// reports a timed-out workload as the exit code its killed process produced. +/// A backend serving `ExecConsumer::Executor` therefore keeps returning +/// [`Exited`](Self::Exited) exactly as before — giving the CLI a timeout channel +/// is a change to its output contract, and belongs with that work rather than +/// here. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ExecOutcome { + /// The process exited with this code. + Exited(i32), + /// The request's timeout elapsed while the process was running, and the + /// process is no longer running when this is reported. + /// + /// **Deadline spent, and the process is gone** — deliberately not "the + /// backend killed it". A workload that overruns its deadline and then exits + /// on its own a moment later has still missed the deadline, and reporting + /// the exit code it happened to produce would hide that from a caller who + /// asked for one. What killed it is not the caller's question; whether the + /// deadline held is. + /// + /// The exit code is deliberately absent: on the killed path it would + /// describe the kill rather than the workload, and on the late-exit path + /// reporting it is exactly the confusion this variant exists to prevent. + /// + /// **How far "gone" reaches is the backend's to state.** Backends that own + /// a process tree or a container kill the whole thing; a backend whose only + /// primitive is the foreground process confirms that process and leaves + /// descendants to whatever owns the sandbox's lifetime. Neither is implied + /// here — see the backend's own documentation. + TimedOut, +} + /// Streaming exec handle. The dispatcher relays `stdout` / `stderr` to the /// executor's own streams, awaits exit via `waiter`, and calls `terminator` to /// tear the exec down. @@ -131,12 +194,25 @@ pub enum ExecConsumer { /// needs an ownership model this type does not have yet — a pipe reaches EOF /// only once *every* write handle is closed, and nothing here can close the /// backend's original. +/// +/// # Reporting failure +/// +/// Both closures are fallible, and for the same reason: the backend is the only +/// layer that knows, and a consumer that cannot be told has to guess. +/// +/// - `waiter` returns an [`ExecOutcome`], so a timeout is reported as one rather +/// than disguised as the exit code of a killed process. `Err` means the exit +/// could not be determined — **not** that the process is gone. +/// - `terminator` returns `Result`, so a kill that the platform refused reaches +/// the caller instead of being swallowed. It reports whether the request was +/// *accepted*; a backend that can also confirm the process died should say so +/// in its own documentation, because this type cannot express the difference. pub struct ExecHandle { pub stdout: PipeHandle, pub stderr: PipeHandle, pub stdin: PipeHandle, - pub waiter: Box Result + Send>, - pub terminator: Box, + pub waiter: Box Result + Send>, + pub terminator: Box Result<(), MxcError> + Send>, } // Manual Debug impl: the boxed closures can't derive Debug. Pipe handles are @@ -228,12 +304,23 @@ pub trait StatefulSandboxBackend { /// host console. Under `Executor` it relays internally and returns null /// handles. /// - **Windows Sandbox** and **WSLc** relay internally and return null - /// handles whatever the caller asked for, so for those two the streaming - /// path still yields a process with no streams — treat them as - /// executor-path only. + /// handles whatever the caller asked for. Under `Library` the streaming + /// adapter now **refuses** such a handle rather than wrapping it, so an + /// in-process caller gets a typed error naming the reason instead of a + /// process with no streams — treat those two as executor-path only. /// /// Reaching any of this from an in-process caller additionally requires the /// experimental opt-in, which the state-aware entry points do not yet expose. + /// + /// # Backends that cannot serve `Library` + /// + /// A backend that relays the workload's output to the *host process's* own + /// stdio, rather than returning streams, must refuse [`ExecConsumer::Library`] + /// **before running anything** — see [`unsupported_library_exec`], which is + /// the shared refusal. Returning a handle with no streams instead is a + /// contract violation: by then the workload has run, so the refusal the + /// caller eventually receives describes side effects that have already + /// happened and output that has already gone somewhere it never asked for. fn exec( &mut self, sandbox_id: &str, diff --git a/src/core/wxc_common/src/state_aware_dispatch.rs b/src/core/wxc_common/src/state_aware_dispatch.rs index fdb0984c8..521fa9202 100644 --- a/src/core/wxc_common/src/state_aware_dispatch.rs +++ b/src/core/wxc_common/src/state_aware_dispatch.rs @@ -28,7 +28,7 @@ use crate::id::parse_sandbox_id_prefix; use crate::models::ContainmentBackend; use crate::mxc_error::{MxcError, ResponseEnvelope}; use crate::state_aware_backend::{ - DeprovisionResult, ExecConsumer, ExecHandle, ProvisionResult, StartResult, + DeprovisionResult, ExecConsumer, ExecHandle, ExecOutcome, ProvisionResult, StartResult, StatefulSandboxBackend, StopResult, }; use crate::state_aware_request::{ParsedStateAwareRequest, Phase}; @@ -280,8 +280,8 @@ struct RelayStreams { /// `Err` directly and observe the exec being terminated and then reaped. fn relay_prepared_streams( streams: Result, - waiter: Box Result + Send>, - terminator: Box, + waiter: Box Result + Send>, + terminator: Box Result<(), MxcError> + Send>, ) -> Result { let streams = match streams { Ok(streams) => streams, @@ -311,7 +311,7 @@ fn relay_prepared_streams( // `terminator` stays alive until the end of this scope: the closure may own // resources tied to the running process, and the waiter-error path below // still needs to invoke it. - let exit_code = waiter(); + let outcome = waiter(); // A waiter error means "I could not determine the exit", not "the child is // dead" -- so the workload may still be running and still holding the write @@ -319,14 +319,29 @@ fn relay_prepared_streams( // is bounded, so the cost of getting this order wrong is not a hang: it is a // stall for the whole grace period, followed by the loss of whatever output // was still buffered behind those write ends. - if exit_code.is_err() { - terminator(); + if outcome.is_err() { + let _ = terminator(); } // Drain what the child wrote before it exited. Bounded -- see `drain_pumps`. drain_pumps(pumps); - exit_code + // `ExecOutcome::TimedOut` is not reachable here, and the mapping says so out + // loud rather than inventing an exit code for it. A backend serving + // `ExecConsumer::Executor` has already run the workload to completion by the + // time it returns, so it reports `Exited`; and the executor has nowhere to + // put a timeout anyway, since `ScriptResponse` carries an exit code and no + // timeout field. Surfacing a contract violation beats fabricating a number + // the CLI would then report as the workload's own. + match outcome { + Ok(ExecOutcome::Exited(code)) => Ok(code), + Ok(ExecOutcome::TimedOut) => Err(MxcError::backend_error( + "backend reported a timeout to the executor relay, which has no way \ + to represent one; a backend serving ExecConsumer::Executor must \ + report ExecOutcome::Exited", + )), + Err(error) => Err(error), + } } /// Join `pumps`, giving them at most [`POST_EXIT_DRAIN_GRACE`] between them. @@ -418,12 +433,12 @@ impl Pump { /// The waiter's own outcome is discarded: the setup error is what the caller /// needs to see, and masking it with a teardown result would hide the cause. fn abort_relay( - waiter: Box Result + Send>, - terminator: Box, + waiter: Box Result + Send>, + terminator: Box Result<(), MxcError> + Send>, pumps: Vec, error: MxcError, ) -> Result { - terminator(); + let _ = terminator(); drain_pumps(pumps); let _ = waiter(); Err(error) @@ -1214,12 +1229,39 @@ mod tests { stdout: null_pipe_handle(), stderr: null_pipe_handle(), stdin: null_pipe_handle(), - waiter: Box::new(|| Ok(42)), - terminator: Box::new(|| {}), + waiter: Box::new(|| Ok(ExecOutcome::Exited(42))), + terminator: Box::new(|| Ok(())), }; assert_eq!(relay_exec_to_stdio(handle).unwrap(), 42); } + /// A backend that reports a timeout to the executor relay has broken the + /// contract, and the relay says so rather than inventing an exit code. + /// + /// `ExecOutcome::TimedOut` is unreachable here by construction — a backend + /// serving `ExecConsumer::Executor` has run the workload to completion + /// before returning, and `ScriptResponse` has no timeout field to carry one + /// anyway. The regression this pins is the tempting alternative: mapping it + /// to some sentinel code, which the CLI would then report as the workload's + /// own exit status. + #[test] + fn relay_refuses_a_timeout_rather_than_inventing_an_exit_code() { + let handle = ExecHandle { + stdout: null_pipe_handle(), + stderr: null_pipe_handle(), + stdin: null_pipe_handle(), + waiter: Box::new(|| Ok(ExecOutcome::TimedOut)), + terminator: Box::new(|| Ok(())), + }; + let err = + relay_exec_to_stdio(handle).expect_err("the executor relay cannot represent a timeout"); + assert!( + err.message.contains("ExecConsumer::Executor"), + "the refusal should name the contract it is enforcing: {}", + err.message + ); + } + /// A waiter error still surfaces unchanged through the relay. #[test] fn relay_with_null_handles_propagates_waiter_error() { @@ -1228,7 +1270,7 @@ mod tests { stderr: null_pipe_handle(), stdin: null_pipe_handle(), waiter: Box::new(|| Err(MxcError::backend_error("waiter blew up"))), - terminator: Box::new(|| {}), + terminator: Box::new(|| Ok(())), }; let err = relay_exec_to_stdio(handle).unwrap_err(); assert_eq!(err.code, MxcErrorCode::BackendError); @@ -1265,8 +1307,8 @@ mod tests { stdout: reader_handle(&reader), stderr: null_pipe_handle(), stdin: null_pipe_handle(), - waiter: Box::new(|| Ok(3)), - terminator: Box::new(|| {}), + waiter: Box::new(|| Ok(ExecOutcome::Exited(3))), + terminator: Box::new(|| Ok(())), }; assert_eq!(relay_exec_to_stdio(handle).unwrap(), 3); @@ -1300,10 +1342,11 @@ mod tests { )), Box::new(move || { let _ = waiter_tx.send("waiter"); - Ok(0) + Ok(ExecOutcome::Exited(0)) }), Box::new(move || { let _ = tx.send("terminator"); + Ok(()) }), ); @@ -1334,9 +1377,10 @@ mod tests { stdout: null_pipe_handle(), stderr: null_pipe_handle(), stdin: null_pipe_handle(), - waiter: Box::new(|| Ok(0)), + waiter: Box::new(|| Ok(ExecOutcome::Exited(0))), terminator: Box::new(move || { let _ = tx.send(()); + Ok(()) }), }; assert_eq!(relay_exec_to_stdio(handle).unwrap(), 0); @@ -1476,10 +1520,10 @@ mod tests { } started += 1; } - Ok(0) + Ok(ExecOutcome::Exited(0)) }); - let result = relay_prepared_streams(Ok(streams), waiter, Box::new(|| {})); + let result = relay_prepared_streams(Ok(streams), waiter, Box::new(|| Ok(()))); assert_eq!( result.expect("the pumps must be draining while the waiter waits"), 0 @@ -1517,6 +1561,7 @@ mod tests { Box::new(move || { flag.store(true, Ordering::Relaxed); closer.lock().expect("writer lock").take(); + Ok(()) }), ); let elapsed = started.elapsed(); @@ -1640,9 +1685,10 @@ mod tests { let started = Instant::now(); let result = abort_relay( - Box::new(|| Ok(0)), + Box::new(|| Ok(ExecOutcome::Exited(0))), Box::new(move || { closer.lock().expect("writer lock").take(); + Ok(()) }), vec![pump], MxcError::backend_error("stream setup failed"),