From 44ea87229f6bfc56e08329a6ec4dacaf9c3d4d43 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Mon, 26 Oct 2026 16:46:02 -0700 Subject: [PATCH 1/8] fix(server): allow exec on exited sandboxes with a live supervisor After the main process exits, the supervisor keeps its access plane, and SSH and port forwarding already reached Completed and Error sandboxes while it stayed connected. Exec still required Ready in both the CLI and the gateway, so it was refused on the same sandboxes. Exec, SSH sessions, and TCP forwarding now share one gate: a sandbox is reachable when it is Ready, or Completed/Error with a supervisor session on this replica or a fresh owner record naming a peer replica. The previous check only looked at the local session map, so on a multi-replica gateway a request landing on a non-owner replica was refused even though relay opening routes to the owner. The CLI exec pre-check and the service forward recheck accept Ready, Completed, and Error and leave the supervisor decision to the gateway. Signed-off-by: Piotr Mlocek --- crates/openshell-cli/src/run.rs | 25 ++-- .../sandbox_exec_streaming_integration.rs | 41 ++++++- .../grpc/mutation_replay/streaming/tests.rs | 41 +++++++ crates/openshell-server/src/grpc/sandbox.rs | 47 ++++---- .../src/supervisor_session.rs | 109 ++++++++++++++++++ 5 files changed, 233 insertions(+), 30 deletions(-) diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index eb224685ed..b78703d9a5 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -2014,8 +2014,9 @@ pub async fn sandbox_exec_grpc( .sandbox .ok_or_else(|| miette::miette!("sandbox not found"))?; - // Verify the sandbox is ready before issuing the exec. - if SandboxPhase::try_from(sandbox.phase()) != Ok(SandboxPhase::Ready) { + // Reject phases where the relay can never be open. The gateway decides + // whether a Completed or Error sandbox still has a reachable supervisor. + if !relay_phase_may_be_reachable(sandbox.phase()) { return Err(miette::miette!( "sandbox '{}' is not ready (phase: {}); wait for it to reach Ready state", name, @@ -2169,7 +2170,7 @@ pub async fn service_forward_tcp( let (bind_addr, bind_port) = parse_tcp_forward_spec(local, target_port)?; let mut client = grpc_client(server, tls).await?; - let mut replica = fetch_ready_sandbox_for_forward(&mut client, name, workspace).await?; + let mut replica = fetch_reachable_sandbox_for_forward(&mut client, name, workspace).await?; let listener = tokio::net::TcpListener::bind((bind_addr.as_str(), bind_port)) .await @@ -2203,7 +2204,7 @@ pub async fn service_forward_tcp( } _ = health_check.tick() => { - replica = fetch_ready_sandbox_for_forward(&mut client, name, workspace).await?; + replica = fetch_reachable_sandbox_for_forward(&mut client, name, workspace).await?; } accepted = listener.accept() => { @@ -2269,8 +2270,18 @@ fn forward_requires_session_token(status: &Status) -> bool { && status.message().contains("authorization_token is required") } -/// Confirm the sandbox is still ready, returning the replica that owns it. -async fn fetch_ready_sandbox_for_forward( +/// Whether the gateway may still relay into a sandbox in this phase. A +/// Completed or Error sandbox keeps its access plane while its supervisor +/// stays connected; only the gateway can tell whether it still is. +fn relay_phase_may_be_reachable(phase: i32) -> bool { + matches!( + SandboxPhase::try_from(phase), + Ok(SandboxPhase::Ready | SandboxPhase::Completed | SandboxPhase::Error) + ) +} + +/// Confirm the sandbox is still reachable, returning the replica that owns it. +async fn fetch_reachable_sandbox_for_forward( client: &mut crate::tls::GrpcClient, name: &str, workspace: &str, @@ -2299,7 +2310,7 @@ async fn fetch_ready_sandbox_for_forward( .sandbox .ok_or_else(|| miette::miette!("sandbox '{name}' not found"))?; - if SandboxPhase::try_from(sandbox.phase()) != Ok(SandboxPhase::Ready) { + if !relay_phase_may_be_reachable(sandbox.phase()) { return Err(miette::miette!( "sandbox '{}' is no longer ready (phase: {}); stopping service forward", name, diff --git a/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs b/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs index 31e67baf0f..ccf94604d7 100644 --- a/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs +++ b/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs @@ -46,6 +46,7 @@ struct Calls { #[derive(Clone)] struct MockGateway { scenario: Scenario, + phase: proto::SandboxPhase, calls: Arc>, } @@ -189,7 +190,7 @@ macro_rules! mock_gateway { ..Default::default() }), status: Some(proto::SandboxStatus { - phase: proto::SandboxPhase::Ready.into(), + phase: self.phase.into(), ..Default::default() }), ..Default::default() @@ -326,6 +327,10 @@ impl Drop for TestGateway { impl TestGateway { async fn start(scenario: Scenario) -> Self { + Self::start_in_phase(scenario, proto::SandboxPhase::Ready).await + } + + async fn start_in_phase(scenario: Scenario, phase: proto::SandboxPhase) -> Self { let (ca, ca_key) = build_ca(); let (server_cert, server_key) = build_server_cert(&ca, &ca_key); let (client_cert, client_key) = build_client_cert(&ca, &ca_key); @@ -346,6 +351,7 @@ impl TestGateway { let calls = Arc::new(Mutex::new(Calls::default())); let service = MockGateway { scenario, + phase, calls: Arc::clone(&calls), }; let task = tokio::spawn(async move { @@ -478,6 +484,39 @@ async fn default_remote_exit_does_not_wait_for_idle_open_stdin() { drop(held_open); } +#[tokio::test] +async fn exec_reaches_gateway_for_completed_and_error_sandboxes() { + // The supervisor keeps its access plane after the main process exits, so + // the gateway, not the CLI, decides whether the relay is still reachable. + for phase in [proto::SandboxPhase::Completed, proto::SandboxPhase::Error] { + let gateway = TestGateway::start_in_phase(Scenario::EarlyExit, phase).await; + let mut child = gateway.spawn(); + let held_open = child.stdin.take().unwrap(); + let output = finish(child).await; + drop(held_open); + assert!( + output.status.success(), + "{phase:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(output.stdout, b"before-exit\n"); + gateway.assert_one_stream(); + } +} + +#[tokio::test] +async fn exec_is_refused_before_starting_for_stopped_sandbox() { + let gateway = + TestGateway::start_in_phase(Scenario::EarlyExit, proto::SandboxPhase::Stopped).await; + let mut child = gateway.spawn(); + drop(child.stdin.take()); + let output = finish(child).await; + assert!(!output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("is not ready (phase: Stopped)"), "{stderr}"); + assert!(gateway.calls.lock().unwrap().starts.is_empty()); +} + #[tokio::test] async fn default_open_pipe_checks_trailer_error_after_exit() { let gateway = TestGateway::start(Scenario::ErrorAfterExit).await; diff --git a/crates/openshell-server/src/grpc/mutation_replay/streaming/tests.rs b/crates/openshell-server/src/grpc/mutation_replay/streaming/tests.rs index 112cf64157..97b05d5a01 100644 --- a/crates/openshell-server/src/grpc/mutation_replay/streaming/tests.rs +++ b/crates/openshell-server/src/grpc/mutation_replay/streaming/tests.rs @@ -167,6 +167,17 @@ async fn receive_relay(state: &ServerState, messages: &mut mpsc::Receiver("default", &req.sandbox) + .await + .unwrap() + .unwrap(); + sandbox.set_phase(phase as i32); + state.store.put_message(&sandbox).await.unwrap(); +} + async fn completion(state: &ServerState) -> Completion { let rows = state .store @@ -210,6 +221,36 @@ fn interactive_fingerprint_uses_start_schema_and_ignores_request_id() { ); } +#[tokio::test] +async fn exec_reaches_terminal_sandbox_only_while_supervisor_is_connected() { + // A supervisor retains its access plane after the main process exits. + // Exec follows the same reachability rule as SSH and port forwarding. + for interactive in [false, true] { + for phase in [SandboxPhase::Completed, SandboxPhase::Error] { + let directory = tempfile::tempdir().unwrap(); + let state = setup("sqlite::memory:", &directory).await; + let req = request(&state).await; + set_phase(&state, &req, phase).await; + let (mut client, server) = wire_client(state.clone(), None, None).await; + + let status = exec_rpc(&mut client, req.clone(), interactive) + .await + .unwrap_err(); + assert_eq!(status.code(), tonic::Code::FailedPrecondition, "{phase:?}"); + assert_eq!(status.message(), "sandbox is not ready"); + + let mut messages = register(&state, &sandbox_id(&state, &req).await); + let mut retry = req; + retry.request_id = uuid::Uuid::new_v4().to_string(); + exec_rpc(&mut client, retry, interactive) + .await + .unwrap_or_else(|status| panic!("{phase:?} interactive={interactive}: {status}")); + receive_relay(&state, &mut messages).await; + server.abort(); + } + } +} + #[tokio::test] async fn concurrent_exec_claims_open_one_relay_and_store_no_secrets() { let directory = tempfile::tempdir().unwrap(); diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index a797da2589..7c3f1bd133 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -276,13 +276,6 @@ pub(super) async fn resolve_and_authorize_sandbox_name( Ok(sandbox) } -fn require_ready_sandbox(sandbox: &Sandbox) -> Result<(), Status> { - match SandboxPhase::try_from(sandbox.phase()).ok() { - Some(SandboxPhase::Ready) => Ok(()), - _ => Err(Status::failed_precondition("sandbox is not ready")), - } -} - fn generate_routable_name() -> String { let name = petname::petname(2, "-").unwrap_or_else(generate_name); let mut truncated = &name[..name.len().min(MAX_ROUTABLE_NAME_LEN)]; @@ -2738,7 +2731,7 @@ pub(super) async fn handle_exec_sandbox( completion.ensure_target(sandbox.object_id())?; } - require_ready_sandbox(&sandbox)?; + require_relay_reachable(state, &sandbox).await?; // Open a relay channel through the supervisor session. Use a 15s // session-wait timeout, enough to cover a transient supervisor reconnect @@ -2872,9 +2865,7 @@ pub(super) async fn handle_forward_tcp( // The main process may finish between minting the SSH token and opening // its transport. Keep the relay reachable until terminal delivery is // finalized so fast commands can attach without a readiness race. - if !sandbox_relay_reachable(state, &sandbox) { - return Err(Status::failed_precondition("sandbox is not ready")); - } + require_relay_reachable(state, &sandbox).await?; let connection_guard = acquire_forward_connection_guard(state, &principal, &init, &sandbox).await?; @@ -3317,7 +3308,7 @@ pub(super) async fn handle_exec_sandbox_interactive_start( completion.ensure_target(sandbox.object_id())?; } - require_ready_sandbox(&sandbox)?; + require_relay_reachable(state, &sandbox).await?; let (channel_id, relay_rx) = crate::supervisor_session::open_routed_relay_with_target( state, @@ -3385,11 +3376,17 @@ pub(super) async fn handle_exec_sandbox_interactive_start( // SSH session handlers // --------------------------------------------------------------------------- -fn sandbox_relay_reachable(state: &ServerState, sandbox: &Sandbox) -> bool { - let phase = SandboxPhase::try_from(sandbox.phase()).ok(); - matches!(phase, Some(SandboxPhase::Ready)) - || (matches!(phase, Some(SandboxPhase::Completed | SandboxPhase::Error)) - && state.supervisor_sessions.has_session(sandbox.object_id())) +/// Exec, SSH, and port forwarding share one access rule: a Completed or +/// Error sandbox stays reachable while its supervisor retains the access plane. +async fn require_relay_reachable( + state: &Arc, + sandbox: &Sandbox, +) -> Result<(), Status> { + if crate::supervisor_session::sandbox_relay_reachable(state, sandbox).await? { + Ok(()) + } else { + Err(Status::failed_precondition("sandbox is not ready")) + } } pub(super) async fn handle_create_ssh_session( @@ -3408,9 +3405,7 @@ pub(super) async fn handle_create_ssh_session( .await?; let sandbox_id = sandbox.object_id().to_string(); - if !sandbox_relay_reachable(state, &sandbox) { - return Err(Status::failed_precondition("sandbox is not ready")); - } + require_relay_reachable(state, &sandbox).await?; let token = uuid::Uuid::new_v4().to_string(); let now_ms = current_time_ms(); @@ -9484,10 +9479,18 @@ mod tests { .supervisor_sessions .finalize_main_process_exit("sandbox-work") ); - assert!(sandbox_relay_reachable(&state, &sandbox)); + assert!( + crate::supervisor_session::sandbox_relay_reachable(&state, &sandbox) + .await + .unwrap() + ); assert!(state.supervisor_sessions.disconnect("sandbox-work")); - assert!(!sandbox_relay_reachable(&state, &sandbox)); + assert!( + !crate::supervisor_session::sandbox_relay_reachable(&state, &sandbox) + .await + .unwrap() + ); } #[tokio::test] diff --git a/crates/openshell-server/src/supervisor_session.rs b/crates/openshell-server/src/supervisor_session.rs index fd54e7ff19..4a33c0f67b 100644 --- a/crates/openshell-server/src/supervisor_session.rs +++ b/crates/openshell-server/src/supervisor_session.rs @@ -1825,6 +1825,34 @@ fn owner_is_fresh(owner: &crate::supervisor_owner::OwnerRecord) -> bool { owner.is_fresh(OWNER_TTL) } +/// Whether the gateway can relay a new connection into this sandbox. +/// +/// A Ready sandbox always qualifies; relay opening waits for and routes to its +/// owner. After the main process exits, the supervisor keeps its access plane, +/// so a Completed or Error sandbox qualifies while its supervisor session is +/// live on this replica or a fresh owner record names a peer replica. +pub async fn sandbox_relay_reachable( + state: &Arc, + sandbox: &Sandbox, +) -> Result { + match SandboxPhase::try_from(sandbox.phase()).ok() { + Some(SandboxPhase::Ready) => Ok(true), + Some(SandboxPhase::Completed | SandboxPhase::Error) => { + let sandbox_id = sandbox.object_id(); + if state.supervisor_sessions.has_session(sandbox_id) { + return Ok(true); + } + let owner_index = SupervisorOwnerIndex::new(state.store.clone(), OWNER_TTL); + Ok(resolve_owner(state, &owner_index, sandbox_id) + .await? + .is_some_and(|owner| { + owner_is_fresh(&owner) && owner.owner_replica_id != state.replica_id + })) + } + _ => Ok(false), + } +} + /// The replica currently holding this sandbox's supervisor session, if known. pub async fn owner_replica_id(state: &Arc, sandbox_id: &str) -> Option { if state.store.is_single_replica() { @@ -3694,6 +3722,87 @@ mod tests { .expect("complete chart peer TLS materials should configure tonic"); } + #[tokio::test] + async fn relay_reachability_after_exit_follows_the_live_session_on_any_replica() { + let state = crate::grpc::test_support::test_server_state().await; + let reachable = |sandbox: Sandbox| { + let state = state.clone(); + async move { sandbox_relay_reachable(&state, &sandbox).await.unwrap() } + }; + let in_phase = |id: &str, phase: SandboxPhase| { + let mut sandbox = sandbox_record(id, id); + sandbox.set_phase(phase as i32); + sandbox + }; + + // Ready routes to the owner during relay opening; never-started or + // stopped sandboxes are refused regardless of sessions. + assert!(reachable(in_phase("relay-ready", SandboxPhase::Ready)).await); + for phase in [ + SandboxPhase::Provisioning, + SandboxPhase::Stopped, + SandboxPhase::Deleting, + ] { + assert!( + !reachable(in_phase("relay-local", phase)).await, + "{phase:?}" + ); + } + + for phase in [SandboxPhase::Completed, SandboxPhase::Error] { + // No session anywhere. + assert!(!reachable(in_phase("relay-none", phase)).await, "{phase:?}"); + + // Session held by this replica. + let (tx, _rx) = mpsc::channel(1); + let (shutdown_tx, _shutdown_rx) = oneshot::channel(); + state.supervisor_sessions.register( + "relay-local".to_string(), + "session".to_string(), + tx, + shutdown_tx, + ); + assert!(reachable(in_phase("relay-local", phase)).await, "{phase:?}"); + assert!(state.supervisor_sessions.disconnect("relay-local")); + assert!( + !reachable(in_phase("relay-local", phase)).await, + "{phase:?}" + ); + } + + // Session held by a peer replica, known only through its owner record. + let owner_index = SupervisorOwnerIndex::new(state.store.clone(), OWNER_TTL); + owner_index + .publish( + "relay-peer", + "session", + "supervisor", + 1, + "peer-replica", + "https://peer", + ) + .await + .unwrap(); + for phase in [SandboxPhase::Completed, SandboxPhase::Error] { + assert!(reachable(in_phase("relay-peer", phase)).await, "{phase:?}"); + } + + // An owner record naming this replica without a local session is a + // leftover from a lost session, not a live supervisor. + owner_index + .publish( + "relay-self", + "session", + "supervisor", + 1, + &state.replica_id, + "https://self", + ) + .await + .unwrap(); + assert!(!reachable(in_phase("relay-self", SandboxPhase::Completed)).await); + } + fn sandbox_record(id: &str, name: &str) -> Sandbox { Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { From acc1eced7ec1dd5938f5c709381a68a4d52cc1e9 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Mon, 26 Oct 2026 16:58:19 -0700 Subject: [PATCH 2/8] fix(server): route exposed services to exited sandboxes with a live supervisor Exposed-service HTTP routing still required Ready while port forwarding to the same sandbox kept working after the main process exited. Service routing now uses the shared relay reachability gate, so a retained Completed or Error sandbox serves its exposed services while its supervisor stays connected. Document what stays reachable after a retained sandbox's main process exits, and that --no-keep sandboxes are deleted once it does. Signed-off-by: Piotr Mlocek --- .../openshell-server/src/service_routing.rs | 88 ++++++++++++++++++- docs/how-it-works/sandboxes/overview.mdx | 7 +- skills/openshell-cli/SKILL.md | 5 +- 3 files changed, 96 insertions(+), 4 deletions(-) diff --git a/crates/openshell-server/src/service_routing.rs b/crates/openshell-server/src/service_routing.rs index ece2add1e0..571c3f150d 100644 --- a/crates/openshell-server/src/service_routing.rs +++ b/crates/openshell-server/src/service_routing.rs @@ -11,7 +11,7 @@ use http::{HeaderMap, HeaderValue, Method, Request, Response, StatusCode, header use hyper_util::rt::TokioIo; use openshell_core::config::ServiceRoutingConfig; use openshell_core::proto::{ - Sandbox, SandboxPhase, ServiceAuthorizationMode, ServiceEndpoint, TcpRelayTarget, relay_open, + Sandbox, ServiceAuthorizationMode, ServiceEndpoint, TcpRelayTarget, relay_open, }; use openshell_core::{ObjectId, ObjectWorkspace}; use openshell_ocsf::{ @@ -727,7 +727,25 @@ async fn proxy_to_endpoint( return Err(route_err); } }; - if SandboxPhase::try_from(sandbox.phase()).ok() != Some(SandboxPhase::Ready) { + // Services follow the same access rule as exec, SSH, and port forwarding. + let reachable = match crate::supervisor_session::sandbox_relay_reachable(&state, &sandbox).await + { + Ok(reachable) => reachable, + Err(err) => { + warn!(error = %err, sandbox_id = %endpoint.sandbox_id, "sandbox service routing: failed to resolve supervisor owner"); + let route_err = ServiceRouteError::internal_error(); + emit_service_http_failure( + &state, + &req, + &sandbox_name, + &service_name, + Some(&endpoint), + &route_err, + ); + return Err(route_err); + } + }; + if !reachable { let err = ServiceRouteError::sandbox_not_ready(); emit_service_http_failure( &state, @@ -2271,6 +2289,72 @@ mod tests { assert_eq!(upstream.headers()[header::UPGRADE], "websocket"); } + #[tokio::test] + async fn completed_sandbox_routes_services_only_while_supervisor_is_live() { + use openshell_core::proto::{GatewayMessage, SandboxPhase, gateway_message}; + + let state = crate::grpc::test_support::test_server_state().await; + let mut ep = endpoint(); + ep.metadata.as_mut().unwrap().id = "svc-completed-endpoint".to_string(); + ep.metadata.as_mut().unwrap().name = "svc-completed--web".to_string(); + ep.sandbox_id = "svc-completed".to_string(); + ep.sandbox = "svc-completed".to_string(); + state.store.put_message(&ep).await.unwrap(); + let mut sandbox = Sandbox { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: "svc-completed".to_string(), + name: "svc-completed".to_string(), + workspace: "default".to_string(), + ..Default::default() + }), + ..Default::default() + }; + sandbox.set_phase(SandboxPhase::Completed as i32); + state.store.put_message(&sandbox).await.unwrap(); + let route = |state: Arc| async move { + let req = Request::builder().uri("/").body(Body::empty()).unwrap(); + proxy_to_endpoint( + state, + req, + "default", + "svc-completed".to_string(), + "web".to_string(), + ) + .await + }; + + let Err(err) = route(state.clone()).await else { + panic!("a Completed sandbox without a supervisor must not route"); + }; + assert_eq!(err.reason, ServiceRouteError::sandbox_not_ready().reason); + + // With the supervisor retaining its access plane, the request passes + // the gate and asks the supervisor for a relay. + let (tx, mut messages) = tokio::sync::mpsc::channel::(1); + let (shutdown_tx, _shutdown_rx) = tokio::sync::oneshot::channel(); + state.supervisor_sessions.register( + "svc-completed".to_string(), + "session".to_string(), + tx, + shutdown_tx, + ); + let routed = tokio::spawn(route(state.clone())); + let message = tokio::time::timeout(Duration::from_secs(5), messages.recv()) + .await + .unwrap() + .unwrap(); + let Some(gateway_message::Payload::RelayOpen(open)) = message.payload else { + panic!("expected relay open"); + }; + state + .supervisor_sessions + .fail_pending_relay(&open.channel_id, "test relay ended".into()); + let Err(err) = routed.await.unwrap() else { + panic!("a failed relay must not produce a response"); + }; + assert_eq!(err.reason, ServiceRouteError::service_unreachable().reason); + } + #[tokio::test] async fn load_endpoint_uses_workspace_for_lookup() { let store = crate::persistence::test_store().await; diff --git a/docs/how-it-works/sandboxes/overview.mdx b/docs/how-it-works/sandboxes/overview.mdx index 2ac6a4f69d..61c874a9cc 100644 --- a/docs/how-it-works/sandboxes/overview.mdx +++ b/docs/how-it-works/sandboxes/overview.mdx @@ -1209,7 +1209,12 @@ temporarily while its supervisor reconnects. Wait for the phase to return to The gateway records a successful canonical main-process exit as `Ready=False` with reason `MainProcessCompleted`. Nonzero and signal-normalized results use `MainProcessFailed` and the `Error` phase. It also sets `status.exit_code`; -signal exits use the standard `128 + signal` convention. When restart policy +signal exits use the standard `128 + signal` convention. After the main +process of a retained sandbox exits, you can still connect, execute commands, +transfer files, forward ports, and reach exposed services while the sandbox +supervisor stays connected. An ephemeral (`--no-keep`) sandbox is deleted after its main process +exits, and an `Error` caused by an infrastructure failure usually has no +connected supervisor, so neither accepts new sessions. When restart policy selects replacement, `status.restart_count` and `status.next_restart_time` show the attempt number and next deadline. Inspect them with `openshell sandbox get `. `sandbox stop` cancels a pending replacement; diff --git a/skills/openshell-cli/SKILL.md b/skills/openshell-cli/SKILL.md index 351d7e3b84..6bc53a4235 100644 --- a/skills/openshell-cli/SKILL.md +++ b/skills/openshell-cli/SKILL.md @@ -522,7 +522,10 @@ openshell sandbox start [name] Both commands default to the last-used sandbox. Stop stops background forwards and waits for `Stopped`; start waits for `Ready`. Connect, exec, file transfer, forwarding, and exposed services are unavailable while -stopped or completed. Starting a retained `Completed` or +stopped. After the main process of a retained sandbox exits (`Completed`, or +`Error` with `MainProcessFailed`), connect, exec, file transfer, forwarding, and +exposed services keep working while the supervisor stays connected. `--no-keep` sandboxes are +deleted once the main process exits. Starting a retained `Completed` or `Error/MainProcessFailed` sandbox launches a fresh canonical-main instance and invalidates SSH sessions from the previous runtime generation. Delete remains the operation that removes retained state. From b0aac7cefded26bd11a95e8cad32bc6883cb1084 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Mon, 26 Oct 2026 16:59:07 -0700 Subject: [PATCH 3/8] test(conformance): certify sandbox retention after main process exit Add two CLI conformance tests, each run for exit codes 0 and 3: an ephemeral (--no-keep) sandbox returns the exit code and is deleted, and a retained sandbox records Completed or Error with the exit code and still accepts exec. Signed-off-by: Piotr Mlocek --- tests/suites/conformance/README.md | 2 +- tests/suites/conformance/tests/cli.rs | 110 ++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) diff --git a/tests/suites/conformance/README.md b/tests/suites/conformance/README.md index ee0502d072..02010fb4c5 100644 --- a/tests/suites/conformance/README.md +++ b/tests/suites/conformance/README.md @@ -8,7 +8,7 @@ Each file in `tests/` is one suite. |---|---| | `smoke` | Create a sandbox, run `echo` in it, delete it | | `lifecycle` | SDK: long-running and run-to-completion sandboxes, the default command, restart of a stopped sandbox, deleting a stopped sandbox | -| `cli` | The same lifecycle through the `openshell` CLI: status, create, get, list, exec, stop, start, delete | +| `cli` | The same lifecycle through the `openshell` CLI: status, create, get, list, exec, stop, start, delete; `--no-keep` sandboxes are deleted after the main process exits, and retained ones still accept exec | | `persistent-workspace` | Files in `/sandbox` survive stop and start | | `policy-advisor` | `policy.local` accepts a proposed rule; a denied address or hostname yields one draft rule | | `file-transfer` | `sandbox upload` and `download`: round trips, `.gitignore` filtering, workspace-escape and option-looking names | diff --git a/tests/suites/conformance/tests/cli.rs b/tests/suites/conformance/tests/cli.rs index d26eed9bb5..54ddbf92e5 100644 --- a/tests/suites/conformance/tests/cli.rs +++ b/tests/suites/conformance/tests/cli.rs @@ -111,6 +111,116 @@ async fn cli_stop_start_delete(cx: &mut Context) -> testkit::Result { cx.expect_absent(&sandbox).await } +/// An ephemeral (`--no-keep`) sandbox streams its main process output, returns +/// the main process's exit code, and is deleted afterwards, whether the main +/// process succeeds or fails. +#[testkit::test] +async fn cli_ephemeral_sandbox_is_deleted_after_exit(cx: &mut Context) -> testkit::Result { + for exit_code in [0, 3] { + let name = cx.name(&format!("e{exit_code}")); + cx.track(&name); + let create = cx + .cli([ + "sandbox", + "create", + "--name", + &name, + "--no-keep", + "--no-tty", + "--", + "sh", + "-c", + &format!("echo ephemeral-done; exit {exit_code}"), + ]) + .await?; + testkit::ensure!( + create.exit_code == exit_code, + "sandbox create exited {}; expected {exit_code}; stderr: {}", + create.exit_code, + create.stderr + ); + testkit::ensure!( + create.stdout.contains("ephemeral-done"), + "sandbox create stdout {:?} lacks the main process output", + create.stdout + ); + + wait_unlisted(cx, &name).await?; + let get = cx.cli(["sandbox", "get", &name]).await?.failure()?; + testkit::ensure!( + get.stderr.to_lowercase().contains("not found"), + "sandbox get {name} failed without reporting it missing: {}", + get.stderr + ); + } + Ok(()) +} + +/// A retained sandbox records Completed or Error with the main process's exit +/// code, and still accepts `sandbox exec` after the main process has exited. +#[testkit::test] +async fn cli_retained_sandbox_accepts_exec_after_exit(cx: &mut Context) -> testkit::Result { + let result_path = "/sandbox/.openshell-retained-result"; + for exit_code in [0, 3] { + let name = cx.name(&format!("r{exit_code}")); + let result = format!("openshell-retained-{}-{exit_code}", cx.run_id()); + cx.track(&name); + cx.cli([ + "sandbox", + "create", + "--name", + &name, + "--detach", + "--no-tty", + "--", + "sh", + "-c", + &format!("printf '%s\\n' '{result}' > '{result_path}' && sync; exit {exit_code}"), + ]) + .await? + .success()?; + let sandbox = cx.adopt(&name).await?; + + let finished = sandbox.wait_finished().await?; + let expected = if exit_code == 0 { + testkit::sdk::SandboxPhase::Completed + } else { + testkit::sdk::SandboxPhase::Error + }; + testkit::ensure!( + finished.phase == expected && finished.exit_code == Some(exit_code), + "sandbox {name} finished in phase {:?} with exit code {:?}; expected {expected:?} with {exit_code}", + finished.phase, + finished.exit_code + ); + + let exec = cx + .cli([ + "sandbox", + "exec", + "--name", + &name, + "--no-tty", + "--", + "cat", + result_path, + ]) + .await? + .success()?; + testkit::ensure!( + exec.stdout == format!("{result}\n"), + "sandbox exec stdout was {:?}; expected {:?}", + exec.stdout, + format!("{result}\n") + ); + + cx.cli(["sandbox", "delete", &name]).await?.success()?; + wait_unlisted(cx, &name).await?; + cx.expect_absent(&sandbox).await?; + } + Ok(()) +} + /// Wait until `sandbox list` no longer shows the sandbox. async fn wait_unlisted(cx: &Context, name: &str) -> testkit::Result { testkit::poll_until( From e3b64f88c5ff056e22784091cfce992ada4736d5 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Mon, 26 Oct 2026 18:47:32 -0700 Subject: [PATCH 4/8] docs(sandboxes): add lifecycle diagram and post-exit behavior Add a phase diagram to the Sandbox Lifecycle section and describe what happens after the main process exits. A --no-keep sandbox is deleted. A retained sandbox keeps its compute and still accepts exec, file transfer, port forwarding and exposed services while its supervisor is connected. sandbox connect fails because it attaches to the exited main process, so the docs point to sandbox exec --tty for a shell. Signed-off-by: Piotr Mlocek --- docs/how-it-works/sandboxes/overview.mdx | 56 +++++++++++++++++++++--- skills/openshell-cli/SKILL.md | 5 ++- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/docs/how-it-works/sandboxes/overview.mdx b/docs/how-it-works/sandboxes/overview.mdx index 61c874a9cc..1c28b4becf 100644 --- a/docs/how-it-works/sandboxes/overview.mdx +++ b/docs/how-it-works/sandboxes/overview.mdx @@ -1161,6 +1161,28 @@ after it attempts every requested deletion if any entry failed. Every sandbox moves through a defined set of phases: +```mermaid +stateDiagram-v2 + [*] --> Provisioning: create + Provisioning --> Ready: supervisor connects + Provisioning --> Error: preparation or admission times out + Ready --> Provisioning: supervisor disconnects + Ready --> Completed: main process exits 0 + Ready --> Error: main process exits nonzero + Ready --> Stopping: stop + Error --> Stopping: stop + Stopping --> Stopped + Stopped --> Starting: start + Completed --> Starting: start or restart policy + Error --> Starting: start or restart policy + Starting --> Ready: new supervisor connects + Completed --> Deleting: delete, or --no-keep cleanup + Error --> Deleting: delete, or --no-keep cleanup + Stopped --> Deleting: delete + Ready --> Deleting: delete + Deleting --> [*] +``` + Before workload activation, OpenShell validates the effective policy and matching provider configuration. A rejection keeps the workload unstarted and exposes a `ConfigurationInvalid` condition in `Provisioning`. Use `openshell sandbox get` @@ -1209,18 +1231,40 @@ temporarily while its supervisor reconnects. Wait for the phase to return to The gateway records a successful canonical main-process exit as `Ready=False` with reason `MainProcessCompleted`. Nonzero and signal-normalized results use `MainProcessFailed` and the `Error` phase. It also sets `status.exit_code`; -signal exits use the standard `128 + signal` convention. After the main -process of a retained sandbox exits, you can still connect, execute commands, -transfer files, forward ports, and reach exposed services while the sandbox -supervisor stays connected. An ephemeral (`--no-keep`) sandbox is deleted after its main process -exits, and an `Error` caused by an infrastructure failure usually has no -connected supervisor, so neither accepts new sessions. When restart policy +signal exits use the standard `128 + signal` convention. When restart policy selects replacement, `status.restart_count` and `status.next_restart_time` show the attempt number and next deadline. Inspect them with `openshell sandbox get `. `sandbox stop` cancels a pending replacement; an explicit later start resets the restart count. Compute runtimes do not apply their own restart policy. +### After the main process exits + +Whether a sandbox outlives its main process depends on how you created it: + +- With `--no-keep`, the sandbox's lifecycle is tied to the main process. The + gateway deletes the sandbox after the main process exits, whatever the exit + code, once its output has been delivered. +- By default, the sandbox is retained. It moves to `Completed` or `Error`, and + its compute keeps running: the workload with its filesystem, and the + supervisor. It stays that way until you delete the sandbox or start it again. + +In a retained `Completed` or `Error` sandbox, these keep working while its +supervisor stays connected: + +- `sandbox exec`, including `sandbox exec --tty -- /bin/bash -l` for an + interactive shell +- `sandbox upload` and `sandbox download` +- port forwarding +- exposed services + +`sandbox connect` fails, because it attaches to the main process and that +process has exited. Use `sandbox exec --tty` instead. + +An `Error` caused by an infrastructure failure, such as +`ImagePreparationTimedOut`, usually has no supervisor left, so these +operations fail with `sandbox is not ready`. + ## Sandbox Runtimes The gateway's configured compute driver determines how OpenShell creates each sandbox. The CLI workflow stays the same across drivers: you create, connect to, inspect, and delete sandboxes through the gateway API. diff --git a/skills/openshell-cli/SKILL.md b/skills/openshell-cli/SKILL.md index 6bc53a4235..bbf0b5bd52 100644 --- a/skills/openshell-cli/SKILL.md +++ b/skills/openshell-cli/SKILL.md @@ -523,8 +523,9 @@ Both commands default to the last-used sandbox. Stop stops background forwards and waits for `Stopped`; start waits for `Ready`. Connect, exec, file transfer, forwarding, and exposed services are unavailable while stopped. After the main process of a retained sandbox exits (`Completed`, or -`Error` with `MainProcessFailed`), connect, exec, file transfer, forwarding, and -exposed services keep working while the supervisor stays connected. `--no-keep` sandboxes are +`Error` with `MainProcessFailed`), exec, file transfer, forwarding, and +exposed services keep working while the supervisor stays connected. `connect` +fails because the main process has exited; use `sandbox exec --tty` for a shell. `--no-keep` sandboxes are deleted once the main process exits. Starting a retained `Completed` or `Error/MainProcessFailed` sandbox launches a fresh canonical-main instance and invalidates SSH sessions from the previous runtime generation. Delete remains From de9968010b80e1be99ee84a6a43d8b3da77ed8e3 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Fri, 30 Oct 2026 18:02:48 -0700 Subject: [PATCH 5/8] refactor(server): route every sandbox entry point through one access gate Exec, interactive exec, SSH sessions, port forwarding, and exposed services now call sandbox_access::acquire and open their relays through the returned SandboxAccess handle. The phase rules live in one module, so anything that must happen before a sandbox can be used applies to every entry point. supervisor_session keeps only the replica-aware supervisor_connected check. Signed-off-by: Piotr Mlocek --- crates/openshell-server/src/grpc/sandbox.rs | 75 ++----- crates/openshell-server/src/lib.rs | 1 + crates/openshell-server/src/sandbox_access.rs | 196 ++++++++++++++++++ .../openshell-server/src/service_routing.rs | 58 +++--- .../src/supervisor_session.rs | 97 +++------ 5 files changed, 270 insertions(+), 157 deletions(-) create mode 100644 crates/openshell-server/src/sandbox_access.rs diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index 7c3f1bd133..013b06e88f 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -2731,20 +2731,10 @@ pub(super) async fn handle_exec_sandbox( completion.ensure_target(sandbox.object_id())?; } - require_relay_reachable(state, &sandbox).await?; - - // Open a relay channel through the supervisor session. Use a 15s - // session-wait timeout, enough to cover a transient supervisor reconnect - // while still failing quickly during normal operation. - let (channel_id, relay_rx) = crate::supervisor_session::open_routed_relay_with_target( - state, - sandbox.object_id(), - relay_open::Target::Ssh(SshRelayTarget {}), - String::new(), - std::time::Duration::from_secs(15), - ) - .await - .map_err(|e| Status::unavailable(format!("supervisor relay failed: {e}")))?; + let (channel_id, relay_rx) = crate::sandbox_access::acquire(state, &sandbox) + .await? + .open_relay(relay_open::Target::Ssh(SshRelayTarget {}), String::new()) + .await?; let command_str = build_remote_exec_command(&req) .map_err(|e| Status::invalid_argument(format!("command construction failed: {e}")))?; @@ -2865,19 +2855,11 @@ pub(super) async fn handle_forward_tcp( // The main process may finish between minting the SSH token and opening // its transport. Keep the relay reachable until terminal delivery is // finalized so fast commands can attach without a readiness race. - require_relay_reachable(state, &sandbox).await?; + let access = crate::sandbox_access::acquire(state, &sandbox).await?; let connection_guard = acquire_forward_connection_guard(state, &principal, &init, &sandbox).await?; - let (channel_id, relay_rx) = crate::supervisor_session::open_routed_relay_with_target( - state, - sandbox.object_id(), - target, - init.service_id.clone(), - std::time::Duration::from_secs(15), - ) - .await - .map_err(|e| Status::unavailable(format!("supervisor relay failed: {e}")))?; + let (channel_id, relay_rx) = access.open_relay(target, init.service_id.clone()).await?; let (tx, rx) = mpsc::channel::>(256); let sandbox_id = sandbox.object_id().to_string(); @@ -3308,17 +3290,10 @@ pub(super) async fn handle_exec_sandbox_interactive_start( completion.ensure_target(sandbox.object_id())?; } - require_relay_reachable(state, &sandbox).await?; - - let (channel_id, relay_rx) = crate::supervisor_session::open_routed_relay_with_target( - state, - sandbox.object_id(), - relay_open::Target::Ssh(SshRelayTarget {}), - String::new(), - std::time::Duration::from_secs(15), - ) - .await - .map_err(|e| Status::unavailable(format!("supervisor relay failed: {e}")))?; + let (channel_id, relay_rx) = crate::sandbox_access::acquire(state, &sandbox) + .await? + .open_relay(relay_open::Target::Ssh(SshRelayTarget {}), String::new()) + .await?; let command_str = build_remote_exec_command(req) .map_err(|e| Status::invalid_argument(format!("command construction failed: {e}")))?; @@ -3376,19 +3351,6 @@ pub(super) async fn handle_exec_sandbox_interactive_start( // SSH session handlers // --------------------------------------------------------------------------- -/// Exec, SSH, and port forwarding share one access rule: a Completed or -/// Error sandbox stays reachable while its supervisor retains the access plane. -async fn require_relay_reachable( - state: &Arc, - sandbox: &Sandbox, -) -> Result<(), Status> { - if crate::supervisor_session::sandbox_relay_reachable(state, sandbox).await? { - Ok(()) - } else { - Err(Status::failed_precondition("sandbox is not ready")) - } -} - pub(super) async fn handle_create_ssh_session( state: &Arc, request: Request, @@ -3405,7 +3367,9 @@ pub(super) async fn handle_create_ssh_session( .await?; let sandbox_id = sandbox.object_id().to_string(); - require_relay_reachable(state, &sandbox).await?; + // The session's transport opens its relay through ForwardTcp, which + // checks access again; checking here refuses tokens that could never work. + crate::sandbox_access::acquire(state, &sandbox).await?; let token = uuid::Uuid::new_v4().to_string(); let now_ms = current_time_ms(); @@ -9480,17 +9444,16 @@ mod tests { .finalize_main_process_exit("sandbox-work") ); assert!( - crate::supervisor_session::sandbox_relay_reachable(&state, &sandbox) + crate::sandbox_access::acquire(&state, &sandbox) .await - .unwrap() + .is_ok() ); assert!(state.supervisor_sessions.disconnect("sandbox-work")); - assert!( - !crate::supervisor_session::sandbox_relay_reachable(&state, &sandbox) - .await - .unwrap() - ); + assert!(matches!( + crate::sandbox_access::acquire(&state, &sandbox).await, + Err(crate::sandbox_access::AccessError::NotReachable) + )); } #[tokio::test] diff --git a/crates/openshell-server/src/lib.rs b/crates/openshell-server/src/lib.rs index 4608161785..6fa7864553 100644 --- a/crates/openshell-server/src/lib.rs +++ b/crates/openshell-server/src/lib.rs @@ -41,6 +41,7 @@ mod provider_profile_sources; mod provider_refresh; mod readiness; mod reflection; +mod sandbox_access; mod sandbox_index; mod sandbox_watch; mod service_routing; diff --git a/crates/openshell-server/src/sandbox_access.rs b/crates/openshell-server/src/sandbox_access.rs new file mode 100644 index 0000000000..92119d7740 --- /dev/null +++ b/crates/openshell-server/src/sandbox_access.rs @@ -0,0 +1,196 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! The one gate in front of every way into a sandbox. +//! +//! Exec, SSH sessions (and the file transfer that rides on them), port +//! forwarding, and exposed services all reach a sandbox through a relay its +//! supervisor opens. They must agree on when a sandbox can be used, so each of +//! them calls [`acquire`] and opens its relays through the returned +//! [`SandboxAccess`]. Anything that has to happen before a sandbox can be used +//! belongs in [`acquire`], so every entry point picks it up together. +//! +//! The rules: +//! +//! - A `Ready` sandbox is usable. Opening a relay waits for its supervisor and +//! routes to whichever replica holds it. +//! - A `Completed` or `Error` sandbox is usable while its supervisor is still +//! connected to any replica. A retained sandbox keeps its access plane after +//! its main process exits. +//! - Every other phase is refused. + +use std::fmt; +use std::sync::Arc; +use std::time::Duration; + +use openshell_core::ObjectId; +use openshell_core::proto::{Sandbox, SandboxPhase, relay_open}; +use tokio::io::DuplexStream; +use tokio::sync::oneshot; +use tonic::Status; + +use crate::ServerState; + +/// How long opening a relay waits for the supervisor session: enough to cover +/// a transient supervisor reconnect while still failing quickly. +const RELAY_SESSION_WAIT: Duration = Duration::from_secs(15); + +/// Why a sandbox could not be used. +#[derive(Debug)] +pub enum AccessError { + /// The sandbox is in a state that does not allow access. + NotReachable, + /// The gateway could not determine whether the sandbox is reachable. + Lookup(Status), + /// The supervisor did not open the relay. + Relay(Status), +} + +impl fmt::Display for AccessError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::NotReachable => f.write_str("sandbox is not ready"), + Self::Lookup(status) => write!(f, "sandbox access check failed: {status}"), + Self::Relay(status) => write!(f, "supervisor relay failed: {status}"), + } + } +} + +impl From for Status { + fn from(err: AccessError) -> Self { + match err { + AccessError::NotReachable => Self::failed_precondition(err.to_string()), + AccessError::Lookup(status) => status, + AccessError::Relay(_) => Self::unavailable(err.to_string()), + } + } +} + +/// A sandbox that passed the access rules. Relays into it are opened here. +pub struct SandboxAccess { + state: Arc, + sandbox_id: String, +} + +/// Apply the access rules to `sandbox`. +pub async fn acquire( + state: &Arc, + sandbox: &Sandbox, +) -> Result { + let reachable = match SandboxPhase::try_from(sandbox.phase()).ok() { + Some(SandboxPhase::Ready) => true, + Some(SandboxPhase::Completed | SandboxPhase::Error) => { + crate::supervisor_session::supervisor_connected(state, sandbox.object_id()) + .await + .map_err(AccessError::Lookup)? + } + _ => false, + }; + if !reachable { + return Err(AccessError::NotReachable); + } + Ok(SandboxAccess { + state: state.clone(), + sandbox_id: sandbox.object_id().to_string(), + }) +} + +impl SandboxAccess { + /// Ask the supervisor to open a relay to `target`, returning the channel + /// id and a receiver that resolves once the supervisor claims it. + pub async fn open_relay( + &self, + target: relay_open::Target, + service_id: String, + ) -> Result<(String, oneshot::Receiver>), AccessError> { + crate::supervisor_session::open_routed_relay_with_target( + &self.state, + &self.sandbox_id, + target, + service_id, + RELAY_SESSION_WAIT, + ) + .await + .map_err(AccessError::Relay) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tokio::sync::mpsc; + + fn sandbox_in(id: &str, phase: SandboxPhase) -> Sandbox { + let mut sandbox = Sandbox { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: id.to_string(), + name: id.to_string(), + workspace: "default".to_string(), + ..Default::default() + }), + ..Default::default() + }; + sandbox.set_phase(phase as i32); + sandbox + } + + #[tokio::test] + async fn access_follows_phase_and_supervisor_session() { + let state = crate::grpc::test_support::test_server_state().await; + let reachable = |phase: SandboxPhase| { + let state = state.clone(); + async move { + match acquire(&state, &sandbox_in("access", phase)).await { + Ok(_) => true, + Err(AccessError::NotReachable) => false, + Err(err) => panic!("unexpected access error: {err:?}"), + } + } + }; + + // Without a supervisor, only Ready is usable; relay opening waits for + // its supervisor. + assert!(reachable(SandboxPhase::Ready).await); + for phase in [ + SandboxPhase::Provisioning, + SandboxPhase::Completed, + SandboxPhase::Error, + SandboxPhase::Stopped, + SandboxPhase::Deleting, + ] { + assert!(!reachable(phase).await, "{phase:?}"); + } + + // A connected supervisor keeps Completed and Error sandboxes usable, + // but never revives one that is not running. + let (tx, _rx) = mpsc::channel(1); + let (shutdown_tx, _shutdown_rx) = oneshot::channel(); + state.supervisor_sessions.register( + "access".to_string(), + "session".to_string(), + tx, + shutdown_tx, + ); + for phase in [ + SandboxPhase::Ready, + SandboxPhase::Completed, + SandboxPhase::Error, + ] { + assert!(reachable(phase).await, "{phase:?}"); + } + for phase in [ + SandboxPhase::Provisioning, + SandboxPhase::Stopped, + SandboxPhase::Deleting, + ] { + assert!(!reachable(phase).await, "{phase:?}"); + } + } + + #[test] + fn not_reachable_keeps_the_existing_status() { + let status = Status::from(AccessError::NotReachable); + assert_eq!(status.code(), tonic::Code::FailedPrecondition); + assert_eq!(status.message(), "sandbox is not ready"); + } +} diff --git a/crates/openshell-server/src/service_routing.rs b/crates/openshell-server/src/service_routing.rs index 571c3f150d..7547eb8891 100644 --- a/crates/openshell-server/src/service_routing.rs +++ b/crates/openshell-server/src/service_routing.rs @@ -28,6 +28,7 @@ use tracing::warn; use crate::ServerState; use crate::persistence::{ObjectListQuery, ObjectType, Store}; +use crate::sandbox_access::{self, AccessError, SandboxAccess}; const ENDPOINT_OBJECT_TYPE: &str = "service_endpoint"; const ROUTING_RULE_NAME: &str = "sandbox_service_routing"; @@ -727,13 +728,18 @@ async fn proxy_to_endpoint( return Err(route_err); } }; - // Services follow the same access rule as exec, SSH, and port forwarding. - let reachable = match crate::supervisor_session::sandbox_relay_reachable(&state, &sandbox).await - { - Ok(reachable) => reachable, + // Every request passes the access gate, including ones served from a + // pooled upstream connection. + let access = match sandbox_access::acquire(&state, &sandbox).await { + Ok(access) => access, Err(err) => { - warn!(error = %err, sandbox_id = %endpoint.sandbox_id, "sandbox service routing: failed to resolve supervisor owner"); - let route_err = ServiceRouteError::internal_error(); + let route_err = match err { + AccessError::NotReachable => ServiceRouteError::sandbox_not_ready(), + AccessError::Lookup(err) | AccessError::Relay(err) => { + warn!(error = %err, sandbox_id = %endpoint.sandbox_id, "sandbox service routing: failed to check sandbox access"); + ServiceRouteError::internal_error() + } + }; emit_service_http_failure( &state, &req, @@ -745,18 +751,6 @@ async fn proxy_to_endpoint( return Err(route_err); } }; - if !reachable { - let err = ServiceRouteError::sandbox_not_ready(); - emit_service_http_failure( - &state, - &req, - &sandbox_name, - &service_name, - Some(&endpoint), - &err, - ); - return Err(err); - } let Ok(target_port) = u16::try_from(endpoint.target_port) else { let err = ServiceRouteError::endpoint_unavailable(); emit_service_http_failure( @@ -809,7 +803,7 @@ async fn proxy_to_endpoint( let reused = pooled.is_some(); let mut sender = match pooled { Some(sender) => sender, - None => open_upstream(&state, &sandbox, &endpoint, target_port, websocket_upgrade).await?, + None => open_upstream(&access, &endpoint, target_port, websocket_upgrade).await?, }; let upstream = build_upstream_request(req, target_port, websocket_upgrade, authorization_mode)?; @@ -835,8 +829,7 @@ async fn proxy_to_endpoint( emit_service_relay_failure(&endpoint, target_port, route_err.reason); return Err(route_err); }; - sender = - open_upstream(&state, &sandbox, &endpoint, target_port, websocket_upgrade).await?; + sender = open_upstream(&access, &endpoint, target_port, websocket_upgrade).await?; sender.send_request(retry_request).await.map_err(|err| { warn!(error = %err, "sandbox service routing: upstream HTTP retry failed"); let route_err = ServiceRouteError::service_unreachable(); @@ -884,23 +877,20 @@ async fn proxy_to_endpoint( } async fn open_upstream( - state: &Arc, - sandbox: &Sandbox, + access: &SandboxAccess, endpoint: &ServiceEndpoint, target_port: u16, websocket_upgrade: bool, ) -> Result { - let (_channel_id, relay_rx) = crate::supervisor_session::open_routed_relay_with_target( - state, - sandbox.object_id(), - relay_open::Target::Tcp(TcpRelayTarget { - host: RELAY_TARGET_HOST.to_string(), - port: u32::from(target_port), - }), - endpoint.object_id().to_string(), - Duration::from_secs(15), - ) - .await + let (_channel_id, relay_rx) = access + .open_relay( + relay_open::Target::Tcp(TcpRelayTarget { + host: RELAY_TARGET_HOST.to_string(), + port: u32::from(target_port), + }), + endpoint.object_id().to_string(), + ) + .await .map_err(|err| { warn!(error = %err, sandbox_id = %endpoint.sandbox_id, "sandbox service routing: supervisor relay unavailable"); let route_err = ServiceRouteError::service_unreachable(); diff --git a/crates/openshell-server/src/supervisor_session.rs b/crates/openshell-server/src/supervisor_session.rs index 4a33c0f67b..e9268c8ca1 100644 --- a/crates/openshell-server/src/supervisor_session.rs +++ b/crates/openshell-server/src/supervisor_session.rs @@ -1825,32 +1825,22 @@ fn owner_is_fresh(owner: &crate::supervisor_owner::OwnerRecord) -> bool { owner.is_fresh(OWNER_TTL) } -/// Whether the gateway can relay a new connection into this sandbox. +/// Whether this sandbox's supervisor is connected to any replica. /// -/// A Ready sandbox always qualifies; relay opening waits for and routes to its -/// owner. After the main process exits, the supervisor keeps its access plane, -/// so a Completed or Error sandbox qualifies while its supervisor session is -/// live on this replica or a fresh owner record names a peer replica. -pub async fn sandbox_relay_reachable( +/// A peer replica counts only through a fresh owner record. An owner record +/// naming this replica without a local session is a leftover from a lost +/// session. +pub async fn supervisor_connected( state: &Arc, - sandbox: &Sandbox, + sandbox_id: &str, ) -> Result { - match SandboxPhase::try_from(sandbox.phase()).ok() { - Some(SandboxPhase::Ready) => Ok(true), - Some(SandboxPhase::Completed | SandboxPhase::Error) => { - let sandbox_id = sandbox.object_id(); - if state.supervisor_sessions.has_session(sandbox_id) { - return Ok(true); - } - let owner_index = SupervisorOwnerIndex::new(state.store.clone(), OWNER_TTL); - Ok(resolve_owner(state, &owner_index, sandbox_id) - .await? - .is_some_and(|owner| { - owner_is_fresh(&owner) && owner.owner_replica_id != state.replica_id - })) - } - _ => Ok(false), + if state.supervisor_sessions.has_session(sandbox_id) { + return Ok(true); } + let owner_index = SupervisorOwnerIndex::new(state.store.clone(), OWNER_TTL); + Ok(resolve_owner(state, &owner_index, sandbox_id) + .await? + .is_some_and(|owner| owner_is_fresh(&owner) && owner.owner_replica_id != state.replica_id)) } /// The replica currently holding this sandbox's supervisor session, if known. @@ -3723,52 +3713,27 @@ mod tests { } #[tokio::test] - async fn relay_reachability_after_exit_follows_the_live_session_on_any_replica() { + async fn supervisor_connected_follows_the_live_session_on_any_replica() { let state = crate::grpc::test_support::test_server_state().await; - let reachable = |sandbox: Sandbox| { + let connected = |sandbox_id: &'static str| { let state = state.clone(); - async move { sandbox_relay_reachable(&state, &sandbox).await.unwrap() } - }; - let in_phase = |id: &str, phase: SandboxPhase| { - let mut sandbox = sandbox_record(id, id); - sandbox.set_phase(phase as i32); - sandbox + async move { supervisor_connected(&state, sandbox_id).await.unwrap() } }; - // Ready routes to the owner during relay opening; never-started or - // stopped sandboxes are refused regardless of sessions. - assert!(reachable(in_phase("relay-ready", SandboxPhase::Ready)).await); - for phase in [ - SandboxPhase::Provisioning, - SandboxPhase::Stopped, - SandboxPhase::Deleting, - ] { - assert!( - !reachable(in_phase("relay-local", phase)).await, - "{phase:?}" - ); - } - - for phase in [SandboxPhase::Completed, SandboxPhase::Error] { - // No session anywhere. - assert!(!reachable(in_phase("relay-none", phase)).await, "{phase:?}"); + assert!(!connected("relay-none").await); - // Session held by this replica. - let (tx, _rx) = mpsc::channel(1); - let (shutdown_tx, _shutdown_rx) = oneshot::channel(); - state.supervisor_sessions.register( - "relay-local".to_string(), - "session".to_string(), - tx, - shutdown_tx, - ); - assert!(reachable(in_phase("relay-local", phase)).await, "{phase:?}"); - assert!(state.supervisor_sessions.disconnect("relay-local")); - assert!( - !reachable(in_phase("relay-local", phase)).await, - "{phase:?}" - ); - } + // Session held by this replica. + let (tx, _rx) = mpsc::channel(1); + let (shutdown_tx, _shutdown_rx) = oneshot::channel(); + state.supervisor_sessions.register( + "relay-local".to_string(), + "session".to_string(), + tx, + shutdown_tx, + ); + assert!(connected("relay-local").await); + assert!(state.supervisor_sessions.disconnect("relay-local")); + assert!(!connected("relay-local").await); // Session held by a peer replica, known only through its owner record. let owner_index = SupervisorOwnerIndex::new(state.store.clone(), OWNER_TTL); @@ -3783,9 +3748,7 @@ mod tests { ) .await .unwrap(); - for phase in [SandboxPhase::Completed, SandboxPhase::Error] { - assert!(reachable(in_phase("relay-peer", phase)).await, "{phase:?}"); - } + assert!(connected("relay-peer").await); // An owner record naming this replica without a local session is a // leftover from a lost session, not a live supervisor. @@ -3800,7 +3763,7 @@ mod tests { ) .await .unwrap(); - assert!(!reachable(in_phase("relay-self", SandboxPhase::Completed)).await); + assert!(!connected("relay-self").await); } fn sandbox_record(id: &str, name: &str) -> Sandbox { From 079c2509064697b50dc83f23ae671dce2baa8c65 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Fri, 30 Oct 2026 18:05:12 -0700 Subject: [PATCH 6/8] fix(cli): let the gateway decide whether a sandbox can be used Exec no longer checks the sandbox phase before calling the gateway; when the gateway refuses, the CLI names the phase it saw. Service forwarding stops only when the sandbox is gone or being deleted, and leaves each connection to the gateway's access check. Signed-off-by: Piotr Mlocek --- crates/openshell-cli/src/run.rs | 54 +++++++++---------- .../sandbox_exec_streaming_integration.rs | 18 ++++++- 2 files changed, 41 insertions(+), 31 deletions(-) diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index b78703d9a5..d23490900c 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -2014,16 +2014,6 @@ pub async fn sandbox_exec_grpc( .sandbox .ok_or_else(|| miette::miette!("sandbox not found"))?; - // Reject phases where the relay can never be open. The gateway decides - // whether a Completed or Error sandbox still has a reachable supervisor. - if !relay_phase_may_be_reachable(sandbox.phase()) { - return Err(miette::miette!( - "sandbox '{}' is not ready (phase: {}); wait for it to reach Ready state", - name, - phase_name(sandbox.phase()) - )); - } - // Resolve TTY mode: explicit --tty / --no-tty wins, otherwise auto-detect. let stdin_is_terminal = std::io::stdin().is_terminal(); let tty = tty_override.unwrap_or_else(|| stdin_is_terminal && std::io::stdout().is_terminal()); @@ -2116,7 +2106,7 @@ pub async fn sandbox_exec_grpc( } (response, _) => response, } - .into_diagnostic()? + .map_err(|status| exec_start_error(status, &sandbox))? .into_inner(); // Stream output to terminal in real-time. @@ -2170,7 +2160,7 @@ pub async fn service_forward_tcp( let (bind_addr, bind_port) = parse_tcp_forward_spec(local, target_port)?; let mut client = grpc_client(server, tls).await?; - let mut replica = fetch_reachable_sandbox_for_forward(&mut client, name, workspace).await?; + let mut replica = fetch_sandbox_for_forward(&mut client, name, workspace).await?; let listener = tokio::net::TcpListener::bind((bind_addr.as_str(), bind_port)) .await @@ -2204,7 +2194,7 @@ pub async fn service_forward_tcp( } _ = health_check.tick() => { - replica = fetch_reachable_sandbox_for_forward(&mut client, name, workspace).await?; + replica = fetch_sandbox_for_forward(&mut client, name, workspace).await?; } accepted = listener.accept() => { @@ -2270,18 +2260,11 @@ fn forward_requires_session_token(status: &Status) -> bool { && status.message().contains("authorization_token is required") } -/// Whether the gateway may still relay into a sandbox in this phase. A -/// Completed or Error sandbox keeps its access plane while its supervisor -/// stays connected; only the gateway can tell whether it still is. -fn relay_phase_may_be_reachable(phase: i32) -> bool { - matches!( - SandboxPhase::try_from(phase), - Ok(SandboxPhase::Ready | SandboxPhase::Completed | SandboxPhase::Error) - ) -} - -/// Confirm the sandbox is still reachable, returning the replica that owns it. -async fn fetch_reachable_sandbox_for_forward( +/// Confirm the sandbox still exists, returning the replica that owns it. +/// +/// The gateway decides whether each connection may reach the sandbox; this +/// only stops a forward whose sandbox is gone or going away. +async fn fetch_sandbox_for_forward( client: &mut crate::tls::GrpcClient, name: &str, workspace: &str, @@ -2310,11 +2293,9 @@ async fn fetch_reachable_sandbox_for_forward( .sandbox .ok_or_else(|| miette::miette!("sandbox '{name}' not found"))?; - if !relay_phase_may_be_reachable(sandbox.phase()) { + if SandboxPhase::try_from(sandbox.phase()) == Ok(SandboxPhase::Deleting) { return Err(miette::miette!( - "sandbox '{}' is no longer ready (phase: {}); stopping service forward", - name, - phase_name(sandbox.phase()) + "sandbox '{name}' is being deleted; stopping service forward" )); } @@ -2587,6 +2568,19 @@ impl Drop for TaskGuard { } } +/// The gateway decides whether a sandbox can be used. When it refuses, name the +/// phase this command saw so the user knows what to wait for. +fn exec_start_error(status: Status, sandbox: &Sandbox) -> miette::Report { + if status.code() == Code::FailedPrecondition && status.message() == "sandbox is not ready" { + return miette::miette!( + "sandbox '{}' is not ready (phase: {}); wait for it to reach Ready state", + sandbox.object_name(), + phase_name(sandbox.phase()) + ); + } + miette::Report::from_err(status) +} + #[allow(clippy::too_many_arguments)] async fn sandbox_exec_streaming_grpc( mut client: crate::tls::GrpcClient, @@ -2649,7 +2643,7 @@ async fn sandbox_exec_streaming_grpc( } response => response, } - .into_diagnostic()? + .map_err(|status| exec_start_error(status, sandbox))? .into_inner(); // Raw mode is only appropriate for an interactive terminal, not a pipe. diff --git a/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs b/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs index ccf94604d7..021138261a 100644 --- a/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs +++ b/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs @@ -67,6 +67,20 @@ fn exit(code: i32) -> proto::ExecSandboxEvent { } impl MockGateway { + /// The gateway's access rule: only it decides whether a sandbox is usable. + fn require_reachable(&self) -> Result<(), Status> { + if matches!( + self.phase, + proto::SandboxPhase::Ready + | proto::SandboxPhase::Completed + | proto::SandboxPhase::Error + ) { + Ok(()) + } else { + Err(Status::failed_precondition("sandbox is not ready")) + } + } + async fn exchange( self, mut input: tonic::Streaming, @@ -201,12 +215,14 @@ macro_rules! mock_gateway { type ExecSandboxStream = EventStream; async fn exec_sandbox(&self, _: Request) -> Result, Status> { + self.require_reachable()?; self.calls.lock().unwrap().unary += 1; Err(Status::unimplemented("test requires streaming exec")) } type ExecSandboxInteractiveStream = EventStream; async fn exec_sandbox_interactive(&self, request: Request>) -> Result, Status> { + self.require_reachable()?; let (sender, receiver) = mpsc::channel(1); let service = self.clone(); tokio::spawn(async move { @@ -505,7 +521,7 @@ async fn exec_reaches_gateway_for_completed_and_error_sandboxes() { } #[tokio::test] -async fn exec_is_refused_before_starting_for_stopped_sandbox() { +async fn exec_names_the_phase_when_the_gateway_refuses() { let gateway = TestGateway::start_in_phase(Scenario::EarlyExit, proto::SandboxPhase::Stopped).await; let mut child = gateway.spawn(); From acb3769ee6158825c2c54a23cf7408aa5dcd2667 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Fri, 30 Oct 2026 18:07:11 -0700 Subject: [PATCH 7/8] docs(sandboxes): drop lifecycle diagram and exec hint Signed-off-by: Piotr Mlocek --- docs/how-it-works/sandboxes/overview.mdx | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/docs/how-it-works/sandboxes/overview.mdx b/docs/how-it-works/sandboxes/overview.mdx index 1c28b4becf..5d6998cc0f 100644 --- a/docs/how-it-works/sandboxes/overview.mdx +++ b/docs/how-it-works/sandboxes/overview.mdx @@ -1161,28 +1161,6 @@ after it attempts every requested deletion if any entry failed. Every sandbox moves through a defined set of phases: -```mermaid -stateDiagram-v2 - [*] --> Provisioning: create - Provisioning --> Ready: supervisor connects - Provisioning --> Error: preparation or admission times out - Ready --> Provisioning: supervisor disconnects - Ready --> Completed: main process exits 0 - Ready --> Error: main process exits nonzero - Ready --> Stopping: stop - Error --> Stopping: stop - Stopping --> Stopped - Stopped --> Starting: start - Completed --> Starting: start or restart policy - Error --> Starting: start or restart policy - Starting --> Ready: new supervisor connects - Completed --> Deleting: delete, or --no-keep cleanup - Error --> Deleting: delete, or --no-keep cleanup - Stopped --> Deleting: delete - Ready --> Deleting: delete - Deleting --> [*] -``` - Before workload activation, OpenShell validates the effective policy and matching provider configuration. A rejection keeps the workload unstarted and exposes a `ConfigurationInvalid` condition in `Provisioning`. Use `openshell sandbox get` @@ -1259,7 +1237,7 @@ supervisor stays connected: - exposed services `sandbox connect` fails, because it attaches to the main process and that -process has exited. Use `sandbox exec --tty` instead. +process has exited. An `Error` caused by an infrastructure failure, such as `ImagePreparationTimedOut`, usually has no supervisor left, so these From ce03ae9eafe6c4b43f6daf84b9d47782a87b1e25 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Fri, 30 Oct 2026 18:56:02 -0700 Subject: [PATCH 8/8] fix(server): keep exposed services off infrastructure errors Exposed services do not authenticate callers, so they now reach an Error sandbox only when its main process failed. Exec, SSH, and forwarding keep the debugging path into an infrastructure Error. The CLI service forward stops again once its sandbox is stopping, stopped, or being deleted, and the TypeScript SDK forward applies the same check instead of requiring Ready. Exec refusals name what makes the sandbox usable again. supervisor_connected skips the owner index on a single replica, the unused relay error variant is gone, and relay opening outside sandbox_access is crate-private. Signed-off-by: Piotr Mlocek --- crates/openshell-cli/src/run.rs | 74 +++++++++++++++---- .../sandbox_exec_streaming_integration.rs | 12 ++- crates/openshell-server/src/compute/mod.rs | 2 +- crates/openshell-server/src/grpc/sandbox.rs | 5 +- crates/openshell-server/src/sandbox_access.rs | 73 ++++++++++++++++-- .../openshell-server/src/service_routing.rs | 4 +- .../src/supervisor_session.rs | 56 ++++++++------ docs/how-it-works/sandboxes/overview.mdx | 4 +- sdk/typescript/src/client.test.ts | 20 ++++- sdk/typescript/src/client.ts | 8 +- 10 files changed, 202 insertions(+), 56 deletions(-) diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index d23490900c..dec7a51986 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -2260,10 +2260,12 @@ fn forward_requires_session_token(status: &Status) -> bool { && status.message().contains("authorization_token is required") } -/// Confirm the sandbox still exists, returning the replica that owns it. +/// Confirm the sandbox can still serve the forward, returning the replica that +/// owns it. /// /// The gateway decides whether each connection may reach the sandbox; this -/// only stops a forward whose sandbox is gone or going away. +/// only stops a forward whose sandbox is gone or is stopping, stopped, or +/// being deleted, since no connection can succeed until a new command acts. async fn fetch_sandbox_for_forward( client: &mut crate::tls::GrpcClient, name: &str, @@ -2293,15 +2295,25 @@ async fn fetch_sandbox_for_forward( .sandbox .ok_or_else(|| miette::miette!("sandbox '{name}' not found"))?; - if SandboxPhase::try_from(sandbox.phase()) == Ok(SandboxPhase::Deleting) { + if forward_ended_by_phase(sandbox.phase()) { return Err(miette::miette!( - "sandbox '{name}' is being deleted; stopping service forward" + "sandbox '{name}' is no longer available (phase: {}); stopping service forward", + phase_name(sandbox.phase()) )); } Ok(replica) } +/// Phases in which no forwarded connection can succeed until another command +/// starts or recreates the sandbox. +fn forward_ended_by_phase(phase: i32) -> bool { + matches!( + SandboxPhase::try_from(phase), + Ok(SandboxPhase::Stopping | SandboxPhase::Stopped | SandboxPhase::Deleting) + ) +} + #[derive(Debug)] struct ForwardTcpConnectionError { message: String, @@ -2569,12 +2581,21 @@ impl Drop for TaskGuard { } /// The gateway decides whether a sandbox can be used. When it refuses, name the -/// phase this command saw so the user knows what to wait for. +/// phase this command saw and what would make the sandbox usable again. fn exec_start_error(status: Status, sandbox: &Sandbox) -> miette::Report { if status.code() == Code::FailedPrecondition && status.message() == "sandbox is not ready" { + let name = sandbox.object_name(); + let hint = match SandboxPhase::try_from(sandbox.phase()) { + Ok(SandboxPhase::Stopping | SandboxPhase::Stopped) => { + format!("start it with `openshell sandbox start {name}`") + } + Ok(SandboxPhase::Completed | SandboxPhase::Error) => { + "its main process exited and its supervisor is no longer connected".to_string() + } + _ => "wait for it to reach Ready state".to_string(), + }; return miette::miette!( - "sandbox '{}' is not ready (phase: {}); wait for it to reach Ready state", - sandbox.object_name(), + "sandbox '{name}' is not ready (phase: {}); {hint}", phase_name(sandbox.phase()) ); } @@ -7129,14 +7150,14 @@ fn format_endpoint(endpoint: &openshell_core::proto::NetworkEndpoint) -> String mod tests { use super::{ ForwardTcpConnectionError, PolicyGetView, ProvisioningStep, build_sandbox_resource_limits, - format_endpoint, format_log_line, forward_requires_session_token, git_sync_files, - has_main_process_result, parse_cli_setting_value, parse_credential_expiry_cli_value, - parse_driver_config_json, parse_env_from_pairs, parse_secret_material_env_pairs, - policy_revision_list_json, policy_revision_to_json, proto_execution_timeout, - provisioning_timeout_message, ready_false_condition_message, relay_local_socket, - resolve_from, rootfs_tar_sources_supported_for_gateway, sandbox_should_persist, - sandbox_upload_plan, service_endpoint_to_json, service_status_error, - service_url_for_gateway, workspace_member_to_json, + format_endpoint, format_log_line, forward_ended_by_phase, forward_requires_session_token, + git_sync_files, has_main_process_result, parse_cli_setting_value, + parse_credential_expiry_cli_value, parse_driver_config_json, parse_env_from_pairs, + parse_secret_material_env_pairs, policy_revision_list_json, policy_revision_to_json, + proto_execution_timeout, provisioning_timeout_message, ready_false_condition_message, + relay_local_socket, resolve_from, rootfs_tar_sources_supported_for_gateway, + sandbox_should_persist, sandbox_upload_plan, service_endpoint_to_json, + service_status_error, service_url_for_gateway, workspace_member_to_json, }; use openshell_core::proto::TcpForwardFrame; @@ -9503,4 +9524,27 @@ mod tests { "authorization_token is required for ForwardTcp" ))); } + + #[test] + fn forward_leaves_reachability_after_exit_to_the_gateway() { + use openshell_core::proto::SandboxPhase; + + for phase in [ + SandboxPhase::Stopping, + SandboxPhase::Stopped, + SandboxPhase::Deleting, + ] { + assert!(forward_ended_by_phase(phase as i32), "{phase:?}"); + } + for phase in [ + SandboxPhase::Provisioning, + SandboxPhase::Starting, + SandboxPhase::Ready, + SandboxPhase::Completed, + SandboxPhase::Error, + SandboxPhase::Suspended, + ] { + assert!(!forward_ended_by_phase(phase as i32), "{phase:?}"); + } + } } diff --git a/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs b/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs index 021138261a..522c5ce18b 100644 --- a/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs +++ b/crates/openshell-cli/tests/sandbox_exec_streaming_integration.rs @@ -528,8 +528,16 @@ async fn exec_names_the_phase_when_the_gateway_refuses() { drop(child.stdin.take()); let output = finish(child).await; assert!(!output.status.success()); - let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.contains("is not ready (phase: Stopped)"), "{stderr}"); + // The error renderer wraps long messages, so compare the words alone. + let stderr = String::from_utf8_lossy(&output.stderr) + .split_whitespace() + .filter(|word| *word != "│") + .collect::>() + .join(" "); + assert!( + stderr.contains("is not ready (phase: Stopped); start it with `openshell sandbox start"), + "{stderr}" + ); assert!(gateway.calls.lock().unwrap().starts.is_empty()); } diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index 2b168f26df..73cdb54196 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -6891,7 +6891,7 @@ fn terminal_stop_exit_code(sandbox: &Sandbox) -> Option { sandbox.status.as_ref()?.exit_code } -fn is_failed_main_process_result(sandbox: &Sandbox) -> bool { +pub fn is_failed_main_process_result(sandbox: &Sandbox) -> bool { sandbox.phase() == SandboxPhase::Error as i32 && sandbox.status.as_ref().is_some_and(|status| { status.exit_code.is_some() diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index 013b06e88f..bef69486ad 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -2853,8 +2853,9 @@ pub(super) async fn handle_forward_tcp( .await?; // The main process may finish between minting the SSH token and opening - // its transport. Keep the relay reachable until terminal delivery is - // finalized so fast commands can attach without a readiness race. + // its transport. The access rules keep a Completed or Error sandbox + // reachable while its supervisor is connected, so fast commands attach + // without a readiness race. let access = crate::sandbox_access::acquire(state, &sandbox).await?; let connection_guard = diff --git a/crates/openshell-server/src/sandbox_access.rs b/crates/openshell-server/src/sandbox_access.rs index 92119d7740..85230ebc2f 100644 --- a/crates/openshell-server/src/sandbox_access.rs +++ b/crates/openshell-server/src/sandbox_access.rs @@ -17,6 +17,9 @@ //! - A `Completed` or `Error` sandbox is usable while its supervisor is still //! connected to any replica. A retained sandbox keeps its access plane after //! its main process exits. +//! - Exposed services, which do not authenticate callers, reach an `Error` +//! sandbox only when its main process failed. An infrastructure failure +//! stays reachable for debugging through the authenticated paths only. //! - Every other phase is refused. use std::fmt; @@ -42,8 +45,6 @@ pub enum AccessError { NotReachable, /// The gateway could not determine whether the sandbox is reachable. Lookup(Status), - /// The supervisor did not open the relay. - Relay(Status), } impl fmt::Display for AccessError { @@ -51,7 +52,6 @@ impl fmt::Display for AccessError { match self { Self::NotReachable => f.write_str("sandbox is not ready"), Self::Lookup(status) => write!(f, "sandbox access check failed: {status}"), - Self::Relay(status) => write!(f, "supervisor relay failed: {status}"), } } } @@ -61,7 +61,6 @@ impl From for Status { match err { AccessError::NotReachable => Self::failed_precondition(err.to_string()), AccessError::Lookup(status) => status, - AccessError::Relay(_) => Self::unavailable(err.to_string()), } } } @@ -95,6 +94,20 @@ pub async fn acquire( }) } +/// Apply the access rules for an exposed service, which serves callers the +/// gateway does not authenticate. +pub async fn acquire_for_exposed_service( + state: &Arc, + sandbox: &Sandbox, +) -> Result { + if sandbox.phase() == SandboxPhase::Error as i32 + && !crate::compute::is_failed_main_process_result(sandbox) + { + return Err(AccessError::NotReachable); + } + acquire(state, sandbox).await +} + impl SandboxAccess { /// Ask the supervisor to open a relay to `target`, returning the channel /// id and a receiver that resolves once the supervisor claims it. @@ -102,7 +115,7 @@ impl SandboxAccess { &self, target: relay_open::Target, service_id: String, - ) -> Result<(String, oneshot::Receiver>), AccessError> { + ) -> Result<(String, oneshot::Receiver>), Status> { crate::supervisor_session::open_routed_relay_with_target( &self.state, &self.sandbox_id, @@ -111,7 +124,7 @@ impl SandboxAccess { RELAY_SESSION_WAIT, ) .await - .map_err(AccessError::Relay) + .map_err(|err| Status::unavailable(format!("supervisor relay failed: {err}"))) } } @@ -187,6 +200,54 @@ mod tests { } } + fn error_sandbox(id: &str, reason: &str, exit_code: Option) -> Sandbox { + let mut sandbox = sandbox_in(id, SandboxPhase::Error); + sandbox.status = Some(openshell_core::proto::SandboxStatus { + phase: SandboxPhase::Error.into(), + exit_code, + conditions: vec![openshell_core::proto::SandboxCondition { + r#type: "Ready".into(), + status: "False".into(), + reason: reason.into(), + ..Default::default() + }], + ..Default::default() + }); + sandbox + } + + #[tokio::test] + async fn exposed_services_skip_infrastructure_errors() { + let state = crate::grpc::test_support::test_server_state().await; + let (tx, _rx) = mpsc::channel(1); + let (shutdown_tx, _shutdown_rx) = oneshot::channel(); + state.supervisor_sessions.register( + "access".to_string(), + "session".to_string(), + tx, + shutdown_tx, + ); + let main_failed = error_sandbox("access", "MainProcessFailed", Some(3)); + let timed_out = error_sandbox("access", "ProvisioningTimedOut", None); + + assert!( + acquire_for_exposed_service(&state, &main_failed) + .await + .is_ok() + ); + assert!(matches!( + acquire_for_exposed_service(&state, &timed_out).await, + Err(AccessError::NotReachable) + )); + // Authenticated paths keep the debugging shell. + assert!(acquire(&state, &timed_out).await.is_ok()); + assert!( + acquire_for_exposed_service(&state, &sandbox_in("access", SandboxPhase::Completed)) + .await + .is_ok() + ); + } + #[test] fn not_reachable_keeps_the_existing_status() { let status = Status::from(AccessError::NotReachable); diff --git a/crates/openshell-server/src/service_routing.rs b/crates/openshell-server/src/service_routing.rs index 7547eb8891..b20843b35d 100644 --- a/crates/openshell-server/src/service_routing.rs +++ b/crates/openshell-server/src/service_routing.rs @@ -730,12 +730,12 @@ async fn proxy_to_endpoint( }; // Every request passes the access gate, including ones served from a // pooled upstream connection. - let access = match sandbox_access::acquire(&state, &sandbox).await { + let access = match sandbox_access::acquire_for_exposed_service(&state, &sandbox).await { Ok(access) => access, Err(err) => { let route_err = match err { AccessError::NotReachable => ServiceRouteError::sandbox_not_ready(), - AccessError::Lookup(err) | AccessError::Relay(err) => { + AccessError::Lookup(err) => { warn!(error = %err, sandbox_id = %endpoint.sandbox_id, "sandbox service routing: failed to check sandbox access"); ServiceRouteError::internal_error() } diff --git a/crates/openshell-server/src/supervisor_session.rs b/crates/openshell-server/src/supervisor_session.rs index e9268c8ca1..be785666b4 100644 --- a/crates/openshell-server/src/supervisor_session.rs +++ b/crates/openshell-server/src/supervisor_session.rs @@ -1613,7 +1613,11 @@ pub(crate) async fn forward_provider_status_query_to_owner( }) } -pub async fn open_routed_relay_with_target( +/// Open a relay into a sandbox wherever its supervisor is connected. +/// +/// Entry points open relays through `sandbox_access::SandboxAccess`, which +/// applies the access rules first. +pub(crate) async fn open_routed_relay_with_target( state: &Arc, sandbox_id: &str, target: relay_open::Target, @@ -1837,10 +1841,19 @@ pub async fn supervisor_connected( if state.supervisor_sessions.has_session(sandbox_id) { return Ok(true); } + if state.store.is_single_replica() { + return Ok(false); + } let owner_index = SupervisorOwnerIndex::new(state.store.clone(), OWNER_TTL); - Ok(resolve_owner(state, &owner_index, sandbox_id) - .await? - .is_some_and(|owner| owner_is_fresh(&owner) && owner.owner_replica_id != state.replica_id)) + let owner = resolve_owner(state, &owner_index, sandbox_id).await?; + Ok(peer_holds_session(&state.replica_id, owner.as_ref())) +} + +fn peer_holds_session( + self_replica_id: &str, + owner: Option<&crate::supervisor_owner::OwnerRecord>, +) -> bool { + owner.is_some_and(|owner| owner_is_fresh(owner) && owner.owner_replica_id != self_replica_id) } /// The replica currently holding this sandbox's supervisor session, if known. @@ -3713,7 +3726,7 @@ mod tests { } #[tokio::test] - async fn supervisor_connected_follows_the_live_session_on_any_replica() { + async fn supervisor_connected_follows_the_local_session() { let state = crate::grpc::test_support::test_server_state().await; let connected = |sandbox_id: &'static str| { let state = state.clone(); @@ -3722,7 +3735,6 @@ mod tests { assert!(!connected("relay-none").await); - // Session held by this replica. let (tx, _rx) = mpsc::channel(1); let (shutdown_tx, _shutdown_rx) = oneshot::channel(); state.supervisor_sessions.register( @@ -3735,9 +3747,10 @@ mod tests { assert!(state.supervisor_sessions.disconnect("relay-local")); assert!(!connected("relay-local").await); - // Session held by a peer replica, known only through its owner record. - let owner_index = SupervisorOwnerIndex::new(state.store.clone(), OWNER_TTL); - owner_index + // A single replica holds every session itself, so an owner record + // naming a peer is a leftover and is not read. + assert!(state.store.is_single_replica()); + SupervisorOwnerIndex::new(state.store.clone(), OWNER_TTL) .publish( "relay-peer", "session", @@ -3748,22 +3761,21 @@ mod tests { ) .await .unwrap(); - assert!(connected("relay-peer").await); + assert!(!connected("relay-peer").await); + } + + #[test] + fn peer_holds_session_only_through_a_fresh_peer_owner_record() { + assert!(peer_holds_session("gw-0", Some(&owner_record("gw-1")))); + assert!(!peer_holds_session("gw-0", None)); + + let mut stale = owner_record("gw-1"); + stale.updated_at_ms = 0; + assert!(!peer_holds_session("gw-0", Some(&stale))); // An owner record naming this replica without a local session is a // leftover from a lost session, not a live supervisor. - owner_index - .publish( - "relay-self", - "session", - "supervisor", - 1, - &state.replica_id, - "https://self", - ) - .await - .unwrap(); - assert!(!connected("relay-self").await); + assert!(!peer_holds_session("gw-0", Some(&owner_record("gw-0")))); } fn sandbox_record(id: &str, name: &str) -> Sandbox { diff --git a/docs/how-it-works/sandboxes/overview.mdx b/docs/how-it-works/sandboxes/overview.mdx index 5d6998cc0f..16af889067 100644 --- a/docs/how-it-works/sandboxes/overview.mdx +++ b/docs/how-it-works/sandboxes/overview.mdx @@ -1241,7 +1241,9 @@ process has exited. An `Error` caused by an infrastructure failure, such as `ImagePreparationTimedOut`, usually has no supervisor left, so these -operations fail with `sandbox is not ready`. +operations fail with `sandbox is not ready`. If the supervisor survived, exec, +file transfer, and port forwarding still work for debugging, but exposed +services stay unavailable. ## Sandbox Runtimes diff --git a/sdk/typescript/src/client.test.ts b/sdk/typescript/src/client.test.ts index 86fdfa3972..a32ecfbb79 100644 --- a/sdk/typescript/src/client.test.ts +++ b/sdk/typescript/src/client.test.ts @@ -2236,18 +2236,34 @@ describe('forward', () => { expect(revokedToken).toBe('fwd-tok'); }); - it('rejects when the sandbox is not ready', async () => { + it('rejects when the sandbox is stopped', async () => { const sandbox = client({ getSandbox: () => ({ sandbox: { metadata: { id: 'sb-id', name: 'sb' }, - status: { phase: SandboxPhase.PROVISIONING }, + status: { phase: SandboxPhase.STOPPED }, }, }), }); await expect(sandbox.forward('sb', { targetPort: 9000 })).rejects.toMatchObject({ code: 'connect' }); }); + it('leaves a completed sandbox to the gateway', async () => { + // A retained sandbox keeps its access plane after the main process exits; + // the gateway checks each forwarded connection. + const sandbox = client({ + getSandbox: () => ({ + sandbox: { + metadata: { id: 'sb-id', name: 'sb' }, + status: { phase: SandboxPhase.COMPLETED, exitCode: 0 }, + }, + }), + }); + const handle = await sandbox.forward('sb', { targetPort: 9000 }); + await handle.close(); + await handle.closed; + }); + // Backpressure (fix #6): the sandbox->local relay must stop pulling gRPC // frames when socket.write() returns false and resume after 'drain', so a // slow local reader cannot make Node buffer sandbox output without bound. diff --git a/sdk/typescript/src/client.ts b/sdk/typescript/src/client.ts index 205796f1bd..65ccdbbf66 100644 --- a/sdk/typescript/src/client.ts +++ b/sdk/typescript/src/client.ts @@ -1731,7 +1731,9 @@ export class SandboxClient { } // Bind a local TCP listener that tunnels each accepted connection into the - // sandbox. Mirrors the CLI service forward: READY check, then per socket mint + // sandbox. Mirrors the CLI service forward: refuse a sandbox that is stopping, + // stopped, or being deleted and leave every other phase to the gateway, which + // checks each connection. Then per socket mint // a short-lived SSH session token, open a forwardTcp bidi whose first frame is // the `init` (TCP target + token), relay bytes both ways in ~64 KiB chunks, // and revoke the token on close. Process-lifetime only. @@ -1749,8 +1751,8 @@ export class SandboxClient { ...(opts.signal ? { signal: opts.signal } : {}), }); const ref = found.ref; - if (ref.phase !== 'ready') { - throw new SdkError('connect', `sandbox '${name}' is not ready (phase: ${ref.phase})`); + if (ref.phase === 'stopping' || ref.phase === 'stopped' || ref.phase === 'deleting') { + throw new SdkError('connect', `sandbox '${name}' is not available (phase: ${ref.phase})`); } sandboxId = ref.id; owner = found.owner;