Repository navigation
fix(replication): arm client-side receive watchdog to catch silently-dead WS - #234
Merged
Merged
Conversation
…dead WS The 30s sendPing tick on outbound replication connections can fail silently (event-loop stall, ws.terminate() not propagating to 'close'), leaving the (peer, db) pair stuck on connected: true with no retry until process restart — see #233. Arm the existing receive-side watchdog on the client too, reset it on incoming ping/pong/message, and suspend it during ws.pause() backpressure to avoid spuriously terminating healthy mid-ingest connections. Extract as createReceiveWatchdog for unit testability. Co-Authored-By: Claude Opus 4.7 <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 introduces a receive-side silence watchdog (createReceiveWatchdog) to detect and terminate dead replication WebSocket connections based on bytesRead inactivity, along with comprehensive unit tests. The review feedback identifies a critical bug in the watchdog's throttling logic where it can become permanently inactive if a throttled reset is followed by silence. A fix is suggested to reschedule the timer when activity is detected, along with a new regression test to verify this behavior.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Reviewed; no blockers found. Prior blocker (throttled-reset-then-silent leaves watchdog permanently inactive) addressed in commit 2bfbaf1 — |
…es bytes changed PR #234 review (Gemini + claude bot) caught a permanently-inactive watchdog bug: when an external reset() is throttled and the in-flight timer later fires with bytesRead advanced beyond the snapshot, the previous implementation exited without re-arming. After that, no external reset() ever arrives (the scenario the watchdog exists to catch is exactly this: peer goes quiet right after a burst), so the watchdog stays dead permanently. Re-arm from the new byte baseline in the non-silence branch. Also rename the existing test that didn't actually exercise the throttle path (1s-spaced resets all bypassed it) and add a dedicated regression test for the throttled-then-silent sequence. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kriszyp
marked this pull request as ready for review
May 28, 2026 20:38
kriszyp
added a commit
that referenced
this pull request
Jun 10, 2026
…oop yield budget (v5.0 backport of #248) Backport of #248 onto v5.0. A follower cloning a large table stalled socket byte movement under legitimate flow control (receiver pausing reads to drain its queue; sender parked awaiting socket drain). The old ping check used a "no bytes since last ping => terminate" heuristic, so it killed the connection mid-copy and the copy restarted from zero, never converging. - Measure keep-alive liveness from the last observed socket byte movement (shouldTerminateIdlePing): terminate only after no activity for the full PING_TIMEOUT. A slow-but-progressing transfer keeps moving bytes and survives; a dead peer (incl. a sender stuck on a drain that never fires) is still terminated. Sole exemption: pauseReasons > 0 (receiver's intentional, self-clearing read-pause), during which we keep liveness fresh and still send pings (ws.pause stops reads, not writes). - Yield the receive decode loop on a time budget (RECEIVE_YIELD_INTERVAL, default 100 ms) even when the consumer keeps up, so a single large message can't decode in one synchronous turn and starve ping responses / trip core's event-delay monitor. Backport notes: - The new replication.pingInterval / replication.pingTimeout core config params (HarperFast/harper#872) are not on the v5.0 core, so PING_INTERVAL and PING_TIMEOUT are hardcoded to the same defaults (#248) used: 30 s / 60 s. Configurability via replication.pingTimeout lands in v5.1+. - The receive-side watchdog (createReceiveWatchdog, RECEIVE_SILENCE_THRESHOLD_MS) introduced by #234 is NOT on v5.0 and is NOT being backported here. v5.0's existing simpler resetPingTimer remains in place; only the client-side sendPing path and the decode-loop yield from #248 are applied. - core submodule pointer NOT bumped — this PR contains only the harper-pro half of the fix. - replication/DESIGN.md from #248 not included (the file did not exist on v5.0; the document also references v5.1+ features such as per-route excludeTables and the configurable ping params). Refs #324, #241.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sendPingtick alone is not sufficient: it can fail silently (event-loop stall,ws.terminate()not propagating toclose) and leave the(peer, db)pair stuck onconnected: trueuntil the process restarts.PING_INTERVAL * 2) is reset on every incomingping,pong, andmessage, and suspended while the WS is intentionally paused for backpressure.createReceiveWatchdogfor unit testability — 10 new tests inunitTests/replication/receiveWatchdog.test.mjs.Purpose
Direct follow-up to #233 — addresses the variant where the close handler never fires at all (
datadb on bza in the diagnosed cluster:connected: true, zero established TCP, no retry attempts in 30+ min). The companionconnected: false/ no-retry-scheduled variant lives in the close-handler retry path and is not addressed here.Where to look
createReceiveWatchdogtracksbytesReadonly. Codex flagged a P2 if bothbytesReadandbytesWrittenwere checked — our ownsendPingadvancesbytesWritteneven when the peer is silent, so including it would defeat the watchdog in the exact missed-tick scenario it exists to recover from. ThebytesRead-only check is the heart of the correctness argument.addPauseReason/removePauseReasonsuspend the watchdog. Codex flagged a P2 here too: under sustained backpressure (ws.pause()held for >60s)bytesReadis frozen by design, and the watchdog would terminate a healthy mid-ingest connection. Stop-on-pause / reset-on-resume avoids this.reset()throttles internally. Gemini flagged the timer churn fromws.on('message')callingreset()per frame — at thousands of frames/sec during a full copy, the rawsetTimeout/clearTimeoutcycle adds GC pressure and event-loop drag. The watchdog now coalesces resets within a~1swindow. Granularity loss is within the noise of the 60s interval and the dedicated test guards correctness.const resetPingTimer = receiveWatchdog.resetis intentional. Keeps the call-site naming continuous with the priorresetPingTimer()function so the message/ping/pong listener registrations stay textually unchanged; tradeoff is one alias line.connected: false, no retryvariant onoauth/system— separate bug in the close-handlersetTimeoutretry path, separate fix.🤖 Generated by Claude Opus 4.7.