From 235e738a5dc725d5d421ad32e5d77657ea73df6d Mon Sep 17 00:00:00 2001 From: divesh Date: Fri, 6 Nov 2026 11:14:23 -0800 Subject: [PATCH 1/2] fix(supervisor): stop recovering a lost boundary after SIGTERM After SIGTERM the supervisor signals, waits for, and terminates the boundary. When the boundary connection drops during that sequence, each call retried reattach for up to 30s, so the supervisor outlived the runtime's stop timeout and was killed. Once shutdown starts, report a lost connection instead of recovering it. Un-ignore the conformance test that covers slow-TERM deletes. Signed-off-by: divesh --- .../openshell-sandbox-backend/src/runtime.rs | 81 +++++++++++++++++++ crates/openshell-supervisor/src/lib.rs | 1 + tests/suites/conformance/src/lifecycle.rs | 4 - 3 files changed, 82 insertions(+), 4 deletions(-) diff --git a/crates/openshell-sandbox-backend/src/runtime.rs b/crates/openshell-sandbox-backend/src/runtime.rs index 9ea72d396c..319a98ce8c 100644 --- a/crates/openshell-sandbox-backend/src/runtime.rs +++ b/crates/openshell-sandbox-backend/src/runtime.rs @@ -958,6 +958,12 @@ impl RunningBoundary { expect_response(response, "boundary_terminated") } + /// Stop waiting for a lost boundary connection to recover. The owner is + /// stopping, and the boundary may be stopping with it. + pub fn begin_stop(&self) { + self.process.client.stopping.store(true, Ordering::Release); + } + /// Whether the negotiated backend advertised suspend/resume support. pub fn supports_suspend(&self) -> bool { self.process @@ -1601,6 +1607,9 @@ struct BoundaryClient { recovery_timeout_secs: Option, /// True once a connection of this client was confirmed. confirmed: tokio::sync::watch::Sender, + /// Set when the owner stops the boundary; a lost connection is then + /// reported instead of recovered. + stopping: AtomicBool, } #[derive(Clone)] @@ -1800,6 +1809,7 @@ impl BoundaryClient { requires_takeover: false, recovery_timeout_secs: None, confirmed: tokio::sync::watch::Sender::new(false), + stopping: AtomicBool::new(false), } } @@ -2208,6 +2218,11 @@ impl BoundaryClient { } cached.transport.abort(); } + if self.stopping.load(Ordering::Acquire) { + return Err(BackendError::Unavailable( + "boundary connection lost while stopping".to_string(), + )); + } *self.mediation.lock().await = None; let attach = self @@ -3970,6 +3985,72 @@ mod tests { assert_eq!(requests.load(Ordering::Acquire), 5); } + #[tokio::test] + async fn stopping_client_reports_a_lost_boundary_without_recovery() { + let certificate = test_certificate(); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let server_config = certificate.server_config.clone(); + let server = tokio::spawn(async move { + let (stream, _) = listener.accept().await.unwrap(); + let stream = tokio_rustls::TlsAcceptor::from(server_config) + .accept(stream) + .await + .unwrap(); + let service = TestGrpcBoundary { + wait_for_half_close: false, + expected_token: "a".repeat(32), + requests: Arc::default(), + mediation_failures: Arc::new(std::sync::atomic::AtomicUsize::new(0)), + mediation_ready: false, + provider_environment_generation: 0, + confirmation: test_confirmation(), + attach_snapshots: Arc::default(), + received: Arc::default(), + }; + tonic::transport::Server::builder() + .add_service(IsolationBackendServer::new(TestIsolationBackend::default())) + .add_service(SandboxProtocolServer::new(service)) + .serve_with_incoming(tokio_stream::iter([Ok::<_, std::io::Error>(TestTlsIo( + Box::new(stream), + ))])) + .await + .unwrap(); + }); + let client = BoundaryClient::new( + tls_runtime_descriptor(address, certificate.client_tls), + test_bearer(&"a".repeat(32)), + ); + let attach = Request::Attach { + supervisor_instance_id: client.supervisor_instance_id, + policy: Box::new(SandboxPolicyWire::from(sandbox().policy)), + supervisor_takeover: None, + supervisor_recovery_timeout_secs: None, + }; + client.call_idempotent(attach).await.unwrap(); + client.call_idempotent(Request::Confirm).await.unwrap(); + + // The boundary stops with its owner: no listener remains to reattach. + server.abort(); + let _ = server.await; + client.stopping.store(true, Ordering::Release); + client + .grpc_channel + .lock() + .await + .as_ref() + .expect("cached channel") + .transport + .abort(); + let result = tokio::time::timeout( + Duration::from_secs(5), + client.call_idempotent(Request::TerminateBoundary), + ) + .await + .expect("a stopping client must not wait for the boundary to recover"); + assert!(matches!(result, Err(BackendError::Unavailable(_)))); + } + #[test] fn wait_recovery_window_begins_at_transport_failure() { let wait_started = tokio::time::Instant::now(); diff --git a/crates/openshell-supervisor/src/lib.rs b/crates/openshell-supervisor/src/lib.rs index b9a5546bf2..68d9126c35 100644 --- a/crates/openshell-supervisor/src/lib.rs +++ b/crates/openshell-supervisor/src/lib.rs @@ -1800,6 +1800,7 @@ pub async fn run_sandbox_instance( )); } () = &mut shutdown_requested => { + running.begin_stop(); let _ = agent .signal(openshell_isolation_interface::contract::BoundarySignal::Term) .await; diff --git a/tests/suites/conformance/src/lifecycle.rs b/tests/suites/conformance/src/lifecycle.rs index dd47c0b4d6..6af1d145b3 100644 --- a/tests/suites/conformance/src/lifecycle.rs +++ b/tests/suites/conformance/src/lifecycle.rs @@ -52,11 +52,7 @@ const DELETE_DEADLINE: Duration = Duration::from_secs(10); /// Many workloads do not exit immediately on TERM, for example while they run /// cleanup handlers. Deleting such a running sandbox should still complete /// promptly, rather than waiting out the runtime's termination grace period. -// -// Unignore this when the supervisor half of #5025 lands: on Podman the -// supervisor container still waits out its stop timeout. #[testkit::test] -#[ignore = "https://github.com/NVIDIA/OpenShell/issues/5025"] async fn delete_is_prompt_when_workload_is_slow_to_handle_term( cx: &mut Context, ) -> testkit::Result { From 91b6ccf005cfbbebc67dadd6aee006423bf79065 Mon Sep 17 00:00:00 2001 From: divesh Date: Fri, 6 Nov 2026 16:24:46 -0800 Subject: [PATCH 2/2] fix(supervisor): bound SIGTERM shutdown and give Docker supervisors 10s Stopping a dedicated sandbox now finishes within 8 seconds, including the exit report and trace flush, while the workload keeps its 5 second TERM grace. A boundary connection lost while stopping is reported as a terminated workload instead of failing the shutdown. Only a dedicated supervisor's own shutdown skips boundary recovery; a shared supervisor removing one sandbox keeps recovering as before. The Docker driver gave the supervisor 5 seconds, the same as its wait for the workload, so stops ended in SIGKILL. It now allows 10 seconds. Signed-off-by: divesh --- crates/openshell-driver-docker/README.md | 4 +- crates/openshell-driver-docker/src/lib.rs | 5 +- crates/openshell-supervisor/README.md | 5 +- crates/openshell-supervisor/src/lib.rs | 87 ++++++++++++++----- .../src/sandbox_instance_host.rs | 1 + 5 files changed, 76 insertions(+), 26 deletions(-) diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index 63caf0d094..f84d792910 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -128,8 +128,8 @@ New generations require matching supervisor and sandbox runtime binaries. ## Stop, Start, and Delete -Stop terminates the supervisor companion and stops the workload container -without removing it. Docker retains the workload writable layer and attached +Stop terminates the supervisor companion, allowing it 10 seconds to stop the +workload, and stops the workload container without removing it. Docker retains the workload writable layer and attached volumes. Start stages a fresh sandbox bootstrap bundle, restarts that workload, and creates a new supervisor companion. A durably stopped sandbox stays stopped across gateway restarts. diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index aa1d7de706..1f0cf90dbf 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -95,6 +95,9 @@ const WATCH_POLL_MAX_BACKOFF: Duration = Duration::from_secs(30); // request a bounded opportunity to mark the control exit intentional before // publishing a fail-closed runtime error. const SUPERVISOR_INTENTIONAL_SHUTDOWN_GRACE: Duration = Duration::from_secs(1); +// Longer than the supervisor's own bound on stopping its workload, so it +// finishes stopping before Docker kills it. +const SUPERVISOR_STOP_TIMEOUT_SECS: i32 = 10; const SUPERVISOR_HEALTH_INTERVAL_NS: i64 = 30_000_000_000; // Docker probes at this interval only before the first healthy result and // within the start period. @@ -5607,7 +5610,7 @@ async fn spawn_docker_control_process( _ = &mut shutdown_requested => { let _ = monitored_docker.stop_container( &monitored_supervisor_id, - Some(StopContainerOptionsBuilder::default().t(5).build()), + Some(StopContainerOptionsBuilder::default().t(SUPERVISOR_STOP_TIMEOUT_SECS).build()), ).await; let _ = remove_supervisor().await; } diff --git a/crates/openshell-supervisor/README.md b/crates/openshell-supervisor/README.md index 1fa7f6ab9e..052f8b7787 100644 --- a/crates/openshell-supervisor/README.md +++ b/crates/openshell-supervisor/README.md @@ -57,7 +57,10 @@ loads the launch configuration from its arguments and environment and prepares push and the OCSF JSON settings with its logging layers, so renewal by either gateway client keeps both current. `run_sandbox_with_backend` sets the process-wide OCSF context, binds the readiness socket and port to instance -readiness, and stops the instance on SIGTERM or SIGINT. +readiness, and stops the instance on SIGTERM or SIGINT. Stopping gives the +workload 5 seconds to exit after TERM and finishes, including the exit report +and trace flush, within 8 seconds. A boundary connection lost while the +process stops is not recovered; the workload is reported as terminated. `sandbox_instance_host::SandboxInstanceHost` runs several sandbox instances in one process. Each instance gets its own Tokio runtime; every thread of that diff --git a/crates/openshell-supervisor/src/lib.rs b/crates/openshell-supervisor/src/lib.rs index 68d9126c35..6c1936e2f7 100644 --- a/crates/openshell-supervisor/src/lib.rs +++ b/crates/openshell-supervisor/src/lib.rs @@ -504,6 +504,17 @@ use provider_readiness::{EnvironmentIdentity, Tracker as ProviderReadinessTracke use tokio::sync::mpsc::UnboundedSender; use tokio::time::timeout; +/// Time the workload gets to exit after TERM before it is killed. +const WORKLOAD_TERM_GRACE: Duration = Duration::from_secs(5); + +/// Bound on stopping an instance, including its exit report and trace flush. +/// Below the 10 second grace the Docker driver gives the supervisor. +const SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(8); + +/// Exit code reported when the boundary is lost before it reports the +/// stopped workload's status: terminated by TERM. +const STOPPED_EXIT_CODE: i32 = 128 + 15; + /// Whether a dedicated supervisor's environment grants the SSH socket to its /// group rather than its owner. #[must_use] @@ -836,6 +847,7 @@ pub async fn run_sandbox_with_backend( bindings, SandboxInstanceControls { shutdown: Box::pin(wait_for_control_shutdown_signal()), + shutdown_ends_process: true, suspend, readiness, }, @@ -999,6 +1011,9 @@ pub struct SandboxInstanceControls { /// Completes when the owner asks the instance to stop its workload. It is /// first polled once the workload has started. pub shutdown: Pin + Send>>, + /// True when `shutdown` also ends this process, so its boundary may be + /// stopping too and a lost connection is not worth recovering. + pub shutdown_ends_process: bool, /// Requests to suspend only this instance. A failed suspension can be /// retried by sending another request. Closing the stream disables it. pub suspend: Pin + Send>>, @@ -1110,6 +1125,7 @@ pub async fn run_sandbox_instance( } = bindings; let SandboxInstanceControls { shutdown: shutdown_requested, + shutdown_ends_process, mut suspend, readiness, } = controls; @@ -1781,6 +1797,7 @@ pub async fn run_sandbox_instance( SandboxInstanceReadiness::start(readiness, boundary_access.session_readiness()); let instance_id = boundary_access.instance_id().to_string(); let mut shutdown_requested = shutdown_requested; + let mut shutdown_deadline = None; // `None` means the boundary was suspended and its reference persisted. let lifecycle = loop { let wait_agent = agent.clone(); @@ -1800,24 +1817,42 @@ pub async fn run_sandbox_instance( )); } () = &mut shutdown_requested => { - running.begin_stop(); - let _ = agent - .signal(openshell_isolation_interface::contract::BoundarySignal::Term) - .await; - let status = if let Ok(result) = timeout(Duration::from_secs(5), agent.wait()).await { - result - } else { - let _ = agent.terminate().await; - agent.wait().await + let deadline = tokio::time::Instant::now() + SHUTDOWN_TIMEOUT; + shutdown_deadline = Some(deadline); + if shutdown_ends_process { + running.begin_stop(); } - .map_err(|error| miette::miette!(error.to_string()))?; - running - .terminate() - .await - .map_err(|error| miette::miette!( - "sandbox did not acknowledge terminal state: {error}" - ))?; - break Some((boundary_exit_code(status), false)); + let stop = async { + let _ = agent + .signal(openshell_isolation_interface::contract::BoundarySignal::Term) + .await; + let status = if let Ok(result) = timeout(WORKLOAD_TERM_GRACE, agent.wait()).await { + result + } else { + let _ = agent.terminate().await; + agent.wait().await + }?; + running.terminate().await?; + Ok(status) + }; + let exit_code = match tokio::time::timeout_at(deadline, stop).await { + Ok(Ok(status)) => boundary_exit_code(status), + Ok(Err( + openshell_isolation_interface::contract::BackendError::Unavailable(error) + | openshell_isolation_interface::contract::BackendError::Terminated(error), + )) => { + warn!(%error, "Isolation boundary lost while stopping; finishing shutdown"); + STOPPED_EXIT_CODE + } + Ok(Err(error)) => { + return Err(miette::miette!("stop sandbox workload: {error}")); + } + Err(_) => { + warn!("Isolation boundary did not stop in time; finishing shutdown"); + STOPPED_EXIT_CODE + } + }; + break Some((exit_code, false)); } // The agent wait is cancelled while suspending so a fenced // generation's stream failure cannot race the suspension. @@ -1854,19 +1889,27 @@ pub async fn run_sandbox_instance( if !retain_access { instance_readiness.release(); } - boundary_access - .publish_main_exit(exit_code, await_main_process_attachment) - .await; + let publish = boundary_access.publish_main_exit(exit_code, await_main_process_attachment); + if let Some(deadline) = shutdown_deadline { + let _ = tokio::time::timeout_at(deadline, publish).await; + } else { + publish.await; + } + let flush_timeout = shutdown_deadline.map_or(otlp_relay::FLUSH_TIMEOUT, |deadline| { + deadline + .saturating_duration_since(tokio::time::Instant::now()) + .min(otlp_relay::FLUSH_TIMEOUT) + }); // Flush relayed agent traces before the exit is reported so the last // spans of a short run are not lost. Bounded; never fails. When the // access plane is retained for exec sessions, the relay keeps serving // them and shuts down at final teardown instead. if retain_access { if let Some(relay) = otlp_relay.as_ref() { - relay.flush(otlp_relay::FLUSH_TIMEOUT).await; + relay.flush(flush_timeout).await; } } else if let Some(relay) = otlp_relay.take() { - relay.shutdown(otlp_relay::FLUSH_TIMEOUT).await; + relay.shutdown(flush_timeout).await; } // `shutdown_requested` has already completed when shutdown won the // lifecycle select above and must not be polled again. diff --git a/crates/openshell-supervisor/src/sandbox_instance_host.rs b/crates/openshell-supervisor/src/sandbox_instance_host.rs index 71af1a6f68..3da50aa3d3 100644 --- a/crates/openshell-supervisor/src/sandbox_instance_host.rs +++ b/crates/openshell-supervisor/src/sandbox_instance_host.rs @@ -689,6 +689,7 @@ fn run_hosted_sandbox_instance( }); let instance = body(SandboxInstanceControls { shutdown, + shutdown_ends_process: false, suspend: Box::pin(tokio_stream::wrappers::ReceiverStream::new(suspend_requested)), readiness, });