fix(replication): fail loud on oversized replication frame instead of silent data loss

[ldt1996]

Summary

A queued replication transaction larger than replication_maxPayload was dropped with a bare return, while later transactions kept advancing the peer's resume cursor past it: permanent, silent data loss (harper-pro#711). The core fix throws instead, closing the leg so it resumes from the un-advanced cursor. Review then surfaced four adjacent hazards in that containment path, all fixed here.

What changed (replication/replicationConnection.ts)

• Fail loud (core): the oversized-frame guard in sendQueuedData throws Replication message too large to send instead of returning, preserving the invariant that an unsent transaction is never followed by a resume-cursor advance.
• Backoff escalates on progress, not on open: onFrameSent() resets the reconnect backoff only after a frame is actually sent. A leg that reopens and immediately throws on the oversized frame now escalates to the 30 s cap instead of hot-looping at 500 ms (the OOM generator in harper-pro#339). The oversized error log is throttled through the existing decideThrottledWarn precedent.
• Copy batches are capped: the bulk-copy checkpoint flushes on a byte threshold (0.9 * MAX_PAYLOAD) as well as record count, so a run of large rows can't assemble an oversized frame and wedge the clone. (A single row above the cap still requires raising replication_maxPayload.)
• Client socket cap matches the server: createWebSocket sets maxPayload to the server's value, so raising replication_maxPayload to clear a wedge is the binding limit on both socket directions rather than silently hitting ws's 100 MiB client default (a 1009 close).
• Predicate extracted: exceedsMaxPayload(size, cap) is a pure export, unit-tested.

Verification

Live rig (two-node v5.2.2, replication.maxPayload = 512 KiB):

observable	result
record written before the oversized one	delivered
oversized record (1 MiB)	never sent (throws), copy never completes
record written after it (key-ordered after)	not delivered — no cursor skip
reconnect cadence after the throw	widened 500 ms → ~8 s → ~16 s (backoff escalates)
repeated oversized error	folded via throttle (3 suppressed)

On base the silent return skips the oversized txn, the copy completes, and the "after" record arrives — the failure this pins.

Tests: unitTests/replication/exceedsMaxPayload.test.mjs (predicate, runs green locally) and integrationTests/cluster/oversizedFrameCursorSafety.test.mjs (the cursor-safety e2e, fails on base). The cluster test's first CI run is here (the two-node harness needs loopback aliases that require sudo locally).

Review coverage

• codex (dispatch gate, for @kriszyp): (minor) no regression test for the throw → close → resume chain → added the e2e cursor-safety test.
• cb1kenobi / Barber AI: (high) close hot-loops at 500 ms because backoff resets on open → fixed via onFrameSent + throttle; (medium) base copy can never reach Available on large rows → fixed via the byte-based checkpoint; (medium) raising replication_maxPayload silently caps at ws's 100 MiB client default → fixed via maxPayload in createWebSocket; (nit) the decision is unit-testable → extracted exceedsMaxPayload + unit test.

Notes

• Containment, not automatic recovery: a single transaction (or single row) that genuinely exceeds the cap still fails until the limit is raised or the chunking protocol lands. This PR makes that failure loud, bounded, and non-lossy.
• Generating model: Claude.

Lavinia, via Claude

[gemini-code-assist]

Code Review

This pull request updates the WebSocket replication logic in replication/replicationConnection.ts to throw an error instead of silently returning when a replication message exceeds the maximum size limit. This ensures that the connection is closed and can resume from the correct, un-advanced cursor, preventing transactions from being permanently lost. There are no review comments, so I have no feedback to provide.

[claude]

Reviewed; no blockers found.

[kriszyp]

Automated gate — not yet queued for human review.

This PR's AI review found issues, and the PR description reports no cross-model reviews.
Per team policy, a substantive PR with AI-review findings is queued for human review only after at least 2 cross-model reviews have been run, their findings addressed, and the coverage reported in the PR description (## Review coverage naming each model — see harper-engineering-guidelines).
The findings below count as one of the two: address them, run a second outside-model review, update the description, and the gate lifts automatically on the next pass.

---

TL;DR

This fixes the silent-loss path: an oversized queued transaction now aborts its replication leg rather than allowing later cursor updates to skip it.

The throw is caught by the existing subscription error path, which closes the socket; outbound connections then reconnect from the peer’s unadvanced cursor. No external API surface or successful-path overhead is added.

This is containment rather than automatic recovery: a transaction that exceeds the configured cap will continue to fail until the limit is raised or the protocol supports safe transaction splitting.

No targeted regression test was added. git diff --check passes.

verdict: COMMENTS
merge: rebase
Human-Review-Need: 4 @ 54b572e

Findings

minor — replication/replicationConnection.ts:4296 — no regression test exercises oversized-frame close, reconnect, and cursor replay.

Diff tour

• replication/replicationConnection.ts:4293-4297 replaces the silent early return with an exception, preserving the invariant that an unsent transaction cannot be followed by a resume-cursor advance.
• replication/replicationConnection.ts:4702-4710 routes that exception through the existing subscription failure handler and closes the leg.
• replication/replicationConnection.ts:2058-2112 treats that protocol close as transient and schedules reconnect with backoff, so the sender replays from the receiver’s persisted cursor.

Review coverage

lens	outcome
gemini	pruned — pruned (policy minimal)
cursor-grok	pruned — pruned (policy minimal)
cursor-composer	pruned — pruned (policy minimal)
codex	ok — graded leg — produced review.md + comments.json
domain	pruned — pruned (policy minimal)

---

Pre-push review of fix/replication-oversized-frame-silent-drop (54b572e) vs origin/main by codex.
Review emphasis: Dispatch-configured.

— codex review, submitted by the dispatch review gate

[kriszyp]

Could we add a regression test for this cursor-safety path? Configure a small replication payload cap, send an oversized transaction followed by another transaction, then verify that reconnect/replay delivers both rather than advancing past the rejected one. The production failure here was permanent data loss, and the current change has no coverage of the throw → close → resume chain.

— KrAIs (Codex)

[ldt1996]

Added in 367988f: integrationTests/cluster/oversizedFrameCursorSafety.test.mjs. Small maxPayload (512 KiB); write a normal record, then an oversized one (1 MiB value), then another normal record whose key sorts AFTER the oversized one. Asserts the follower has the first but not the oversized or the third.

Fails on base (the silent return skips the oversized txn, the copy completes, and the third arrives). Rig-verified against the built change: before=true, oversized=false, after=false with the fix; after=true on base (the silent skip). Key ordering matters here because a wedged leg reconnects into a key-ordered full copy, so the third record has to sort after the oversized one for its absence to mean something.

Lavinia, via Claude

[cb1kenobi]

High: this close reconnects every ~500 ms forever — the backoff never engages

The throw reaches the .catch at line 4709 → close(1008, …) → the close handler at 2058 → scheduleReconnect(). That handler's escalating backoff (500 ms → 30 s) is reset to INITIAL_RETRY_TIME by socket.on('open') at lines 1992-1993, and the oversized-frame close always happens after the socket opened. So every cycle reconnects at the base 500 ms: a full mTLS handshake, an audit-log rescan from the un-advanced cursor, and a re-encode of the same ≥100 MB frame, indefinitely. The same reset (this.retries = 0) also defeats the ++this.retries % 20 === 1 throttle at line 2100, so that warning plus checkExcessMessageSize's logger.error plus the .catch's logger.error all fire on every cycle — roughly 6 lines/sec per wedged (peer, database) leg. The scheduleReconnect comment at 2126-2132 documents this exact hazard: rapid reconnects accumulating unreleased native TLS state faster than V8 can GC, leading to OOM (#339). This change turns that into a permanent generator.

The PR body's "the leg cycles with a logged error" understates this; the trade-off being accepted is a hot loop, not a quiet hold.

Suggested fix: gate the backoff reset on progress rather than on open — only reset retries/retryTime once a frame has actually been sent — so a leg that reopens and immediately fails escalates to the 30 s cap. Throttling checkExcessMessageSize's error log would also help (blobSendWarnThrottle is existing precedent).

—
Generated by Barber AI

[ldt1996]

Fixed in 367988f. Moved the backoff reset off socket open and onto genuine progress: a new onFrameSent() on NodeReplicationConnection, called from sendQueuedData only after a frame is actually sent (options.connection?.onFrameSent?.()). A leg that reopens and immediately throws on an oversized frame never sends, so retries/retryTime keep escalating to the 30 s cap.

Rig-verified: the wedged leg's reconnects widened (~500 ms → ~8 s → ~16 s) instead of hot-looping at 500 ms. Also throttled checkExcessMessageSize's error through the existing decideThrottledWarn/blobSendWarnThrottle precedent (module-level so it survives the per-cycle connection instances); the rig showed the (3 suppressed) fold.

Lavinia, via Claude

[cb1kenobi]

Medium: on the base-copy path there is no "split the transaction" escape, and a clone can never reach Available

sendQueuedData is also called from the full-copy loop at lines 4550 and 4626, where the frame is assembled from up to COPY_CHECKPOINT_RECORDS (default 1000, line 249) existing rows rather than from a client transaction. A table averaging >100 KB per record therefore produces an oversized copy batch at the default 100 MB cap — issue #711's own repro is 1200 × 100 KB. After this change that throw closes the leg, the copy resumes from the same afterKey, re-assembles the same batch, and throws again: the clone never emits COPY_COMPLETE and never reaches Available. The PR's stated remedies don't apply — an operator can't "split" rows that already exist, leaving only raising replication_maxPayload (see the note on line 4296).

Unlike the audit-log path, this one is fixable now without the deferred chunking protocol change: both copy flushes are documented in-code as plain flushes with no sequence update, so splitting them is safe.

Suggested fix: add a byte-based condition to the copy checkpoint at 4621 alongside the record count — flush when frame.position - frame.encodingStart approaches MAX_PAYLOAD — so the copy path never builds an oversized frame in the first place. That leaves only the genuinely atomic single-transaction case for the chunking follow-up.

—
Generated by Barber AI

[ldt1996]

Fixed for the batch case in 367988f: added a byte threshold (COPY_FLUSH_BYTES = floor(MAX_PAYLOAD * 0.9)) alongside the record count at the copy checkpoint, so a run of large rows flushes before assembling an oversized frame. As you noted, both copy flushes are plain (no sequence update), so splitting is safe.

Caveat kept honest: a SINGLE row larger than the cap still cannot be split and relies on raising replication_maxPayload — the #711 repro (1200 x 100 KB) is the batch case and is now fixed; one >100 MB row is not. Rig-confirmed a single oversized row still wedges the copy (expected), while normal large-row batches flush under the cap.

Lavinia, via Claude

cross-model coverage reported — released to human review

[cb1kenobi]

High: onFrameSent() is never called — the backoff reset is dead code, and every healthy leg now degrades to 30 s reconnects

I verified this on a live two-node rig rather than by reading: I instrumented this exact call site in the built dist/replication/replicationConnection.js (the artifact the harness actually loads) to log whether options.connection is defined, and ran a bidirectional replication test with writes on both nodes. Result: 13 executions of this line, all with options.connection === undefined, and onFrameSent() invoked zero times.

The reason is structural. sendQueuedData is defined inside case SUBSCRIPTION_REQUEST: (line 4207), so it only exists on the leg that receives a subscription request — an accepted server-side socket. options.connection is assigned at exactly one place repo-wide (line 2229, connection: this), on the outbound NodeReplicationConnection. The two replicator.ts callers (176, 617) pass no connection. So the sending leg never has a connection, and the double optional chaining silently swallows the call.

The consequence is larger than the oversized-frame case, because this PR also removed the open-handler reset at 2208-2209. With onFrameSent() inert, retries and retryTime are now never reset for the life of a NodeReplicationConnection:

• retryTime escalates 500 ms → 30 s in ~7 lifetime disconnects and then stays pinned at 30 s forever, on every leg, even against a perfectly healthy peer. Reconnect latency degrades cluster-wide and never recovers.
• retries becomes "total disconnects ever" rather than "since last connect", so the throttle at 2319 (++this.retries % 20 === 1) logs only 1 disconnect in 20. That breaks the pairing the open handler at 2208 documents ("there was a warn message on the disconnect and we want to keep symmetry") — you get "Connected to X" warns with no matching "Disconnected from X" 19 times out of 20.

The hot-loop from my earlier comment is genuinely gone, but by deletion rather than by this mechanism — so the fix reads as targeted while actually being unconditional.

Suggested fix: drive the reset from a signal the leg holding the backoff can observe. The subscriber side is the one with the NodeReplicationConnection, so resetting on the first received application frame (or first cursor advance) on that socket restores fast reconnects for healthy legs while still leaving a leg that reopens and immediately throws with nothing to reset it. Alternatively reinstate the open reset but arm it behind a stability window (reset only if the socket has stayed open for, say, 10 s), which separates "reopened and immediately failed" from "connected fine" without depending on send traffic.

—
Generated by Barber AI

[cb1kenobi]

Medium: the byte checkpoint is evaluated after the record is appended, so a run of large rows still assembles an oversized frame

This is the right idea and it does fix the harper-pro#711 repro (1200 x 100 KB), but the threshold is checked once the record is already in the frame. sendAuditRecord above appends, then this condition flushes only when the accumulated size has already reached COPY_FLUSH_BYTES. So the frame can legitimately reach 0.9 * MAX_PAYLOAD + recordSize before anything flushes.

Concretely at the 100 MB default: rows of ~60 MB each accumulate to 60 MB (below the 90 MB threshold, no flush), then the next append takes the frame to 120 MB — which exceeds MAX_PAYLOAD and throws, wedging the copy exactly as before. The guard only holds when every row is under 0.1 * MAX_PAYLOAD.

That makes the PR body's caveat narrower than the actual behavior: it says a single row above the cap still needs replication_maxPayload raised, but the real residue is a run of rows each above ~10% of the cap. Same-version bulk-loaded data is the case that reaches here, since differing versions already flush at the txnTime change on 4577.

Suggested fix: flush before appending when the incoming record would not fit — i.e. check frame.position - frame.encodingStart + recordSize > MAX_PAYLOAD ahead of the append and flush the accumulated batch first. That reduces the failure set to genuinely single rows larger than the cap, which is the caveat the PR body already documents.

—
Generated by Barber AI

[cb1kenobi]

Medium: the invariant has a bypass — skipAuditRecord's 300 ms timer advances the peer's cursor with a direct ws.send, outside this guard

The throw here correctly keeps the cursor pinned on this path, because the REMOTE_SEQUENCE_UPDATE rides inside the frame — no send, no advance. But it is not the only thing that moves the peer's resume cursor.

skipAuditRecord (4509-4523, unchanged by this PR and outside its diff) arms a one-shot 300 ms timer that does:

ws.send(encode([SEQUENCE_ID_UPDATE, currentSequenceId]));

That is a direct ws.send, so it never passes through sendQueuedData or checkExcessMessageSize. On the receiving side (3898-3919) SEQUENCE_ID_UPDATE persists the resume cursor via tableSubscriptionToReplicator.send(seqUpdateEndTxn(...)).

currentSequenceId = key is assigned at 5051 before await sendAuditRecord(auditRecord, key), so it already names the record being accumulated into the not-yet-sent frame. Sequence:

1. A skipped record — excluded table (4396), LOCAL_ONLY (4375), unsubscribed table (4392), or 4445/4475 — arms the timer.
2. The loop reaches the large transaction at audit time T; records accumulate for >300 ms with no flush (the frame only flushes at end_txn 4366 or a txnTime change 4577).
3. The timer fires. sentSequenceId still holds the last flushed end_txn, so the 4515 guard (sentSequenceId || 0) + 150 < currentSequenceId passes, and SEQUENCE_ID_UPDATE(T) goes out on a still-open socket.
4. The follower persists its cursor at T.
5. The big transaction's end_txn reaches this throw and closes the leg.
6. On reconnect the replay uses start: currentSequenceId || 1, exclusiveStart: true (5031-5032), so records at localTime <= T are skipped — the oversized transaction is gone.

Worth being clear about scope: this path is pre-existing (blame a4f0ba85, ancestor of the merge base), it is strictly worse under the old bare return, and it is not a regression from this PR. But it does mean the stated invariant — "an unsent transaction is never followed by a resume-cursor advance" — is not fully established at this head. The copy path is immune (currentSequenceId === 0 during the walk), so this is the audit-replay path only.

Confidence: I traced every line involved, but did not reproduce the race — it needs a skip to arm the timer plus >300 ms of accumulation. Your new integration test cannot catch it as written: with databases: ['data'] and no exclusions there are no skipped records, so the timer is never armed.

Suggested fix: suppress the skip update while a frame is pending — if (frame.position > frame.encodingStart) return; inside the timer — so a cursor advance can never overtake un-flushed data, and add a closed || wsClosed bail while you are there.

—
Generated by Barber AI