test(e2e): docker build e2e suite — real-world matrix, scenarios D1–D5 + D9–D10

tests/e2e/src/docker.rs

@@ -5,6 +5,8 @@
 //! via `DOCKER_HOST`, so the developer's Docker context and any host
 //! daemon stay untouched (see tests/e2e/README.md on isolation).
 
+use std::io::Read as _;
+use std::os::unix::process::CommandExt as _;
 use std::path::Path;
 use std::process::{Command, Stdio};
 use std::thread;
@@ -140,23 +142,170 @@ pub fn ensure_image(data_dir: &Path, image: &str) -> Result<()> {
     Err(last_err.expect("loop ran at least once")).context("docker pull (3 attempts)")
 }
 
+/// SIGKILLs the process group led by `pgid`, which `process_group(0)` made
+/// equal to the spawned child's pid. Safe to call after the leader has been
+/// reaped: POSIX forbids reusing a pid while it is still the group id of an
+/// existing group, so this either reaches that group or nothing at all
+/// (`ESRCH`), never an unrelated one.
+fn kill_process_group(pgid: i32) {
+    // SAFETY: `killpg` takes no pointers and cannot fail unsoundly; a stale
+    // or empty group yields ESRCH, which we ignore.
+    unsafe { libc::killpg(pgid, libc::SIGKILL) };
+}
+
 /// Runs a command, killing it once `timeout` passes.
