From 6adcd30c9eb722fe6ab8853e0451e9e63db4206d Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Thu, 30 Jul 2026 18:03:39 -0700 Subject: [PATCH 1/4] fix(podman): combine sandbox stop and removal Signed-off-by: Piotr Mlocek --- crates/openshell-driver-podman/src/client.rs | 53 +++++++++++++++++--- crates/openshell-driver-podman/src/driver.rs | 35 ++++++++----- 2 files changed, 67 insertions(+), 21 deletions(-) diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 952938d47a..1fe7aef9f7 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -479,15 +479,28 @@ impl PodmanClient { } } - /// Force-remove a container and its anonymous volumes. - pub async fn remove_container(&self, name: &str) -> Result<(), PodmanApiError> { + /// Gracefully stop, then force-remove a container and its anonymous volumes. + 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 + let http_timeout = Duration::from_secs(u64::from(timeout_secs) + 5); + let (status, bytes) = self + .request( + hyper::Method::DELETE, + &format!("/libpod/containers/{name}?force=true&v=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. @@ -962,4 +975,28 @@ mod tests { ); let _ = std::fs::remove_file(socket_path); } + + #[tokio::test] + async fn remove_container_uses_atomic_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&v=true&timeout=10"] + ); + let _ = std::fs::remove_file(socket_path); + } } diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index aa3df9cbd7..0bb9f1a8bc 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -682,7 +682,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)); } @@ -748,13 +751,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)), @@ -1798,9 +1802,7 @@ mod tests { vec![ // list_containers by label StubResponse::new(StatusCode::OK, list_body), - // stop_container - StubResponse::new(StatusCode::NO_CONTENT, ""), - // remove_container + // atomic stop and remove_container StubResponse::new(StatusCode::NO_CONTENT, ""), // remove_volume StubResponse::new(StatusCode::NO_CONTENT, ""), @@ -1820,10 +1822,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&v=true&timeout=10" + )) + ) + ); + assert_eq!( + requests[2], format!( "DELETE {}", api_path(&format!("/libpod/volumes/{volume_name}")) From fce21cc36c81800f244046842dfdec2ed5a1cda1 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Thu, 30 Jul 2026 18:18:39 -0700 Subject: [PATCH 2/4] fix(podman): use libpod volume removal parameter Signed-off-by: Piotr Mlocek --- crates/openshell-driver-podman/src/client.rs | 13 +++++++++---- crates/openshell-driver-podman/src/driver.rs | 4 ++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 1fe7aef9f7..36422c9dc0 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -479,7 +479,10 @@ impl PodmanClient { } } - /// Gracefully stop, then force-remove a container and its anonymous volumes. + /// 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, @@ -490,7 +493,9 @@ impl PodmanClient { let (status, bytes) = self .request( hyper::Method::DELETE, - &format!("/libpod/containers/{name}?force=true&v=true&timeout={timeout_secs}"), + &format!( + "/libpod/containers/{name}?force=true&volumes=true&timeout={timeout_secs}" + ), None, http_timeout, ) @@ -977,7 +982,7 @@ mod tests { } #[tokio::test] - async fn remove_container_uses_atomic_timed_libpod_removal() { + 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, "")], @@ -995,7 +1000,7 @@ mod tests { .lock() .expect("request log lock should not be poisoned") .as_slice(), - ["DELETE /v5.0.0/libpod/containers/sandbox-123?force=true&v=true&timeout=10"] + ["DELETE /v5.0.0/libpod/containers/sandbox-123?force=true&volumes=true&timeout=10"] ); let _ = std::fs::remove_file(socket_path); } diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index 0bb9f1a8bc..79d89850de 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -1802,7 +1802,7 @@ mod tests { vec![ // list_containers by label StubResponse::new(StatusCode::OK, list_body), - // atomic stop and remove_container + // single timed remove_container operation StubResponse::new(StatusCode::NO_CONTENT, ""), // remove_volume StubResponse::new(StatusCode::NO_CONTENT, ""), @@ -1827,7 +1827,7 @@ mod tests { format!( "DELETE {}", api_path(&format!( - "/libpod/containers/{container_id}?force=true&v=true&timeout=10" + "/libpod/containers/{container_id}?force=true&volumes=true&timeout=10" )) ) ); From 96d020c75adb45092a63f2b059bb54b9d4631b67 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Fri, 31 Jul 2026 10:44:21 -0700 Subject: [PATCH 3/4] fix(podman): preserve removal timeout margin Signed-off-by: Piotr Mlocek --- crates/openshell-driver-podman/src/client.rs | 22 ++++++++++++++++++- .../openshell-driver-podman/src/test_utils.rs | 10 ++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 36422c9dc0..89cda11a81 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -489,7 +489,10 @@ impl PodmanClient { timeout_secs: u32, ) -> Result<(), PodmanApiError> { validate_name(name)?; - let http_timeout = Duration::from_secs(u64::from(timeout_secs) + 5); + // 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, @@ -1004,4 +1007,21 @@ mod tests { ); let _ = std::fs::remove_file(socket_path); } + + #[tokio::test] + 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()); + + client + .remove_container("sandbox-123", 0) + .await + .expect("container removal should retain the API timeout for cleanup"); + + handle.await.expect("stub task should finish"); + let _ = std::fs::remove_file(socket_path); + } } diff --git a/crates/openshell-driver-podman/src/test_utils.rs b/crates/openshell-driver-podman/src/test_utils.rs index 94794bc220..ec5c8f7f11 100644 --- a/crates/openshell-driver-podman/src/test_utils.rs +++ b/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) From 7cf58c002edf986d7d9ddd71618322a11c1a15e5 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Fri, 31 Jul 2026 12:07:54 -0700 Subject: [PATCH 4/4] test(podman): use virtual time for removal delay Signed-off-by: Piotr Mlocek --- crates/openshell-driver-podman/Cargo.toml | 1 + crates/openshell-driver-podman/src/client.rs | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/crates/openshell-driver-podman/Cargo.toml b/crates/openshell-driver-podman/Cargo.toml index ed798c0ab2..b8a3e06c04 100644 --- a/crates/openshell-driver-podman/Cargo.toml +++ b/crates/openshell-driver-podman/Cargo.toml @@ -38,6 +38,7 @@ miette = { workspace = true } [dev-dependencies] prost-types = { workspace = true } temp-env = "0.3" +tokio = { workspace = true, features = ["test-util"] } [lints] workspace = true diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 89cda11a81..6893cfb90d 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -1008,17 +1008,28 @@ mod tests { let _ = std::fs::remove_file(socket_path); } - #[tokio::test] + #[tokio::test(start_paused = true)] async fn remove_container_allows_cleanup_after_stop_timeout() { - let (socket_path, _request_log, handle) = spawn_podman_stub( + 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()); - client - .remove_container("sandbox-123", 0) + 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");