fix(driver-mxc): resolve StopSandbox by sandbox_id

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<String>,
+    ) -> 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<DriverSandbox> {
+        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<F>(
+        backend: &MxcComputeBackend,
+        id: &str,
+        mut pred: F,
+    ) -> Option<DriverSandbox>
+    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};

crates/openshell-driver-mxc/src/grpc.rs

@@ -122,10 +122,15 @@ impl ComputeDriver for ComputeDriverService {
         request: Request<StopSandboxRequest>,
     ) -> Result<Response<StopSandboxResponse>, 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"));
+    }
 }