engine: export node-control provider-disconnect + fix dropped-provider aggregate leak#244
Conversation
…ontrol Out-of-process node-control socket owners (the relaycast-cloud NodeDO) own their sockets but not the DB. On socket close they must flip the right liveness — one provider offline while the node stays online, or the whole node offline when the last connection drops — and that logic already lives in `engine/node.js` (`handleProviderDisconnect`, `markNodeOffline`) but was unreachable from the `@relaycast/engine/node-control` entrypoint, forcing the host to hand-roll a SQL mirror. Re-export both so the host delegates instead. Both encapsulate the full effect (provider row + agents + node aggregate + invocation reschedule); a conformance test asserts the public exports produce the provider-scoped, last-connection, and node-wide offline states. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request exports handleProviderDisconnect and markNodeOffline from the node-control module to support out-of-process socket owners driving the node-offline lifecycle, and adds a conformance test suite to verify their behavior. The feedback suggests using optional chaining (stack?.close()) in the test teardown to prevent masking setup errors if initialization fails.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new Vitest conformance test suite verifying node-control's provider-disconnect behaviors (partial disconnect, full disconnect, and markNodeOffline), including helpers to enroll nodes, attach simulated provider sockets, and query provider/node status. Adds an explanatory comment in node-control.ts documenting these lifecycle semantics. ChangesNode-control disconnect behavior verification
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant EngineAPI
participant ProviderSocket
participant DB
Test->>EngineAPI: enrollNode()
EngineAPI->>DB: create node record
Test->>ProviderSocket: attachProvider() registration + heartbeat
ProviderSocket->>DB: write provider status
Test->>EngineAPI: handleProviderDisconnect(remaining)
EngineAPI->>DB: update provider/node status
Test->>DB: query nodeProviders.status / nodes.status
DB-->>Test: status results
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…n error Addresses review feedback on #244. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… correct `markProviderOffline` and `markNodeOffline` left `node_providers.active_agents` at its last value, but `recomputeNodeAggregate` sums active_agents across ALL providers (online and offline). So after a provider's socket dropped with others still connected, `nodes.active_agents` kept counting the dropped provider's now- offline agents until it reconnected. Reset it to 0 on offline; a reconnect / heartbeat repopulates it from the provider's own report. Regression test in nodeControlExports asserts the node aggregate drops the dropped provider's count and restores it on the next heartbeat, and that markNodeOffline zeros every provider's count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
willwashburn
changed the title
![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 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="packages/engine/src/__tests__/conformance/nodeControlExports.test.ts">
<violation number="1" location="packages/engine/src/__tests__/conformance/nodeControlExports.test.ts:152">
P3: The aggregate reconnect test reuses the disconnected socket's handle to send the reconnect heartbeat. In production a new socket creates a new handle via `attachNodeSocket`; reusing the old handle tests only the DB write path, not the full lifecycle. Consider calling `attachNodeSocket` + heartbeat through the fresh handle to match the reconnect path more closely.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…tion Close the dropped provider's socket (driving the disconnect through the adapter's own close path) and register a fresh socket with a new instance for the restore, instead of re-heartbeating the stale handle — matches how a provider actually reconnects. Addresses review feedback on #244. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Two changes on the node-control provider-disconnect path: expose it to
out-of-process socket owners, and fix an aggregate-count leak in it.
1. Export the provider-disconnect lifecycle
@relaycast/engine/node-controlre-exportshandleProviderDisconnectandmarkNodeOfflinealongsidehandleNodeControlMessage.An out-of-process node-control socket owner — the relaycast-cloud
NodeDO, whichholds the
/v1/node/wssockets in a Durable Object — owns its sockets but drivesthe engine for everything else. On socket close it must flip the correct
liveness:
(and its agents) goes offline, the node stays online;
disconnect) → the whole node, its providers, and its agents go offline.
That logic already lives in
engine/node.ts(handleProviderDisconnect,markNodeOffline) — what the in-process adapter calls on its own socket close —but it was not exported from
node-control, so the DO had to hand-roll a SQLmirror of it. Multi-provider nodes turned that one mirror into several. Both
functions encapsulate the full effect (provider row, hosted agents, node
aggregate, invocation reschedule), so a host never reassembles them.
2. Fix the dropped-provider aggregate leak
markProviderOfflineandmarkNodeOfflineleftnode_providers.active_agentsat its last value, but
recomputeNodeAggregatesumsactive_agentsacross ALLproviders (online and offline). So after a provider's socket dropped with others
still connected,
nodes.active_agentskept counting the dropped provider's now-offline agents until it reconnected. Both markers now zero
active_agentsonoffline; a reconnect / heartbeat repopulates it from the provider's own report.
This is in the exact function the export above exposes, so it ships here rather
than as a separate follow-up.
Test
nodeControlExports.test.tsdrives the public exports against the in-memoryengine: provider-scoped offline (node stays online), last-connection offline, and
node-wide offline; plus the aggregate correction — the node total drops a dropped
provider's agents and restores them on the next heartbeat, and
markNodeOfflinezeros every provider's count.
Full engine suite green (414).
Ships with
The consumer is relaycast-cloud#27 (NodeDO multi-provider sockets), which deletes
its hand-rolled
node/registration.tsmirror and delegates to these exports.Both ride the same next
@relaycast/enginerelease.🤖 Generated with Claude Code