From 5d691f212fa6963d94f820f7c3900df1fc493988 Mon Sep 17 00:00:00 2001 From: Proactive Runtime Bot Date: Tue, 4 Aug 2026 22:58:11 +0200 Subject: [PATCH] fix(broker): verify worker process before spawn success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit agent-relay node agent spawn could report success while the spawned worker process had already exited (e.g. a wrapper that fails to launch its harness) — Command::spawn only proves the wrapper was created, not that it survived. Add a brief stability window after spawn that non-blockingly checks the child is still alive before reporting success; if it already exited, remove the stale registry entry and return the real error (exit status + log path) instead of letting node agent list briefly advertise a dead process. Verified: - cargo test --package agent-relay-broker worker:: (62/62, 4 consecutive clean runs, plus 5 isolated runs of the specific new tests) - Real integration test: RELAY_INTEGRATION_REAL_CLI=1 node --test tests/integration/broker/dist/cli-spawn.test.js (missing-CLI rejection path, 21.5s, real broker) --- CHANGELOG.md | 1 + crates/broker/src/worker.rs | 86 ++++++++++++++++++++++ tests/integration/broker/cli-spawn.test.ts | 30 ++++++++ 3 files changed, 117 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe64be4c9..7a51e6eac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `agent-relay node up` resolves its installed broker through canonical package-manager links and Relay's user install directories, so mise-managed and minimal-`PATH` launches no longer fail when the broker binary is already installed. - `agent-relay up` / `node up` use one precedence ladder: `--workspace-key` → workspace environment variables → repository pin → machine-global active workspace → creating one. A fresh project joins the active workspace instead of silently creating another, and startup announces the winning source. - Enrolled-node restarts preserve the repository-pinned workspace while resuming the enrolled identity. A conflicting enrollment stops startup, names both non-secret sources, and points to `workspace rebind ` as the recovery path. +- `agent-relay node agent spawn` now verifies that the worker process survives startup before reporting success, and reports its exit status and log path when launch fails. - First-run telemetry notices are written to stderr so JSON stdout remains parseable. - Detached `node up --background` surfaces early child failures and stops polling when the child exits without trying to kill an already dead process. diff --git a/crates/broker/src/worker.rs b/crates/broker/src/worker.rs index 798f20c2b..71f201568 100644 --- a/crates/broker/src/worker.rs +++ b/crates/broker/src/worker.rs @@ -53,6 +53,12 @@ const APP_SERVER_RELEASE_GRACE: Duration = Duration::from_secs(35); /// healthy-but-slow agent is far worse than listing a dead one a little longer. const WORKER_READY_DEADLINE: Duration = Duration::from_secs(90); +/// Briefly hold the spawn acknowledgement so a wrapper that cannot launch its +/// harness has time to exit. `Command::spawn` only proves that the wrapper was +/// created; without this stability window the HTTP API can report success even +/// though the wrapper is already gone by the time the caller lists agents. +const WORKER_SPAWN_STABILITY_WINDOW: Duration = Duration::from_millis(250); + /// How long to wait for a SIGKILLed orphan wrapper to be reaped before giving /// up. Bounded so a wrapper stuck in uninterruptible sleep cannot stall the /// maintenance tick, which also drives delivery retries. @@ -119,6 +125,32 @@ pub(crate) fn orphaned_worker( None } +/// Confirm that a freshly-created worker process survives its initial handoff. +/// +/// This is intentionally narrower than `worker_ready`: PTY readiness may take +/// up to 25 seconds and is processed by the same runtime loop that services the +/// spawn request. The short stability probe catches launch failures without +/// deadlocking that loop or making every successful spawn wait for a TUI. +async fn confirm_worker_process_alive( + name: &str, + child: &mut Child, + log_path: Option<&Path>, + stability_window: Duration, +) -> Result<()> { + tokio::time::sleep(stability_window).await; + let Some(status) = child + .try_wait() + .with_context(|| format!("failed to verify agent '{name}' process after spawn"))? + else { + return Ok(()); + }; + + let log_hint = log_path + .map(|path| format!("; see worker log {}", path.display())) + .unwrap_or_default(); + anyhow::bail!("agent '{name}' process exited during startup ({status}){log_hint}") +} + // Working/idle activity inference from PTY output comes from the // harness-agnostic `relay-pty` crate. pub(crate) use relay_pty::detection; @@ -912,6 +944,7 @@ impl WorkerRegistry { let stdout = child.stdout.take().context("worker missing stdout pipe")?; let stderr = child.stderr.take().context("worker missing stderr pipe")?; let log_file = self.worker_log_path(&spec.name); + let startup_log_file = log_file.clone(); spawn_worker_reader( self.event_tx.clone(), @@ -956,6 +989,28 @@ impl WorkerRegistry { ) .await?; + let startup_confirmation = { + let handle = self + .workers + .get_mut(&spec.name) + .with_context(|| format!("unknown worker '{}' after spawn", spec.name))?; + confirm_worker_process_alive( + &spec.name, + &mut handle.child, + startup_log_file.as_deref(), + WORKER_SPAWN_STABILITY_WINDOW, + ) + .await + }; + if let Err(error) = startup_confirmation { + // `try_wait` reaped an exited wrapper. Remove the stale registry + // entry before returning the error so `node agent list` cannot + // briefly advertise a process the spawn call just rejected. + self.workers.remove(&spec.name); + self.initial_tasks.remove(&spec.name); + return Err(error); + } + tracing::info!( target = "broker::spawn", name = %spec.name, @@ -2015,6 +2070,37 @@ mod tests { assert!(reg.list(&HashMap::new()).is_empty()); } + #[tokio::test] + async fn spawn_confirmation_rejects_a_process_that_exits_immediately() { + let mut child = Command::new("sleep").arg("0").spawn().unwrap(); + + let error = confirm_worker_process_alive( + "failed-worker", + &mut child, + Some(Path::new("/tmp/failed-worker.log")), + Duration::from_millis(100), + ) + .await + .unwrap_err(); + + let message = error.to_string(); + assert!(message.contains("process exited during startup")); + assert!(message.contains("/tmp/failed-worker.log")); + } + + #[tokio::test] + async fn spawn_confirmation_accepts_a_process_that_stays_alive() { + let mut child = Command::new("sleep").arg("30").spawn().unwrap(); + + confirm_worker_process_alive("live-worker", &mut child, None, Duration::from_millis(100)) + .await + .unwrap(); + + terminate_child(&mut child, Duration::from_millis(200)) + .await + .unwrap(); + } + // The wrapper process can outlive the harness it hosts, so reaping on the // wrapper alone leaves a dead agent listed as `working` forever. mod orphaned_worker { diff --git a/tests/integration/broker/cli-spawn.test.ts b/tests/integration/broker/cli-spawn.test.ts index 74c984c17..f0b500960 100644 --- a/tests/integration/broker/cli-spawn.test.ts +++ b/tests/integration/broker/cli-spawn.test.ts @@ -480,6 +480,36 @@ test('cli-spawn: duplicate name — second spawn with same name fails', { timeou } }); +test( + 'cli-spawn: missing CLI fails before success and leaves no listed agent', + { timeout: 30_000 }, + async (t) => { + if (skipIfMissing(t)) return; + + const harness = new BrokerHarness(); + await harness.start(); + const suffix = uniqueSuffix(); + const agentName = `missing-cli-${suffix}`; + const missingCli = `agent-relay-missing-${suffix}`; + + try { + await assert.rejects( + () => harness.spawnAgent(agentName, missingCli, ['general']), + /process exited during startup/, + 'a wrapper that cannot launch its CLI must reject the spawn request' + ); + + const agents = await harness.listAgents(); + assert.ok( + !agents.some((agent) => agent.name === agentName), + 'a rejected startup must not leave a stale agent in the broker list' + ); + } finally { + await harness.stop(); + } + } +); + // ── Cat Process Tests (lightweight, no real CLI needed) ──────────────────── test('cli-spawn: cat — spawn lightweight process and deliver', { timeout: 30_000 }, async (t) => {