Fix silent worker spawn and strict identity failures#1342
Fix silent worker spawn and strict identity failures#1342miyaontherelay wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughWorker startup now requires a readiness handshake, strict agent identities use conflict-safe registration, and dead-letter moves emit structured warnings. Tests cover failed startup, strict registration recovery and conflicts, and bootstrap behavior. ChangesRuntime safety and observability
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a worker readiness handshake during startup to prevent false success reports, switches strict worker registration to a conflict-safe atomic registration to avoid token rotation conflicts, and adds structured warning logs for dead-lettered messages. The feedback suggests optimizing the worker executable path resolution in crates/broker/src/worker.rs by using lazy evaluation instead of eager evaluation with unwrap_or to avoid unnecessary system calls.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let worker_program = self.worker_program_override.clone(); | ||
| let mut command = Command::new( | ||
| worker_program | ||
| .unwrap_or(std::env::current_exe().context("failed to locate current executable")?), | ||
| ); |
There was a problem hiding this comment.
Using unwrap_or eagerly evaluates its argument, which means std::env::current_exe() (a system call) is always executed and its potential errors are propagated even when self.worker_program_override is Some. Using a match expression avoids this eager evaluation, making it more efficient and robust, especially in tests where an override is provided.
let mut command = Command::new(match &self.worker_program_override {
Some(path) => path.clone(),
None => std::env::current_exe().context("failed to locate current executable")?,
});![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let startup_result = match timeout(WORKER_STARTUP_READY_TIMEOUT, startup_ready_rx).await { | ||
| Ok(Ok(Ok(()))) => Ok(()), | ||
| Ok(Ok(Err(error))) => Err(anyhow::anyhow!(error)), | ||
| Ok(Err(_)) => Err(anyhow::anyhow!( | ||
| "worker '{}' closed its startup stream before worker_ready", | ||
| spec.name | ||
| )), | ||
| Err(_) => Err(anyhow::anyhow!( | ||
| "worker '{}' did not emit worker_ready within {} seconds", | ||
| spec.name, | ||
| WORKER_STARTUP_READY_TIMEOUT.as_secs() | ||
| )), | ||
| }; | ||
| if let Err(error) = startup_result { | ||
| self.remove_failed_startup(&spec.name).await; | ||
| return Err(error).context("worker failed startup readiness handshake"); | ||
| } |
There was a problem hiding this comment.
🔴 Broker stops handling all work while waiting for a new agent to start, and can kill a healthy agent
The broker now waits inline for a newly launched agent's readiness signal (timeout(WORKER_STARTUP_READY_TIMEOUT, startup_ready_rx) at crates/broker/src/worker.rs:883) inside its single event loop, so while any agent is starting the broker processes nothing else and stops reading agents' output; that output backlog can fill and block the very readiness message being waited for, so a healthy agent is torn down as failed.
Impact: Every agent launch freezes the whole broker for up to 30 seconds, and under concurrent output a starting agent that is actually fine can be killed and reported as a spawn failure.
Event-loop starvation and shared-channel backpressure deadlock during the readiness handshake
The runtime is a single-task tokio::select! loop (crates/broker/src/runtime/event_loop.rs:212-227) that awaits spawn handlers inline (handle_fleet_action_spawn → spawn_worker_from_request at crates/broker/src/runtime/relaycast_events.rs:481 → WorkerRegistry::spawn). The new readiness wait at crates/broker/src/worker.rs:883 blocks that loop for up to WORKER_STARTUP_READY_TIMEOUT (30s), during which worker_event_rx (capacity 1024, created at crates/broker/src/runtime/init.rs:505) is never drained.
All worker stdout/stderr reader tasks share the single cloned event_tx. The readiness oneshot is sent before tx.send(...) only for the worker_ready/worker_exited line itself; every line emitted before worker_ready still goes through tx.send(WorkerEvent::Message {...}).await at crates/broker/src/worker.rs:1658. If the shared channel fills — from this worker's own pre-ready worker_stream output or from other already-running chatty workers whose readers keep producing while the loop is blocked — the new worker's reader blocks on that tx.send().await and can never advance to the worker_ready line (crates/broker/src/worker.rs:1623-1636). The oneshot never fires, the 30s timeout elapses, and remove_failed_startup (crates/broker/src/worker.rs:135-147) kills and unregisters a healthy worker, returning spawn_failed.
This backpressure interaction is new to this change: prior inline blocking during spawn (e.g. Codex model detection) happened before the child/readers existed, so no reader was producing into the shared channel while the loop was blocked.
Prompt for agents
The readiness handshake at crates/broker/src/worker.rs:883 is awaited inline inside the single-task broker event loop (crates/broker/src/runtime/event_loop.rs run() -> handle_fleet_action_spawn -> spawn_worker_from_request -> WorkerRegistry::spawn). While it waits (up to 30s), the loop does not drain worker_event_rx (mpsc capacity 1024, init.rs:505), which is shared by every worker reader task via cloned event_tx. Worker reader tasks forward each pre-worker_ready output line through tx.send(...).await (worker.rs:1658). If that shared channel fills during the wait (from the new worker's own pre-ready worker_stream output, or from other running workers' output), the new worker's reader blocks before it can reach and detect the worker_ready line, the readiness oneshot never fires, the 30s timeout expires, and remove_failed_startup kills a healthy worker and returns spawn_failed. Additionally, the whole broker (API, deliveries, relaycast, other workers' event processing) is frozen for the entire handshake. Consider decoupling the readiness wait from the event loop so worker events keep being drained during startup (for example: keep processing worker_event_rx while a spawn is pending, drive the readiness handshake from a task that does not hold the event loop, or ensure the readiness signal cannot be starved by shared-channel backpressure — e.g. detect readiness on a path independent of the shared bounded WorkerEvent channel).
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/broker/src/worker.rs (2)
1622-1657: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant re-fetch of the JSON
typefield.
msg_typeis already computed at line 1622 but line 1637-1640 recomputesvalue.get("type").and_then(Value::as_str)instead of reusing it.♻️ Reuse the already-computed `msg_type`
- if value - .get("type") - .and_then(Value::as_str) - .is_some_and(|msg_type| msg_type == "worker_stream") - { + if msg_type.is_some_and(|msg_type| msg_type == "worker_stream") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/broker/src/worker.rs` around lines 1622 - 1657, Reuse the existing msg_type value in the worker_stream check instead of fetching and parsing value["type"] again. Update the condition following the startup readiness match while preserving the current worker_stream payload handling and append_log_chunk behavior.
326-330: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
unwrap_oreagerly evaluatescurrent_exe()even when an override is set.
Option::unwrap_orevaluates its argument unconditionally (unlikeunwrap_or_else), sostd::env::current_exe().context(...)?runs — and can fail/short-circuitspawn()— even whenworker_program_overrideisSome. In practice this only affects the#[cfg(test)]override path, but it's a real logic smell that could break the override's intent ifcurrent_exe()ever fails in a constrained test/sandbox environment.♻️ Proposed fix using lazy evaluation
- let worker_program = self.worker_program_override.clone(); - let mut command = Command::new( - worker_program - .unwrap_or(std::env::current_exe().context("failed to locate current executable")?), - ); + let worker_program = self.worker_program_override.clone(); + let program = match worker_program { + Some(program) => program, + None => std::env::current_exe().context("failed to locate current executable")?, + }; + let mut command = Command::new(program);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/broker/src/worker.rs` around lines 326 - 330, Update the worker command construction in the relevant spawn flow to use lazy fallback evaluation, replacing the eager Option fallback around worker_program_override with the corresponding deferred variant. Preserve the existing current_exe lookup, context, and error propagation when no override is configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/broker/src/worker.rs`:
- Around line 1622-1657: Reuse the existing msg_type value in the worker_stream
check instead of fetching and parsing value["type"] again. Update the condition
following the startup readiness match while preserving the current worker_stream
payload handling and append_log_chunk behavior.
- Around line 326-330: Update the worker command construction in the relevant
spawn flow to use lazy fallback evaluation, replacing the eager Option fallback
around worker_program_override with the corresponding deferred variant. Preserve
the existing current_exe lookup, context, and error propagation when no override
is configured.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f30d72e7-9c26-4300-9f21-08378666719a
📒 Files selected for processing (7)
CHANGELOG.mdcrates/broker/src/runtime/dead_letter.rscrates/broker/src/worker.rspackages/cli/src/cli/agent-relay-mcp.startup.test.tspackages/cli/src/cli/agent-relay-mcp.test.tspackages/cli/src/cli/agent-relay-mcp.tspackages/sdk/src/messaging/thin-client.ts
|
Additional production evidence for the strict-identity registration fix: at approximately 09:47 UTC on 2026-07-19, a live NightCTO lead session failed mid- |
|
CI diagnosis, from outside this repo — the failing check is not what the review-risk note predicted. Posting because the PR notes flagged "the 30s readiness deadline / cold-cache PTY behavior" as the review risk, and that turns out not to be what the E2E is failing on. Anyone reaching for a wider deadline would get a green test that fixed nothing. Actual failing assertions (run 29679806461, step 10 No The same job passes on main: Verified as the same job, not a similarly-named one. What that establishes, and what it doesn't. Green on main makes "this PR introduced it" the leading explanation and moves the PR from blocked by flaky CI to blocked by its own regression. It does not prove causation — that is one main run against one PR run, and an intermittent flake produces the same picture. Two runs is not a rate. Cheapest way to settle it: re-run the failing job. Green on re-run → flake. Red again → regression confirmed. One click, and it resolves what a single-run comparison can only suggest. Separately, the production evidence for the bugs this PR fixes has grown and is worsening:
To be explicit, because these two halves get conflated: the severity of the underlying bugs does not make a failing fix mergeable. The evidence argues for someone picking this up, not for merging it as-is. Not my repo. No changes made, nothing pushed, no merge attempted — diagnosis only, so whoever owns this does not have to re-derive it. |
2 participants
Summary
Fix three related silent-agent-failure paths:
worker_readyhandshake; an immediately exiting CLI now fails the action and is removed instead of being reported asspawned: true.dead_letter_addedbroker event so persisted undeliverable messages are visible in operational logs.RELAY_STRICT_AGENT_NAME=1, use atomic plain registration rather than token-rotating registration, so a second session gets an explicit conflict instead of invalidating the first session.The commits are intentionally separable: readiness, dead-letter observability, strict-identity registration, then changelog.
Concrete evidence
Bug 1 — false spawn success
Pre-fix path on
main:crates/broker/src/worker.rs:798launched the process,:834inserted it into the registry, and:852returnedOk(spec)without a readiness acknowledgement.crates/broker/src/runtime/fleet.rs:447-454then emitted success solely because the registry contained the name.nightcto-sfreproduction completed:{"output":{"name":"ncto-probe","spawned":true},"status":"completed"}.codexdeliberately replaced by a command that exits 64 before readiness. The spawned action reached the terminal result{"status":"failed","error":"spawn_failed"}(the relay also deliveredspawn_failed). Nospawned:truecompletion was emitted.The regression test executes
falseafter receivinginit_worker; before the readiness gate it failed because spawn returnedOk, and it now rejects and leaves no registered worker.Bug 3 — strict identity token ping-pong
Before this change, strict-name rebind/bootstrap unconditionally called
registerOrRotateatpackages/cli/src/cli/agent-relay-mcp.ts:360and:916-921. The SDK describes that operation as adopting an existing identity and invalidating the old token atpackages/sdk/src/facade.ts:189-193. Two sessions with the same strict name therefore alternated invalidating one another. This was a live operational issue: this workspace'sleadidentity was invalidated four times in one session by another live session sharing the name.Strict rebind/bootstrap now calls atomic
agents.register; an existing live identity produces a server conflict and leaves its token usable. Non-strict callers retain rotation behavior.Bug 2 — silent dead letters
dead_letter_pending_deliverynow writes a structured warning containingworker,delivery_id,event_id,attempts, andreasonimmediately before the existingdead_letter_addedevent.Verification
cargo fmt --checkcargo test -p agent-relay-broker— 783 passed, 4 ignoredcargo build --release --bin agent-relay-brokernpm run typechecknpx vitest run packages/cli/src/cli/agent-relay-mcp.test.ts packages/cli/src/cli/agent-relay-mcp.startup.test.ts— 28 passedRisk / review focus
The 30-second outer
worker_readydeadline is the riskiest change and deserves scrutiny. It deliberately fails closed: everyWorkerRegistry::spawncaller, including fleet placement, now waits for readiness. PTY workers emit readiness after their existing boot/prompt detection, with their existing 25-second fallback; the outer five seconds are transport headroom. Thus a coldnpxfetch or first-run binary can still reach the PTY fallback around 25 seconds, but a worker that cannot emit the broker frame by 30 seconds fails instead of becoming a phantom agent.Headless and app-server workers already emit
worker_readyimmediately after receivinginit_worker, so they take the same handshake path without waiting for PTY prompt detection. This is a liveness handshake, not a stronger proof that an external provider is fully authenticated; reviewers should assess whether the PTY fallback and 30-second cutoff are right for unusually slow cold-cache CLIs.