Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions crates/openshell-sandbox-backend/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1601,6 +1607,9 @@ struct BoundaryClient {
recovery_timeout_secs: Option<u32>,
/// True once a connection of this client was confirmed.
confirmed: tokio::sync::watch::Sender<bool>,
/// Set when the owner stops the boundary; a lost connection is then
/// reported instead of recovered.
stopping: AtomicBool,
}

#[derive(Clone)]
Expand Down Expand Up @@ -1800,6 +1809,7 @@ impl BoundaryClient {
requires_takeover: false,
recovery_timeout_secs: None,
confirmed: tokio::sync::watch::Sender::new(false),
stopping: AtomicBool::new(false),
}
}

Expand Down Expand Up @@ -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(),
));
}
Comment on lines +2221 to +2225

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this fires during the supervisor's shutdown, BoundaryProcess::wait() maps it to Terminated("boundary lost during wait: ..."). Then the .map_err(...)? after the 5s wait in run_sandbox_instance returns Err. The supervisor exits with an error and skips publish_main_exit, the OTLP relay flush, and the gateway exit report.

That's no worse than main, where it got SIGKILLed. But now that we fail fast, should the supervisor treat "boundary lost while stopping" as the workload having ended? It could record an exit status, skip TerminateBoundary, and continue through normal teardown. Otherwise every Podman/K8s delete where the boundary goes first ends with an error-level failure from the supervisor on a routine operation.


*self.mediation.lock().await = None;
let attach = self
Expand Down Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions crates/openshell-supervisor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1800,6 +1800,7 @@ pub async fn run_sandbox_instance(
));
}
() = &mut shutdown_requested => {
running.begin_stop();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions about the scope of this:

  1. shutdown_requested isn't only SIGTERM. For hosted instances in a shared supervisor it also fires on Departure::Stop from SandboxInstanceHost (sandbox_instance_host.rs:440, :685). In that case the supervisor process isn't exiting and the remote boundary is usually still up. Before this change, a transient connection drop during that stop was recovered, and the boundary got a clean TerminateBoundary. Now the instance reports Failed, TerminateBoundary is never sent, and boundary cleanup is left to the driver. Is that intended? If not, should begin_stop() be limited to the dedicated-process signal path, or use a short recovery window rather than none?

  2. The flag is client-wide and never cleared, so once it's set nothing on this client reconnects. That includes the workload's network mediation during the TERM grace window, which is exactly the "slow to handle TERM, running cleanup handlers" scenario the un-ignored conformance test describes. If the connection really does drop in that window, the workload's cleanup traffic fails instead of recovering. Probably rare, but it's a behavior change the PR description doesn't mention.

let _ = agent
.signal(openshell_isolation_interface::contract::BoundarySignal::Term)
.await;
Expand Down
4 changes: 0 additions & 4 deletions tests/suites/conformance/src/lifecycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for remembering to remove this!

async fn delete_is_prompt_when_workload_is_slow_to_handle_term(
cx: &mut Context,
) -> testkit::Result {
Expand Down
Loading