fix: reconnect after websocket heartbeat timeout - #63
Conversation
|
I reproduced #60 and ran a generation-aware follow-up against this PR. The production change looks correct, and the full three-file patch replays byte-for-byte on current
The loopback test gates the first handshake until the subscription reconnection handler is ready, then waits for a real text This adds a boundary that the current regression test does not distinguish: its server shares one subscription receiver across sockets, broadcasts messages to every live socket, and ignores PING on every generation. A later subscription plus The full library suite ( I used an AI coding assistant to help prepare and execute this synthetic loopback matrix; I reviewed the evidence and exact comment before posting. |
guoran8
left a comment
There was a problem hiding this comment.
We independently root-caused this defect in production before finding this PR (analysis in #99; it is the same bug as #60), so reviewing it from that angle:
The fix is correct, and it targets the exact mechanism that makes the stall permanent.
The failure chain on main is: heartbeat_loop detects the dead connection and exits → ping_tx is dropped → the Some(()) = ping_rx.recv() arm's pattern match fails, which merely disables that branch. Meanwhile Some(msg) = read.next() on a half-open TCP connection is enabled-but-pending forever, so the else arm (which requires all branches disabled) is unreachable, and handle_connection never returns. The SDK knows the connection is dead but no layer that could act on it is ever told.
This PR changes the arm to bind the Option (ping = ping_rx.recv()), so channel closure completes the branch instead of disabling it, and the None case returns Err — turning the heartbeat task's own exit into the death notification, with no new channel or state. connection_loop then reconnects through its existing path. Elegant and minimal.
It also fixes the secondary leak we documented in #99: because handle_connection now returns, connection_loop's top-of-loop sender_rx.is_closed() check becomes reachable again, so dropping the Client reaps the background task and socket within one heartbeat cycle instead of leaking the half-open connection until OS keepalive (~2h).
Two additional observations, neither blocking:
- The
pong_tx.send(...).is_err()→Err(Timeout)path (PONG arrives but the heartbeat task is gone) and the write-failure paths returningErrinstead ofbreakare consistent with the above: every "the peer or the heartbeat is gone" condition now funnels into the reconnect path. - The regression test (server withholds PONG → assert re-subscription) covers the primary path. The
Client-drop reap behavior is untested but follows structurally.
Production context for maintainers weighing priority: this defect cost us a 27-hour silent outage (process healthy, zero errors, zero data) before we added external inbound-silence watchdogs downstream. Those watchdogs work but leak one socket per redial cycle — this PR is what lets consumers delete that scaffolding. Would be glad to see it land; happy to run our reproduction matrix against the branch if useful.
|
Production validation report, following up on the earlier review: We cherry-picked this PR's commit onto the Results from the first ~24h:
Test side, on the cherry-picked tree: the full websocket suite passes 42/42 including this PR's For anyone needing the same backport while this is unmerged: Maintainers: with an independent reproduction earlier in #60, the mechanism analysis in #99, and now a production soak, this fix looks as de-risked as an unmerged PR gets. Merging would resolve #60 and #99 together. |
Summary
Fixes #60.
This updates the websocket connection handler so heartbeat failures cause the active connection to terminate and reconnect.
Previously, the heartbeat task could time out waiting for
PONGand exit without propagating that failure back tohandle_connection. In that state, the connection manager did not necessarily observe a connection error, so automatic reconnect/resubscribe could be missed.This patch makes
handle_connectionreturn a websocket timeout when:PONGcannot be delivered to the heartbeat taskA regression test covers the missing-PONG case by using a mock websocket server that ignores
PING, then asserting the client reconnects, resubscribes, and receives messages after reconnect.Verification
uv tool run pre-commit run --all-files(note: this introduced the whitespace change to.gitignore)cargo test --features clob,ws,tracing reconnects_when_heartbeat_pong_is_missingcargo test --workspace --all-featuresNote
Medium Risk
Changes live WebSocket connection teardown and reconnection behavior when heartbeats fail; impact is limited to the ws connection loop and is covered by a new integration test.
Overview
Fixes stale WebSocket sessions where the heartbeat task could die on a missing PONG without the connection manager treating it as a failure, so reconnect and resubscribe might never run.
handle_connectionnow surfaces heartbeat failures asWsError::Timeout: when a PONG cannot be forwarded to the heartbeat task, and when the heartbeat task exits and the ping channel closes. Failed PING sends returnConnectionClosedinstead of only breaking the loop. PONG handling is refactored alongside normal text parsing.Adds
reconnects_when_heartbeat_pong_is_missing(mock server ignores PING) to assert reconnect, resubscribe, and post-reconnect messages..gitignoregets a trivialspecs/*line-ending fix.Reviewed by Cursor Bugbot for commit 2085ef9. Bugbot is set up for automated code reviews on this repo. Configure here.