From 82d341d6282165f87387fcd5ba1e2ecac610885b Mon Sep 17 00:00:00 2001 From: Yan Date: Tue, 14 Apr 2026 16:36:47 -0400 Subject: [PATCH] fix(sandbox): try Docker socket before CLI binary check (#2467) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sandbox): try Docker socket before CLI binary check The sandbox detection checked `which docker` first and returned NotInstalled if the CLI binary was absent — even when the Docker daemon was reachable via a bind-mounted socket. This broke container-in-container deployments (e.g., Nomad shards with /var/run/docker.sock mounted) where bollard can talk to the daemon but no CLI is installed in the slim image. Reorder check_docker() to try connect_docker() (bollard socket ping) first. If the daemon responds, return Available immediately. The CLI check is now only used as a fallback for error-message quality when the socket connection fails. Co-Authored-By: Claude Opus 4.6 (1M context) * fix(sandbox): skip slow daemon ping when Docker is clearly absent check_docker() called connect_docker() before checking whether Docker was even present, causing a 120s bollard timeout on hosts with an unreachable DOCKER_HOST and no Docker installation. Add a fast-path that checks for the docker binary, DOCKER_HOST env var, and socket files on disk before attempting the daemon ping. This preserves DinD support (bind-mounted socket, no CLI binary) while avoiding the latency regression for non-Docker hosts. Co-Authored-By: Claude Opus 4.6 (1M context) * test(sandbox): add regression tests for check_docker fast-path Extract should_skip_daemon_ping() predicate from check_docker() and add unit tests covering all combinations: skip when no binary, no DOCKER_HOST, and no socket (the bug scenario); no skip when any of the three signals is present (DinD socket, DOCKER_HOST, CLI binary). Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- src/sandbox/container.rs | 2 +- src/sandbox/detect.rs | 122 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 114 insertions(+), 10 deletions(-) diff --git a/src/sandbox/container.rs b/src/sandbox/container.rs index f5ef014702..f8b65e7922 100644 --- a/src/sandbox/container.rs +++ b/src/sandbox/container.rs @@ -683,7 +683,7 @@ pub async fn connect_docker() -> Result { } #[cfg(unix)] -fn unix_socket_candidates() -> Vec { +pub(crate) fn unix_socket_candidates() -> Vec { unix_socket_candidates_from_env( std::env::var_os("HOME").map(PathBuf::from), std::env::var_os("XDG_RUNTIME_DIR").map(PathBuf::from), diff --git a/src/sandbox/detect.rs b/src/sandbox/detect.rs index e7db8819c6..0f1ccb240b 100644 --- a/src/sandbox/detect.rs +++ b/src/sandbox/detect.rs @@ -1,8 +1,13 @@ //! Proactive Docker detection with platform-specific guidance. //! -//! Checks whether Docker is both installed (binary on PATH) and running -//! (daemon responding to ping), and provides platform-appropriate -//! installation or startup instructions when it is not. +//! First performs cheap filesystem checks (socket existence, `DOCKER_HOST`, +//! PATH lookup) to fast-exit on hosts where Docker is clearly absent — +//! avoiding bollard's 120 s daemon-ping timeout. Then attempts a direct +//! socket connection via bollard (covers container-in-container deployments +//! where the socket is bind-mounted but the CLI is absent), and falls back +//! to a `which docker` PATH check for error-message quality. Provides +//! platform-appropriate installation or startup instructions when Docker +//! is not available. //! //! # Detection Limitations //! @@ -102,21 +107,33 @@ pub struct DockerDetection { /// Check whether Docker is installed and running. /// -/// 1. Checks if `docker` binary exists on PATH -/// 2. If found, tries to connect and ping the Docker daemon via `connect_docker()` -/// 3. Returns `Available`, `NotInstalled`, or `NotRunning` +/// 1. Fast path: if no Docker socket exists on the filesystem, `DOCKER_HOST` +/// is unset, *and* the `docker` CLI binary is absent, returns `NotInstalled` +/// immediately — without a daemon ping. This avoids a 120 s bollard timeout +/// on hosts where Docker is clearly not present (see #P2). +/// 2. Tries to connect and ping the Docker daemon directly via +/// `connect_docker()` (bollard). This covers container-in-container (DinD) +/// deployments where the socket is bind-mounted but the CLI binary is not +/// installed inside the container. +/// 3. If the daemon is unreachable and the binary is also missing, returns +/// `NotInstalled`; otherwise `NotRunning`. pub async fn check_docker() -> DockerDetection { let platform = Platform::current(); + let binary_found = docker_binary_exists(); + let docker_host_set = std::env::var_os("DOCKER_HOST").is_some(); + let socket_found = any_docker_socket_exists(); - // Step 1: Check if docker binary is on PATH - if !docker_binary_exists() { + // Fast path: no CLI binary, no DOCKER_HOST, and no socket file on disk. + // Skip the daemon ping that would block on an unreachable host. + if should_skip_daemon_ping(binary_found, docker_host_set, socket_found) { return DockerDetection { status: DockerStatus::NotInstalled, platform, }; } - // Step 2: Try to connect to the daemon + // Authoritative check: try to connect and ping the daemon via bollard. + // Covers DinD (socket bind-mounted, no CLI binary) and standard installs. if crate::sandbox::connect_docker().await.is_ok() { return DockerDetection { status: DockerStatus::Available, @@ -124,6 +141,14 @@ pub async fn check_docker() -> DockerDetection { }; } + // Daemon unreachable. Distinguish "not installed" from "not running". + if !binary_found { + return DockerDetection { + status: DockerStatus::NotInstalled, + platform, + }; + } + // Windows fallback: if the named pipe probe fails but docker CLI can still // reach the daemon/server, treat Docker as available. #[cfg(windows)] @@ -162,6 +187,38 @@ fn docker_binary_exists() -> bool { } } +/// Check if any well-known Docker socket file exists on disk. +/// +/// This is a cheap filesystem probe (no daemon ping) used to decide whether +/// it is worth attempting the slower `connect_docker()` call. Covers the +/// default `/var/run/docker.sock` plus the user-space candidates that +/// `connect_docker()` already tries. +fn any_docker_socket_exists() -> bool { + #[cfg(unix)] + { + use std::path::PathBuf; + + // Default socket that bollard's connect_with_local_defaults() checks. + if PathBuf::from("/var/run/docker.sock").exists() { + return true; + } + + // Same user-space candidates that connect_docker() iterates. + crate::sandbox::container::unix_socket_candidates() + .iter() + .any(|sock| sock.exists()) + } + + #[cfg(windows)] + { + // On Windows, bollard probes the named pipe `//./pipe/docker_engine`. + // `Path::exists()` doesn't work for named pipes, so we conservatively + // return true — the binary-exists check is the primary fast-path on + // Windows. + true + } +} + #[cfg(windows)] fn docker_cli_daemon_reachable() -> bool { let stdout = std::process::Stdio::null(); @@ -188,6 +245,12 @@ fn docker_cli_daemon_reachable() -> bool { .is_ok_and(|s| s.success()) } +/// Returns `true` when we can confidently say Docker is not installed without +/// performing a daemon ping. All three signals must be absent. +fn should_skip_daemon_ping(binary_found: bool, docker_host_set: bool, socket_found: bool) -> bool { + !binary_found && !docker_host_set && !socket_found +} + #[cfg(test)] mod tests { use super::*; @@ -232,4 +295,45 @@ mod tests { DockerStatus::Disabled => panic!("check_docker should never return Disabled"), } } + + // --- Regression tests for the fast-path that skips the daemon ping --- + + #[test] + fn skip_ping_when_no_binary_no_host_no_socket() { + // The bug: connect_docker() was called unconditionally, blocking for + // 120 s on hosts with an unreachable DOCKER_HOST and no Docker. + assert!(should_skip_daemon_ping(false, false, false)); + } + + #[test] + fn no_skip_when_docker_host_set() { + // DOCKER_HOST points somewhere — must attempt the ping even without + // a binary (could be a remote Docker host). + assert!(!should_skip_daemon_ping(false, true, false)); + } + + #[test] + fn no_skip_when_socket_exists() { + // Socket on disk (DinD bind-mount) — must ping even without the CLI. + assert!(!should_skip_daemon_ping(false, false, true)); + } + + #[test] + fn no_skip_when_binary_found() { + // CLI binary present — Docker may be installed but daemon stopped. + assert!(!should_skip_daemon_ping(true, false, false)); + } + + #[test] + fn no_skip_when_all_signals_present() { + assert!(!should_skip_daemon_ping(true, true, true)); + } + + #[cfg(unix)] + #[test] + fn any_docker_socket_nonexistent_path_returns_false() { + // Sanity: a path that doesn't exist should not count as a socket. + use std::path::PathBuf; + assert!(!PathBuf::from("/tmp/definitely-not-a-docker-socket.sock").exists()); + } }