fix(broker): detect a blackholed node-control connection instead of trusting writes - #1462
fix(broker): detect a blackholed node-control connection instead of trusting writes#1462khaliqgant wants to merge 1 commit into
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. |
|
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 (2)
📝 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 Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
| and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | ||
|
|
||
| ## [Unreleased] | ||
| ## [Unreleased - Patch] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required [Unreleased] heading.
Line 8 uses ## [Unreleased - Patch]. Put pending entries under ## [Unreleased] and retain this item in ### Fixed.
As per coding guidelines, “Curate the root CHANGELOG.md under [Unreleased] using Keep a Changelog and SemVer conventions.”
Proposed fix
-## [Unreleased - Patch]
+## [Unreleased]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## [Unreleased - Patch] | |
| ## [Unreleased] |
🤖 Prompt for 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.
In `@CHANGELOG.md` at line 8, Update the root changelog heading from “##
[Unreleased - Patch]” to “## [Unreleased]”, keeping the existing pending entry
under its “### Fixed” section.
Source: Coding guidelines
![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".
|
|
||
| ### Fixed | ||
|
|
||
| - A fleet node no longer drops out of the roster indefinitely when its `/v1/node/ws` connection is blackholed. The broker pings each heartbeat interval and reconnects when no frame arrives for 48s, instead of trusting writes that keep succeeding into a dead socket while `agent-relay fleet nodes` shows the node offline and `/health` still reports `nodeConnected: true`. |
There was a problem hiding this comment.
This bullet goes beyond the user-visible fix and includes the internal WebSocket route, timeout, write-liveness mechanism, CLI state, and health-field details. Keep it to a short impact-first statement—such as the broker now reconnecting blackholed fleet-node connections—so the release narrative follows the repository requirement to omit implementation backstory.
AGENTS.md reference: AGENTS.md:L45-L49
Useful? React with 👍 / 👎.
![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
…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>
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 |
1 participant
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