fix(replication): close on per-record value-decode failure instead of skipping past it (#440) by kriszyp · Pull Request #521 · HarperFast/harper-pro

kriszyp · 2026-07-05T03:52:51Z

What

The inner per-record value-decode catch in onWSMessage (around decodeBlobsWithWrites) previously logged and swallowed a decode failure while the batch resume cursor still advanced past the un-decoded record — maxBatchVersion → the end_txn localTime → the sender's COMMITTED_UPDATE confirm all move forward regardless of whether event was produced. The result is a permanent, undetected [error, head] gap on the receiver: the record is never applied and never redelivered.

This is the #1163/#1453 structure-fork error class (an "end of buffer" / undecodable-record throw when the receiver's decoder structures diverge from the sender's), and the residual explicitly deferred from #511 — epic #430 Theme B, workstream #440.

The fix

On a resolved-decoder value-decode failure the catch now latches the (boxed) error and re-throws — after the existing accepted-blob cleanup, and before the maxBatchVersion/RECEIVED_VERSION advance and the end_txn send — so it rides the outer transient-close path introduced by #511 (closeOnInboundMessageError → close(1011) → reconnect + resume from the last durable cursor).

Because the throw precedes every cursor advance, the resume cursor cannot move past the hole, and the sender re-streams from the last durable position on reconnect.

Why close, not skip (the deferred decision)

A reconnect rebuilds this table's per-connection decoder from the peer's freshly re-sent typedStructs/structures (the SET_TABLE handshake), so a forked/stale-structure decode heals on resume. Skip-and-log has zero chance of healing a fork (same forked decoder, same connection, forever) and permanently loses the record — so close strictly dominates skip. A genuinely undecodable frame degrades to a visible, backed-off reconnect loop rather than silent loss; bounding that loop with an escalation budget is W2 (#432) territory.

An unknown tableId (decoder unresolved — a distinct, transient schema-propagation case, not a structure fork) still skips, since a reconnect wouldn't supply the missing table def. The disposition is extracted as the pure helper classifyRecordDecodeError (following #440's decision-helper method) and pinned by recordDecodeErrorRecovery.test.mjs.

Cross-model review (thorough — Codex + Gemini + Harper domain pass)

No blockers. Traced and confirmed: cursor cannot advance past the failed record; accepted blobs cleaned before the throw; outstandingCommits balanced (incremented only after the record loop, which the throw exits first); copy-mode cursor safe; unknown-tableId still skips. The re-throw is boxed ({ error }) so a falsy thrown value can't defeat it.

Known, pre-existing tradeoff (not introduced here)

Any mid-frame error that reaches the outer catch after ≥1 record was already dispatched can, on reconnect, commit that frame prefix without the failed record (core Table.ts resolves the stale txnInProgress on the next beginTxn). This is inherent to #511's close-resume path — this change only adds a new trigger onto it — and is strictly better than the skip-and-log status quo, which committed the prefix and dropped the record permanently. Under Harper's versioned/convergent source-apply the redelivered records dedup/apply idempotently, so the frame converges to whole once the failed record heals. If strict per-frame atomicity is ever required, the fix (decode-all-before-dispatch, or an abort barrier on reconnect) is a candidate for W2/#432. A related bounded disk-hygiene follow-up: sweep earlier-record accepted blobs on frame abort.

Stacking

Stacked on #511 (base kris/decode-loop-error-close-440, rebased onto current main). Merge #511 first.

Test

unitTests/replication/recordDecodeErrorRecovery.test.mjs — 3 cases pinning both dispositions and the resume-cursor-safety contract.

🤖 Generated with Claude Code

…swallowing them (#440) An error escaping onWSMessage was logged and swallowed: the rest of the failed frame was silently dropped while later frames kept applying and confirming higher sequence ids — a permanent, undetected gap on the receiver (epic #430 Theme B / workstream #440 land-first safety item). Now the catch latches inbound processing off (queued frames on the messageProcessing chain must not commit past the hole) and closes with 1011 as a transient protocol close, so the normal retry path reconnects and resumes from the last durable cursor, re-streaming everything past it. Extracted as closeOnInboundMessageError with unit coverage, following the file's pure-decision-helper idiom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…rame-vs-record error boundary (#440) Cross-model review findings on the previous commit: an unknown tableId made the inner decode-catch's own log line throw (unguarded tableDecoder.decoder deref), masking the original error and escaping to the outer close path by accident. Guard the derefs so the intended skip-and-log semantics and truthful diagnostics are restored. Also scope the closeOnInboundMessageError docstring: per-record value-decode failures are still skip-and-logged with the cursor advancing past them (residual tracked in #440 / #432). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…review) Gemini findings: guard the logger access fully (the log must never prevent the close) and make the decode-error log readable when the table decoder is unknown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… skipping past it The inner catch around decodeBlobsWithWrites in onWSMessage logged and swallowed a per-record value-decode error while the batch resume cursor (maxBatchVersion -> end_txn -> sender COMMITTED_UPDATE) advanced past the un-decoded record regardless of `event` -- a permanent, undetected [error, head] gap on the receiver. This is the #1163/#1453 structure-fork error class (epic harper-pro#430 Theme B; workstream #440). Route it onto the outer transient-close path established by #511: on a resolved-decoder value-decode failure the catch now re-throws (after the existing accepted-blob cleanup, before the cursor advances) so closeOnInboundMessageError closes 1011 -> reconnect + resume from the last durable cursor. The reconnect rebuilds the per-connection table decoder from the peer's re-sent typedStructs/structures (the SET_TABLE handshake), so a forked/stale-structure decode heals on resume; a genuinely undecodable frame degrades to a visible, backed-off reconnect loop rather than silent loss (bounding that loop is W2 / harper-pro#432). An unknown tableId (decoder unresolved -- a distinct, transient schema-propagation case, not a structure fork) stays skip-and-log, since a reconnect wouldn't supply the missing table def. The decision is extracted as the pure helper classifyRecordDecodeError and pinned by recordDecodeErrorRecovery.test.mjs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request improves replication reliability by ensuring that value-decode failures on resolved table decoders trigger a connection close and reconnect rather than being silently skipped, which previously led to permanent data gaps. It introduces a helper function classifyRecordDecodeError to distinguish between resolved decoders (which should close) and unresolved decoders (which should skip and log), accompanied by unit tests. The review feedback suggests a cleaner, more idiomatic alternative to boxing the error in an object, recommending a boolean flag and a separate error variable instead.

gemini-code-assist · 2026-07-05T03:54:45Z

+				// Boxed (not the bare error) so the re-throw below can't be defeated by a falsy thrown value.
+				let decodeFailure: { error: unknown } | undefined;


Instead of boxing the error in an object { error } to avoid falsy values in a truthiness check, you can use a simple boolean flag hasDecodeFailure and a separate decodeError variable. This is cleaner, more idiomatic, and avoids the need for explanatory comments about boxing.

Suggested change

// Boxed (not the bare error) so the re-throw below can't be defeated by a falsy thrown value.

let decodeFailure: { error: unknown } | undefined;

let hasDecodeFailure = false;

let decodeError: unknown;

gemini-code-assist · 2026-07-05T03:54:45Z

+					// A resolved-decoder value-decode failure must close, not skip: `maxBatchVersion` (and the
+					// end_txn resume cursor) advance below regardless of `event`, so skipping loses the record
+					// permanently. Latch it to re-throw onto the outer close path. See classifyRecordDecodeError.
+					if (classifyRecordDecodeError(!!tableDecoder) === 'close') decodeFailure = { error };


If you adopt the hasDecodeFailure boolean flag pattern, update this line to set the flag and store the error.

Suggested change

if (classifyRecordDecodeError(!!tableDecoder) === 'close') decodeFailure = { error };

if (classifyRecordDecodeError(!!tableDecoder) === 'close') {

hasDecodeFailure = true;

decodeError = error;

}

gemini-code-assist · 2026-07-05T03:54:46Z

 					// a window to complete, then unlink the files. (mirrors the pattern at the relocate path above.)
 					setTimeout(() => receivedBlobs.forEach(deleteBlob), 60000).unref();
 				}
+				if (decodeFailure) throw decodeFailure.error; // re-throw after blob cleanup, before the resume cursor advances past this record


If you adopt the hasDecodeFailure boolean flag pattern, update this line to throw the stored error.

Suggested change

if (decodeFailure) throw decodeFailure.error; // re-throw after blob cleanup, before the resume cursor advances past this record

if (hasDecodeFailure) throw decodeError; // re-throw after blob cleanup, before the resume cursor advances past this record

claude · 2026-07-05T03:56:32Z

Reviewed; no blockers found.

cb1kenobi

My AI review sees nothing wrong with this PR, but I think there's a dash of bloat. Not approving, but also not blocking.

cb1kenobi · 2026-07-10T03:51:36Z

+					// A resolved-decoder value-decode failure must close, not skip: `maxBatchVersion` (and the
+					// end_txn resume cursor) advance below regardless of `event`, so skipping loses the record
+					// permanently. Latch it to re-throw onto the outer close path. See classifyRecordDecodeError.
+					if (classifyRecordDecodeError(!!tableDecoder) === 'close') decodeFailure = { error };


This seems like an overly complicated way of saying:

Suggested change

if (classifyRecordDecodeError(!!tableDecoder) === 'close') decodeFailure = { error };

if (tableDecoder) decodeFailure = { error };

Maybe add a comment?

kriszyp and others added 4 commits July 4, 2026 16:01

kriszyp requested a review from a team as a code owner July 5, 2026 03:52

kriszyp requested review from DavidCockerill and heskew July 5, 2026 03:53

kriszyp mentioned this pull request Jul 5, 2026

Replication W11: Code organization, protocol versioning & decode-loop safety #440

Open

13 tasks

gemini-code-assist Bot reviewed Jul 5, 2026

View reviewed changes

cb1kenobi reviewed Jul 10, 2026

View reviewed changes

Base automatically changed from kris/decode-loop-error-close-440 to main July 10, 2026 04:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(replication): close on per-record value-decode failure instead of skipping past it (#440)#521

fix(replication): close on per-record value-decode failure instead of skipping past it (#440)#521
kriszyp wants to merge 4 commits into
mainfrom
kris/inner-decode-close-440

kriszyp commented Jul 5, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Jul 5, 2026

Uh oh!

gemini-code-assist Bot Jul 5, 2026

Uh oh!

gemini-code-assist Bot Jul 5, 2026

Uh oh!

claude Bot commented Jul 5, 2026

Uh oh!

cb1kenobi left a comment

Uh oh!

cb1kenobi Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Boxed (not the bare error) so the re-throw below can't be defeated by a falsy thrown value.
		let decodeFailure: { error: unknown } \| undefined;

-					if (classifyRecordDecodeError(!!tableDecoder) === 'close') decodeFailure = { error };
+					if (classifyRecordDecodeError(!!tableDecoder) === 'close') {
+						hasDecodeFailure = true;
+						decodeError = error;
+					}

	if (decodeFailure) throw decodeFailure.error; // re-throw after blob cleanup, before the resume cursor advances past this record
	if (hasDecodeFailure) throw decodeError; // re-throw after blob cleanup, before the resume cursor advances past this record

	if (classifyRecordDecodeError(!!tableDecoder) === 'close') decodeFailure = { error };
	if (tableDecoder) decodeFailure = { error };

Uh oh!

Conversation

kriszyp commented Jul 5, 2026

What

The fix

Why close, not skip (the deferred decision)

Cross-model review (thorough — Codex + Gemini + Harper domain pass)

Known, pre-existing tradeoff (not introduced here)

Stacking

Test

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jul 5, 2026

Uh oh!

cb1kenobi left a comment

Choose a reason for hiding this comment

Uh oh!

cb1kenobi Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants