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-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/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 b9a5546bf2..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,23 +1817,42 @@ pub async fn run_sandbox_instance( )); } () = &mut shutdown_requested => { - 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. @@ -1853,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, }); 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 {