From f0a5731a2bf186bf27665288e0932ffc71bf6c7b Mon Sep 17 00:00:00 2001 From: ashwind Date: Thu, 5 Nov 2026 00:20:41 -0800 Subject: [PATCH 1/2] 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( From 6d3d2ce10856cee33d289c19d98c41c2923c05d7 Mon Sep 17 00:00:00 2001 From: ashwind Date: Mon, 9 Nov 2026 22:55:46 +0530 Subject: [PATCH 2/2] fix(windows): rely on mio 1.2.1 for non-inheritable listener sockets Addresses review feedback on #5299. mio 1.2.1 creates Windows sockets with WSA_FLAG_NO_HANDLE_INHERIT (1.2.0 used plain socket(), which is inheritable), so a sandbox process the gateway spawns can no longer keep a copy of the gateway's listening socket and hold its port after a gateway crash. Update mio from 1.2.0 to 1.2.1 and remove the Win32 wrapper that cleared the inherit flag after bind, together with the windows dependency it added to openshell-server; windows is now a test-only dependency there. The listener tests stay, so a regression to an inheritable socket still fails the build. Cargo.lock changes only the mio entry (version and checksum); its dependency list is unchanged and tokio 1.52.1 requires mio ^1.2.0. Tests: on Windows x64, bound_gateway_listener_is_not_inheritable and listener_accepts_connections pass without the wrapper. With three sandboxes running, hard-killing the gateway with the job object disabled leaves the nine sandbox processes alive but the port can be bound and a new gateway starts (2 of 2 runs), so the mio update alone frees the port. With the job object enabled no process survives and the port can be bound (3 of 3 runs). cargo fmt, cargo check --locked and clippy with -D warnings pass for openshell-gateway and openshell-server on Linux. Refs #5298 Signed-off-by: ashwind --- Cargo.lock | 4 +- crates/openshell-server/Cargo.toml | 8 +-- .../openshell-server/src/gateway_listener.rs | 72 ++++--------------- crates/openshell-server/src/lib.rs | 4 +- 4 files changed, 22 insertions(+), 66 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8a7b6fc48f..ec3286b914 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3849,9 +3849,9 @@ dependencies = [ [[package]] name = "mio" -version = "1.2.0" +version = "1.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "50b7e5b27aa02a74bac8c3f23f448f8d87ff11f92d3aac1a6ed369ee08cc56c1" +checksum = "02bd0af71c67b473010cbbc60715ee815645a4dc942899111f494b4b737d6fda" dependencies = [ "libc", "log", diff --git a/crates/openshell-server/Cargo.toml b/crates/openshell-server/Cargo.toml index e8b35997d3..83e5f491ff 100644 --- a/crates/openshell-server/Cargo.toml +++ b/crates/openshell-server/Cargo.toml @@ -116,10 +116,6 @@ 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). @@ -150,5 +146,9 @@ wiremock = "0.6" # `testing` provides InMemorySpanExporter, so span assertions do not need a # collector, a network hop, or a flush barrier. opentelemetry_sdk = { workspace = true, features = ["testing"] } +# Windows-only tests read the inherit flag of the gateway's listening socket. +[target.'cfg(target_os = "windows")'.dev-dependencies] +windows = { workspace = true } + [lints] workspace = true diff --git a/crates/openshell-server/src/gateway_listener.rs b/crates/openshell-server/src/gateway_listener.rs index 23a5e8a99f..3ebfc3ec02 100644 --- a/crates/openshell-server/src/gateway_listener.rs +++ b/crates/openshell-server/src/gateway_listener.rs @@ -5,8 +5,6 @@ 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, @@ -18,7 +16,6 @@ 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)) } - } + use windows::Win32::Foundation::{GetHandleInformation, HANDLE, HANDLE_FLAG_INHERIT}; - #[cfg(test)] - pub(super) fn is_inheritable(listener: &TcpListener) -> bool { - use windows::Win32::Foundation::GetHandleInformation; + use super::*; + fn is_inheritable(listener: &TcpListener) -> bool { + let handle = HANDLE(listener.as_raw_socket() as *mut c_void); 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"); + unsafe { GetHandleInformation(handle, &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() { @@ -95,13 +53,13 @@ mod tests { .await .unwrap(); assert!( - !windows_impl::is_inheritable(&bound.listener), + !is_inheritable(&bound.listener), "child processes would inherit the gateway's listening socket" ); } #[tokio::test] - async fn listener_still_accepts_connections_after_disabling_inheritance() { + async fn listener_accepts_connections() { let bound = bind_gateway_listener("127.0.0.1:0".parse().unwrap()) .await .unwrap(); diff --git a/crates/openshell-server/src/lib.rs b/crates/openshell-server/src/lib.rs index 8b88d25410..1390f61a11 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, disable_handle_inheritance}; +use gateway_listener::{BoundGatewayListener, bind_gateway_listener}; pub use grpc::OpenShellService; pub use http::{health_router, http_router, metrics_router, service_http_router}; @@ -854,7 +854,6 @@ 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 @@ -881,7 +880,6 @@ 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(