fix(engine): replay backlog to all certified sessions on node reconnect - #445
Conversation
…nnect 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>
…ated 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>
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Warning Review limit reachedNext included review available in 33 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 (12)
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 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| const mode = registry.providerDeliveryReadinessMode?.(workspaceId, nodeId, providerName, connectionId); | ||
| if (mode !== undefined) return mode === 'agent_scoped'; | ||
| return providerAdvertisesDeliveryCursor(db, workspaceId, nodeId, providerName); |
There was a problem hiding this comment.
🟡 Stale inventory replays to replacement connection
A superseded connection's queued inventory.sync can use the replacement registration's persisted cursor capability. The stale connection ID returns no mode, but persistent readiness can still permit replay. Frames reach the replacement before it certifies those sessions.
Learn more
providerDeliveryReadinessMode distinguishes a registry without mode support from a stale or unbound connection. Its contract returns undefined for a connection ID that is no longer current. Falling back in both cases loses that distinction and reads capabilities written by whichever registration now owns the provider.
Example: Connection B begins node.register while connection A still owns the provider. An inventory.sync arriving on A queues behind B and captures A's provider name. B then supersedes A and persists its cursor capability. When A's sync runs, the mode lookup rejects A's connection ID, but the fallback classifies the sync from B's capability. A remote owner whose ready-set survives reconnect can send A-certified deliveries to B before B certifies those sessions.
Recommended fix: Use the database fallback only when providerDeliveryReadinessMode is absent. When the method exists and returns undefined, treat the frame as non-cursor-gated and do not apply full certified replay.
| const mode = registry.providerDeliveryReadinessMode?.(workspaceId, nodeId, providerName, connectionId); | |
| if (mode !== undefined) return mode === 'agent_scoped'; | |
| return providerAdvertisesDeliveryCursor(db, workspaceId, nodeId, providerName); | |
| if (registry.providerDeliveryReadinessMode) { | |
| const mode = registry.providerDeliveryReadinessMode(workspaceId, nodeId, providerName, connectionId); | |
| return mode === 'agent_scoped'; | |
| } | |
| return providerAdvertisesDeliveryCursor(db, workspaceId, nodeId, providerName); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Addressed in c8c70fc: when providerDeliveryReadinessMode exists, an undefined result (stale/superseded connection) now means not-cursor-gated — the persisted capabilities fallback is only used when the API itself is absent. New conformance test 'does not let a stale connection certify a replacement connection''s backlog' covers exactly this path and fails without the fix.
…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>
2 participants
Summary
Deliveries queued while a node's delivery socket is down were never replayed on reconnect — observed live: a GitHub webhook event landed in an agent's channel while its broker socket was down, and after the socket recovered the event sat durable-but-unsent until a new event arrived.
Root cause:
inventory.syncreplay on a cursor-negotiated reconnect was scoped to newly-routed agents only, so deliveries queued for already-ready agents had nothing to re-mark them due. This change makes reconnect replay every session theinventory.synccertifies, with the replay scope taken from the connection's negotiated delivery mode — connection-scoped readiness (providerDeliveryReadinessMode()) rather thannode_providers.capabilities, whichheartbeatNoderewrites wholesale and could silently un-flush the backlog.Test plan
npx vitest runinpackages/engine: 99 files, 1,151 tests passednpx turbo test: 18/18 tasksKnown limits (documented, not introduced here)
deliveredinto a dead socket (replay still recovers it).Review evidence:
FLOW_NOTES.md+REVIEW_VERDICT.jsonon this branch (fresh-eyes Codex review, approved at HEAD).Generated with Devin
Note
Medium Risk
Changes core node reconnect delivery replay and heartbeat capability merging; behavior is well covered by new conformance tests but affects at-least-once message delivery to brokers.
Overview
Fixes stranded mailbox rows when a broker node reconnects after its delivery socket was down: messages queued during the outage were not replayed on
inventory.syncif the socket owner already reported those agents as delivery-ready.inventory.syncreplay scope now follows the connection’s negotiated delivery mode instead of readiness/routing transitions. On cursor-negotiated reconnects (no register-time flush), replay covers every identity in the certified inventory (reconciledAgentIds). Legacy immediate-delivery connections still only replay identities newly routed on that sync, avoiding a duplicate flush afternode.register.Handshake stability: replay mode is read from the live connection via optional
NodeConnectionRegistry.providerDeliveryReadinessMode()(implemented on the in-process adapter).node.heartbeatroster updates merge capabilities withwithRegisteredProtocolCapabilities()so heartbeats cannot drop or addrelay:delivery-cursor-v1and silently change replay behavior. Superseded connections getundefinedfrom the registry and do not drain backlog on stale syncs.Conformance tests cover outage replay, ordering/dedupe after ACK, heartbeat roster edge cases, and stale-connection certification. Root and engine changelogs add an
[Unreleased - Patch]fix entry; review notes and trajectory artifacts are included in the branch.Reviewed by Cursor Bugbot for commit c8c70fc. Bugbot is set up for automated code reviews on this repo. Configure here.