fix(engine): accept a release result after the node deregistered the agent - #455
khaliqgant wants to merge 1 commit into
Conversation
…agent A relay broker completes a release by stopping the worker, queueing agent.deregister and then sending action.result on its one control channel. Since #448 routed plain releases through the guarded completion, that deregister left no active binding by the time the result arrived, so every broker-completed release failed with agent_release_generation_conflict. Accept the result when the agent was bound to the reporting node and has not been bound to any node but its own implicit direct node since. A release whose agent moved to a different node is still refused. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 34 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| AND ${agentNodeBindings.status} = 'active' | ||
| AND ${agentNodeBindings.nodeId} <> ${nodeId} | ||
| AND ${agentNodeBindings.nodeId} <> ${`node_direct_${agent.id}`} |
There was a problem hiding this comment.
🔴 Plain release strands direct-node capacity
After broker deregistration, boundOnlyHere accepts the new active direct binding for a plain release. Completion clears the agent's location but leaves that binding active and its capacity slot occupied.
Learn more
Broker deregistration re-homes the agent using ensureDirectNodeForAgent, creating an active direct binding and setting the direct node's activeAgents to one. The node-completion writes only decrement and deactivate bindings on the reporting broker, then set the agent offline with no location. As a result, the accepted plain release retains a binding and a charged direct-node slot. completeLocally releases all active bindings and their slots.
Example: Node A deregisters worker, creating an active node_direct_workerId binding. A then returns a successful plain release; worker is offline without a location, but the direct binding stays active and its node still has activeAgents = 1.
Recommended fix: In completeReleaseNodeInvocation, conditionally deactivate and decrement the implicit direct binding and node as part of the same atomic completion, provided the invocation won and no newer direct session owns the binding. Cover both plain and delete_agent releases and verify capacity and bindings in the deregister-before-result test.
Was this helpful? React with 👍 or 👎 to provide feedback.
| AND ${agentNodeBindings.status} = 'active' | ||
| AND ${agentNodeBindings.nodeId} <> ${nodeId} | ||
| AND ${agentNodeBindings.nodeId} <> ${`node_direct_${agent.id}`} |
There was a problem hiding this comment.
🔴 Broker result releases a new direct session
If an agent reconnects directly after broker deregistration, boundOnlyHere still accepts the broker's plain release result. The old result takes the new direct session offline and clears its location.
Learn more
deregisterAgentViaNode re-homes the agent to its implicit direct node, which has an active binding even while its node is offline. The direct connection can subsequently become live before the original broker reports its release result. This predicate treats that active binding exactly like the offline fallback. A plain release lacks an expected_token_hash; completion therefore changes the active direct agent to offline and nulls its location.
Example: A dispatches a plain release for worker. A deregisters worker; worker connects through node_direct_<id>. A's delayed result then completes and sets worker offline, despite its new direct connection.
Recommended fix: Distinguish a direct fallback left offline by deregistration from a subsequent active direct connection. Fence the completion against a newer direct session using an atomic agent/binding state or generation check; do not allow the reporting broker to release the newer session.
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: 140b1b5ec9
ℹ️ 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".
| AND ${agentNodeBindings.status} = 'active' | ||
| AND ${agentNodeBindings.nodeId} <> ${nodeId} | ||
| AND ${agentNodeBindings.nodeId} <> ${`node_direct_${agent.id}`} |
There was a problem hiding this comment.
If the agent opens its direct-node connection after agent.deregister but before the broker's action.result, the direct binding is genuinely live, yet this exception ignores it and accepts the stale result. The completion then overwrites the newly hosted agent's location/status (and with delete_agent, deletes its live direct node), even though the same code rejects a rebind to every other node. Check whether the implicit node is merely the offline fallback created by deregistration rather than exempting every direct binding.
Useful? React with 👍 / 👎.
| AND ${agentNodeBindings.nodeId} <> ${`node_direct_${agent.id}`} | ||
| ) | ||
| )`; | ||
| const releaseCanApply = sql`(${generationStillCurrent}) AND (${boundOnlyHere})`; |
There was a problem hiding this comment.
In the broker ordering this change is designed to accept, deregisterAgentViaNode has already called emitAgentExitedEffects(... reason: 'deregistered'); after this predicate permits completion, completeReleaseNodeInvocation calls it again with reason released. The workspace event log, observer stream, and webhook outbox therefore receive two distinct agent.exited events for the same exit (the reason is part of the delivery key, so it is not deduplicated), which can trigger downstream cleanup twice.
Useful? React with 👍 / 👎.
| // A relay broker queues `agent.deregister` ahead of the release result on | ||
| // its one control channel, so the binding this release targets is usually | ||
| // already inactive when the result lands. Accept that, as long as the agent | ||
| // was bound to this node and has not since been bound to any other node | ||
| // than its own implicit direct node (where deregistration re-homes it). |
There was a problem hiding this comment.
For the ordinary non-deleting release covered by the new test, agent.deregister creates an active implicit-direct binding and sets that direct node's activeAgents to 1. This predicate now accepts that state, but the completion batch only deactivates/decrements the dispatched broker node before setting the agent's location to null, leaving the fallback binding active and its node capacity occupied. Node-agent listings and later binding selection consequently continue to report the released agent as hosted; the accepted fallback binding and its capacity need to be retired as part of completion.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 140b1b5. Configure here.
| AND ${agentNodeBindings.nodeId} <> ${`node_direct_${agent.id}`} | ||
| ) | ||
| )`; | ||
| const releaseCanApply = sql`(${generationStillCurrent}) AND (${boundOnlyHere})`; |
There was a problem hiding this comment.
Release leaves implicit binding active
Low Severity
Accepting a release after agent.deregister leaves the implicit-direct binding that deregister just activated as active. The agent is marked offline with no location, but host discovery still treats that binding as a live placement.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 140b1b5. Configure here.
1 participant
Problem
Releasing an agent hosted by a relay broker fails with
agent_release_generation_conflict, even for a plain{ "name": … }release that carries no generation guard.A relay broker completes a release by stopping the worker, then on its single ordered control channel:
agent.deregister(relaycrates/broker/src/runtime/fleet.rs,deregister_fleet_agent)action.resultThe engine processes the deregister first, which deactivates the agent's binding on that node and re-homes it to its implicit direct node. Since #448, plain releases go through the guarded
completeReleaseNodeInvocation, which requires an active binding on the reporting node. So by the time the result arrives, the completion fails. Before #448, the legacy completion tolerated a missing binding.Found through relay's fleet e2e "resume: a resumable spawn re-binds to the agent ORIGIN node". Bisecting relaycast
mainbetween #387 (eb2fcd51, passes) and v8.12.0 (fails) points to603d6d71(#448). The test only stayed green in relay CI because relay pins a pre-#448 relaycast commit.Fix
In
completeReleaseNodeInvocation, replace the "active binding on this node" requirement with: the agent was bound to this node, and it has no active binding on any other node except its own implicit direct node, which is where deregistration re-homes it. The generation/identity guards are unchanged. A release whose agent has since been bound to a different node is still refused withagent_release_generation_conflict.The
activeAgentsdecrement and the binding deactivation stay conditioned on the binding still being active, so a deregister that already did them isn't repeated.Test plan
conformance/releaseAfterNodeDeregister.test.ts:agent.deregisterprecedesaction.result→completed, agent offline. This fails before the fix.failedwithagent_release_generation_conflict, and the agent is untouched.tests/e2e/fleet/against this engine build, 29/29 passed, including the resume scenario and the addressing e2e from test(e2e): agent@machine addressing to a Cloud-shaped sandbox node relay#1852.🤖 Generated with Claude Code
Note
Medium Risk
Touches guarded agent-release lifecycle completion in the engine; behavior is narrowed with explicit tests, but incorrect binding logic could still allow or block releases wrongly.
Overview
Fixes relay-broker agent releases that were incorrectly failing with
agent_release_generation_conflictwhen the broker sendsagent.deregisterbeforeaction.resulton its ordered control channel.In
completeReleaseNodeInvocation,releaseCanApplyno longer requires an active binding on the reporting node. It now accepts completion when the agent was ever bound to that node and has no active binding on another node (except the implicit direct node used after deregistration). Generation/token guards are unchanged; releases whose agent has moved to a different node still fail withagent_release_generation_conflict.Adds conformance coverage for the broker ordering (deregister then result →
completed) and for a concurrent re-bind to another node (still refused). Root and engine changelogs document the patch fix.Reviewed by Cursor Bugbot for commit 140b1b5. Bugbot is set up for automated code reviews on this repo. Configure here.