Skip to content

fix(broker): verify worker process before spawn success - #1428

Closed
khaliqgant wants to merge 1 commit into
fix/workspace-restore-rebindfrom
fix/node-up-enrollment-precedence-and-spawn-reliability
Closed

fix(broker): verify worker process before spawn success#1428
khaliqgant wants to merge 1 commit into
fix/workspace-restore-rebindfrom
fix/node-up-enrollment-precedence-and-spawn-reliability

Conversation

@khaliqgant

@khaliqgantkhaliqgant commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

  • hold the broker spawn acknowledgement through a short process-stability window
  • return the wrapper's exit status and worker-log path when it dies during startup
  • remove the rejected worker from the registry so node agent list cannot show a stale entry
  • add unit and real broker integration coverage for failed and successful startup confirmation

This is a stacked draft on #1425. That base PR supplies the requested Bug A workspace-resolution fix and its regression coverage: explicit flag/environment selection wins the shared workspace ladder, while conflicting persisted enrollment state is surfaced rather than silently re-homing the node. This commit supplies Bug B.

Reproduction evidence

Before this change, spawning a nonexistent provider printed:

Spawned repro-missing (definitely-not-a-cli, pty).

The immediate agent list was empty, while the worker log contained failed to spawn wrapped command.

After this change, the same real broker/CLI path exits non-zero with:

agent 'repro-missing-fixed' process exited during startup (exit status: 1); see worker log .../repro-missing-fixed.log

The immediate agent list remains empty. A cat control spawn still reports success, appears in the agent list, and releases normally.

Validation

  • env -u AGENT_RELAY_MCP_COMMAND cargo test -p agent-relay-broker --quiet — 853 passed, 4 ignored; integration crates also passed
  • cargo clippy -p agent-relay-broker --lib --bins -- -D warnings
  • cargo fmt --all -- --check
  • targeted broker integration — missing provider rejected and absent from list
  • workspace-resolution regression files — 6 files, 102 tests passed
  • npm run typecheck
  • npm run lint — no errors (existing warnings only)
  • npm run format:check
  • npm test — 1,733 passed; the two existing relayfile stale-daemon tests failed locally, matching the known base-branch issue

Stack

Review in cubic

@coderabbitai

coderabbitaiBot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3a7b734c-7db6-4b93-bfcb-7943c1ffff83

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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)
@khaliqgant
khaliqgantforce-pushed the fix/node-up-enrollment-precedence-and-spawn-reliability branch from b62607f to 5d691f2CompareAugust 4, 2026 20:58
@khaliqgant

Copy link
Copy Markdown
MemberAuthor

Closing as a duplicate of #1429.

Both PRs point at the same branch fix/node-up-enrollment-precedence-and-spawn-reliability at the same head 5d691f212, opened 38 minutes apart on 2026-08-04. Neither has any review — the only comment on each is CodeRabbit's identical "Review skipped — Draft detected", so nothing is lost by consolidating.

Keeping #1429 because its description states the user-facing symptom directly (the CLI reporting Spawned X success while the spawn actually failed) rather than only the mechanism.

Worth flagging on the way past: both were left as drafts, so no bot has ever reviewed this branch, and it has sat untouched for two days. That fix is more valuable than it looks — the exact failure it describes cost real diagnosis time overnight on 2026-08-05, when a spawn onto finn-mini reported success and the PTY died instantly because the launchd PATH omitted the directory holding claude. With this merged, that would have surfaced as an immediate error instead of a healthy-looking agent that was never alive.

Also note the branch name mentions enrollment precedence, but the diff is spawn-acknowledgement only — it does not address #1432 (node up silently ignoring a fleet enrollment when a project workspace pin lacks enrolledNodeId). That issue still has no fix in flight.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@khaliqgant