fix(podman): combine sandbox stop and removal (#2570)

* fix(podman): combine sandbox stop and removal

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>

* fix(podman): use libpod volume removal parameter

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>

* fix(podman): preserve removal timeout margin

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>

* test(podman): use virtual time for removal delay

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>

---------

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>

crates/openshell-driver-podman/Cargo.toml

@@ -39,6 +39,7 @@ url = { workspace = true }
 [dev-dependencies]
 prost-types = { workspace = true }
 temp-env = "0.3"
+tokio = { workspace = true, features = ["test-util"] }
 
 [lints]
 workspace = true

crates/openshell-driver-podman/src/client.rs

@@ -481,15 +481,36 @@ impl PodmanClient {
         }
     }
 
-    /// Force-remove a container and its anonymous volumes.
-    pub async fn remove_container(&self, name: &str) -> Result<(), PodmanApiError> {
+    /// Remove a container in one timed, forced Libpod delete operation.
+    ///
+    /// The Libpod endpoint uses `volumes` for anonymous-volume removal. Its
+    /// Docker-compatible counterpart uses the shorter `v` parameter.
+    pub async fn remove_container(
+        &self,
+        name: &str,
+        timeout_secs: u32,
+    ) -> Result<(), PodmanApiError> {
         validate_name(name)?;
-        self.request_ok(
-            hyper::Method::DELETE,
-            &format!("/libpod/containers/{name}?force=true&v=true"),
-            None,
-        )
-        .await
+        // The delete request covers both the graceful stop and the subsequent
+        // storage, network, and anonymous-volume cleanup. Preserve the normal
+        // API timeout as cleanup headroom after the stop grace period.
+        let http_timeout = Duration::from_secs(u64::from(timeout_secs)) + API_TIMEOUT;
+        let (status, bytes) = self
+            .request(
+                hyper::Method::DELETE,
+                &format!(
+                    "/libpod/containers/{name}?force=true&volumes=true&timeout={timeout_secs}"
+                ),
+                None,
+                http_timeout,
+            )
+            .await?;
+        let code = status.as_u16();
+        if status.is_success() || code == 304 {
+            Ok(())
+        } else {
+            Err(error_from_response(code, &bytes))
+        }
     }
 
     /// Inspect a container by name or ID.
@@ -982,4 +1003,56 @@ mod tests {
         );
         let _ = std::fs::remove_file(socket_path);
     }
+
+    #[tokio::test]
+    async fn remove_container_uses_single_timed_libpod_removal() {
+        let (socket_path, request_log, handle) = spawn_podman_stub(
+            "remove-container",
+            vec![StubResponse::new(StatusCode::NO_CONTENT, "")],
+        );
+        let client = PodmanClient::new(socket_path.clone());
+
+        client
+            .remove_container("sandbox-123", 10)
+            .await
+            .expect("container removal should succeed");
+
+        handle.await.expect("stub task should finish");
+        assert_eq!(
+            request_log
+                .lock()
+                .expect("request log lock should not be poisoned")
+                .as_slice(),
+            ["DELETE /v5.0.0/libpod/containers/sandbox-123?force=true&volumes=true&timeout=10"]
+        );
+        let _ = std::fs::remove_file(socket_path);
+    }
+
+    #[tokio::test(start_paused = true)]
+    async fn remove_container_allows_cleanup_after_stop_timeout() {
+        let (socket_path, request_log, handle) = spawn_podman_stub(
+            "remove-container-delayed",
+            vec![StubResponse::new(StatusCode::NO_CONTENT, "").with_delay(Duration::from_secs(6))],
+        );
+        let client = PodmanClient::new(socket_path.clone());
+
+        let removal = tokio::spawn(async move { client.remove_container("sandbox-123", 0).await });
+        while request_log
+            .lock()
+            .expect("request log lock should not be poisoned")
+            .is_empty()
+        {
+            tokio::task::yield_now().await;
+        }
+        tokio::task::yield_now().await;
+        tokio::time::advance(Duration::from_secs(6)).await;
+
+        removal
+            .await
+            .expect("removal task should finish")
+            .expect("container removal should retain the API timeout for cleanup");
+
+        handle.await.expect("stub task should finish");
+        let _ = std::fs::remove_file(socket_path);
+    }
 }

