Skip to content

Require transport evidence before a copy-progress watchdog fire - #782

Open
kriszyp wants to merge 5 commits into
mainfrom
fix/copy-progress-watchdog-transport-liveness
Open

Require transport evidence before a copy-progress watchdog fire#782
kriszyp wants to merge 5 commits into
mainfrom
fix/copy-progress-watchdog-transport-liveness

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 29, 2026

Copy link
Copy Markdown
Member

Stops the copy-progress watchdog (#453) from killing healthy base copies whose peer goes briefly transport-dark — the 2026-08-29 scheduled Integration Tests failure on main (run 33239813839), where copyProgressBackpressureFalseFire.test.mjs failed with a fire whose own truth payload read connected: true … liveness: 2s ago.

Diagnosis (from the run's server-log artifacts). The #695 pause-guard did NOT regress: no fire occurred during the paused window. The subscriber resumed from commit-backlog back-pressure at 07:03:53.808, re-arming the watchdog on its 1500ms test budget — while the source's event loop was dark from 07:03:51.445 to 07:03:55.810 (4.365s: zero log lines, zero bytes, not even WS pongs, and the reconnect's TLS handshake was only accepted the moment it woke — a starved 2-core CI runner running concurrent test files). The truth payload was consistent, not contradictory: liveness staleness (2s) exceeded the watchdog budget (1.5s), so nothing at all had arrived in the window. The defect is jurisdictional: an app-level watchdog acted on transport-level silence with an app-level budget. Transport-dark connections belong to the byte-silence machinery (receive watchdog at copyTimeout during copy, keepalive idle terminate), which would have let this stall ride and the copy resume on the same connection.

Fix. createReceiveWatchdog accepts an optional transport-activity sampler (received socket bytes); the silent-window check re-arms unless the sampled counter moved and the strike survives a confirmation. A primary-silent window fires only when peer bytes moved during it AND keep moving through a transport-scale confirmation (2× pingInterval, floor 1s) — the #453 wedge signature is ping-alive-but-frame-dead, and a live peer proves itself with a fresh pong within a ping cycle. The confirmation exists because one window of movement is not proof: bytes buffered behind a receive pause or a suspended peer's kernel send buffer land moments after a re-arm (observed on a real run of the new suite as a partial-frame tail draining right after a back-pressure resume), and that residue always empties within milliseconds while it cannot recur a ping cycle later. Sizing the confirmation from the ping cadence — not a second blobTimeout — keeps the configured value the recovery bound (detection stays 1–2× blobTimeout + ≤2× pingInterval; invariant recorded in DESIGN.md item 15(c)). Windows dark on both counters re-arm and stand down. An unobservable or throwing sampler is "no evidence" — stand down and report once — never licence to fire. Only the copy-progress watchdog supplies the sampler; the byte, pause-stall, and finalize watchdogs keep their exact pre-gate path (selected at construction). Look hardest at the dark-branch state machine around the pending strike (transportEvidencePending) — it is the piece with the most edge cases (baseline acquisition, residue decay, strike cleared by progress), each pinned by a unit case.

Budget context. With production defaults (copyTimeout 300s < blobTimeout 900s) a continuously dark transport was already reaped by the byte watchdog long before the copy-progress budget, so this gate mostly matters where blobTimeout is configured below copyTimeout (tests, tightened deployments) and as the stated invariant; findStalledReceivingNodeUrls remains the 15-minute defense in depth behind both. Wedge detection with defaults moves from ≤2× blobTimeout to ≤2× blobTimeout + 60s (two default ping cycles).

harper-pro#697 should be reopened or this PR linked from it: the false-fire class it describes had a third mechanism beyond the two #695 closed (pause re-arm, superseded-instance fires) — transport-dark windows judged on the app-level budget.

For the human reviewer

  • Framing-Verdict: chosen-approach-sound (planning review, round 1; the two-window confirmation was that round's "single-window evidence vs. byte-active-at-expiry are different contracts" finding, made concrete by a real failure of the new suite 2).
  • The confirmation window began as a full second blobTimeout (round 2 of this branch); the round-2 reviewers correctly flagged that as silently redefining the configured timeout, so it is now 2× pingInterval. copyProgressWedgeRecovery pins the documented bound and that no byte-level fallback acts on the ping-alive wedge.
  • Fire-line wording changed (for <N>ms (and through a <M>ms confirmation) while peer bytes kept arriving). Nothing greps the old wording in-repo; external log dashboards keyed on the old text would need updating.
  • ws._socket?.bytesRead in the sampler adds one more instance of the file's pre-existing _socket-not-on-WebSocket type complaint (build is || true on type errors, emit unaffected). Matches the existing idiom at the byte watchdog's getBytesRead.

Verification

Refs #697


Written by Claude Fable (dispatch dev-agent).

🤖 Generated with Claude Code

Review-Coverage: authored=claude; ran=codex; adjudicated=domain; blocked=gemini(permission-denied); declined=cursor-grok,cursor-composer; rounds=3 @ b2714a1

Human-Review-Need: 4 (decisions: confirmation-window-sizing, watchdog-layer-choice, evidence-signal-choice, fail-open-on-no-evidence, pause-guard-redundancy, sigstop-integration-shape) @ b2714a1

kriszyp and others added 5 commits August 29, 2026 06:34
The copy-progress watchdog (#453) exists for ping-alive-but-frame-dead
copies, but its fire condition was only "no copy frames for blobTimeout
while unpaused" — also true when the peer is completely dark (event-loop
stall, suspended process), where the byte-silence watchdogs own recovery
on their own budgets. On the 2026-08-29 nightly (run 33239813839) a CI
runner starved the copy source's event loop for 4.4s and the subscriber's
watchdog, re-armed by a back-pressure resume with a 1500ms test budget,
killed a healthy connection that resumed on its own moments later.

createReceiveWatchdog now accepts an optional transport-activity sampler:
with it, a window silent on the primary counter fires only if received
peer bytes moved during that same window; a window dark on both counters
re-arms instead. An unobservable or throwing sampler counts as no
evidence (stand down, one debug line), never as licence to fire. Only the
copy-progress watchdog supplies the sampler; the pause-stall and
copy-finalize watchdogs guard windows where byte silence is expected.

Deterministic repro (fails on unmodified sources with the exact nightly
log line): copyProgressSourceStallFalseFire.test.mjs SIGSTOPs the source
mid-copy — below copyTimeout nothing may fire and the copy rides out the
stall on its original connection; past copyTimeout the byte-level
machinery reconnects while the copy-progress line still never appears.

Refs #697

Co-Authored-By: Claude Fable <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DTor8fFaqUcv5sqN69yvmc
Review findings from the first pre-push round, plus a race the new
suite 2 exposed on a real run: bytes buffered behind a receive pause (or
a suspended peer's kernel send buffer) can land moments after a re-arm —
observed as a partial-frame tail draining right after a back-pressure
resume — so one window of byte movement can be stale residue. A fire now
takes two consecutive copy-silent windows that each show peer-byte
movement; residue empties within one window, while a live-but-frame-dead
peer pongs in every window. Worst-case wedge detection becomes
2x blobTimeout, versus never for the wedges this watchdog exists for.

Also from the round: ungated watchdogs keep their exact pre-gate path
(sampler machinery selected at construction), a throwing
onTransportUnobservable reporter is contained like the sampler, the
stall-repro pingTimeout now exceeds the entire SIGSTOP interval instead
of only the dark window, and duplicated caller comments are trimmed.

Refs #697

Co-Authored-By: Claude Fable <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DTor8fFaqUcv5sqN69yvmc
Round-2 review: requiring a second full application window silently
redefined blobTimeout as half the effective recovery bound (900s
configured, ~30min observed). The strike confirmation now runs on a
transport-scale window (2x pingInterval, floor 1s) — long enough for a
live peer to prove itself with a fresh pong, short enough that drained
buffer residue cannot recur — so detection stays 1-2x blobTimeout plus
at most two ping cycles. copyProgressWedgeRecovery now pins that: the
copy-progress actor fires within its documented bound and no byte-level
watchdog acts on the ping-alive wedge.

Also from the round: degraded observability is reported when either
endpoint of a window was unobservable (not only the expiry sample), and
narrating comments are trimmed.

Refs #697

Co-Authored-By: Claude Fable <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DTor8fFaqUcv5sqN69yvmc
The new fires-within-bound assertions matched 'bulk copy starting from'
and 'Receive watchdog:' anywhere in the subscriber log, so the system
database's own (healthy) copy could skew the detection anchor or fail
the no-fallback check. Anchor on the db-scoped full-copy request line
and filter watchdog lines by their (db: "data") tag. Also trims
narrating comments from the delta review round.

Refs #697

Co-Authored-By: Claude Fable <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DTor8fFaqUcv5sqN69yvmc
The timing oracle measured from the subscription request, but the
watchdog only arms at COPY_START — the setup gates in between can add
unbounded scheduling delay on a loaded runner, failing the bound on a
correct run. The copy-start debug line now carries the (db: "...") tag
so the oracle can anchor on the data connection's actual arm point.
Also condenses the gate's option-block comment to the invariant
(rationale lives in DESIGN.md item 15(c)).

Refs #697

Co-Authored-By: Claude Fable <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DTor8fFaqUcv5sqN69yvmc

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a transport-evidence gate to the copy-progress watchdog to prevent false fires when a replication source goes dark (e.g., due to an event-loop stall or SIGSTOP) below the copyTimeout threshold. It updates the watchdog implementation to verify that peer bytes are actively arriving before triggering a reconnect, documents this behavior in replication/DESIGN.md, and adds comprehensive integration and unit tests. The review feedback suggests wrapping individual node teardown steps in try-catch blocks within the test cleanup hooks to ensure robust cleanup and better diagnostics.

Comment thread integrationTests/cluster/copyProgressSourceStallFalseFire.test.mjs
Comment thread integrationTests/cluster/copyProgressSourceStallFalseFire.test.mjs
@kriszyp kriszyp added this to the v5.2 milestone Aug 29, 2026
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Release cherry-pick v5.2: tests ✅ passed

Cherry-picked PR #782 onto v5.2 at branch cherry-pick/v5.2/pr-782.
Integration tests: ✅ passedhttps://github.com/HarperFast/harper-pro/actions/runs/33256348575

On merge of this PR the cherry-pick branch will be fast-forwarded into v5.2.

@kriszyp
kriszyp requested review from heskew and removed request for Devin-Holland August 31, 2026 13:20
@kriszyp
kriszyp marked this pull request as ready for review August 31, 2026 13:25
@kriszyp
kriszyp requested a review from a team as a code owner August 31, 2026 13:25
@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant