fix(broker): spawn admission fails closed on agent-name collision - #1438
fix(broker): spawn admission fails closed on agent-name collision#1438kjgbot wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAgent registration now uses shared identity-aware admission in strict and non-strict modes. Same-name collisions fail unless the supplied identity matches stored hashed metadata. Matching identities reclaim the incumbent with token rotation. Broker startup derives identity from configuration or persisted state. ChangesAgent registration admission
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant connect_relay
participant Relaycast
participant Admission
participant IncumbentAgent
connect_relay->>Relaycast: explicit or stable identity key
Relaycast->>Admission: startup_session_set_with_identity
Admission->>IncumbentAgent: fetch same-name registration
IncumbentAgent-->>Admission: hashed identity metadata
Admission->>IncumbentAgent: rotate token when identity matches
Admission-->>Relaycast: admitted or rejected session
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
crates/broker/src/relaycast/auth.rs (4)
1349-1353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the full error chain for the token, not only the top-level message.
to_string()renders only the outermostanyhowcontext. A token that leaks through a wrapped source error would not fail this assertion. Use the alternate format so the whole chain is checked.♻️ Proposed assertion change
- let message = result.unwrap_err().to_string(); + let message = format!("{:#}", result.unwrap_err());🤖 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/relaycast/auth.rs` around lines 1349 - 1353, Update the assertion around result.unwrap_err() in the token rejection test to format the full anyhow error chain using alternate formatting instead of to_string(). Continue asserting that the complete rendered chain does not contain "at_live_rotated".
1460-1470: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that no token rotation is attempted in the non-strict test.
The test asserts only
result.is_err(). Norotate-tokenmock is registered, so an unexpected rotation attempt would return a mock-server 404 and the test would still pass. Register a rotate mock and assert zero hits, as the strict test does at Line 1356.♻️ Proposed mock and assertion
+ let rotate = server.mock(|when, then| { + when.method(POST) + .path("/v1/agents/lead/rotate-token") + .header("authorization", "Bearer rk_live_cached"); + then.status(200) + .header("content-type", "application/json") + .body(r#"{"ok":true,"data":{"name":"lead","token":"at_live_rotated"}}"#); + }); + let client = AuthClient::new(Some(server.base_url())); let result = client.startup_session(Some("lead")).await; assert!( result.is_err(), "non-strict registration must also reject an unproven name collision, not mint a silent -suffix sibling" ); workspace.assert_hits(1); conflict.assert_hits(1); get_existing.assert_hits(1); + rotate.assert_hits(0);🤖 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/relaycast/auth.rs` around lines 1460 - 1470, Update the non-strict registration test around AuthClient::startup_session to register the rotate-token mock endpoint, then assert that its hit count remains zero alongside the existing request-count assertions. Match the strict test’s rotation-mock setup without changing the expected error behavior.
1363-1421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a present but different identity key.
The suite now covers an absent identity key and a matching identity key. It does not cover the case where the caller supplies a non-empty
RELAY_AGENT_IDENTITY_KEYthat differs from the storedmetadata.identity_key. That case is the main bypass shape the gate exists to stop. Add a test that sets a wrong key and asserts rejection plusrotate.assert_hits(0).🤖 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/relaycast/auth.rs` around lines 1363 - 1421, Add a test alongside strict_name_conflict_with_matching_identity_reclaims_existing_agent that sets a non-empty RELAY_AGENT_IDENTITY_KEY differing from the existing agent’s metadata.identity_key, exercises startup_session_with_options, and asserts the request is rejected. Reuse the conflict and existing-agent mocks as appropriate, add a rotate-token mock, and assert rotate.assert_hits(0).
906-931: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle a missing incumbent on the collision path.
Line 909 propagates any
get_agenterror, including a 404. If the incumbent agent is deleted between the 409 response and theget_agentcall, registration fails with a "not found" error even though the name is now free. Map that case back to a single re-attempt ofregister_agent, or return the identity-mismatch error so the failure reason stays stable.♻️ Proposed handling for a vanished incumbent
- let existing = relay.get_agent(name).await.map_err(relay_error_to_anyhow)?; + let existing = match relay.get_agent(name).await { + Ok(existing) => existing, + Err(error) if is_not_found(&relay_error_to_anyhow(error_ref(&error))) => { + // The incumbent disappeared between the conflict and the + // lookup; the name is free again, so retry once. + let result = relay + .register_agent(retry_request) + .await + .map_err(relay_error_to_anyhow)?; + return Ok((result.id, result.name, result.token, result.workspace_id)); + } + Err(error) => return Err(relay_error_to_anyhow(error)), + };The retry needs the request rebuilt, so extract the
CreateAgentRequestconstruction into a small closure and call it in both places.🤖 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/relaycast/auth.rs` around lines 906 - 931, Handle a 404 from get_agent in the collision branch of the registration flow instead of propagating it: rebuild the CreateAgentRequest via a small reusable closure and perform exactly one register_agent retry, or return the existing identity-mismatch error to preserve a stable failure reason. Use the closure for both the initial registration and retry while leaving non-404 relay errors unchanged.CHANGELOG.md (1)
8-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm the release level for this behavior change.
The heading is
[Unreleased - Minor]. This change removes two previously working outcomes: the strict path no longer returns the incumbent's credentials, and the non-strict path no longer creates a-suffixsibling agent. Any deployment that relied on either outcome now fails registration until it setsRELAY_AGENT_IDENTITY_KEY. Under SemVer that is a breaking change, which suggests[Unreleased - Major]. If the project treats fail-closed security corrections as Minor, keep the current level.The bullet text itself is accurate and impact-first, and it correctly stays as one bullet for one user-visible change.
As per coding guidelines: "set the appropriate monotonic release level for pending user-visible changes".
🤖 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 `@CHANGELOG.md` around lines 8 - 20, Update the unreleased changelog heading to the appropriate monotonic release level for the registration behavior change, using Major if breaking changes are classified under SemVer. Keep the existing security bullet unchanged; only adjust the release-level designation unless the project explicitly treats fail-closed security corrections as Minor.Sources: Coding guidelines, Learnings
🤖 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 `@CHANGELOG.md`:
- Around line 8-20: Update the unreleased changelog heading to the appropriate
monotonic release level for the registration behavior change, using Major if
breaking changes are classified under SemVer. Keep the existing security bullet
unchanged; only adjust the release-level designation unless the project
explicitly treats fail-closed security corrections as Minor.
In `@crates/broker/src/relaycast/auth.rs`:
- Around line 1349-1353: Update the assertion around result.unwrap_err() in the
token rejection test to format the full anyhow error chain using alternate
formatting instead of to_string(). Continue asserting that the complete rendered
chain does not contain "at_live_rotated".
- Around line 1460-1470: Update the non-strict registration test around
AuthClient::startup_session to register the rotate-token mock endpoint, then
assert that its hit count remains zero alongside the existing request-count
assertions. Match the strict test’s rotation-mock setup without changing the
expected error behavior.
- Around line 1363-1421: Add a test alongside
strict_name_conflict_with_matching_identity_reclaims_existing_agent that sets a
non-empty RELAY_AGENT_IDENTITY_KEY differing from the existing agent’s
metadata.identity_key, exercises startup_session_with_options, and asserts the
request is rejected. Reuse the conflict and existing-agent mocks as appropriate,
add a rotate-token mock, and assert rotate.assert_hits(0).
- Around line 906-931: Handle a 404 from get_agent in the collision branch of
the registration flow instead of propagating it: rebuild the CreateAgentRequest
via a small reusable closure and perform exactly one register_agent retry, or
return the existing identity-mismatch error to preserve a stable failure reason.
Use the closure for both the initial registration and retry while leaving
non-404 relay errors unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6769008d-4340-4719-85b4-20e4a321f59e
📒 Files selected for processing (2)
CHANGELOG.mdcrates/broker/src/relaycast/auth.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/broker/src/relaycast/auth.rs">
<violation number="1" location="crates/broker/src/relaycast/auth.rs:934">
P1: If the verified agent is deleted and another agent takes the same name between these requests, reclaim rotates and returns the replacement agent's token even though its identity was never checked. An atomic compare-and-rotate operation keyed to the verified agent ID/identity would preserve the admission decision across this race.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| let token_response = relay | ||
| .rotate_agent_token(&existing.name) |
There was a problem hiding this comment.
P1: If the verified agent is deleted and another agent takes the same name between these requests, reclaim rotates and returns the replacement agent's token even though its identity was never checked. An atomic compare-and-rotate operation keyed to the verified agent ID/identity would preserve the admission decision across this race.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/relaycast/auth.rs, line 934:
<comment>If the verified agent is deleted and another agent takes the same name between these requests, reclaim rotates and returns the replacement agent's token even though its identity was never checked. An atomic compare-and-rotate operation keyed to the verified agent ID/identity would preserve the admission decision across this race.</comment>
<file context>
@@ -899,6 +844,123 @@ fn is_conflict_code(code: &str) -> bool {
+ }
+
+ let token_response = relay
+ .rotate_agent_token(&existing.name)
+ .await
+ .map_err(relay_error_to_anyhow)?;
</file context>
…g over the incumbent's token
register_agent_with_workspace_key's strict-name branch reclaimed any name
collision via register_or_get_agent, returning the existing agent's id,
name, AND bearer token to whoever asked for that name — identity handover,
not admission control. The non-strict branch diverged instead of agreeing:
a silent one-shot `-{uuid8}` suffix retry, unreachable in production since
every live agent runs RELAY_STRICT_AGENT_NAME=1. Cloud keys Relayfile
tokens by agent name, so a collision shared credential scope; a silent
suffix would have produced a duplicate-work sibling agent under a
near-identical name, the AR-448 class this closes.
Both paths now route through one admit_agent_registration decision that
fails closed on a bare name collision. Reclaim (rotate and return the
existing agent's token, keeping crash-recovery resume working) is only
granted when the registering caller proves the same work-unit identity via
RELAY_AGENT_IDENTITY_KEY matching the identity_key stamped on the existing
agent's metadata at its own creation — never by the name string alone.
Red-first: the new
strict_name_conflict_without_identity_proof_is_rejected_not_handed_incumbent_token
test fails against the pre-fix code (asserts Err; pre-fix returns Ok with
the incumbent's rotated token) and passes after. Companion tests cover
identity-matched reclaim and non-strict/strict agreement; the two prior
tests that encoded the vulnerable behavior as correct are removed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…he identity proof - The spawn-admission fail-closed gate had no path for a fleet node's own legitimate restart: nothing set RELAY_AGENT_IDENTITY_KEY for a node's own startup registration, so a node killed and restarted before its stale registration was reaped collided on its own name and was rejected outright, never coming back online (the "Two-node fleet matrix" restart-reconcile flake). connect_relay now derives a stable identity from the broker's own persisted state directory (falling back to an explicit RELAY_AGENT_IDENTITY_KEY override) and passes it through a new startup_session_set_with_identity, so a restart of the same node reclaims its name while a different node colliding on that name is still rejected. - The identity proof stamped on an agent's metadata was stored raw, but metadata is readable by any caller holding the same workspace key — so a co-tenant could read another work unit's identity key and replay it to reclaim its credentials, defeating the admission gate. It's now stored and compared as a one-way SHA-256 hash. Both new tests are proven against the pre-fix code: without the identity threading, node_restart_reclaims_its_own_prior_registration_via_stable_identity fails to compile (no such method); reverting the hash-storage change makes strict_name_conflict_with_matching_identity_reclaims_existing_agent fail on the mock's exact-body match. cargo test -p agent-relay-broker --lib: 855 passed, 0 failed, 4 ignored. cargo fmt -p agent-relay-broker -- --check: clean. cargo clippy -p agent-relay-broker --lib --tests -- -D warnings: no new warnings (3 pre-existing errors in unrelated files, unchanged by this PR, per the PR's own test plan). Note on the two remaining cubic P1 findings (auth.rs:917, auth.rs:934): - auth.rs:917 (raw identity key readable via shared workspace key): fixed above via hashing. - auth.rs:934 (rotate_agent_token(&existing.name) is name-keyed, not ID-keyed, so a delete+re-register race between get_agent and rotate could hand a token for an unverified agent): valid, but not fixable from this repo — the relaycast SDK's rotate-token/get-agent endpoints are fundamentally name-keyed (TokenRotateResponse has no id field), so closing this race needs a new ID-scoped or compare-and-swap primitive in the relaycast crate/server, a separate repository. Left as-is rather than ship a cosmetic mitigation that doesn't actually close the window. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
0d09a6f to
5c2ad8e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Summary
crates/broker/src/relaycast/auth.rs'sregister_agent_with_workspace_keyhad two divergent, both-wrong ways of handling a name collision at agent registration:RELAY_STRICT_AGENT_NAME=1): calledregister_or_get_agent, which on collision fetches the existing agent and rotates + returns its id, name, and bearer token to whoever asked for that name. Identity handover, not admission control. Cloud keys Relayfile tokens by agent name, so a collision shared credential scope.-{uuid8}suffix and retried once, then hard-failed. Never exercised live, and a silent suffix is its own problem — a second agent quietly doing duplicate work under a near-identical name is exactly the AR-448 duplicate-agent class this gate exists to stop.Fix: both paths now route through a single
admit_agent_registrationdecision.RELAY_AGENT_IDENTITY_KEYto a value stable across that work unit's restarts. It's stamped into the agent'smetadata.identity_keyat creation; a later collision only reclaims (rotates + returns the existing token) if the presented key matches what's stored on the existing agent — never by the name string alone.strict_nameparameter is retained only for the caller's logging/config plumbing inruntime/session.rsand no longer selects a different collision strategy.Scope: touched only
crates/broker/src/relaycast/auth.rs(+ its tests) andCHANGELOG.md, per the assignment's boundary. Depends conceptually on #1436 (TS MCP metadata pass-through) for any future caller that wants to thread an identity key through the CLI spawn path — not touched here, and not required for this fix, sinceRELAY_AGENT_IDENTITY_KEYis read directly from the broker process environment.Red-first test
strict_name_conflict_without_identity_proof_is_rejected_not_handed_incumbent_tokenis the primary regression test.RED (before the fix, i.e. with the vulnerable
register_or_get_agent-always-reclaims / silent-suffix code paths in place):GREEN (after the fix):
Companion tests:
strict_name_conflict_with_matching_identity_reclaims_existing_agent— proves reclaim still works (requirement: crash-recovery resume must not break).non_strict_name_conflict_without_identity_proof_is_rejected— proves strict and non-strict now agree.The two prior tests that encoded the vulnerable behavior as correct (
strict_name_conflict_reclaims_via_sdk_register_or_get_agent,default_name_conflict_retries_with_suffix_once) were removed; their scenarios are superseded by the tests above.Test plan
cargo test -p agent-relay-broker --lib relaycast::auth::tests::— 23 passed, 0 failedcargo test -p agent-relay-broker --lib(full crate) — 852 passed, 0 failed, 4 ignoredcargo fmt -p agent-relay-broker -- --check— cleancargo clippy -p agent-relay-broker --lib --tests -- -D warnings— no new warnings inauth.rs(3 pre-existing clippy errors in unrelated files onorigin/main, unchanged by this PR)🤖 Generated with Claude Code