+///
+/// Both pipes are drained on background threads for the whole run: an
+/// undrained pipe fills at ~64 KiB and blocks the child, which turns any
+/// chatty command (a `--progress=plain` build, a large pull) into a bogus
+/// timeout. On a real timeout the error carries the output tail, so a
+/// killed command still leaves forensics.
+///
+/// The child leads its own process group and **both** exit paths signal the
+/// whole group before joining. Killing just the direct child is not enough:
+/// `docker build` runs the build in a `docker-buildx` grandchild that
+/// inherits these pipes, so the write end stays open and the drain-thread
+/// joins block past the deadline — indefinitely if that descendant is itself
+/// wedged, which is exactly what this suite exists to catch. The same holds
+/// when the command *succeeds* while leaving a descendant behind, so the
+/// group kill is not conditional on timing out: once the direct child is
+/// gone, nothing else may hold pipes this function is about to join on.
 pub fn run_with_timeout(command: &mut Command, timeout: Duration) -> Result<std::process::Output> {
     let mut child = command
         .stdout(Stdio::piped())
         .stderr(Stdio::piped())
+        .process_group(0)
         .spawn()?;
+    // Captured before any reap: `Child::id` is not meaningful afterwards.
+    let pgid = i32::try_from(child.id()).expect("pid fits in i32");
+    let drain = |pipe: Option<Box<dyn std::io::Read + Send>>| {
+        thread::spawn(move || {
+            let mut buf = Vec::new();
+            if let Some(mut pipe) = pipe {
+                let _ = pipe.read_to_end(&mut buf);
+            }
+            buf
+        })
+    };
+    let stdout_thread = drain(
+        child
+            .stdout
+            .take()
+            .map(|p| Box::new(p) as Box<dyn std::io::Read + Send>),
+    );
+    let stderr_thread = drain(
+        child
+            .stderr
+            .take()
+            .map(|p| Box::new(p) as Box<dyn std::io::Read + Send>),
+    );
+
     let start = Instant::now();
     while start.elapsed() < timeout {
-        if child.try_wait()?.is_some() {
-            return child
-                .wait_with_output()
-                .context("collecting command output");
+        if let Some(status) = child.try_wait()? {
+            kill_process_group(pgid);
+            let stdout = stdout_thread.join().unwrap_or_default();
+            let stderr = stderr_thread.join().unwrap_or_default();
+            return Ok(std::process::Output {
+                status,
+                stdout,
+                stderr,
+            });
         }
         thread::sleep(Duration::from_millis(100));
     }
 
-    let _ = child.kill();
+    kill_process_group(pgid);
     let _ = child.wait();
-    Err(anyhow!("command timed out after {}s", timeout.as_secs()))
+    // The whole group is gone, so every inherited write end is closed and the
+    // drain threads see EOF.
+    let stdout = stdout_thread.join().unwrap_or_default();
+    let stderr = stderr_thread.join().unwrap_or_default();
+    let combined = format!(
+        "{}{}",
+        String::from_utf8_lossy(&stdout),
+        String::from_utf8_lossy(&stderr)
+    );
+    let mut tail_start = combined.len().saturating_sub(2000);
+    while !combined.is_char_boundary(tail_start) {
+        tail_start += 1;
+    }
+    Err(anyhow!(
+        "command timed out after {}s; output tail:\n{}",
+        timeout.as_secs(),
+        &combined[tail_start..]
+    ))
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// A command whose output exceeds the OS pipe buffer must complete: an
+    /// undrained pipe blocks the child at ~64 KiB and the old implementation
+    /// turned every chatty command into a bogus timeout (caught by the
+    /// docker_build D9 streaming scenario).
+    #[test]
+    fn chatty_command_is_drained_not_deadlocked() {
+        let output = run_with_timeout(
+            Command::new("sh").args([
+                "-c",
+                "dd if=/dev/zero bs=1024 count=256 2>/dev/null | base64",
+            ]),
+            Duration::from_secs(20),
+        )
+        .expect("chatty command must not time out");
+        assert!(output.status.success());
+        assert!(output.stdout.len() > 64 * 1024);
+    }
+
+    /// The success path has the same hazard as the timeout path: a command
+    /// can exit promptly while leaving a descendant holding the inherited
+    /// pipes, and the drain-thread joins would then block on that descendant
+    /// with no deadline left to enforce. Returns in ~0s once the group is
+    /// killed, ~30s if the success branch stops doing so.
+    #[test]
+    fn success_returns_promptly_despite_surviving_descendant() {
+        let start = Instant::now();
+        let output = run_with_timeout(
+            Command::new("sh").args(["-c", "echo done; sleep 30 & exit 0"]),
+            Duration::from_secs(60),
+        )
+        .expect("command must succeed");
+        let elapsed = start.elapsed();
+        assert!(output.status.success());
+        assert!(
+            elapsed < Duration::from_secs(10),
+            "success took {elapsed:?}; a descendant outlived the command and \
+             held the pipes open"
+        );
+    }
+
+    /// A genuine timeout must surface the output tail for forensics, and
+    /// must return at the deadline even though the shell leaves a `sleep`
+    /// descendant holding the inherited pipes. Killing only the direct child
+    /// leaves that write end open, so the drain-thread joins block until the
+    /// descendant exits — ~30s here, unbounded when the survivor is a wedged
+    /// `docker-buildx`, which is the shape this suite hits for real.
+    ///
+    /// `sleep 30 & wait` is deliberate: with a plain `sleep 30` the shell
+    /// `exec`s it as the last command, so there is no grandchild and the bug
+    /// hides. Backgrounding forces the shell to stay alive as a real parent.
+    /// The elapsed bound is the regression; the tail is the original contract.
+    #[test]
+    fn timeout_returns_at_deadline_despite_surviving_descendant() {
+        let start = Instant::now();
+        let error = run_with_timeout(
+            Command::new("sh").args(["-c", "echo tail-marker; sleep 30 & wait"]),
+            Duration::from_secs(1),
+        )
+        .expect_err("command must time out");
+        let elapsed = start.elapsed();
+        assert!(error.to_string().contains("tail-marker"));
+        assert!(
+            elapsed < Duration::from_secs(10),
+            "timeout took {elapsed:?}; the `sleep` descendant outlived the \
+             kill and held the pipes open"
+        );
+    }
 }