fix(broker): cover overloaded mcp-args registration - #1742
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
94efb14
into
fix/relay-1715-spawn-overload
There was a problem hiding this comment.
💡 Codex Review
relay/crates/broker/src/relaycast/ws.rs
Lines 1563 to 1564 in 94efb14
When registration collides, register_agent_token enters the takeover path, whose 401/403, agent_not_found, and other terminal API failures are converted to RelaycastRegistrationError::Transport at lines 455-459. The SDK classifier used here considers transport errors retryable, so a rejected takeover now replays the entire create/conflict/lookup/takeover sequence three times; because with_registration_attempts only restamps Api errors, the resulting spawn and mcp-args diagnostic also omits the promised total attempt count. Preserve/classify takeover API failures as fatal while retaining retries for genuine registration transport failures.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let agent_token = match tokio::time::timeout( | ||
| timeout, | ||
| client.register_agent_token(agent_name, Some(&cli_lower)), | ||
| retry_agent_registration(&client, agent_name, Some(&cli_lower)), |
There was a problem hiding this comment.
Update the exact-head diagnostic proof for registration retries
Routing mcp-args --register through this helper changes a persistent 503 from one POST with attempts: 1 to three POSTs with attempts: 3, but tests/relayflows/cases/1673-relaycast-registration-diagnostics/run.mjs lines 147-153 still accepts the head only when requestCount === 1 and stderr contains attempts: 1. Consequently that compiled exact-head RelayFlow proof throws an unexpected-observation error at this head, and the new 1715 proof exercises only /api/spawn, not the Cloud mcp-args --register call site; update or replace the exact base/head proof alongside this behavior change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 94efb14. Configure here.
Summary
Stacked on #1730 to complete issue #1715 across both registration call sites:
mcp-args --registerthrough the broker-owned bounded Relaycast registration retry helper;attempts: 3diagnostics;Exact proof
4306bb29be696ec84a4e3844f86fdc4a91584407bugspawn_503_not_retried_and_safe_fallback_missingbde1d4257d7a95484f4e3bcb86f0e3eb95124022fixedspawn_503_retried_and_safe_fallback_is_liveValidation
cargo test -p agent-relay-broker --no-default-features: 1067 passed, 4 ignored.Closes #1715.
Note
Medium Risk
Changes Relaycast agent registration behavior for
mcp-args --register(extra POST retries on typed overloads), reusing the same bounded helper as spawn rather than new ad-hoc logic.Overview
Completes overload handling for #1715 by routing
mcp-args --registerthrough the same broker-ownedretry_agent_registrationhelper used for worker spawns, instead of a singleregister_agent_tokencall.Terminal failures now surface
attempts: 3when transient503overloads exhaust the bounded backoff. Unit coverage was flipped from “no replay on ambiguous 503” to three mock hits and matching diagnostics. The Unreleased changelog entry now calls outmcp-args --registeralongside spawn retries.The 1715-spawn-overload RelayFlow base arm no longer requires an
attempts:marker, matching the pre-fix SDK omitting that field while still checking status, error code, and request id.Reviewed by Cursor Bugbot for commit 94efb14. Bugbot is set up for automated code reviews on this repo. Configure here.