crates/openshell-driver-podman/src/driver.rs

@@ -793,7 +793,10 @@ impl PodmanComputeDriver {
                 error = %e,
                 "Failed to start container; cleaning up"
             );
-            let _ = self.client.remove_container(&name).await;
+            let _ = self
+                .client
+                .remove_container(&name, self.config.stop_timeout_secs)
+                .await;
             cleanup_created().await;
             return Err(ComputeDriverError::from(e));
         }
@@ -859,13 +862,14 @@ impl PodmanComputeDriver {
         };
         info!(sandbox_id = %sandbox_id, container = %container_id, "Deleting sandbox container");
 
-        // Stop (best-effort).
-        let _ = self
+        // Keep stop, timeout, and removal in one Podman operation. Splitting
+        // stop and remove can race with another container starting an image
+        // mount when the stop reaches its timeout.
+        let container_existed = match self
             .client
-            .stop_container(&container_id, self.config.stop_timeout_secs)
-            .await;
-
-        let container_existed = match self.client.remove_container(&container_id).await {
+            .remove_container(&container_id, self.config.stop_timeout_secs)
+            .await
+        {
             Ok(()) => true,
             Err(PodmanApiError::NotFound(_)) => false,
             Err(e) => return Err(ComputeDriverError::from(e)),
@@ -2202,9 +2206,7 @@ mod tests {
             vec![
                 // list_containers by label
                 StubResponse::new(StatusCode::OK, list_body),
-                // stop_container
-                StubResponse::new(StatusCode::NO_CONTENT, ""),
-                // remove_container
+                // single timed remove_container operation
                 StubResponse::new(StatusCode::NO_CONTENT, ""),
                 // remove_volume
                 StubResponse::new(StatusCode::NO_CONTENT, ""),
@@ -2224,10 +2226,17 @@ mod tests {
             .expect("request log lock should not be poisoned")
             .clone();
         assert!(requests[0].contains("/libpod/containers/json"));
-        assert!(requests[1].contains(&format!("/libpod/containers/{container_id}/stop")));
-        assert!(requests[2].contains(&format!("/libpod/containers/{container_id}")));
         assert_eq!(
-            requests[3],
+            requests[1],
+            format!(
+                "DELETE {}",
+                api_path(&format!(
+                    "/libpod/containers/{container_id}?force=true&volumes=true&timeout=10"
+                ))
+            )
+        );
+        assert_eq!(
+            requests[2],
             format!(
                 "DELETE {}",
                 api_path(&format!("/libpod/volumes/{volume_name}"))

crates/openshell-driver-podman/src/test_utils.rs

@@ -13,7 +13,7 @@ use std::collections::VecDeque;
 use std::convert::Infallible;
 use std::path::PathBuf;
 use std::sync::{Arc, Mutex};
-use std::time::{SystemTime, UNIX_EPOCH};
+use std::time::{Duration, SystemTime, UNIX_EPOCH};
 use tokio::net::UnixListener;
 
 /// A canned HTTP response for the Podman stub server.
@@ -21,6 +21,7 @@ use tokio::net::UnixListener;
 pub struct StubResponse {
     pub status: StatusCode,
     pub body: String,
+    pub delay: Duration,
 }
 
 impl StubResponse {
@@ -28,8 +29,14 @@ impl StubResponse {
         Self {
             status,
             body: body.into(),
+            delay: Duration::ZERO,
         }
     }
+
+    pub fn with_delay(mut self, delay: Duration) -> Self {
+        self.delay = delay;
+        self
+    }
 }
 
 /// Generate a unique Unix socket path for a test.
@@ -97,6 +104,7 @@ pub fn spawn_podman_stub(
                                 .expect("response queue lock should not be poisoned")
                                 .pop_front()
                                 .expect("stub response should exist");
+                            tokio::time::sleep(response.delay).await;
                             Ok::<_, Infallible>(
                                 hyper::Response::builder()
                                     .status(response.status)