fix(nodes): adopt node provider and commit agent move atomically on bind - #443
Conversation
A broker-spawned agent is HTTP-registered first (provider "default", implicit direct-* location), then the broker's create-only agent.register fails and it falls back to POST /v1/nodes/:name/agents. upsertAgentNodeBinding moved locationType/locationNodeId but providerName stayed "default", so deliveries routed to the right node yet were pushed to a provider connection that does not exist — the spawned agent was never woken (AgentWorkforce/relay#1794). bindAgentToNode now adopts a provider the node actually serves (served -> sole -> "default" -> keep), stamps origin, and refuses to steal an agent active on another live node. The binding row, location move, old-binding retirement and old-node slot refund commit as one unit via runAtomicWrites (interactive transaction, D1 batch, or loud refusal), so no failure prefix can leave a node charged for a retired binding or holding an unreserved one. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| ...agentNodeBindingWrites(writeDb, workspaceId, agent, node.id, { | ||
| sessionRef: opts.session_ref ?? null, | ||
| priority: opts.priority ?? 0, | ||
| providerName: adoptedProviderName, |
There was a problem hiding this comment.
🔴 Cursor-aware fallback binds stay gated
When bindAgentToNode adopts an agent onto a cursor-aware provider, it never adds that identity to the provider's ready set. The readiness initialization starts that set empty, so delivery remains queued until a later agent announcement.
Learn more
Cursor-aware providers negotiate per-agent delivery readiness during node.register. The registry then rejects live and replay delivery for every identity until markProviderAgentsDeliveryReady adds it. Successful agent.register, agent.recover, and inventory handling perform that transition, but this HTTP bind path has no registry dependency and only changes database routing. The fallback-bound agent therefore points at the correct provider socket but remains gated from using it.
Example: A broker provider advertises relay:delivery-cursor-v1, then an HTTP-created agent causes agent.register to return agent_already_exists. The broker calls the bind endpoint successfully. A message for that agent remains queued because (node_broker, broker, agent_id) is absent from the provider's ready set.
Recommended fix: Extend the bind flow to establish readiness through NodeConnectionRegistry only after the database move commits, or define a provider control handshake that announces the adopted identity and its authoritative cursor. Preserve the cursor contract: do not mark readiness before the provider has received the identity's cursor state.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const nodeProviderNames = (await tx | ||
| .select({ name: nodeProviders.name }) | ||
| .from(nodeProviders) | ||
| .where(and(eq(nodeProviders.workspaceId, workspaceId), eq(nodeProviders.nodeId, node.id)))) | ||
| .map((row) => row.name); | ||
| const adoptedProviderName = adoptNodeProviderName(agent.providerName, nodeProviderNames); |
There was a problem hiding this comment.
🟡 Stale providers defeat provider adoption
When a node retains an offline provider row, adoptedProviderName can select it instead of the live provider. Provider disconnect preserves that row, so the moved agent becomes unreachable.
Learn more
Provider rows persist after a provider disconnect so the node can retain its capability manifest. This query treats those historical rows exactly like providers with live sockets. adoptNodeProviderName prefers the agent's current provider first, then the sole row, then default, so any of those choices can target an offline provider even while another provider keeps the node live.
Example: A node has an offline persisted default row and a live broker row. An HTTP-created agent currently has provider default. Binding keeps default, but only the broker socket exists, so deliveries cannot reach the agent.
Recommended fix: Select the provider liveness fields and run adoption against providers satisfying isProviderLive, or otherwise derive candidates from the registry's connected provider set. Keep historical rows for display, but exclude them from routing decisions.
Was this helpful? React with 👍 or 👎 to provide feedback.
…iders Two gaps in the bind path left adopted agents unable to receive pushes: - Cursor-aware (agent_scoped) providers gate deliveries per identity until it is announced. The bind moved the agent onto the provider but never marked it ready, so deliveries queued forever. The bind now marks the adopted identity delivery-ready after the move commits and drains its pending queue, and returns delivery_ack_seq so the caller learns the authoritative cursor — the same contract agent.register/recover carry. - Provider adoption read every persisted provider row including offline ones kept for the capability manifest. An agent whose own provider had gone stale could be kept on (or moved to) a provider with no live socket. Adoption now considers only live providers. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Both Devin Review flags addressed in de75a31:
Focused suite: 63 tests green; typecheck and lint clean. |
![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 high 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 de75a31. Configure here.
…onse Cursor Bugbot: marking the adopted agent ready and draining its queue inside bindAgentToNode let deliver frames reach the provider socket before the HTTP response carrying delivery_ack_seq reached the caller. agent.register and agent.recover order their cursor-bearing reply ahead of the ready mark and replay; the bind path now does the same — bindAgentToNode returns the binding plus the resolved move, the route sends the response first, then runs the ready mark and scoped replay in the request background. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ct (#445) * fix(engine): replay every session an inventory.sync certifies on reconnect A node whose delivery socket dropped got nothing back after reconnecting: messages fanned out during the outage stayed queued in the mailbox until their TTL and the agents were never woken. A cursor-negotiated `node.register` deliberately does not replay (PR #443: no identity is cursor-ready yet), so `inventory.sync` is the node's only reconnect-replay trigger. It scoped that replay to identities whose delivery readiness or provider routing *changed* during the sync, which is empty whenever the socket owner already reports the certified sessions as delivery-ready — an owner whose ready-set is keyed per node+provider rather than per connection, or a registry that omits the optional readiness hooks, where the shared helper defaults to ready. The drain then ran with an empty scope and returned 0. The in-process adapter resets its ready-set on every new connection, which is why no in-tree test caught it. Replay the full certified set on a cursor-negotiated connection instead, matching `agent.register` / `agent.recover` and the documented contract that an `inventory.sync` certifies sessions that retained their cursors and may replay. Legacy immediate-delivery connections are unchanged: `node.register` already flushed the node to them, so only newly routed identities replay and the register-time flush is not duplicated. The handshake mode is recovered from the engine's own registration record through a new `providerAdvertisesDeliveryCursor()` helper, which also replaces the two duplicated inline capability lookups in agent register/recover. Dedupe, ordering and gating are unchanged: the cumulative delivery cursor still suppresses acked rows, pages still drain oldest-first under a bounded high-water mark, and every frame is still gated on per-identity delivery readiness. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(engine): take reconnect replay scope from the connection's negotiated mode `inventory.sync` inferred the cursor handshake from the provider's persisted capabilities, which `node.heartbeat` rewrites whenever a roster snapshot rides along. A heartbeat that advertised only spawn capacity demoted a live cursor-gated connection to the legacy branch, so a certification that had just readied the identities replayed nothing and the outage backlog stayed queued until its TTL; the inverse promoted an immediate connection and re-sent the frames `node.register` had already flushed. Recover the mode from the connection it was negotiated on: the registry now exposes `providerDeliveryReadinessMode()`, the per-connection mode `node.register` configures. Registries on the older contract fall back to the persisted advertisement, which a heartbeat can no longer change — capabilities negotiated at registration (`relay:delivery-cursor-v1`) are carried over a heartbeat roster refresh instead of being replaced by it, so the fallback answers for the registration rather than the latest snapshot. Either fix alone holds the behaviour; out-of-process socket owners get the second for free. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: flow review notes and trajectories for reconnect replay fix Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(engine): treat a stale connection's missing delivery mode as non-cursor A superseded connection's queued inventory.sync could inherit the replacement registration's persisted cursor advertisement: the registry mode lookup returns undefined for a connection that no longer owns the provider, but the fallback then read capabilities written by whichever registration owns it now. With an out-of-process owner whose ready-set survives reconnect, the stale certification replayed the outage backlog to sessions the replacement never certified. When providerDeliveryReadinessMode exists its undefined is the answer; the persisted-capability fallback now only serves registries that cannot answer at all. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: agentrelaybot <agentrelaybot@agentrelay.dev> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1 participant
Summary
Fixes AgentWorkforce/relay#1794 — subscription recipients (and any fallback-bound agent) never wake because deliveries are pushed to a provider connection that does not exist.
Bug chain: broker spawn flows HTTP-register the agent first (
providerName='default', implicitdirect-*location), then the create-onlyagent.registerfails → broker falls back toPOST /v1/nodes/:name/agents→bindAgentToNode. The binding upsert movedlocationType/locationNodeIdbut leftproviderName='default', sosendToProvider(node, 'default')found no connection on a node whose live provider is e.g.broker— deliver frames never sent, verified live (events sat unread in the agent's inbox withpending_messages: 0).Fix:
bindAgentToNodenow adopts a provider the node actually serves — precedence: keep the agent's provider if the node serves it → sole provider →default→ keep — and stampsoriginNodeId.agent_location_conflict(409) when the agent is active on another live node; dead/missing/implicit-direct locations are adopted (mirrorsreconcileInventory).runAtomicWrites— interactive transaction on Node,batchon D1, loud refusal otherwise. No failure prefix can leave a node charged for a retired binding or holding an unreserved active binding.Evidence
agentLifecycle.test.ts(+17 new): location move, provider-adoption table, live/dead/implicit guard boundaries, delivery push to the real provider connection, and a 2×4 failure matrix (transactional × D1-batch × each failure point) asserting nothing commits and retries complete cleanly.fix-1794.flow.ts): implementer agent → deterministic verify → two fresh-context codex review rounds → fixer rounds, each finding addressed.Known follow-ups (out of scope — systemic, pre-existing)
Recorded by the final review round; they exist in every
reserveNodeAgentSlot→mutate→compensate caller (register, recover, inventory), not just this path:Test plan
npx vitest run src/__tests__/conformance/agentLifecycle.test.ts— 61 pass (Node 22; better-sqlite3 ABI)npm run typecheck— cleannpx vitest run— 1136 passnpm run lint— cleanGenerated with Devin
Note
High Risk
Changes core agent placement, provider routing, and multi-table bind atomicity; misbehavior would strand agents or corrupt node capacity counters, though behavior is heavily regression-tested.
Overview
Fixes fallback-bound agents (e.g. broker spawn after HTTP register) that never received pushes because bind only created a binding row while the agent stayed on its implicit
direct-*node withprovider_name: default.Bind is now a full location move:
bindAgentToNodeupdateslocation_node_id, stampsorigin_node_id, adopts a live node provider via clear precedence rules, and returnsdelivery_ack_seqon the bind response (OpenAPI updated). It rejectsagent_location_conflict(409) when the agent is active on another live node; stale, pruned, or implicit-direct locations can still be adopted.Atomicity: binding insert, agent move, old-binding retirement, and old-node slot refund commit in one unit (
runAtomicWrites/ transaction); handles without atomic capability are refused. After the cursor-bearing HTTP response,completeBoundAgentDeliveryruns in the background to mark cursor-aware providers delivery-ready and drain queued deliveries.Conformance coverage adds bind routing, provider adoption, conflict boundaries, and failure-matrix atomicity tests; shared DB failure helpers move into the test harness.
Reviewed by Cursor Bugbot for commit c9c7f70. Bugbot is set up for automated code reviews on this repo. Configure here.