From fbc1d3f4ed8e38fa054e99176246e84e846bf833 Mon Sep 17 00:00:00 2001
From: aaltshuler <andrew@collectivelab.io>
Date: Thu, 2 Jul 2026 02:55:18 +0300
Subject: [PATCH] fix(cli-tests): retry lost port races in the server-spawn
 harness; surface server stderr
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

spawn_server_process picked a port by binding :0 and DROPPING the
listener before the spawned omnigraph-server rebound it — a TOCTOU
window in which any concurrently-spawning test steals the port. The
loser exits "address in use", but stderr was nulled, so the test
panicked with a bare "server exited before becoming healthy: exit
status: 1" on whichever parity/system test lost the race that run
(~1-in-2 parallel runs locally; 12/12 pass serially).

Fix, two halves:
- Retry the spawn on a fresh port (up to 5 attempts) when the child
  exits before passing the health check. A lost race becomes invisible;
  a deterministic startup failure still panics loudly after the retries.
- Capture stderr to a temp file (not a pipe, which a chatty healthy
  server would fill and block on; not null) and include it in the panic
  and per-retry messages, so genuine startup failures carry the server's
  own error text.

StdCommand isn't Clone and a reused command would trip clap's
duplicate --bind rejection, so retries respawn via clone_command
(program/args/envs/cwd).

Verified: parity_matrix 3x parallel clean (previously flaked ~every
other run), full omnigraph-cli suite green.
---
 crates/omnigraph-cli/tests/support/mod.rs | 103 ++++++++++++++++------
 1 file changed, 78 insertions(+), 25 deletions(-)

diff --git a/crates/omnigraph-cli/tests/support/mod.rs b/crates/omnigraph-cli/tests/support/mod.rs
index ff6a5d48..c8531164 100644
--- a/crates/omnigraph-cli/tests/support/mod.rs
+++ b/crates/omnigraph-cli/tests/support/mod.rs
@@ -191,33 +191,86 @@ fn free_port() -> u16 {
     port
 }
 
-fn spawn_server_process(mut command: StdCommand) -> TestServer {
-    let port = free_port();
-    let bind = format!("127.0.0.1:{}", port);
-    let mut child = command
-        .arg("--bind")
-        .arg(&bind)
-        .stdout(Stdio::null())
-        .stderr(Stdio::null())
-        .spawn()
-        .unwrap();
-    let base_url = format!("http://{}", bind);
-    let client = Client::new();
-    for _ in 0..300 {
-        if client
-            .get(format!("{}/healthz", base_url))
-            .send()
-            .map(|response| response.status().is_success())
-            .unwrap_or(false)
-        {
-            return TestServer { child, base_url };
+/// Rebuild a spawnable copy of `command` (program, args, envs, cwd).
+/// `StdCommand` is not `Clone`, and a retry cannot re-use the original —
+/// appending a second `--bind` would trip clap's duplicate-argument error.
+fn clone_command(command: &StdCommand) -> StdCommand {
+    let mut fresh = StdCommand::new(command.get_program());
+    fresh.args(command.get_args());
+    for (key, value) in command.get_envs() {
+        match value {
+            Some(value) => {
+                fresh.env(key, value);
+            }
+            None => {
+                fresh.env_remove(key);
+            }
         }
-        if let Some(status) = child.try_wait().unwrap() {
-            panic!("server exited before becoming healthy: {status}");
-        }
-        sleep(Duration::from_millis(100));
     }
-    panic!("server did not become healthy");
+    if let Some(dir) = command.get_current_dir() {
+        fresh.current_dir(dir);
+    }
+    fresh
+}
+
+fn spawn_server_process(command: StdCommand) -> TestServer {
+    // `free_port()` releases the port before the server rebinds it, so under
+    // parallel tests a concurrent spawn can steal it and the loser exits with
+    // "address in use" — retry on a fresh port. Stderr goes to a temp file
+    // (not a pipe: a chatty healthy server would fill a pipe's buffer and
+    // block; not null: a genuine startup failure must panic with the server's
+    // own error, not a bare exit status).
+    const SPAWN_ATTEMPTS: usize = 5;
+    for attempt in 1..=SPAWN_ATTEMPTS {
+        let port = free_port();
+        let bind = format!("127.0.0.1:{}", port);
+        let mut stderr_log = tempfile::tempfile().unwrap();
+        let mut child = clone_command(&command)
+            .arg("--bind")
+            .arg(&bind)
+            .stdout(Stdio::null())
+            .stderr(Stdio::from(stderr_log.try_clone().unwrap()))
+            .spawn()
+            .unwrap();
+        let base_url = format!("http://{}", bind);
+        let client = Client::new();
+        let mut early_exit = None;
+        for _ in 0..300 {
+            if client
+                .get(format!("{}/healthz", base_url))
+                .send()
+                .map(|response| response.status().is_success())
+                .unwrap_or(false)
+            {
+                return TestServer { child, base_url };
+            }
+            if let Some(status) = child.try_wait().unwrap() {
+                early_exit = Some(status);
+                break;
+            }
+            sleep(Duration::from_millis(100));
+        }
+        let Some(status) = early_exit else {
+            let _ = child.kill();
+            let _ = child.wait();
+            panic!("server did not become healthy on {bind}");
+        };
+        use std::io::{Read, Seek, SeekFrom};
+        let mut stderr = String::new();
+        let _ = stderr_log.seek(SeekFrom::Start(0));
+        let _ = stderr_log.read_to_string(&mut stderr);
+        if attempt == SPAWN_ATTEMPTS {
+            panic!(
+                "server exited before becoming healthy on every port \
+                 ({SPAWN_ATTEMPTS} attempts; last: {status} on {bind}); stderr:\n{stderr}"
+            );
+        }
+        eprintln!(
+            "server spawn attempt {attempt}/{SPAWN_ATTEMPTS} exited early ({status} on {bind}), \
+             likely a lost port race — retrying on a fresh port; stderr:\n{stderr}"
+        );
+    }
+    unreachable!("loop either returns a healthy server or panics");
 }
 
 pub fn spawn_server(graph: &Path) -> TestServer {