fix(broker): detect a blackholed node-control connection instead of trusting writes - #1462
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. |
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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. 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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe broker now detects inbound-idle periods on node-control WebSocket connections, sends WebSocket pings with heartbeats, reconnects after prolonged silence, updates configuration and tests, and documents the fix. ChangesNode-control WebSocket liveness
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change adds detection and reconnection for silent node-control connections; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant BrokerLoop
participant WebSocketPeer
participant ReconnectController
BrokerLoop->>WebSocketPeer: Send heartbeat and WebSocket ping
WebSocketPeer-->>BrokerLoop: Return inbound frame
BrokerLoop->>BrokerLoop: Refresh inbound activity
BrokerLoop->>ReconnectController: Disconnect after idle timeout
ReconnectController->>WebSocketPeer: Establish replacement connection
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@CHANGELOG.md`:
- Line 8: Update the root changelog heading from “## [Unreleased - Patch]” to
“## [Unreleased]”, keeping the existing pending entry under its “### Fixed”
section.
In `@crates/broker/src/node_control.rs`:
- Around line 1718-1719: Update the idle timeout comparison in the heartbeat
handling logic around last_inbound and read_idle_timeout to use a
greater-than-or-equal check, ensuring clients disconnect exactly when the
configured deadline is reached while preserving the existing timeout handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33f5a438-1650-4010-bb9d-074edfbe69a0
📒 Files selected for processing (3)
CHANGELOG.mdcrates/broker/src/node_control.rscrates/broker/src/runtime/init.rs
![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: a9e50e8b9d
ℹ️ 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".
![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 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
a9e50e8 to
f5877a6
Compare
|
Rebased onto Applied — Applied — shortened the changelog entry (@chatgpt-codex-connector, P1). Correct per AGENTS.md L45-49: the entry carried the route, timeout, write-liveness mechanism and health-field detail, which is implementation backstory. Now one impact-first line. Not applied —
11.5.0 was cut on 2026-08-10 leaving an empty Verification after the change The regression test still bites with |
|
To use Codex here, create an environment for this repo. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Focused commit diff ---'
git show --format=fuller --find-renames --find-copies -- CHANGELOG.md crates/broker/src/node_control.rs AGENTS.md f5877a604
printf '%s\n' '--- Changelog policy ---'
if [ -f AGENTS.md ]; then
sed -n '30,55p' AGENTS.md
else
fd -HI '^AGENTS\.md$' . -x sh -c 'printf "\n--- %s ---\n" "$1"; sed -n "30,55p" "$1"' sh {}
fi
printf '%s\n' '--- Current idle-timeout guard context ---'
rg -n -C 6 'idle\s*(>=|>)\s*read_idle_timeout|inclusive|four heartbeat' crates/broker/src/node_control.rs
printf '%s\n' '--- Current changelog heading and entry ---'
sed -n '1,35p' CHANGELOG.mdLength of output: 20355
I also verified The changelog entry is concise and impact-first. The reported test and formatting results are sufficient for this follow-up. ✏️ Learnings added
You are interacting with an AI system. |
@khaliqgant The The changelog heading decision also matches your repo rule — first pending change on top of a release that left an empty The only mild note: the Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
ee6a1ab to
48dfba9
Compare
…rusting writes Closes #1457. Every disconnect path in run_connected_once's select! keyed off send_wire(...).is_err(), which is write-only liveness. On a blackholed /v1/node/ws the kernel keeps accepting 12-second heartbeat frames into the send buffer, so the writes never fail; there was no heartbeat ack, no WS ping/pong, and no read-side deadline, so stream.next() waited forever and the client never reached the reconnect/backoff that already existed below it. finn-mini sat like this for 80 minutes on 2026-08-07: engine-side lastHeartbeatAt frozen at 11:52:35Z and the node hidden from `fleet nodes`, while the broker's own /health still reported nodeConnected: true and the TLS socket was still ESTABLISHED. Each heartbeat tick now also sends a WS ping, so a live peer always owes us a frame even when the engine has nothing to say, and any inbound frame refreshes a last-seen stamp. Silence past four heartbeat intervals (48s) returns ControlRunResult::Disconnected and lets the existing backoff reconnect. The window is injectable via FleetControlConfig::read_idle_timeout so the regression test covers it in 400ms rather than 48s. Proven to bite: neutralizing the idle check to `false && idle > ...` makes node_control_reconnects_when_peer_goes_silent_but_writes_still_succeed hang its full 20s bound and fail with "client never reconnected after the peer went silent"; with the check it reconnects in 1.42s. The test's silent server holds the socket open without polling it, because a server that keeps calling next() makes tungstenite answer pings automatically and would not model a blackhole. cargo test -p agent-relay-broker --lib: 860 passed, 0 failed, 4 ignored. cargo fmt -p agent-relay-broker -- --check: clean. cargo clippy -p agent-relay-broker --lib --tests -- -D warnings: 3 pre-existing errors in snippets.rs / runtime/api.rs / runtime/worker_events.rs, untouched by this change and already documented in 5c2ad8e's test plan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…connected Positive control arm for node_control_reconnects_when_peer_goes_silent_but_writes_still_succeed, under the same 400ms read_idle_timeout. The blackhole test proves the read-idle detector CAN fire; on its own that is also what a detector that disconnects unconditionally after the window would do. This proves it does not fire when the peer is merely idle at the application layer but still servicing the socket (so pings get answered), which is the actual claim the mechanism makes. Verified discriminating: temporarily neutralizing the check to `if true` makes this new test fail with "client reconnected even though the peer stayed live and kept polling"; with the real `idle >= read_idle_timeout` check it passes. cargo test -p agent-relay-broker --lib node_control::: 53 passed, 0 failed (after clearing stale incremental build state that had produced spurious failures unrelated to this change).
48dfba9 to
215d2c3
Compare
2 participants
Closes #1457.
The bug
Every disconnect path in
run_connected_once'sselect!keyed offsend_wire(...).is_err(). That is write-only liveness: on a blackholed/v1/node/wsthe kernel keeps accepting 12-second heartbeat frames into the send buffer, so the writes never fail. There was no heartbeat ack, no WS ping/pong, and no read-side deadline —stream.next()waits forever — so the client never left theselect!and never reached the reconnect/backoff that already existed below it.How it showed up
finn-minisat like this for 80 minutes on 2026-08-07. Engine-sidelastHeartbeatAtfroze at11:52:35Zand the node vanished fromagent-relay fleet nodes(visible only under--all, asoffline), while on the box itself:{"nodeConnected":true,"nodeDelivery":{"connected":true,"tokenPresent":true},"relaycastConnected":true,"status":"ok"}…and
lsofstill showed the TLS socketESTABLISHED. Broker uptime was 26h, so this was not a startup failure. Nothing recovered it but a full broker restart, which cost 11 agent sessions.The fix
Each heartbeat tick now also sends a WS ping, so a live peer always owes us a frame even when the engine has nothing to say, and any inbound frame refreshes a last-seen stamp. Silence past four heartbeat intervals (48s) returns
ControlRunResult::Disconnectedand lets the existing backoff reconnect.The engine is not guaranteed to send unsolicited traffic, so an application-level ack would not have been sufficient on its own — the socket needs its own keepalive. #1450 notes the events WebSocket is already pinged every 30s; node-control was the one that wasn't.
Proof the test bites
Neutralizing the guard to
false && idle > read_idle_timeout:With the guard restored:
ok ... finished in 1.42s.Two things worth knowing about the test:
next()is not silent — tungstenite answers pings automatically, so the first version of this test failed with the fix in place.FleetControlConfig::read_idle_timeout(400ms in test, 48s in production).#[tokio::test(start_paused = true)]was tried first and rejected: virtual time raced past the real TCP handshake, producing a 0.01s spurious failure.Test plan
Clippy reports 3 errors, all pre-existing and in files this branch does not touch (
snippets.rs:1500,runtime/api.rs:2488,runtime/worker_events.rs:38) — the same three documented in5c2ad8ee3's test plan.Scope
This fixes detection, not the underlying cause of the blackhole, which was never identified. The misleading
nodeConnected: truethat made this hard to spot is also still there — that is #1386's stated secondary defect and is left alone here.🤖 Generated with Claude Code