fix(cli-tests): retry lost port races in the server-spawn harness; surface server stderr

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.
This commit is contained in:
aaltshuler 2026-07-02 02:55:18 +03:00 committed by Andrew Altshuler
parent 4bc7318408
commit fbc1d3f4ed

View file

@ -191,33 +191,86 @@ fn free_port() -> u16 {
port port
} }
fn spawn_server_process(mut command: StdCommand) -> TestServer { /// Rebuild a spawnable copy of `command` (program, args, envs, cwd).
let port = free_port(); /// `StdCommand` is not `Clone`, and a retry cannot re-use the original —
let bind = format!("127.0.0.1:{}", port); /// appending a second `--bind` would trip clap's duplicate-argument error.
let mut child = command fn clone_command(command: &StdCommand) -> StdCommand {
.arg("--bind") let mut fresh = StdCommand::new(command.get_program());
.arg(&bind) fresh.args(command.get_args());
.stdout(Stdio::null()) for (key, value) in command.get_envs() {
.stderr(Stdio::null()) match value {
.spawn() Some(value) => {
.unwrap(); fresh.env(key, value);
let base_url = format!("http://{}", bind); }
let client = Client::new(); None => {
for _ in 0..300 { fresh.env_remove(key);
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() {
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 { pub fn spawn_server(graph: &Path) -> TestServer {