From 2d59d8653822555e65448ab7f74ffc17c268c162 Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Mon, 13 Jul 2026 16:12:25 -0400 Subject: [PATCH 1/4] fix(core): fall back to podman CLI when no API socket responds Auto-detection only checked well-known Podman socket paths, so a Podman machine exposing its API socket at a non-standard location went undetected. The symlink at a well-known path is not always present; it varies by Podman version, machine provider, and platform. Extend detect_podman_socket() to fall back to podman CLI discovery when no well-known candidate responds: podman info --format json determines whether the service is local or remote, and podman machine inspect resolves the host-side forwarded socket for VM-backed machines. All existing callers (driver auto-detection, the Podman driver, and the VM driver's container-engine fallback) pick this up without change. Select the machine backing the active Podman connection instead of the first entry from podman machine inspect, honoring podman's connection precedence: CONTAINER_CONNECTION, then CONTAINER_HOST (mapped to a connection by URI), then the containers.conf default. An explicit endpoint that maps to no known machine is left unresolved rather than guessing an unrelated machine. When CONTAINER_HOST is an explicit unix:// socket, use that path directly since podman info connects through it. Update the gateway config reference and the Podman driver README, which described probe-only detection. Signed-off-by: Russell Bryant --- crates/openshell-core/src/config.rs | 561 ++++++++++++++++++++++- crates/openshell-driver-podman/README.md | 2 +- docs/reference/gateway-config.mdx | 5 +- 3 files changed, 559 insertions(+), 9 deletions(-) diff --git a/crates/openshell-core/src/config.rs b/crates/openshell-core/src/config.rs index 62411e20b5..dec1722b4b 100644 --- a/crates/openshell-core/src/config.rs +++ b/crates/openshell-core/src/config.rs @@ -13,6 +13,7 @@ use std::net::SocketAddr; #[cfg(unix)] use std::os::unix::fs::FileTypeExt; use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; use std::str::FromStr; use std::time::Duration; @@ -214,9 +215,16 @@ pub fn is_podman_available() -> bool { detect_podman_socket().is_some() } -/// Return the first responsive Podman API socket, or `None` if none respond. +/// Return the Podman API socket, or `None` if Podman is not available. +/// +/// Probes the well-known socket candidates first, then falls back to asking +/// the Podman CLI where its socket lives. The symlink at a well-known path is +/// not always present — it varies by Podman version, machine provider, and +/// platform — so the CLI fallback is what makes detection work on hosts where +/// Podman is functional but the socket is somewhere else. pub fn detect_podman_socket() -> Option { detect_podman_socket_from_candidates(&podman_socket_candidates()) + .or_else(discover_podman_socket) } fn detect_podman_socket_from_candidates(candidates: &[PathBuf]) -> Option { @@ -226,6 +234,228 @@ fn detect_podman_socket_from_candidates(candidates: &[PathBuf]) -> Option Option { + let output = Command::new("podman") + .args(["info", "--format", "json"]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .output() + .ok() + .filter(|o| o.status.success())?; + + // podman info succeeded, so an explicit unix:// CONTAINER_HOST is the exact + // working host-side socket. This must be checked before the machine path, + // which cannot map a raw unix:// endpoint to a machine. + if let Some(path) = explicit_unix_container_host() { + return Some(path); + } + + let info: serde_json::Value = serde_json::from_slice(&output.stdout).ok()?; + let is_remote = info["host"]["serviceIsRemote"].as_bool().unwrap_or(false); + + if is_remote { + discover_podman_machine_socket() + } else { + parse_podman_info_socket(&info) + } +} + +/// Return the socket path when `CONTAINER_HOST` is an explicit `unix://` URL. +/// +/// Honors Podman's precedence: `CONTAINER_CONNECTION` outranks `CONTAINER_HOST`, +/// so a set `CONTAINER_CONNECTION` means `podman info` did not use +/// `CONTAINER_HOST` and this returns `None`. +fn explicit_unix_container_host() -> Option { + if env_var_nonempty("CONTAINER_CONNECTION").is_some() { + return None; + } + let host = env_var_nonempty("CONTAINER_HOST")?; + unix_url_socket_path(&host) +} + +/// Parse the socket path from a `unix://` URL, or `None` for other schemes. +fn unix_url_socket_path(url: &str) -> Option { + let path = url.trim().strip_prefix("unix://")?; + (!path.is_empty()).then(|| PathBuf::from(path)) +} + +/// Extract the socket path from `podman info` JSON output. +/// Used on native Linux where `remoteSocket.path` is the real local socket. +fn parse_podman_info_socket(info: &serde_json::Value) -> Option { + let path_str = info["host"]["remoteSocket"]["path"].as_str()?; + let path = path_str.strip_prefix("unix://").unwrap_or(path_str); + if path.is_empty() { + return None; + } + Some(PathBuf::from(path)) +} + +/// Which Podman machine `podman info` connected through. +/// +/// Podman resolves its endpoint (highest precedence first) from +/// `CONTAINER_CONNECTION` (a named connection), then `CONTAINER_HOST` (a URL), +/// then the default connection in `containers.conf`. +#[derive(Debug, PartialEq, Eq)] +enum ActiveMachine { + /// An explicit selector (`CONTAINER_CONNECTION`, or `CONTAINER_HOST` mapped + /// to a connection by URL) named this connection. It must match a machine + /// exactly; guessing another machine would connect to the wrong socket. + Explicit(String), + /// An explicit `CONTAINER_HOST` is set but maps to no known connection + /// (e.g. a plain remote server, not a local machine). The active machine + /// cannot be determined and must not be guessed. + UnresolvedExplicit, + /// No explicit selector; the `containers.conf` default connection name, if + /// any. Falling back to the sole/first machine is acceptable here. + Default(Option), +} + +/// Run `podman machine inspect` to discover the host-side forwarded socket. +/// Used on macOS/Windows where the Podman service runs inside a VM. +/// +/// `podman machine inspect` lists every machine, so the entry backing the +/// active Podman connection is selected rather than blindly taking the first +/// one — otherwise a host with multiple machines could be pointed at the wrong +/// machine's socket. +fn discover_podman_machine_socket() -> Option { + let output = Command::new("podman") + .args(["machine", "inspect"]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .output() + .ok() + .filter(|o| o.status.success())?; + + let machines: serde_json::Value = serde_json::from_slice(&output.stdout).ok()?; + parse_podman_machine_inspect(&machines, &active_podman_machine()) +} + +/// Determine which machine `podman info` connected through. +fn active_podman_machine() -> ActiveMachine { + if let Some(name) = env_var_nonempty("CONTAINER_CONNECTION") { + return ActiveMachine::Explicit(name); + } + let container_host = env_var_nonempty("CONTAINER_HOST"); + resolve_active_podman_machine(container_host.as_deref(), podman_connection_list().as_ref()) +} + +/// Resolve the active machine from `CONTAINER_HOST` and the connection list. +/// +/// `CONTAINER_CONNECTION` is handled by the caller (it needs no connection +/// list). This is split out as a pure function for testing. +fn resolve_active_podman_machine( + container_host: Option<&str>, + connections: Option<&serde_json::Value>, +) -> ActiveMachine { + if let Some(host) = container_host { + return connections + .and_then(|c| podman_connection_name_for_uri(c, host)) + .map_or(ActiveMachine::UnresolvedExplicit, ActiveMachine::Explicit); + } + ActiveMachine::Default(connections.and_then(parse_default_podman_connection)) +} + +fn env_var_nonempty(key: &str) -> Option { + std::env::var(key) + .ok() + .filter(|value| !value.trim().is_empty()) +} + +/// Run `podman system connection list --format json`. +fn podman_connection_list() -> Option { + let output = Command::new("podman") + .args(["system", "connection", "list", "--format", "json"]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .output() + .ok() + .filter(|o| o.status.success())?; + serde_json::from_slice(&output.stdout).ok() +} + +/// Extract the default machine connection name from +/// `podman system connection list --format json`. +fn parse_default_podman_connection(connections: &serde_json::Value) -> Option { + connections + .as_array()? + .iter() + .find(|c| { + c["Default"].as_bool().unwrap_or(false) && c["IsMachine"].as_bool().unwrap_or(false) + }) + .and_then(|c| c["Name"].as_str()) + .map(str::to_string) +} + +/// Find the machine connection whose URI matches `CONTAINER_HOST`. +/// +/// Only machine connections (`IsMachine: true`) map to a local socket, so a +/// `CONTAINER_HOST` pointing at a plain remote server yields `None`. +fn podman_connection_name_for_uri(connections: &serde_json::Value, uri: &str) -> Option { + connections + .as_array()? + .iter() + .find(|c| c["IsMachine"].as_bool().unwrap_or(false) && c["URI"].as_str() == Some(uri)) + .and_then(|c| c["Name"].as_str()) + .map(str::to_string) +} + +/// Extract the host-side socket path from `podman machine inspect` JSON, +/// selecting the machine backing the active connection. +fn parse_podman_machine_inspect( + machines: &serde_json::Value, + active: &ActiveMachine, +) -> Option { + let machine = select_podman_machine(machines.as_array()?, active)?; + let path_str = machine["ConnectionInfo"]["PodmanSocket"]["Path"].as_str()?; + if path_str.is_empty() { + return None; + } + Some(PathBuf::from(path_str)) +} + +/// Select the machine entry backing the active Podman connection. +/// +/// An explicit selection must match a machine by name (a rootless connection +/// shares its machine's name; a rootful connection is named `-root`, +/// so the `-root`-stripped name is also tried). An explicit endpoint that +/// matches no machine returns `None` rather than guessing an unrelated machine. +/// Only the non-explicit default path falls back to the first entry, which is +/// correct on the common single-machine host. +fn select_podman_machine<'a>( + machines: &'a [serde_json::Value], + active: &ActiveMachine, +) -> Option<&'a serde_json::Value> { + let matches_name = |name: &str| machines.iter().find(|m| m["Name"].as_str() == Some(name)); + let match_connection = |name: &str| { + matches_name(name).or_else(|| name.strip_suffix("-root").and_then(matches_name)) + }; + + match active { + ActiveMachine::Explicit(name) => match_connection(name), + ActiveMachine::UnresolvedExplicit => None, + ActiveMachine::Default(name) => name + .as_deref() + .and_then(match_connection) + .or_else(|| machines.first()), + } +} + fn podman_socket_candidates() -> Vec { let socket = std::env::var("OPENSHELL_PODMAN_SOCKET") .ok() @@ -1088,12 +1318,15 @@ const fn default_ssh_session_ttl_secs() -> u64 { #[cfg(test)] mod tests { use super::{ - ComputeDriverKind, Config, DEFAULT_SERVICE_ROUTING_DOMAIN, GatewayInterceptorBindingPolicy, - GatewayInterceptorConfig, GatewayInterceptorFailurePolicy, GatewayJwtConfig, - GatewayProviderProfileSourceConfig, PolicyValidationFailureMode, + ActiveMachine, ComputeDriverKind, Config, DEFAULT_SERVICE_ROUTING_DOMAIN, + GatewayInterceptorBindingPolicy, GatewayInterceptorConfig, GatewayInterceptorFailurePolicy, + GatewayJwtConfig, GatewayProviderProfileSourceConfig, PolicyValidationFailureMode, detect_docker_socket_from_candidates, detect_driver, detect_podman_socket_from_candidates, - docker_host_unix_socket_path, docker_socket_responds, normalize_compute_driver_name, - podman_socket_candidates_from_env, podman_socket_responds, + docker_host_unix_socket_path, docker_socket_responds, explicit_unix_container_host, + normalize_compute_driver_name, parse_default_podman_connection, parse_podman_info_socket, + parse_podman_machine_inspect, podman_connection_name_for_uri, + podman_socket_candidates_from_env, podman_socket_responds, resolve_active_podman_machine, + unix_url_socket_path, }; #[cfg(unix)] use super::{is_reachable_unix_socket, is_unix_socket}; @@ -1595,6 +1828,23 @@ mod tests { ); } + #[test] + fn parse_podman_info_socket_extracts_linux_local_socket() { + let info: serde_json::Value = serde_json::json!({ + "host": { + "serviceIsRemote": false, + "remoteSocket": { + "path": "unix:///run/user/1000/podman/podman.sock", + "exists": true + } + } + }); + assert_eq!( + parse_podman_info_socket(&info), + Some(PathBuf::from("/run/user/1000/podman/podman.sock")) + ); + } + #[test] fn supervisor_image_tag_sanitizes_build_metadata_for_oci() { use super::resolve_supervisor_image_tag; @@ -1608,6 +1858,22 @@ mod tests { ); } + #[test] + fn parse_podman_info_socket_handles_path_without_unix_prefix() { + let info: serde_json::Value = serde_json::json!({ + "host": { + "remoteSocket": { + "path": "/run/user/1000/podman/podman.sock", + "exists": true + } + } + }); + assert_eq!( + parse_podman_info_socket(&info), + Some(PathBuf::from("/run/user/1000/podman/podman.sock")) + ); + } + #[test] fn default_supervisor_image_is_version_pinned() { use super::default_supervisor_image; @@ -1616,4 +1882,287 @@ mod tests { let tag = image.rsplit_once(':').unwrap().1; assert!(!tag.is_empty()); } + + #[test] + fn parse_podman_info_socket_returns_none_for_missing_path() { + let info: serde_json::Value = serde_json::json!({ + "host": { + "remoteSocket": {} + } + }); + assert_eq!(parse_podman_info_socket(&info), None); + } + + #[test] + fn parse_podman_info_socket_returns_none_for_empty_path() { + let info: serde_json::Value = serde_json::json!({ + "host": { + "remoteSocket": { + "path": "", + "exists": false + } + } + }); + assert_eq!(parse_podman_info_socket(&info), None); + } + + #[test] + fn parse_podman_machine_inspect_extracts_macos_socket() { + let machines: serde_json::Value = serde_json::json!([ + { + "ConnectionInfo": { + "PodmanSocket": { + "Path": "/var/folders/1q/jx7s14b928n8zvstgfk98lj00000gn/T/podman/podman-machine-default-api.sock" + }, + "PodmanPipe": null + }, + "Name": "podman-machine-default" + } + ]); + assert_eq!( + parse_podman_machine_inspect(&machines, &ActiveMachine::Default(None)), + Some(PathBuf::from( + "/var/folders/1q/jx7s14b928n8zvstgfk98lj00000gn/T/podman/podman-machine-default-api.sock" + )) + ); + } + + #[test] + fn parse_podman_machine_inspect_returns_none_for_empty_array() { + let machines: serde_json::Value = serde_json::json!([]); + assert_eq!( + parse_podman_machine_inspect(&machines, &ActiveMachine::Default(None)), + None + ); + } + + #[test] + fn parse_podman_machine_inspect_returns_none_for_missing_socket() { + let machines: serde_json::Value = serde_json::json!([ + { + "ConnectionInfo": {}, + "Name": "podman-machine-default" + } + ]); + assert_eq!( + parse_podman_machine_inspect(&machines, &ActiveMachine::Default(None)), + None + ); + } + + fn two_machine_inspect() -> serde_json::Value { + serde_json::json!([ + { + "Name": "podman-machine-default", + "ConnectionInfo": { + "PodmanSocket": { "Path": "/tmp/podman/default-api.sock" } + } + }, + { + "Name": "work", + "ConnectionInfo": { + "PodmanSocket": { "Path": "/tmp/podman/work-api.sock" } + } + } + ]) + } + + #[test] + fn parse_podman_machine_inspect_selects_active_machine() { + let machines = two_machine_inspect(); + // The active connection points at the second machine, not the first. + assert_eq!( + parse_podman_machine_inspect(&machines, &ActiveMachine::Explicit("work".to_string())), + Some(PathBuf::from("/tmp/podman/work-api.sock")) + ); + } + + #[test] + fn parse_podman_machine_inspect_matches_rootful_connection_to_machine() { + let machines = two_machine_inspect(); + // Rootful connections are named `-root`. + assert_eq!( + parse_podman_machine_inspect( + &machines, + &ActiveMachine::Explicit("work-root".to_string()) + ), + Some(PathBuf::from("/tmp/podman/work-api.sock")) + ); + } + + #[test] + fn parse_podman_machine_inspect_default_falls_back_to_first() { + let machines = two_machine_inspect(); + // No selector: first entry is used (correct on single-machine hosts). + assert_eq!( + parse_podman_machine_inspect(&machines, &ActiveMachine::Default(None)), + Some(PathBuf::from("/tmp/podman/default-api.sock")) + ); + // A default connection naming an absent machine also falls back. + assert_eq!( + parse_podman_machine_inspect( + &machines, + &ActiveMachine::Default(Some("missing".to_string())) + ), + Some(PathBuf::from("/tmp/podman/default-api.sock")) + ); + } + + #[test] + fn parse_podman_machine_inspect_does_not_guess_for_unmatched_explicit() { + let machines = two_machine_inspect(); + // An explicit selection that matches no machine must NOT fall back to + // an unrelated machine — that would reintroduce the wrong-machine bug. + assert_eq!( + parse_podman_machine_inspect(&machines, &ActiveMachine::Explicit("other".to_string())), + None + ); + // CONTAINER_HOST pointing at a non-machine endpoint likewise does not + // guess. + assert_eq!( + parse_podman_machine_inspect(&machines, &ActiveMachine::UnresolvedExplicit), + None + ); + } + + #[test] + fn resolve_active_podman_machine_maps_container_host_to_connection() { + let connections: serde_json::Value = serde_json::json!([ + { "Name": "work", "IsMachine": true, "Default": false, + "URI": "ssh://core@127.0.0.1:5555/run/user/1000/podman/podman.sock" }, + { "Name": "podman-machine-default", "IsMachine": true, "Default": true, + "URI": "ssh://core@127.0.0.1:4444/run/user/1000/podman/podman.sock" } + ]); + // CONTAINER_HOST pointing at the non-default machine's URI resolves to + // that machine, not the default. + assert_eq!( + resolve_active_podman_machine( + Some("ssh://core@127.0.0.1:5555/run/user/1000/podman/podman.sock"), + Some(&connections) + ), + ActiveMachine::Explicit("work".to_string()) + ); + } + + #[test] + fn resolve_active_podman_machine_unmatched_host_is_unresolved() { + let connections: serde_json::Value = serde_json::json!([ + { "Name": "podman-machine-default", "IsMachine": true, "Default": true, + "URI": "ssh://core@127.0.0.1:4444/run/user/1000/podman/podman.sock" } + ]); + // A CONTAINER_HOST that matches no machine connection is unresolved, + // never silently mapped to the default machine. + assert_eq!( + resolve_active_podman_machine(Some("tcp://192.0.2.10:2375"), Some(&connections)), + ActiveMachine::UnresolvedExplicit + ); + } + + #[test] + fn resolve_active_podman_machine_defaults_without_host() { + let connections: serde_json::Value = serde_json::json!([ + { "Name": "podman-machine-default", "IsMachine": true, "Default": true, + "URI": "ssh://core@127.0.0.1:4444/run/user/1000/podman/podman.sock" } + ]); + assert_eq!( + resolve_active_podman_machine(None, Some(&connections)), + ActiveMachine::Default(Some("podman-machine-default".to_string())) + ); + } + + #[test] + fn podman_connection_name_for_uri_ignores_non_machine_matches() { + let connections: serde_json::Value = serde_json::json!([ + { "Name": "remote", "IsMachine": false, "Default": false, + "URI": "tcp://192.0.2.10:2375" } + ]); + // A URI match against a non-machine connection does not map to a local + // machine socket. + assert_eq!( + podman_connection_name_for_uri(&connections, "tcp://192.0.2.10:2375"), + None + ); + } + + #[test] + fn unix_url_socket_path_parses_unix_urls() { + assert_eq!( + unix_url_socket_path("unix:///run/user/1000/podman/podman.sock"), + Some(PathBuf::from("/run/user/1000/podman/podman.sock")) + ); + // Non-unix schemes and empty paths are not sockets. + assert_eq!(unix_url_socket_path("ssh://core@127.0.0.1:22/x"), None); + assert_eq!(unix_url_socket_path("tcp://127.0.0.1:2375"), None); + assert_eq!(unix_url_socket_path("unix://"), None); + } + + #[test] + #[allow(unsafe_code)] // std::env::set_var/remove_var require unsafe in Rust 2024 + fn explicit_unix_container_host_honors_scheme_and_precedence() { + fn set(key: &str, value: Option<&str>) { + unsafe { + match value { + Some(v) => std::env::set_var(key, v), + None => std::env::remove_var(key), + } + } + } + + let original_host = std::env::var("CONTAINER_HOST").ok(); + let original_connection = std::env::var("CONTAINER_CONNECTION").ok(); + + // A unix:// CONTAINER_HOST with no CONTAINER_CONNECTION is used directly. + set("CONTAINER_CONNECTION", None); + set("CONTAINER_HOST", Some("unix:///tmp/podman/custom.sock")); + assert_eq!( + explicit_unix_container_host(), + Some(PathBuf::from("/tmp/podman/custom.sock")) + ); + + // CONTAINER_CONNECTION outranks CONTAINER_HOST. + set("CONTAINER_CONNECTION", Some("work")); + assert_eq!(explicit_unix_container_host(), None); + + // A non-unix CONTAINER_HOST is not a direct socket. + set("CONTAINER_CONNECTION", None); + set( + "CONTAINER_HOST", + Some("ssh://core@127.0.0.1:5555/run/podman.sock"), + ); + assert_eq!(explicit_unix_container_host(), None); + + // Nothing set. + set("CONTAINER_HOST", None); + assert_eq!(explicit_unix_container_host(), None); + + set("CONTAINER_HOST", original_host.as_deref()); + set("CONTAINER_CONNECTION", original_connection.as_deref()); + } + + #[test] + fn parse_default_podman_connection_picks_default_machine() { + let connections: serde_json::Value = serde_json::json!([ + { "Name": "podman-machine-default", "IsMachine": true, "Default": true }, + { "Name": "podman-machine-default-root", "IsMachine": true, "Default": false } + ]); + assert_eq!( + parse_default_podman_connection(&connections), + Some("podman-machine-default".to_string()) + ); + } + + #[test] + fn parse_default_podman_connection_ignores_non_machine_and_missing_default() { + // Default connection that is not a machine is ignored. + let non_machine: serde_json::Value = serde_json::json!([ + { "Name": "remote-host", "IsMachine": false, "Default": true } + ]); + assert_eq!(parse_default_podman_connection(&non_machine), None); + + // No default at all. + let no_default: serde_json::Value = serde_json::json!([ + { "Name": "work", "IsMachine": true, "Default": false } + ]); + assert_eq!(parse_default_podman_connection(&no_default), None); + } } diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index 8639d53c24..7540bd75e7 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -374,7 +374,7 @@ Podman resources after out-of-band container removal or label drift. | Environment Variable | CLI Flag | Default | Description | |---|---|---|---| -| `OPENSHELL_PODMAN_SOCKET` | `--podman-socket` | Probes known local Podman API sockets and uses the first responsive socket. Fails to start if none respond. | Podman API Unix socket path. | +| `OPENSHELL_PODMAN_SOCKET` | `--podman-socket` | Probes known local Podman API sockets and uses the first responsive socket, then falls back to asking the `podman` CLI for the host-side socket. Fails to start if neither finds one. | Podman API Unix socket path. | | `OPENSHELL_SANDBOX_IMAGE` | `--sandbox-image` | From gateway config | Default OCI image for sandboxes. | | `OPENSHELL_SANDBOX_IMAGE_PULL_POLICY` | `--sandbox-image-pull-policy` | `missing` | Pull policy: `always`, `missing`, `never`, or `newer`. | | `OPENSHELL_GRPC_ENDPOINT` | `--grpc-endpoint` | Auto-detected via `host.containers.internal` | Gateway gRPC endpoint for sandbox callbacks. | diff --git a/docs/reference/gateway-config.mdx b/docs/reference/gateway-config.mdx index 59fc35c485..8eaae5f69e 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -616,8 +616,9 @@ compute_drivers = ["podman"] [openshell.drivers.podman] # Rootless socket path. For root Podman use /run/podman/podman.sock. -# Omit to auto-detect: the driver probes for a responsive Podman socket and -# fails to start if none respond. +# Omit to auto-detect: the driver probes for a responsive Podman socket, then +# asks the podman CLI where its socket is, and fails to start if neither finds +# one. Set this to pin a specific Podman machine instead. socket_path = "/run/user/1000/podman/podman.sock" default_image = "ghcr.io/nvidia/openshell/sandbox:latest" image_pull_policy = "missing" # always | missing | never | newer From 3d178245d3f8a5a6142c651b5311229f4436d067 Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Mon, 24 Aug 2026 09:48:53 -0400 Subject: [PATCH 2/4] fix(core): inspect podman machine by name and bound discovery probes Address two startup defects in Podman socket auto-detection: - Remote discovery ran `podman machine inspect` with no arguments, which inspects only `podman-machine-default`. On a host whose default connection is a different machine, selection fell back to the first (wrong) entry, so the gateway could operate against the wrong backend. Resolve the active machine first and inspect it by name, trying the `-root`-stripped machine for rootful connections and returning None when no machine can be mapped instead of substituting another. - `podman info`, `podman machine inspect`, and `podman system connection list` used unbounded `Command::output()`, so a stalled machine, SSH connection, or helper could hang gateway startup indefinitely. Route all three through a bounded runner that kills and reaps the child on a documented deadline and returns None so detection can continue. Signed-off-by: Russell Bryant --- crates/openshell-core/src/config.rs | 328 +++++++++++++++++----------- 1 file changed, 198 insertions(+), 130 deletions(-) diff --git a/crates/openshell-core/src/config.rs b/crates/openshell-core/src/config.rs index dec1722b4b..586c497146 100644 --- a/crates/openshell-core/src/config.rs +++ b/crates/openshell-core/src/config.rs @@ -15,7 +15,7 @@ use std::os::unix::fs::FileTypeExt; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::str::FromStr; -use std::time::Duration; +use std::time::{Duration, Instant}; // ── Public default constants ──────────────────────────────────────────── // @@ -234,6 +234,77 @@ fn detect_podman_socket_from_candidates(candidates: &[PathBuf]) -> Option` with a bounded deadline, returning captured stdout on a +/// successful exit. Returns `None` on spawn failure, non-zero exit, or timeout. +fn run_podman_capture(args: &[&str]) -> Option> { + run_bounded_command("podman", args, PODMAN_DISCOVERY_TIMEOUT) +} + +/// Run `program ` with a deadline, capturing stdout. +/// +/// Unlike `Command::output()`, which blocks until the child exits, this kills +/// and reaps the child once `timeout` elapses so a hung probe cannot stall the +/// caller. Returns the captured stdout only on a successful exit within the +/// deadline; otherwise `None`. +fn run_bounded_command(program: &str, args: &[&str], timeout: Duration) -> Option> { + use std::io::Read as _; + + let mut child = Command::new(program) + .args(args) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .ok()?; + + // Drain stdout on a separate thread so a child that fills the pipe buffer + // cannot deadlock against the polling loop below. + let mut stdout = child.stdout.take()?; + let reader = std::thread::spawn(move || { + let mut buf = Vec::new(); + let _ = stdout.read_to_end(&mut buf); + buf + }); + + let start = Instant::now(); + let status = loop { + match child.try_wait() { + Ok(Some(status)) => break status, + Ok(None) => { + if start.elapsed() >= timeout { + let _ = child.kill(); + let _ = child.wait(); + let _ = reader.join(); + return None; + } + std::thread::sleep(PODMAN_DISCOVERY_POLL_INTERVAL); + } + Err(_) => { + let _ = child.kill(); + let _ = child.wait(); + let _ = reader.join(); + return None; + } + } + }; + + let stdout = reader.join().ok()?; + status.success().then_some(stdout) +} + /// Query the Podman CLI to discover the host-side API socket path. /// /// Strategy: @@ -248,14 +319,7 @@ fn detect_podman_socket_from_candidates(candidates: &[PathBuf]) -> Option Option { - let output = Command::new("podman") - .args(["info", "--format", "json"]) - .stdin(Stdio::null()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .output() - .ok() - .filter(|o| o.status.success())?; + let stdout = run_podman_capture(&["info", "--format", "json"])?; // podman info succeeded, so an explicit unix:// CONTAINER_HOST is the exact // working host-side socket. This must be checked before the machine path, @@ -264,7 +328,7 @@ fn discover_podman_socket() -> Option { return Some(path); } - let info: serde_json::Value = serde_json::from_slice(&output.stdout).ok()?; + let info: serde_json::Value = serde_json::from_slice(&stdout).ok()?; let is_remote = info["host"]["serviceIsRemote"].as_bool().unwrap_or(false); if is_remote { @@ -320,29 +384,52 @@ enum ActiveMachine { /// cannot be determined and must not be guessed. UnresolvedExplicit, /// No explicit selector; the `containers.conf` default connection name, if - /// any. Falling back to the sole/first machine is acceptable here. + /// any. When absent, Podman's built-in default machine is inspected by name. Default(Option), } -/// Run `podman machine inspect` to discover the host-side forwarded socket. -/// Used on macOS/Windows where the Podman service runs inside a VM. +/// Run `podman machine inspect ` to discover the host-side forwarded +/// socket. Used on macOS/Windows where the Podman service runs inside a VM. /// -/// `podman machine inspect` lists every machine, so the entry backing the -/// active Podman connection is selected rather than blindly taking the first -/// one — otherwise a host with multiple machines could be pointed at the wrong -/// machine's socket. +/// The active machine is resolved first and inspected *by name*: a no-argument +/// `podman machine inspect` inspects only `podman-machine-default`, so a host +/// whose default connection is a different machine would otherwise be pointed +/// at the wrong machine's socket. When the active machine cannot be mapped to a +/// name, this returns `None` rather than substituting an unrelated machine. fn discover_podman_machine_socket() -> Option { - let output = Command::new("podman") - .args(["machine", "inspect"]) - .stdin(Stdio::null()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .output() - .ok() - .filter(|o| o.status.success())?; + let targets = podman_machine_inspect_targets(&active_podman_machine())?; + targets.iter().find_map(|name| { + let stdout = run_podman_capture(&["machine", "inspect", name])?; + let machines: serde_json::Value = serde_json::from_slice(&stdout).ok()?; + parse_podman_machine_inspect_socket(&machines) + }) +} - let machines: serde_json::Value = serde_json::from_slice(&output.stdout).ok()?; - parse_podman_machine_inspect(&machines, &active_podman_machine()) +/// Machine names to try with `podman machine inspect`, most specific first. +/// +/// `None` means the active machine cannot be determined; inspection must not +/// guess, since picking an unrelated machine would return the wrong socket. A +/// rootful connection is named `-root` while the machine itself is +/// ``, so the `-root`-stripped name is offered as a fallback. +fn podman_machine_inspect_targets(active: &ActiveMachine) -> Option> { + fn names_for(connection: &str) -> Vec { + let mut names = vec![connection.to_string()]; + if let Some(stripped) = connection.strip_suffix("-root") + && !stripped.is_empty() + { + names.push(stripped.to_string()); + } + names + } + + match active { + ActiveMachine::Explicit(name) | ActiveMachine::Default(Some(name)) => Some(names_for(name)), + ActiveMachine::UnresolvedExplicit => None, + // No explicit selector and no default connection: `podman info` used + // Podman's built-in default machine, so inspect it by name rather than + // guessing an arbitrary entry. + ActiveMachine::Default(None) => Some(vec!["podman-machine-default".to_string()]), + } } /// Determine which machine `podman info` connected through. @@ -378,15 +465,8 @@ fn env_var_nonempty(key: &str) -> Option { /// Run `podman system connection list --format json`. fn podman_connection_list() -> Option { - let output = Command::new("podman") - .args(["system", "connection", "list", "--format", "json"]) - .stdin(Stdio::null()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .output() - .ok() - .filter(|o| o.status.success())?; - serde_json::from_slice(&output.stdout).ok() + let stdout = run_podman_capture(&["system", "connection", "list", "--format", "json"])?; + serde_json::from_slice(&stdout).ok() } /// Extract the default machine connection name from @@ -415,13 +495,10 @@ fn podman_connection_name_for_uri(connections: &serde_json::Value, uri: &str) -> .map(str::to_string) } -/// Extract the host-side socket path from `podman machine inspect` JSON, -/// selecting the machine backing the active connection. -fn parse_podman_machine_inspect( - machines: &serde_json::Value, - active: &ActiveMachine, -) -> Option { - let machine = select_podman_machine(machines.as_array()?, active)?; +/// Extract the host-side socket path from a `podman machine inspect ` +/// JSON array (which contains only the inspected machine). +fn parse_podman_machine_inspect_socket(machines: &serde_json::Value) -> Option { + let machine = machines.as_array()?.first()?; let path_str = machine["ConnectionInfo"]["PodmanSocket"]["Path"].as_str()?; if path_str.is_empty() { return None; @@ -429,33 +506,6 @@ fn parse_podman_machine_inspect( Some(PathBuf::from(path_str)) } -/// Select the machine entry backing the active Podman connection. -/// -/// An explicit selection must match a machine by name (a rootless connection -/// shares its machine's name; a rootful connection is named `-root`, -/// so the `-root`-stripped name is also tried). An explicit endpoint that -/// matches no machine returns `None` rather than guessing an unrelated machine. -/// Only the non-explicit default path falls back to the first entry, which is -/// correct on the common single-machine host. -fn select_podman_machine<'a>( - machines: &'a [serde_json::Value], - active: &ActiveMachine, -) -> Option<&'a serde_json::Value> { - let matches_name = |name: &str| machines.iter().find(|m| m["Name"].as_str() == Some(name)); - let match_connection = |name: &str| { - matches_name(name).or_else(|| name.strip_suffix("-root").and_then(matches_name)) - }; - - match active { - ActiveMachine::Explicit(name) => match_connection(name), - ActiveMachine::UnresolvedExplicit => None, - ActiveMachine::Default(name) => name - .as_deref() - .and_then(match_connection) - .or_else(|| machines.first()), - } -} - fn podman_socket_candidates() -> Vec { let socket = std::env::var("OPENSHELL_PODMAN_SOCKET") .ok() @@ -1324,9 +1374,9 @@ mod tests { detect_docker_socket_from_candidates, detect_driver, detect_podman_socket_from_candidates, docker_host_unix_socket_path, docker_socket_responds, explicit_unix_container_host, normalize_compute_driver_name, parse_default_podman_connection, parse_podman_info_socket, - parse_podman_machine_inspect, podman_connection_name_for_uri, - podman_socket_candidates_from_env, podman_socket_responds, resolve_active_podman_machine, - unix_url_socket_path, + parse_podman_machine_inspect_socket, podman_connection_name_for_uri, + podman_machine_inspect_targets, podman_socket_candidates_from_env, podman_socket_responds, + resolve_active_podman_machine, run_bounded_command, unix_url_socket_path, }; #[cfg(unix)] use super::{is_reachable_unix_socket, is_unix_socket}; @@ -1337,6 +1387,8 @@ mod tests { use std::os::unix::net::UnixListener; use std::path::PathBuf; use std::time::Duration; + #[cfg(unix)] + use std::time::Instant; #[test] fn compute_driver_kind_parses_supported_values() { @@ -1907,7 +1959,8 @@ mod tests { } #[test] - fn parse_podman_machine_inspect_extracts_macos_socket() { + fn parse_podman_machine_inspect_socket_extracts_macos_socket() { + // `podman machine inspect ` returns only the inspected machine. let machines: serde_json::Value = serde_json::json!([ { "ConnectionInfo": { @@ -1920,7 +1973,7 @@ mod tests { } ]); assert_eq!( - parse_podman_machine_inspect(&machines, &ActiveMachine::Default(None)), + parse_podman_machine_inspect_socket(&machines), Some(PathBuf::from( "/var/folders/1q/jx7s14b928n8zvstgfk98lj00000gn/T/podman/podman-machine-default-api.sock" )) @@ -1928,99 +1981,114 @@ mod tests { } #[test] - fn parse_podman_machine_inspect_returns_none_for_empty_array() { + fn parse_podman_machine_inspect_socket_returns_none_for_empty_array() { let machines: serde_json::Value = serde_json::json!([]); - assert_eq!( - parse_podman_machine_inspect(&machines, &ActiveMachine::Default(None)), - None - ); + assert_eq!(parse_podman_machine_inspect_socket(&machines), None); } #[test] - fn parse_podman_machine_inspect_returns_none_for_missing_socket() { + fn parse_podman_machine_inspect_socket_returns_none_for_missing_socket() { let machines: serde_json::Value = serde_json::json!([ { "ConnectionInfo": {}, "Name": "podman-machine-default" } ]); + assert_eq!(parse_podman_machine_inspect_socket(&machines), None); + } + + #[test] + fn podman_machine_inspect_targets_uses_explicit_machine_by_name() { + // The active connection points at `work`; discovery must inspect `work` + // explicitly rather than the no-argument default machine, which would + // return a different machine's socket. assert_eq!( - parse_podman_machine_inspect(&machines, &ActiveMachine::Default(None)), - None + podman_machine_inspect_targets(&ActiveMachine::Explicit("work".to_string())), + Some(vec!["work".to_string()]) ); } - fn two_machine_inspect() -> serde_json::Value { - serde_json::json!([ - { - "Name": "podman-machine-default", - "ConnectionInfo": { - "PodmanSocket": { "Path": "/tmp/podman/default-api.sock" } - } - }, - { - "Name": "work", - "ConnectionInfo": { - "PodmanSocket": { "Path": "/tmp/podman/work-api.sock" } - } - } - ]) + #[test] + fn podman_machine_inspect_targets_strips_rootful_suffix() { + // Rootful connections are named `-root`; the machine itself is + // ``, offered as a fallback after the connection name. + assert_eq!( + podman_machine_inspect_targets(&ActiveMachine::Explicit("work-root".to_string())), + Some(vec!["work-root".to_string(), "work".to_string()]) + ); } #[test] - fn parse_podman_machine_inspect_selects_active_machine() { - let machines = two_machine_inspect(); - // The active connection points at the second machine, not the first. + fn podman_machine_inspect_targets_uses_default_connection_name() { assert_eq!( - parse_podman_machine_inspect(&machines, &ActiveMachine::Explicit("work".to_string())), - Some(PathBuf::from("/tmp/podman/work-api.sock")) + podman_machine_inspect_targets(&ActiveMachine::Default(Some("work".to_string()))), + Some(vec!["work".to_string()]) ); } #[test] - fn parse_podman_machine_inspect_matches_rootful_connection_to_machine() { - let machines = two_machine_inspect(); - // Rootful connections are named `-root`. + fn podman_machine_inspect_targets_falls_back_to_builtin_default() { + // No explicit selector and no default connection: inspect Podman's own + // built-in default machine by name. assert_eq!( - parse_podman_machine_inspect( - &machines, - &ActiveMachine::Explicit("work-root".to_string()) - ), - Some(PathBuf::from("/tmp/podman/work-api.sock")) + podman_machine_inspect_targets(&ActiveMachine::Default(None)), + Some(vec!["podman-machine-default".to_string()]) ); } #[test] - fn parse_podman_machine_inspect_default_falls_back_to_first() { - let machines = two_machine_inspect(); - // No selector: first entry is used (correct on single-machine hosts). + fn podman_machine_inspect_targets_does_not_guess_for_unresolved_explicit() { + // CONTAINER_HOST pointing at a non-machine endpoint cannot be mapped to + // a machine; discovery must not guess an unrelated one. assert_eq!( - parse_podman_machine_inspect(&machines, &ActiveMachine::Default(None)), - Some(PathBuf::from("/tmp/podman/default-api.sock")) + podman_machine_inspect_targets(&ActiveMachine::UnresolvedExplicit), + None ); - // A default connection naming an absent machine also falls back. + } + + #[cfg(unix)] + #[test] + fn run_bounded_command_captures_stdout_on_success() { assert_eq!( - parse_podman_machine_inspect( - &machines, - &ActiveMachine::Default(Some("missing".to_string())) - ), - Some(PathBuf::from("/tmp/podman/default-api.sock")) + run_bounded_command("printf", &["hello"], Duration::from_secs(5)), + Some(b"hello".to_vec()) ); } + #[cfg(unix)] #[test] - fn parse_podman_machine_inspect_does_not_guess_for_unmatched_explicit() { - let machines = two_machine_inspect(); - // An explicit selection that matches no machine must NOT fall back to - // an unrelated machine — that would reintroduce the wrong-machine bug. + fn run_bounded_command_returns_none_on_nonzero_exit() { assert_eq!( - parse_podman_machine_inspect(&machines, &ActiveMachine::Explicit("other".to_string())), + run_bounded_command("false", &[], Duration::from_secs(5)), None ); - // CONTAINER_HOST pointing at a non-machine endpoint likewise does not - // guess. + } + + #[cfg(unix)] + #[test] + fn run_bounded_command_kills_child_that_exceeds_deadline() { + // A process that would otherwise block startup indefinitely must be + // bounded: `run_bounded_command` returns within the deadline instead of + // hanging until the child exits. + let start = Instant::now(); + let result = run_bounded_command("sleep", &["30"], Duration::from_millis(200)); + let elapsed = start.elapsed(); + assert_eq!(result, None); + assert!( + elapsed < Duration::from_secs(5), + "bounded command did not return promptly: {elapsed:?}" + ); + } + + #[cfg(unix)] + #[test] + fn run_bounded_command_returns_none_for_missing_program() { assert_eq!( - parse_podman_machine_inspect(&machines, &ActiveMachine::UnresolvedExplicit), + run_bounded_command( + "openshell-nonexistent-binary-xyz", + &[], + Duration::from_secs(5) + ), None ); } From 852736eff8eb6a26cf8f8fb67bcff461cf155067 Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Mon, 24 Aug 2026 10:41:03 -0400 Subject: [PATCH 3/4] fix(core): bound podman probe stdout drainage and kill descendants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous timeout only bounded waiting for the direct child to exit. Draining stdout still called read_to_end, which waits for pipe EOF — a Podman probe can leave a daemonized descendant (SSH multiplexer, gvproxy) that inherited the stdout pipe, so drainage could wait indefinitely even after the direct child exited, defeating the deadline. Run each probe as its own process-group leader, cap stdout drainage by the same deadline via a channel, and on expiry kill the whole process group so descendants holding the pipe are terminated and EOF is reached. Add a regression test where the direct child exits but a backgrounded descendant keeps stdout open. Signed-off-by: Russell Bryant --- crates/openshell-core/src/config.rs | 132 ++++++++++++++++++++++------ 1 file changed, 106 insertions(+), 26 deletions(-) diff --git a/crates/openshell-core/src/config.rs b/crates/openshell-core/src/config.rs index 586c497146..6d50eb5d24 100644 --- a/crates/openshell-core/src/config.rs +++ b/crates/openshell-core/src/config.rs @@ -255,54 +255,111 @@ fn run_podman_capture(args: &[&str]) -> Option> { /// Run `program ` with a deadline, capturing stdout. /// -/// Unlike `Command::output()`, which blocks until the child exits, this kills -/// and reaps the child once `timeout` elapses so a hung probe cannot stall the -/// caller. Returns the captured stdout only on a successful exit within the -/// deadline; otherwise `None`. +/// Unlike `Command::output()`, which blocks until the child exits, the deadline +/// bounds the whole call so a hung probe cannot stall gateway startup. The +/// probe runs in its own process group and the deadline covers both waiting for +/// the child and draining its stdout: a Podman probe can leave a daemonized +/// descendant (an SSH multiplexer, `gvproxy`, etc.) that inherited the stdout +/// pipe, so `read_to_end` would otherwise wait for EOF forever even after the +/// direct child exits. On expiry the entire process group is killed, which +/// terminates such descendants and releases the pipe. Returns the captured +/// stdout only on a successful exit within the deadline; otherwise `None`. fn run_bounded_command(program: &str, args: &[&str], timeout: Duration) -> Option> { use std::io::Read as _; + use std::sync::mpsc; - let mut child = Command::new(program) + let mut command = Command::new(program); + command .args(args) .stdin(Stdio::null()) .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .spawn() - .ok()?; + .stderr(Stdio::null()); + // Run the probe as its own process-group leader so a daemonized descendant + // that inherited the stdout pipe can be terminated as a group on timeout. + set_new_process_group(&mut command); + let mut child = command.spawn().ok()?; // Drain stdout on a separate thread so a child that fills the pipe buffer - // cannot deadlock against the polling loop below. + // cannot deadlock against the polling loop below. The reader signals through + // a channel so the drain can itself be bounded by the deadline. let mut stdout = child.stdout.take()?; - let reader = std::thread::spawn(move || { + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { let mut buf = Vec::new(); let _ = stdout.read_to_end(&mut buf); - buf + let _ = tx.send(buf); }); - let start = Instant::now(); + let deadline = Instant::now() + timeout; + + // Wait for the direct child to exit, bounded by the deadline. let status = loop { match child.try_wait() { - Ok(Some(status)) => break status, + Ok(Some(status)) => break Some(status), Ok(None) => { - if start.elapsed() >= timeout { - let _ = child.kill(); - let _ = child.wait(); - let _ = reader.join(); - return None; + if Instant::now() >= deadline { + break None; } std::thread::sleep(PODMAN_DISCOVERY_POLL_INTERVAL); } - Err(_) => { - let _ = child.kill(); - let _ = child.wait(); - let _ = reader.join(); - return None; - } + Err(_) => break None, } }; - let stdout = reader.join().ok()?; - status.success().then_some(stdout) + // If the child is still running at the deadline, kill its whole process + // group so any descendants (and the child) are terminated together. + if status.is_none() { + terminate_process_group(&mut child); + } + + // Collect stdout, but never past the deadline. A surviving descendant can + // hold the pipe open after the direct child exits, so cap the wait and, on + // expiry, kill the group to force EOF before collecting what was written. + let remaining = deadline.saturating_duration_since(Instant::now()); + let stdout = match rx.recv_timeout(remaining) { + Ok(buf) => buf, + Err(mpsc::RecvTimeoutError::Timeout) => { + terminate_process_group(&mut child); + rx.recv().unwrap_or_default() + } + Err(mpsc::RecvTimeoutError::Disconnected) => Vec::new(), + }; + + let _ = child.wait(); + match status { + Some(status) if status.success() => Some(stdout), + _ => None, + } +} + +/// Configure `command` to start its child as a new process-group leader so the +/// group can be signaled as a unit. No-op on non-Unix platforms. +#[cfg(unix)] +fn set_new_process_group(command: &mut Command) { + use std::os::unix::process::CommandExt as _; + command.process_group(0); +} + +#[cfg(not(unix))] +fn set_new_process_group(_command: &mut Command) {} + +/// Kill the child's whole process group so daemonized descendants that inherited +/// the stdout pipe are terminated too. Falls back to killing just the child. +#[cfg(unix)] +fn terminate_process_group(child: &mut std::process::Child) { + // `set_new_process_group` made the child a group leader, so its PID doubles + // as the group ID; signaling the negated PID targets the entire group. The + // group stays valid while a descendant is alive, so this reaches survivors + // even after the leader has been reaped. + let raw_pid = i32::try_from(child.id()).unwrap_or(i32::MAX); + let pgid = nix::unistd::Pid::from_raw(-raw_pid); + let _ = nix::sys::signal::kill(pgid, nix::sys::signal::Signal::SIGKILL); + let _ = child.kill(); +} + +#[cfg(not(unix))] +fn terminate_process_group(child: &mut std::process::Child) { + let _ = child.kill(); } /// Query the Podman CLI to discover the host-side API socket path. @@ -2080,6 +2137,29 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn run_bounded_command_bounds_drain_when_descendant_holds_stdout() { + // The shell exits immediately after `echo`, but the backgrounded child + // inherits and holds the stdout pipe open. Without bounding the drain + // (and killing the process group), `read_to_end` would wait ~30s for + // EOF even though the direct child already exited. + let start = Instant::now(); + let result = run_bounded_command( + "sh", + &["-c", "sleep 30 & echo done"], + Duration::from_millis(300), + ); + let elapsed = start.elapsed(); + assert!( + elapsed < Duration::from_secs(5), + "drain blocked on a descendant holding stdout: {elapsed:?}" + ); + // The direct child exited 0 and its flushed output is still collected + // once the group is killed and the pipe reaches EOF. + assert_eq!(result, Some(b"done\n".to_vec())); + } + #[cfg(unix)] #[test] fn run_bounded_command_returns_none_for_missing_program() { From 9fa6e98dd62072160fb94bf8c4af3f7b5e3b77fd Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Mon, 24 Aug 2026 12:20:32 -0400 Subject: [PATCH 4/4] fix(core): make podman probe deadline absolute against escaped descendants The prior drainage timeout still ended in an untimed rx.recv() after killing the process group. A descendant that escaped the probe's process group (e.g. via setsid) while holding the stdout pipe would not be killed, so read_to_end never saw EOF, the reader never sent, and that recv() could block gateway startup indefinitely. On drainage timeout, best-effort kill the process group and return None immediately, abandoning the reader thread instead of waiting on it again. The deadline now bounds the whole call regardless of what descendants do. Add a regression test whose stdout-holding descendant escapes the process group (`set -m`) and assert the call still returns promptly. Signed-off-by: Russell Bryant --- crates/openshell-core/src/config.rs | 102 +++++++++++++++++----------- 1 file changed, 64 insertions(+), 38 deletions(-) diff --git a/crates/openshell-core/src/config.rs b/crates/openshell-core/src/config.rs index 6d50eb5d24..ed5736eff7 100644 --- a/crates/openshell-core/src/config.rs +++ b/crates/openshell-core/src/config.rs @@ -256,14 +256,18 @@ fn run_podman_capture(args: &[&str]) -> Option> { /// Run `program ` with a deadline, capturing stdout. /// /// Unlike `Command::output()`, which blocks until the child exits, the deadline -/// bounds the whole call so a hung probe cannot stall gateway startup. The -/// probe runs in its own process group and the deadline covers both waiting for -/// the child and draining its stdout: a Podman probe can leave a daemonized -/// descendant (an SSH multiplexer, `gvproxy`, etc.) that inherited the stdout -/// pipe, so `read_to_end` would otherwise wait for EOF forever even after the -/// direct child exits. On expiry the entire process group is killed, which -/// terminates such descendants and releases the pipe. Returns the captured -/// stdout only on a successful exit within the deadline; otherwise `None`. +/// is absolute: the call returns within `timeout` no matter what the probe or +/// its descendants do. A Podman probe can leave a daemonized descendant (an SSH +/// multiplexer, `gvproxy`, etc.) that inherited the stdout pipe, so +/// `read_to_end` would otherwise wait for EOF forever even after the direct +/// child exits — and such a descendant may even have escaped the probe's +/// process group. To stay bounded, the deadline covers both waiting for the +/// child and draining its stdout; on expiry the call best-effort kills the +/// process group (cleaning up in-group descendants) and gives up immediately, +/// abandoning the reader thread rather than waiting on it again. The abandoned +/// reader exits on its own once the pipe finally closes. Returns the captured +/// stdout only on a successful exit whose output was fully drained within the +/// deadline; otherwise `None`. fn run_bounded_command(program: &str, args: &[&str], timeout: Duration) -> Option> { use std::io::Read as _; use std::sync::mpsc; @@ -274,14 +278,15 @@ fn run_bounded_command(program: &str, args: &[&str], timeout: Duration) -> Optio .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::null()); - // Run the probe as its own process-group leader so a daemonized descendant - // that inherited the stdout pipe can be terminated as a group on timeout. + // Run the probe as its own process-group leader so in-group descendants that + // inherited the stdout pipe can be terminated as a group on timeout. set_new_process_group(&mut command); let mut child = command.spawn().ok()?; // Drain stdout on a separate thread so a child that fills the pipe buffer - // cannot deadlock against the polling loop below. The reader signals through - // a channel so the drain can itself be bounded by the deadline. + // cannot deadlock against the polling loop below. The reader reports through + // a channel so the drain can be bounded by the deadline and abandoned if it + // outlives it. let mut stdout = child.stdout.take()?; let (tx, rx) = mpsc::channel(); std::thread::spawn(move || { @@ -306,29 +311,28 @@ fn run_bounded_command(program: &str, args: &[&str], timeout: Duration) -> Optio } }; - // If the child is still running at the deadline, kill its whole process - // group so any descendants (and the child) are terminated together. - if status.is_none() { + // The child never exited within the deadline: kill the group, reap the + // (now-killed) direct child, and give up. `wait()` is bounded because the + // direct child is dead. + let Some(status) = status else { terminate_process_group(&mut child); - } + let _ = child.wait(); + return None; + }; - // Collect stdout, but never past the deadline. A surviving descendant can - // hold the pipe open after the direct child exits, so cap the wait and, on - // expiry, kill the group to force EOF before collecting what was written. + // The direct child exited (already reaped by `try_wait`). Collect its stdout + // without ever blocking past the deadline. A surviving descendant — possibly + // one that escaped the process group — can hold the pipe open indefinitely, + // so on expiry best-effort kill the group for cleanup and return None rather + // than waiting on the reader again. let remaining = deadline.saturating_duration_since(Instant::now()); - let stdout = match rx.recv_timeout(remaining) { - Ok(buf) => buf, - Err(mpsc::RecvTimeoutError::Timeout) => { + match rx.recv_timeout(remaining) { + Ok(stdout) if status.success() => Some(stdout), + Ok(_) => None, + Err(_) => { terminate_process_group(&mut child); - rx.recv().unwrap_or_default() + None } - Err(mpsc::RecvTimeoutError::Disconnected) => Vec::new(), - }; - - let _ = child.wait(); - match status { - Some(status) if status.success() => Some(stdout), - _ => None, } } @@ -2139,11 +2143,11 @@ mod tests { #[cfg(unix)] #[test] - fn run_bounded_command_bounds_drain_when_descendant_holds_stdout() { + fn run_bounded_command_bounds_drain_when_in_group_descendant_holds_stdout() { // The shell exits immediately after `echo`, but the backgrounded child - // inherits and holds the stdout pipe open. Without bounding the drain - // (and killing the process group), `read_to_end` would wait ~30s for - // EOF even though the direct child already exited. + // (in the same process group) inherits and holds the stdout pipe open. + // Without a bounded drain, `read_to_end` would wait ~30s for EOF even + // though the direct child already exited. let start = Instant::now(); let result = run_bounded_command( "sh", @@ -2152,12 +2156,34 @@ mod tests { ); let elapsed = start.elapsed(); assert!( - elapsed < Duration::from_secs(5), + elapsed < Duration::from_secs(2), "drain blocked on a descendant holding stdout: {elapsed:?}" ); - // The direct child exited 0 and its flushed output is still collected - // once the group is killed and the pipe reaches EOF. - assert_eq!(result, Some(b"done\n".to_vec())); + // Draining hit the deadline, so the probe is treated as "not found". + assert_eq!(result, None); + } + + #[cfg(unix)] + #[test] + fn run_bounded_command_bounds_drain_when_descendant_escapes_process_group() { + // `set -m` runs the background job in its OWN process group, so it + // survives the group kill while still holding the stdout pipe. The + // deadline must remain absolute: the call must not fall back to an + // untimed receive that waits for the escaped descendant's EOF. + let start = Instant::now(); + let result = run_bounded_command( + "bash", + &["-c", "set -m; sleep 5 & echo done"], + Duration::from_millis(300), + ); + let elapsed = start.elapsed(); + // A regression (blocking receive after the timeout) would wait ~5s for + // the escaped `sleep`; the bounded implementation returns promptly. + assert!( + elapsed < Duration::from_secs(2), + "drain blocked on a descendant that escaped the process group: {elapsed:?}" + ); + assert_eq!(result, None); } #[cfg(unix)]