diff --git a/crates/openshell-driver-mxc/src/driver.rs b/crates/openshell-driver-mxc/src/driver.rs index 62f134205b..565506ea15 100644 --- a/crates/openshell-driver-mxc/src/driver.rs +++ b/crates/openshell-driver-mxc/src/driver.rs @@ -438,24 +438,42 @@ impl MxcComputeBackend { Ok(()) } - pub async fn stop_sandbox(&self, sandbox_name: &str) -> Result<(), tonic::Status> { - let (sandbox_id, lifecycle_gate) = { + pub async fn stop_sandbox( + &self, + sandbox_id: &str, + sandbox_name: &str, + ) -> Result<(), tonic::Status> { + let lifecycle_gate = { let registry = self.registry.lock().await; - let entry = registry - .values() - .find(|entry| entry.sandbox.name == sandbox_name) - .ok_or_else(|| { - tonic::Status::not_found(format!("sandbox {sandbox_name} not found")) - })?; - (entry.sandbox.id.clone(), entry.lifecycle_gate.clone()) + let entry = registry.get(sandbox_id).ok_or_else(|| { + tonic::Status::not_found(format!("sandbox {sandbox_name} not found")) + })?; + + if entry.sandbox.name != sandbox_name { + return Err(tonic::Status::failed_precondition( + "sandbox_id did not match sandbox_name", + )); + } + + entry.lifecycle_gate.clone() }; let _lifecycle_guard = lifecycle_gate.lock().await; let (iso_id, mut isolation_stopped, cancel, monitor_task) = { let mut registry = self.registry.lock().await; - let entry = registry.get_mut(&sandbox_id).ok_or_else(|| { + let entry = registry.get_mut(sandbox_id).ok_or_else(|| { tonic::Status::not_found(format!("sandbox {sandbox_name} not found")) })?; + // The registry lock was released while this request waited for the + // gate. Every create mints a fresh gate, so an entry whose gate is a + // different allocation is a replacement registered under the same id + // rather than the sandbox validated above. Once this check passes the + // held gate blocks delete, so the remaining steps stay on this entry. + if !Arc::ptr_eq(&entry.lifecycle_gate, &lifecycle_gate) { + return Err(tonic::Status::aborted( + "sandbox was replaced while the stop request waited", + )); + } ( entry.iso_sandbox_id.clone(), entry.isolation_stopped, @@ -481,7 +499,7 @@ impl MxcComputeBackend { } let mut registry = self.registry.lock().await; - if let Some(entry) = registry.get_mut(&sandbox_id) { + if let Some(entry) = registry.get_mut(sandbox_id) { entry.isolation_stopped = isolation_stopped; entry.phase_state = PhaseState::Stopped; entry.sandbox = make_sandbox_with_condition( @@ -525,6 +543,13 @@ impl MxcComputeBackend { let Some(entry) = registry.get_mut(sandbox_id) else { return Ok(false); }; + // See stop_sandbox: a gate that is a different allocation means the + // requested sandbox is already gone and this id now belongs to a + // replacement, which this request must not remove. Deleting nothing + // keeps the operation idempotent. + if !Arc::ptr_eq(&entry.lifecycle_gate, &lifecycle_gate) { + return Ok(false); + } ( entry.iso_sandbox_id.clone(), entry.isolation_stopped, @@ -931,6 +956,19 @@ mod lifecycle_tests { status: None, } } + + fn driver_sandbox_named( + id: &str, + name: &str, + workspace: &str, + command: Vec, + ) -> DriverSandbox { + let mut sandbox = driver_sandbox_with_command(id, "", command); + sandbox.name = name.to_string(); + sandbox.workspace = workspace.to_string(); + sandbox + } + fn fs_policy(read_write: &[&str]) -> SandboxPolicy { SandboxPolicy { filesystem: Some(FilesystemPolicy { @@ -976,6 +1014,38 @@ mod lifecycle_tests { None } + /// Look a sandbox up by its stable id. + /// + /// `get_sandbox` resolves by name, so it cannot address an individual entry + /// once two sandboxes in different workspaces share one. + async fn sandbox_by_id(backend: &MxcComputeBackend, id: &str) -> Option { + backend + .list_sandboxes() + .await + .into_iter() + .find(|sandbox| sandbox.id == id) + } + + /// Poll a specific sandbox id until the predicate matches or the deadline hits. + async fn wait_for_id( + backend: &MxcComputeBackend, + id: &str, + mut pred: F, + ) -> Option + where + F: FnMut(&DriverSandbox) -> bool, + { + for _ in 0..100 { + if let Some(sandbox) = sandbox_by_id(backend, id).await + && pred(&sandbox) + { + return Some(sandbox); + } + tokio::time::sleep(Duration::from_millis(100)).await; + } + None + } + #[test] fn mxc_config_defaults_to_default_deny_process_container() { assert_eq!( @@ -1209,14 +1279,184 @@ mod lifecycle_tests { let running = backend.get_sandbox("sb-stop").await.unwrap(); assert_eq!(ready_condition(&running).unwrap().reason, "AgentRunning"); - tokio::time::timeout(Duration::from_secs(5), backend.stop_sandbox("sb-stop")) - .await - .expect("stop should not wait for the child sleep") - .expect("stop should terminate and reap the child"); + tokio::time::timeout( + Duration::from_secs(5), + backend.stop_sandbox("sb-stop", "sb-stop"), + ) + .await + .expect("stop should not wait for the child sleep") + .expect("stop should terminate and reap the child"); let stopped = backend.get_sandbox("sb-stop").await.unwrap(); assert_eq!(ready_condition(&stopped).unwrap().reason, "Stopped"); } + #[tokio::test] + async fn stop_targets_the_requested_id_when_two_workspaces_share_a_name() { + let tmp = tempfile::tempdir().unwrap(); + let share = tmp.path().to_string_lossy().replace('\\', "/"); + let sleeper = || { + vec![ + "powershell".into(), + "-NoProfile".into(), + "-Command".into(), + format!("$null = '{share}'; Start-Sleep -Seconds 60"), + ] + }; + let backend = MxcComputeBackend::new_mocked(MxcComputeConfig::default()); + + // Sandbox names are unique per workspace, not globally, so "demo" is a + // legal name in both. The driver registry is one flat map keyed by id and + // holds both entries at once. + for (id, workspace) in [("sb-alpha", "alpha"), ("sb-beta", "beta")] { + let sandbox = with_policy( + driver_sandbox_named(id, "demo", workspace, sleeper()), + fs_policy(&[&share]), + ); + backend + .create_sandbox(&sandbox) + .await + .expect("create accepted"); + } + for id in ["sb-alpha", "sb-beta"] { + wait_for_id(&backend, id, |sandbox| { + ready_condition(sandbox).is_some_and(|condition| condition.reason == "AgentRunning") + }) + .await + .unwrap_or_else(|| panic!("{id} should start")); + } + tokio::time::sleep(Duration::from_millis(250)).await; + + tokio::time::timeout( + Duration::from_secs(5), + backend.stop_sandbox("sb-beta", "demo"), + ) + .await + .expect("stop should not wait for the child sleep") + .expect("stop should terminate the requested sandbox"); + + let beta = sandbox_by_id(&backend, "sb-beta") + .await + .expect("sb-beta should still be registered"); + assert_eq!( + ready_condition(&beta).unwrap().reason, + "Stopped", + "the sandbox named by sandbox_id should be stopped" + ); + + // The load-bearing assertion. Resolving by name alone picks whichever + // entry the registry's hash order yields first, so a name-based lookup + // fails this intermittently rather than every run. + let alpha = sandbox_by_id(&backend, "sb-alpha") + .await + .expect("sb-alpha should still be registered"); + assert_eq!( + ready_condition(&alpha).unwrap().reason, + "AgentRunning", + "a same-named sandbox in another workspace must keep running" + ); + } + + #[tokio::test] + async fn stop_rejects_a_sandbox_id_that_does_not_match_the_sandbox_name() { + let tmp = tempfile::tempdir().unwrap(); + let share = tmp.path().to_string_lossy().replace('\\', "/"); + let backend = MxcComputeBackend::new_mocked(MxcComputeConfig::default()); + let sandbox = with_policy( + driver_sandbox_named( + "sb-alpha", + "demo", + "alpha", + vec!["cmd".into(), "/c".into(), "exit 0".into()], + ), + fs_policy(&[&share]), + ); + backend + .create_sandbox(&sandbox) + .await + .expect("create accepted"); + + let status = backend + .stop_sandbox("sb-alpha", "some-other-name") + .await + .expect_err("a mismatched name must not stop the sandbox"); + assert_eq!(status.code(), tonic::Code::FailedPrecondition); + + let alpha = sandbox_by_id(&backend, "sb-alpha") + .await + .expect("sb-alpha should still be registered"); + assert_ne!( + ready_condition(&alpha).map(|condition| condition.reason), + Some("Stopped".into()), + "a rejected stop must not mutate the sandbox" + ); + } + + #[tokio::test] + async fn stop_aborts_when_the_sandbox_is_replaced_while_it_waits_for_the_gate() { + let tmp = tempfile::tempdir().unwrap(); + let share = tmp.path().to_string_lossy().replace('\\', "/"); + let backend = Arc::new(MxcComputeBackend::new_mocked(MxcComputeConfig::default())); + let sandbox = with_policy( + driver_sandbox_named( + "sb-reused", + "demo", + "alpha", + vec!["cmd".into(), "/c".into(), "exit 0".into()], + ), + fs_policy(&[&share]), + ); + backend + .create_sandbox(&sandbox) + .await + .expect("create accepted"); + + // Hold the original gate so the stop request parks after validating the + // id and name but before it touches the entry. + let original_gate = { + let registry = backend.registry.lock().await; + registry + .get("sb-reused") + .expect("original registered") + .lifecycle_gate + .clone() + }; + let held = original_gate.lock_owned().await; + + let stopper = tokio::spawn({ + let backend = backend.clone(); + async move { backend.stop_sandbox("sb-reused", "demo").await } + }); + tokio::time::sleep(Duration::from_millis(100)).await; + + // Stand in for a delete followed by a create that reuses the id. The + // registry is edited directly because the gateway mints a fresh UUID per + // create and cannot produce a reused id; this exercises the driver's own + // guard rather than a reachable gateway sequence. + { + let mut registry = backend.registry.lock().await; + let mut entry = registry.remove("sb-reused").expect("original registered"); + entry.lifecycle_gate = Arc::new(Mutex::new(())); + registry.insert("sb-reused".into(), entry); + } + drop(held); + + let status = stopper + .await + .expect("stop task joined") + .expect_err("stop must not act on a sandbox that replaced the requested one"); + assert_eq!(status.code(), tonic::Code::Aborted); + } + + #[tokio::test] + async fn stop_reports_not_found_for_an_unknown_sandbox_id() { + let backend = MxcComputeBackend::new_mocked(MxcComputeConfig::default()); + let status = backend + .stop_sandbox("sb-missing", "demo") + .await + .expect_err("an unknown id must not fall back to a name match"); + assert_eq!(status.code(), tonic::Code::NotFound); + } + #[tokio::test] async fn unmappable_network_policy_fails_create_lifecycle() { use openshell_core::proto::{NetworkEndpoint, NetworkPolicyRule}; diff --git a/crates/openshell-driver-mxc/src/grpc.rs b/crates/openshell-driver-mxc/src/grpc.rs index dfe8f66b29..b6de14ee9d 100644 --- a/crates/openshell-driver-mxc/src/grpc.rs +++ b/crates/openshell-driver-mxc/src/grpc.rs @@ -122,10 +122,15 @@ impl ComputeDriver for ComputeDriverService { request: Request, ) -> Result, Status> { let req = request.into_inner(); + if req.sandbox_id.is_empty() { + return Err(Status::invalid_argument("sandbox_id is required")); + } if req.sandbox_name.is_empty() { return Err(Status::invalid_argument("sandbox_name is required")); } - self.backend.stop_sandbox(&req.sandbox_name).await?; + self.backend + .stop_sandbox(&req.sandbox_id, &req.sandbox_name) + .await?; Ok(Response::new(StopSandboxResponse {})) } @@ -216,4 +221,32 @@ mod tests { .await .expect("MXC workspace deletion must remain idempotent"); } + + #[tokio::test] + async fn stop_sandbox_requires_both_identifiers() { + let service = + ComputeDriverService::new(MxcComputeBackend::new(MxcComputeConfig::default())); + + // sandbox_id is the authoritative identity. Without it the driver would + // have to resolve by name, which is only unique within a workspace. + let missing_id = service + .stop_sandbox(Request::new(StopSandboxRequest { + sandbox_name: "demo".into(), + ..Default::default() + })) + .await + .expect_err("a stop request without sandbox_id must be rejected"); + assert_eq!(missing_id.code(), tonic::Code::InvalidArgument); + assert!(missing_id.message().contains("sandbox_id is required")); + + let missing_name = service + .stop_sandbox(Request::new(StopSandboxRequest { + sandbox_id: "sb-alpha".into(), + ..Default::default() + })) + .await + .expect_err("a stop request without sandbox_name must be rejected"); + assert_eq!(missing_name.code(), tonic::Code::InvalidArgument); + assert!(missing_name.message().contains("sandbox_name is required")); + } }