From f0a5731a2bf186bf27665288e0932ffc71bf6c7b Mon Sep 17 00:00:00 2001 From: ashwind Date: Thu, 5 Nov 2026 00:20:41 -0800 Subject: [PATCH] fix(windows): end sandbox processes when the gateway is killed When the MXC driver runs inside the gateway, a hard-killed gateway left its sandbox processes (wxc-exec, openshell-supervisor and openshell-windows-sandbox) running with no owner. They also kept the gateway's listening socket open, so the port stayed bound and a new gateway failed to start with "Only one usage of each socket address" (os error 10048). There were two causes, so there are two changes: - The gateway now puts itself in a kill-on-close job object before it spawns anything. Every sandbox process joins the job, and Windows ends them all when the gateway process exits for any reason, including a hard kill. If the job cannot be created the gateway logs a warning and continues. - mio creates listening sockets with plain socket(), which leaves the handle inheritable, and child processes are started with handle inheritance enabled, so each sandbox process held a copy of the socket. The gateway, health and metrics listeners now clear the inherit flag right after bind. Either change releases the port on its own; the job object also removes the orphaned processes. Tests: unit tests check that the process is in a kill-on-close job and that a bound gateway listener is not inheritable and still accepts connections. On a native Windows 11 x64 host with three running sandboxes, hard-killing the gateway left 9 processes and an unbindable port in 5 of 5 runs before this change. With it, no process survived, the port could be bound and a new gateway started in 3 of 3 runs on the final build, and in 5 of 5 runs on an earlier build that differed only by two lint-driven edits. The existing MXC end-to-end scenarios give the same results with and without the change. Signed-off-by: ashwind --- Cargo.lock | 2 + crates/openshell-gateway/Cargo.toml | 1 + crates/openshell-gateway/src/lib.rs | 2 + crates/openshell-gateway/src/main.rs | 6 + .../openshell-gateway/src/process_lifetime.rs | 114 ++++++++++++++++++ crates/openshell-server/Cargo.toml | 4 + .../openshell-server/src/gateway_listener.rs | 88 ++++++++++++++ crates/openshell-server/src/lib.rs | 4 +- 8 files changed, 220 insertions(+), 1 deletion(-) create mode 100644 crates/openshell-gateway/src/process_lifetime.rs diff --git a/Cargo.lock b/Cargo.lock index a438d9a878..8a7b6fc48f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4643,6 +4643,7 @@ dependencies = [ "tonic", "tower", "tracing", + "windows", ] [[package]] @@ -5011,6 +5012,7 @@ dependencies = [ "tracing-subscriber", "url", "uuid", + "windows", "wiremock", "x509-parser", ] diff --git a/crates/openshell-gateway/Cargo.toml b/crates/openshell-gateway/Cargo.toml index 17b9c959e3..f164c64a64 100644 --- a/crates/openshell-gateway/Cargo.toml +++ b/crates/openshell-gateway/Cargo.toml @@ -38,6 +38,7 @@ tracing = { workspace = true, optional = true } [target.'cfg(target_os = "windows")'.dependencies] openshell-driver-mxc = { path = "../openshell-driver-mxc", optional = true } +windows = { workspace = true, features = ["Win32_System_JobObjects"] } [features] default = ["telemetry", "in-tree-compute-drivers"] diff --git a/crates/openshell-gateway/src/lib.rs b/crates/openshell-gateway/src/lib.rs index 463edd3bc0..64861e8314 100644 --- a/crates/openshell-gateway/src/lib.rs +++ b/crates/openshell-gateway/src/lib.rs @@ -16,6 +16,8 @@ compile_error!( build a telemetry-free gateway with `--no-default-features --features defaults-without-telemetry`" ); +pub mod process_lifetime; + #[cfg(all(not(target_os = "windows"), feature = "compute-driver-vm"))] mod vm; diff --git a/crates/openshell-gateway/src/main.rs b/crates/openshell-gateway/src/main.rs index 85d0611867..cc635aa568 100644 --- a/crates/openshell-gateway/src/main.rs +++ b/crates/openshell-gateway/src/main.rs @@ -3,6 +3,12 @@ #[tokio::main] async fn main() -> miette::Result<()> { + // Must run before anything is spawned so every sandbox process joins the job. + if let Err(error) = openshell_gateway::process_lifetime::kill_children_on_exit() { + eprintln!( + "warning: could not tie sandbox processes to the gateway lifetime; they will outlive it: {error}" + ); + } openshell_server::cli::run_cli_with_compute_drivers( openshell_gateway::install_default_compute_drivers(), ) diff --git a/crates/openshell-gateway/src/process_lifetime.rs b/crates/openshell-gateway/src/process_lifetime.rs new file mode 100644 index 0000000000..ee620acbf9 --- /dev/null +++ b/crates/openshell-gateway/src/process_lifetime.rs @@ -0,0 +1,114 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Tie the lifetime of every sandbox process to the gateway process. +//! +//! With the MXC driver built into the gateway, the gateway starts `wxc-exec`, +//! `openshell-supervisor` and `openshell-windows-sandbox` for each sandbox. If +//! the gateway is killed (crash, `taskkill`, logoff), those processes used to +//! survive with no owner and, because they inherited the gateway's listening +//! socket, kept its port bound so a new gateway could not start. +//! +//! A Windows *job object* with `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` fixes the +//! first half: the gateway puts itself in the job, every child it spawns +//! inherits membership, and Windows ends all members when the last handle to +//! the job closes, which happens when the gateway process ends for any reason. +//! (The listening sockets are also made non-inheritable; see +//! `openshell_server::gateway_listener`.) + +/// Put the current process into a new kill-on-close job object. +/// +/// Call this first thing in `main`, before anything is spawned. The job handle +/// is deliberately never closed: it must stay open for as long as the process +/// lives, and closing it (which only happens when the process exits) is what +/// triggers the clean-up. On other platforms this does nothing. +/// +/// # Errors +/// +/// Returns a description of the failing Win32 call. The caller decides whether +/// that is fatal; nested jobs are supported on Windows 8 and later, so this +/// normally only fails on unusual hosts. +pub fn kill_children_on_exit() -> Result<(), String> { + #[cfg(windows)] + { + windows_impl::kill_children_on_exit() + } + #[cfg(not(windows))] + { + Ok(()) + } +} + +#[cfg(windows)] +#[allow(unsafe_code)] // Win32 job object calls. +mod windows_impl { + use std::mem::size_of; + + use windows::Win32::System::JobObjects::{ + AssignProcessToJobObject, CreateJobObjectW, JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + JOBOBJECT_EXTENDED_LIMIT_INFORMATION, JobObjectExtendedLimitInformation, + SetInformationJobObject, + }; + use windows::Win32::System::Threading::GetCurrentProcess; + use windows::core::PCWSTR; + + pub(super) fn kill_children_on_exit() -> Result<(), String> { + // SAFETY: plain Win32 calls with valid arguments; the info struct + // outlives the call and `size_of` matches its type. + unsafe { + let job = CreateJobObjectW(None, PCWSTR::null()) + .map_err(|e| format!("CreateJobObjectW failed: {e}"))?; + let mut info = JOBOBJECT_EXTENDED_LIMIT_INFORMATION::default(); + info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + SetInformationJobObject( + job, + JobObjectExtendedLimitInformation, + (&raw const info).cast(), + u32::try_from(size_of::()) + .map_err(|e| format!("job info size: {e}"))?, + ) + .map_err(|e| format!("SetInformationJobObject failed: {e}"))?; + AssignProcessToJobObject(job, GetCurrentProcess()) + .map_err(|e| format!("AssignProcessToJobObject failed: {e}"))?; + // `job` is intentionally leaked (see the module docs). + let _ = job; + } + Ok(()) + } +} + +#[cfg(all(test, windows))] +#[allow(unsafe_code)] // Win32 job object query. +mod tests { + use std::mem::size_of; + + use windows::Win32::System::JobObjects::{ + JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, JOBOBJECT_EXTENDED_LIMIT_INFORMATION, + JobObjectExtendedLimitInformation, QueryInformationJobObject, + }; + + #[test] + fn puts_the_process_in_a_kill_on_close_job() { + super::kill_children_on_exit().expect("job object"); + + // A null job handle asks about the job the calling process is in; with + // nested jobs that is the innermost one, i.e. the one just created. + let mut info = JOBOBJECT_EXTENDED_LIMIT_INFORMATION::default(); + // SAFETY: `info` outlives the call and the size matches its type. + unsafe { + QueryInformationJobObject( + None, + JobObjectExtendedLimitInformation, + (&raw mut info).cast(), + u32::try_from(size_of::()).unwrap(), + None, + ) + } + .expect("QueryInformationJobObject"); + assert_ne!( + info.BasicLimitInformation.LimitFlags.0 & JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE.0, + 0, + "the process job must kill its members when closed" + ); + } +} diff --git a/crates/openshell-server/Cargo.toml b/crates/openshell-server/Cargo.toml index d3bed5daff..e8b35997d3 100644 --- a/crates/openshell-server/Cargo.toml +++ b/crates/openshell-server/Cargo.toml @@ -116,6 +116,10 @@ x509-parser = "0.16" arc-swap = "1" notify = "8" +[target.'cfg(target_os = "windows")'.dependencies] +# Clears the handle-inherit flag on the gateway's listening sockets (see gateway_listener.rs). +windows = { workspace = true } + [features] default = ["telemetry"] ## Compile in anonymous telemetry emission (forwards to openshell-core/telemetry). diff --git a/crates/openshell-server/src/gateway_listener.rs b/crates/openshell-server/src/gateway_listener.rs index afc2f4a763..23a5e8a99f 100644 --- a/crates/openshell-server/src/gateway_listener.rs +++ b/crates/openshell-server/src/gateway_listener.rs @@ -5,6 +5,8 @@ use openshell_core::{Error, Result}; use std::net::SocketAddr; use tokio::net::TcpListener; use tracing::info; +#[cfg(windows)] +use tracing::warn; pub struct BoundGatewayListener { pub listener: TcpListener, @@ -16,6 +18,7 @@ pub async fn bind_gateway_listener(address: SocketAddr) -> Result Result HANDLE { + // A SOCKET is a pointer-sized handle value; this is an integer-to-pointer cast. + HANDLE(listener.as_raw_socket() as *mut c_void) + } + + pub(super) fn disable_handle_inheritance(listener: &TcpListener) -> windows::core::Result<()> { + // SAFETY: `listener` owns a live socket handle for the whole call. + unsafe { SetHandleInformation(handle_of(listener), HANDLE_FLAG_INHERIT.0, HANDLE_FLAGS(0)) } + } + + #[cfg(test)] + pub(super) fn is_inheritable(listener: &TcpListener) -> bool { + use windows::Win32::Foundation::GetHandleInformation; + + let mut flags = 0u32; + // SAFETY: `listener` owns a live socket handle and `flags` outlives the call. + unsafe { GetHandleInformation(handle_of(listener), &raw mut flags) } + .expect("GetHandleInformation"); + flags & HANDLE_FLAG_INHERIT.0 != 0 + } +} + +#[cfg(all(test, windows))] +mod tests { + use super::*; + + #[tokio::test] + async fn bound_gateway_listener_is_not_inheritable() { + let bound = bind_gateway_listener("127.0.0.1:0".parse().unwrap()) + .await + .unwrap(); + assert!( + !windows_impl::is_inheritable(&bound.listener), + "child processes would inherit the gateway's listening socket" + ); + } + + #[tokio::test] + async fn listener_still_accepts_connections_after_disabling_inheritance() { + let bound = bind_gateway_listener("127.0.0.1:0".parse().unwrap()) + .await + .unwrap(); + let client = tokio::net::TcpStream::connect(bound.address); + let (accepted, connected) = tokio::join!(bound.listener.accept(), client); + accepted.expect("accept"); + connected.expect("connect"); + } +} diff --git a/crates/openshell-server/src/lib.rs b/crates/openshell-server/src/lib.rs index 1390f61a11..8b88d25410 100644 --- a/crates/openshell-server/src/lib.rs +++ b/crates/openshell-server/src/lib.rs @@ -89,7 +89,7 @@ pub(crate) fn install_jsonwebtoken_crypto_provider() { } use compute::ComputeRuntime; -use gateway_listener::{BoundGatewayListener, bind_gateway_listener}; +use gateway_listener::{BoundGatewayListener, bind_gateway_listener, disable_handle_inheritance}; pub use grpc::OpenShellService; pub use http::{health_router, http_router, metrics_router, service_http_router}; @@ -854,6 +854,7 @@ pub(crate) async fn run_server( "failed to bind health port {health_bind_address}: {e}" )) })?; + disable_handle_inheritance(&health_listener, "health"); info!(address = %health_bind_address, "Health server listening"); // `health_router` returns immediately; the listener serves // `Initializing → 503` until the background monitor publishes the @@ -880,6 +881,7 @@ pub(crate) async fn run_server( "failed to bind metrics port {metrics_bind_address}: {e}", )) })?; + disable_handle_inheritance(&metrics_listener, "metrics"); info!(address = %metrics_bind_address, "Metrics server listening"); tokio::spawn(async move { if let Err(e) = axum::serve(