fix(broker): restore live agent roster after reconnect - #1494
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
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 (1)
📝 WalkthroughWalkthroughWorker spawn flows now retain registration metadata, resolve supplied-token identities, and record successful workers in fleet reconnect inventory. Inventory updates preserve existing entries and await control-channel delivery. The changelog records roster recovery after node-control reconnects. ChangesFleet inventory recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FleetSpawn
participant SpawnRequest
participant Relaycast
participant WorkerSpawn
participant FleetInventory
participant FleetControl
FleetSpawn->>SpawnRequest: pass mutable fleet inventory
SpawnRequest->>Relaycast: resolve registration identity
Relaycast-->>SpawnRequest: return agent and session metadata
SpawnRequest->>WorkerSpawn: start worker
WorkerSpawn-->>SpawnRequest: report successful spawn
SpawnRequest->>FleetInventory: record agent metadata
FleetInventory->>FleetControl: publish updated snapshot
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0eb4abd1da
ℹ️ 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".
| if let Some((token, invocation_id, session_ref)) = fleet_registration.take() | ||
| { | ||
| super::fleet::record_fleet_inventory_agent( |
There was a problem hiding this comment.
When a successful HTTP spawn uses the caller-supplied agent_token path—or falls back to HTTP registration and node binding—fleet_registration remains None, so this new inventory code never records the live, node-bound worker. After node control disconnects, Relaycast marks that worker offline, and the subsequent inventory.sync still omits it, reproducing the offline-roster failure for these supported spawn paths. The equivalent supplied-token branch in spawn_worker_from_request has the same omission.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6e4981b. Supplied and HTTP-fallback tokens are now resolved through the authenticated current-agent endpoint to recover the authoritative agent ID; the requested name must match before the identity enters delivery bookkeeping or reconnect inventory. Both HTTP and action.invoke spawn paths retain the resolved registration only after process launch succeeds. Added matching-identity and mismatched-name regression tests; full broker lib suite is green (915 passed, 4 ignored).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/broker/src/runtime/api.rs (1)
350-354: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve fleet registration metadata for supplied tokens.
If a supplied token can represent a node-control registration, preserve its
AgentRegistrationTokenidentity and invocation/session metadata before spawning. Both supplied-token branches leavefleet_registrationasNone, sorecord_fleet_inventory_agentnever updatesfleet_inventory; reconnect cannot restore that worker throughinventory.sync.Apply this to
crates/broker/src/runtime/api.rsandcrates/broker/src/runtime/relaycast_events.rs. Otherwise, enforce that these inputs are never fleet registrations.🤖 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/runtime/api.rs` around lines 350 - 354, Preserve fleet registration metadata for supplied tokens in crates/broker/src/runtime/api.rs:350-354 and crates/broker/src/runtime/relaycast_events.rs:492-499 by recognizing node-control AgentRegistrationToken values and populating fleet_registration with their identity, invocation, and session metadata before spawning; ensure record_fleet_inventory_agent can update fleet_inventory for reconnect via inventory.sync, or explicitly enforce that these supplied-token inputs cannot be fleet registrations.
🤖 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.
Outside diff comments:
In `@crates/broker/src/runtime/api.rs`:
- Around line 350-354: Preserve fleet registration metadata for supplied tokens
in crates/broker/src/runtime/api.rs:350-354 and
crates/broker/src/runtime/relaycast_events.rs:492-499 by recognizing
node-control AgentRegistrationToken values and populating fleet_registration
with their identity, invocation, and session metadata before spawning; ensure
record_fleet_inventory_agent can update fleet_inventory for reconnect via
inventory.sync, or explicitly enforce that these supplied-token inputs cannot be
fleet registrations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f03335c-0408-4df5-bc62-8758bce058dc
📒 Files selected for processing (4)
CHANGELOG.mdcrates/broker/src/runtime/api.rscrates/broker/src/runtime/fleet.rscrates/broker/src/runtime/relaycast_events.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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/runtime/api.rs">
<violation number="1" location="crates/broker/src/runtime/api.rs:363">
P2: A failed HTTP spawn can leave a dead worker's authoritative identity in `FleetDeliveryBook` even though no reconnect inventory entry was created. Deferring the identity binding until launch succeeds, or explicitly rolling it back on the spawn error path, would prevent later delivery and release operations from targeting stale state.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| .await | ||
| { | ||
| Ok(registration) => { | ||
| fleet_registration = Some((registration, None, session_ref.clone())); |
There was a problem hiding this comment.
P2: A failed HTTP spawn can leave a dead worker's authoritative identity in FleetDeliveryBook even though no reconnect inventory entry was created. Deferring the identity binding until launch succeeds, or explicitly rolling it back on the spawn error path, would prevent later delivery and release operations from targeting stale state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/runtime/api.rs, line 363:
<comment>A failed HTTP spawn can leave a dead worker's authoritative identity in `FleetDeliveryBook` even though no reconnect inventory entry was created. Deferring the identity binding until launch succeeds, or explicitly rolling it back on the spawn error path, would prevent later delivery and release operations from targeting stale state.</comment>
<file context>
@@ -348,16 +348,35 @@ impl BrokerRuntime {
+ .await
+ {
+ Ok(registration) => {
+ fleet_registration = Some((registration, None, session_ref.clone()));
+ }
+ Err(error) => {
</file context>
1 participant
Summary
Root cause
Relaycast deliberately marks all agents attached to a node provider offline when its node-control socket disconnects. The broker reconnect path can restore those agents through inventory.sync, but production fleet_inventory started empty and successful agent.register calls only populated FleetDeliveryBook. The node therefore reconnected with an empty inventory while its local worker processes remained alive. Node heartbeat and activeAgents could be healthy at the same time that individual roster rows remained offline.
This records inventory only after process launch succeeds, so a failed spawn is not advertised as live. Existing release and failed-spawn cleanup paths already prune the same inventory. Opaque pre-minted tokens are resolved through the authenticated current-agent endpoint; a token for a different name is rejected from reconnect inventory. Inventory publication now awaits bounded-channel capacity instead of dropping the only updated snapshot.
Refs #1458. This is complementary to #1462: that PR detects blackholed node-control sockets; this PR makes any eventual reconnect restore individual agent state.
Test Plan
Note: clippy with --tests reaches two pre-existing unrelated warnings in crates/broker/src/snippets.rs and crates/broker/src/runtime/worker_events.rs. The non-test library target is clean.
Screenshots
Not applicable.