From e45275898a4e768bed138da9ff7e39bef7a82599 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Thu, 4 Jun 2026 11:40:56 -0700 Subject: [PATCH 1/4] fix(replay): recover from corrupt transaction-log frames instead of aborting startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A torn or corrupt transaction-log frame (a torn write at crash time, or a flipped length byte in an unflushed/corrupt log) makes rocksdb-js 1.4.x's reader throw a bounded RangeError ("declared length N overruns the log" / "truncated entry header") out of its query iterator's .next(). That throw escaped uncaught through RocksTransactionLogStore.getRange()'s consumers — startup replay (replayLogs.ts) and replication broadcast (transactionBroadcast.ts) — aborting the boot with an uncaughtException, then a fatal upgrade-abort on the next boot. rocksdb-js is correct to surface corruption loudly (#612); the consumer decides policy. Wrap each log.query() iterator in getRange with endIteratorOnCorruptFrame (new pure, callback-based helper in replayLogsGuards.ts): catch the framing RangeError, log once via harperLogger.warn, and treat the corrupt frame as end-of-log — torn-write semantics, since once framing is lost the next entry cannot be located. Entries before the corruption have already been replayed; non-RangeError failures propagate unchanged. Also de-flakes the replay-stress suite's byte-flip test (#1136), the top Integration Tests flake: it replaces nondeterministic random byte flips plus a 20s wall-clock replay budget and an rss-growth heuristic with a deterministic corruption — corrupt the last well-framed entry's length prefix (the unflushed tail replay actually reads) — and a behavioral recovery assertion (server comes back, tables queryable), guarded by corrupted>0 so it can't pass vacuously. Closes #1135 Closes #1136 Co-Authored-By: Claude Opus 4.8 (1M context) --- integrationTests/server/replay-stress.test.ts | 93 ++++++++++--------- resources/RocksTransactionLogStore.ts | 12 ++- resources/replayLogsGuards.ts | 46 +++++++++ unitTests/resources/replayLogs.test.js | 60 +++++++++++- 4 files changed, 166 insertions(+), 45 deletions(-) diff --git a/integrationTests/server/replay-stress.test.ts b/integrationTests/server/replay-stress.test.ts index 9fb3e30f16..9049662ffd 100644 --- a/integrationTests/server/replay-stress.test.ts +++ b/integrationTests/server/replay-stress.test.ts @@ -8,9 +8,10 @@ * 1. Clean crash — replay must recover every row inserted before SIGKILL * 2. Truncated tails — last bytes of each txnlog stripped (simulates a torn * write at the moment of crash); replay must still come back up - * 3. Random byte flips — msgpack-shaped corruption sprinkled across every - * txnlog; replay must finish without the CPU-spin regression that - * replayLogsGuards.ts (commit d0190ff5a) fixed + * 3. Corrupt length prefix — the first entry's declared length is forced to + * overrun the log (a torn/corrupt frame). rocksdb-js's reader throws a bounded + * RangeError for this; replay/broadcast must treat it as end-of-log and the + * server must come back up rather than aborting startup (HarperFast/harper#1135) * * All three scenarios share one suite/ctx to avoid the schema-registry leak * that surfaces when multiple suites each call create_database in the same @@ -18,9 +19,8 @@ */ import { suite, test, before, after } from 'node:test'; import { ok, equal } from 'node:assert/strict'; -import { readdirSync, statSync, openSync, readSync, writeSync, truncateSync, closeSync } from 'node:fs'; +import { readdirSync, readFileSync, statSync, openSync, writeSync, truncateSync, closeSync } from 'node:fs'; import { join } from 'node:path'; -import { setTimeout as sleep } from 'node:timers/promises'; import { startHarper, @@ -29,6 +29,12 @@ import { type ContextWithHarper, type HarperContext, } from '@harperfast/integration-testing'; +import { constants } from '@harperfast/rocksdb-js'; + +// Transaction-log framing (all big-endian): a fixed-size file header, then a run of entries +// each shaped [float64 timestamp][uint32 length][flags byte][length bytes of data]. So an +// entry's declared length lives at entryStart + 8, and the byte there is its most-significant. +const { TRANSACTION_LOG_FILE_HEADER_SIZE, TRANSACTION_LOG_ENTRY_HEADER_SIZE } = constants; const DB = 'stress'; const TABLES = ['orders', 'items', 'events']; @@ -128,26 +134,33 @@ function truncateTail(path: string, bytes: number) { truncateSync(path, size - bytes); } -function flipBytes(path: string, count: number, seed: number) { - const size = statSync(path).size; - // Skip the 13-byte file header (4 token + 1 version + 8 ts) so we exercise - // the per-entry decoder hardening, not file-open validation. - const start = 13; - if (size <= start + 32) return; +function corruptLastEntryLength(path: string): boolean { + // Walk the entry frames to the *last* well-framed entry, then force its declared length + // to overrun the log by setting the most-significant byte of its big-endian uint32 length + // to 0xff (≥ 4 GB). Targeting the last entry puts the corruption in the unflushed tail + // that replay actually reads (replay starts from the last-flushed position), rather than a + // flushed prefix it skips over. Deterministic — same frame, same corruption — unlike the + // old random byte flips, whose effect depended on what RocksDB had flushed before SIGKILL. + const buf = readFileSync(path); + let pos = TRANSACTION_LOG_FILE_HEADER_SIZE; + let lastLengthPos = -1; + while (pos + TRANSACTION_LOG_ENTRY_HEADER_SIZE <= buf.length) { + if (buf.readDoubleBE(pos) === 0) break; // a zero timestamp marks end-of-log to the reader + const lengthPos = pos + 8; + const length = buf.readUInt32BE(lengthPos); + const next = pos + TRANSACTION_LOG_ENTRY_HEADER_SIZE + length; + if (length === 0 || next > buf.length) break; // ran past the end / already unframable + lastLengthPos = lengthPos; + pos = next; + } + if (lastLengthPos < 0) return false; const fd = openSync(path, 'r+'); try { - const buf = Buffer.alloc(1); - let s = seed; - for (let i = 0; i < count; i++) { - s = (s * 1103515245 + 12345) & 0x7fffffff; - const pos = start + (s % (size - start)); - readSync(fd, buf, 0, 1, pos); - buf[0] ^= 0xa5; - writeSync(fd, buf, 0, 1, pos); - } + writeSync(fd, Buffer.from([0xff]), 0, 1, lastLengthPos); } finally { closeSync(fd); } + return true; } async function crashAndRestart(ctx: ContextWithHarper, mutate?: (dataRootDir: string) => void) { @@ -197,37 +210,33 @@ suite('Transaction log replay stress', (ctx: ContextWithHarper) => { } }); - test('crash with random byte flips in txnlogs', async () => { + test('crash with corrupt length-prefix recovers without aborting startup', async () => { for (const table of TABLES) { const records = []; for (let i = 0; i < 500; i++) records.push(makeRecord(200_000 + i)); await op(ctx.harper, { operation: 'insert', database: DB, table, records }); } - const replayMs = await crashAndRestart(ctx, (dataRootDir) => { - // User-DB only: flipping bytes inside system/ txnlogs corrupts - // version-tracking and Harper aborts on next boot with an upgrade - // error. That's a real failure mode but not what this test is about — - // here we want to exercise replay's per-entry decode hardening on - // records that replay genuinely needs to skip rather than reject the - // whole boot. - const files = listTxnLogFiles(dataRootDir, { userOnly: true }); - files.forEach((f, i) => flipBytes(f, 32, 0xcafe + i)); + let corrupted = 0; + await crashAndRestart(ctx, (dataRootDir) => { + // User-DB only: corrupting system/ txnlogs trips the version-tracking + // upgrade-abort path on next boot — a real but different failure mode. + // Here we want the framing-corruption case (#1135): a declared length that + // overruns the log used to throw an uncaught RangeError out of the txnlog + // iterator and abort startup; replay must now treat it as end-of-log. + for (const f of listTxnLogFiles(dataRootDir, { userOnly: true })) { + if (corruptLastEntryLength(f)) corrupted++; + } }); - // Tighter than the 60s global cap. With the per-entry decode guard in place, - // healthy startup is ~3s on this corpus. Without it, every corrupt entry - // logs a stack trace inside the loop and startup balloons to ~50s. 20s - // catches that regression while leaving CI headroom. - ok(replayMs < 20_000, `replay took ${replayMs}ms — guard regression?`); + // Fail loudly rather than vacuously pass if the framing ever changes and we corrupt + // nothing — otherwise this would silently stop being a regression test for #1135. + ok(corrupted > 0, 'expected to corrupt at least one user-DB txnlog'); + // Behavioral assertion, not a wall-clock budget: crashAndRestart resolving above + // means the server came back up (startHarper saw 'successfully started'); a + // regression resurfaces as a failed/timed-out restart, not a slow one — so there + // is nothing to tune per-runner. Confirm every table is still queryable. for (const t of TABLES) { const c = await countRows(ctx.harper, t); ok(typeof c === 'number' && c >= 0, `count on ${t} should be a number, got ${c}`); } - // Confirm Harper isn't in a CPU-spin loop post-replay: rss should be - // roughly stable after startup is reported done. The pre-fix bug pinned - // a core forever and rss grew steadily under the spin. - const rssBefore = ctx.harper.process.resourceUsage?.()?.maxRSS ?? 0; - await sleep(500); - const rssAfter = ctx.harper.process.resourceUsage?.()?.maxRSS ?? 0; - ok(rssAfter - rssBefore < 200 * 1024, `rss grew by ${rssAfter - rssBefore}KB after replay`); }); }); diff --git a/resources/RocksTransactionLogStore.ts b/resources/RocksTransactionLogStore.ts index 8cd899dfb0..5096c7b86b 100644 --- a/resources/RocksTransactionLogStore.ts +++ b/resources/RocksTransactionLogStore.ts @@ -2,6 +2,7 @@ import { TransactionLog, RocksDatabase, shutdown, type TransactionEntry } from ' import { ExtendedIterable } from '@harperfast/extended-iterable'; import { getIdOfRemoteNode } from './nodeIdMapping.ts'; import { Decoder, readAuditEntry, ENTRY_DATAVIEW, AuditRecord, createAuditEntry } from './auditStore.ts'; +import { endIteratorOnCorruptFrame } from './replayLogsGuards.ts'; import { isMainThread } from 'node:worker_threads'; import { EventEmitter } from 'node:events'; import { asBinary } from 'lmdb'; @@ -21,6 +22,13 @@ type TransactionLogIterator = Iterator & { removeLog(logName: string); }; +// Logs (once per log) when a corrupt frame ends a query iterator early; see +// endIteratorOnCorruptFrame in replayLogsGuards.ts for why this is end-of-log, not a crash. +function warnCorruptFrame(logName: string) { + return (error: RangeError) => + harperLogger.warn(`Stopping transaction log "${logName}" at a corrupt entry during replay`, error); +} + /** * Represents a transaction log store backed by RocksDB. * This class provides methods that conform to a standard store interface @@ -190,7 +198,7 @@ export class RocksTransactionLogStore extends EventEmitter { log = this.rootStore.useLog(options.log); } } - const queryIterator = log.query(options); + const queryIterator = endIteratorOnCorruptFrame(log.query(options), warnCorruptFrame(log.name)); iterable.iterate = () => queryIterator; } else { const onlyKeys = options.onlyKeys; @@ -217,7 +225,7 @@ export class RocksTransactionLogStore extends EventEmitter { // condition of potentially missing an initial update queryOptions = { ...options, start: options.start ?? 0 }; } - iterators.push(log.query(queryOptions)); + iterators.push(endIteratorOnCorruptFrame(log.query(queryOptions), warnCorruptFrame(log.name))); } } latestUpdates = this.updates; diff --git a/resources/replayLogsGuards.ts b/resources/replayLogsGuards.ts index 054f3c7f6f..fb03e546c9 100644 --- a/resources/replayLogsGuards.ts +++ b/resources/replayLogsGuards.ts @@ -40,3 +40,49 @@ export function classifyAuditEntryForReplay( if ((action & RECORD_BEARING_FLAGS) !== 0 && !hasRecord) return 'missing-record'; return null; } + +/** + * Wraps a transaction-log query iterator so a framing-level corruption error ends + * iteration of that log cleanly instead of escaping as an uncaughtException. + * + * rocksdb-js's txnlog reader throws a bounded `RangeError` when an entry's declared + * length overruns the log or its header is truncated (intentional hardening — it would + * otherwise OOM on `allocUnsafe(bogusLength)` or deref an undefined buffer). A torn write + * at SIGKILL time, or a flipped byte in an unflushed/corrupt log, looks exactly like that. + * Once the framing is lost we can't locate the next entry, so the corrupt frame marks the + * usable end of this log (torn-write semantics): entries before it have already been + * yielded, and startup replay / replication broadcast must continue rather than abort the + * boot. The latch means a persistently-corrupt log is reported once, not on every re-poll. + * + * `onCorruptFrame` is invoked once, with the error, when a corrupt frame is hit — kept as + * a callback (rather than logging here) so this module stays free of the Harper module + * graph and the behavior is unit-testable. Non-`RangeError` failures propagate unchanged. + */ +export function endIteratorOnCorruptFrame( + iterator: Iterator, + onCorruptFrame: (error: RangeError) => void +): IterableIterator { + let stopped = false; + return { + [Symbol.iterator]() { + return this; + }, + next(): IteratorResult { + if (stopped) return { done: true, value: undefined }; + try { + return iterator.next(); + } catch (error) { + // rocksdb-js's txnlog reader signals frame corruption with a RangeError; the + // message wording is version-dependent (1.4.2 added hex offsets), so we key on + // the class, not the text. Anything else is unexpected and propagates. Treating + // a stray non-framing RangeError as end-of-log is the deliberate tradeoff: on + // this cold replay/boot path it still beats an uncaughtException aborting + // startup, and onCorruptFrame logs every occurrence so it is never silent. + if (!(error instanceof RangeError)) throw error; + stopped = true; + onCorruptFrame(error); + return { done: true, value: undefined }; + } + }, + }; +} diff --git a/unitTests/resources/replayLogs.test.js b/unitTests/resources/replayLogs.test.js index ca44699e2d..417ce5df36 100644 --- a/unitTests/resources/replayLogs.test.js +++ b/unitTests/resources/replayLogs.test.js @@ -2,7 +2,11 @@ const assert = require('node:assert'); // The helper lives in a dependency-free module so the test doesn't need to bootstrap // the full Resource/RocksDB module graph (which has a circular require chain). -const { classifyAuditEntryForReplay, RECORD_BEARING_FLAGS } = require('#src/resources/replayLogsGuards'); +const { + classifyAuditEntryForReplay, + RECORD_BEARING_FLAGS, + endIteratorOnCorruptFrame, +} = require('#src/resources/replayLogsGuards'); // Regression tests for the unclean-shutdown replay guards. Without these, an audit log // containing entries with corrupt MessagePack values caused replayLogs to write @@ -59,3 +63,57 @@ describe('classifyAuditEntryForReplay', () => { assert.strictEqual(RECORD_BEARING_FLAGS, HAS_RECORD | HAS_PARTIAL_RECORD); }); }); + +// Regression tests for HarperFast/harper#1135: rocksdb-js's txnlog reader throws a bounded +// RangeError when an entry's declared length overruns the log (a torn/corrupt frame). That +// used to escape uncaught out of the replay/broadcast iterator and abort startup. The wrapper +// must turn it into a clean end-of-log instead, while leaving every other failure untouched. +describe('endIteratorOnCorruptFrame', () => { + it('yields entries up to a corrupt frame, then ends cleanly and reports it once', () => { + let calls = 0; + const source = { + next() { + calls++; + if (calls === 1) return { done: false, value: 'a' }; + if (calls === 2) return { done: false, value: 'b' }; + throw new RangeError('declared length 1778384896 overruns the log (limit=5439)'); + }, + }; + const reported = []; + const wrapped = endIteratorOnCorruptFrame(source, (error) => reported.push(error)); + + assert.deepStrictEqual([...wrapped], ['a', 'b']); + assert.strictEqual(reported.length, 1); + assert.ok(reported[0] instanceof RangeError); + // Latched: stays done without re-invoking the source (no repeated reporting/spam). + assert.deepStrictEqual(wrapped.next(), { done: true, value: undefined }); + assert.strictEqual(calls, 3); + assert.strictEqual(reported.length, 1); + }); + + it('does not swallow non-RangeError failures', () => { + const source = { + next() { + throw new TypeError('boom'); + }, + }; + let reported = 0; + const wrapped = endIteratorOnCorruptFrame(source, () => reported++); + assert.throws(() => wrapped.next(), TypeError); + assert.strictEqual(reported, 0); + }); + + it('passes a normal exhaustion through without reporting a corrupt frame', () => { + let calls = 0; + const source = { + next() { + calls++; + return calls === 1 ? { done: false, value: 1 } : { done: true, value: undefined }; + }, + }; + let reported = 0; + const wrapped = endIteratorOnCorruptFrame(source, () => reported++); + assert.deepStrictEqual([...wrapped], [1]); + assert.strictEqual(reported, 0); + }); +}); From a70a2eb7ea5c43377fb2328494ac017119e35fe3 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Thu, 4 Jun 2026 12:16:20 -0700 Subject: [PATCH 2/4] address review: delegate return()/throw() in endIteratorOnCorruptFrame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both PR review bots flagged that the corrupt-frame iterator wrapper dropped the underlying iterator's optional return()/throw(), so early termination (a for-of break or an outer .return()) would skip the source iterator's cleanup. The current rocksdb-js query iterator implements neither, so nothing leaks today, but the wrapper should stay a faithful proxy — delegate both when present (guarded by typeof), and latch stopped on return(). Adds unit tests for delegation and for the no-synthesis-when-absent case. Co-Authored-By: Claude Opus 4.8 (1M context) --- resources/replayLogsGuards.ts | 16 +++++++++- unitTests/resources/replayLogs.test.js | 43 ++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/resources/replayLogsGuards.ts b/resources/replayLogsGuards.ts index fb03e546c9..0dc0c45eef 100644 --- a/resources/replayLogsGuards.ts +++ b/resources/replayLogsGuards.ts @@ -63,7 +63,7 @@ export function endIteratorOnCorruptFrame( onCorruptFrame: (error: RangeError) => void ): IterableIterator { let stopped = false; - return { + const wrapped: IterableIterator = { [Symbol.iterator]() { return this; }, @@ -85,4 +85,18 @@ export function endIteratorOnCorruptFrame( } }, }; + // Stay a faithful proxy: delegate the optional return()/throw() so early termination + // (a for-of break, or an outer .return()) still releases whatever the source iterator + // holds. The current rocksdb-js query iterator implements neither, but a future one that + // adds cleanup must not be silently bypassed by this wrapper. + if (iterator.return) { + wrapped.return = (value?: any): IteratorResult => { + stopped = true; + return iterator.return!(value); + }; + } + if (iterator.throw) { + wrapped.throw = (error?: any): IteratorResult => iterator.throw!(error); + } + return wrapped; } diff --git a/unitTests/resources/replayLogs.test.js b/unitTests/resources/replayLogs.test.js index 417ce5df36..b61daecbd2 100644 --- a/unitTests/resources/replayLogs.test.js +++ b/unitTests/resources/replayLogs.test.js @@ -116,4 +116,47 @@ describe('endIteratorOnCorruptFrame', () => { assert.deepStrictEqual([...wrapped], [1]); assert.strictEqual(reported, 0); }); + + it('delegates return()/throw() to the underlying iterator so early-exit cleanup runs', () => { + let returnedWith; + let threwWith; + const source = { + next() { + return { done: false, value: 1 }; + }, + return(value) { + returnedWith = value; + return { done: true, value }; + }, + throw(error) { + threwWith = error; + return { done: true, value: undefined }; + }, + }; + const wrapped = endIteratorOnCorruptFrame(source, () => {}); + + assert.strictEqual(typeof wrapped.return, 'function'); + assert.deepStrictEqual(wrapped.return('cleanup'), { done: true, value: 'cleanup' }); + assert.strictEqual(returnedWith, 'cleanup'); + // after return(), the wrapper is latched done and never touches the source again + assert.deepStrictEqual(wrapped.next(), { done: true, value: undefined }); + + assert.strictEqual(typeof wrapped.throw, 'function'); + const boom = new Error('boom'); + wrapped.throw(boom); + assert.strictEqual(threwWith, boom); + }); + + it('does not synthesize return()/throw() when the underlying iterator lacks them', () => { + const wrapped = endIteratorOnCorruptFrame( + { + next() { + return { done: true, value: undefined }; + }, + }, + () => {} + ); + assert.strictEqual(wrapped.return, undefined); + assert.strictEqual(wrapped.throw, undefined); + }); }); From 8c04b88593feee15b140b00147bfb7c45b130308 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Thu, 4 Jun 2026 12:29:33 -0700 Subject: [PATCH 3/4] review: align return()/throw() delegation with reviewers' suggested form Both review bots (gemini-code-assist, claude) suggested the identical shape: always define return()/throw() on the wrapper, delegate to the source when it implements them, otherwise fall back to the protocol defaults (return -> done, throw -> rethrow), marking stopped first so a later next() can't re-enter. Adopt that verbatim so the applied fix matches the inline suggestions exactly. Functionally equivalent to the prior conditional form for the current rocksdb-js query iterator (which implements neither), and still forwards cleanup if a future iterator adds it. Unit test updated for the always-defined defaults. Co-Authored-By: Claude Opus 4.8 (1M context) --- resources/replayLogsGuards.ts | 30 +++++++++++++------------- unitTests/resources/replayLogs.test.js | 28 ++++++++++++++++-------- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/resources/replayLogsGuards.ts b/resources/replayLogsGuards.ts index 0dc0c45eef..3172669041 100644 --- a/resources/replayLogsGuards.ts +++ b/resources/replayLogsGuards.ts @@ -63,7 +63,7 @@ export function endIteratorOnCorruptFrame( onCorruptFrame: (error: RangeError) => void ): IterableIterator { let stopped = false; - const wrapped: IterableIterator = { + return { [Symbol.iterator]() { return this; }, @@ -84,19 +84,19 @@ export function endIteratorOnCorruptFrame( return { done: true, value: undefined }; } }, - }; - // Stay a faithful proxy: delegate the optional return()/throw() so early termination - // (a for-of break, or an outer .return()) still releases whatever the source iterator - // holds. The current rocksdb-js query iterator implements neither, but a future one that - // adds cleanup must not be silently bypassed by this wrapper. - if (iterator.return) { - wrapped.return = (value?: any): IteratorResult => { + // Forward early termination so the source iterator's cleanup (e.g. releasing a + // rocksdb read handle / lock) still runs when a consumer exits a for-of early via + // break/return/throw. Mark stopped first so a later next() can't re-enter. The + // current rocksdb-js query iterator implements neither, hence the protocol defaults. + return(value?: any): IteratorResult { + stopped = true; + if (typeof iterator.return === 'function') return iterator.return(value); + return { done: true, value }; + }, + throw(error?: any): IteratorResult { stopped = true; - return iterator.return!(value); - }; - } - if (iterator.throw) { - wrapped.throw = (error?: any): IteratorResult => iterator.throw!(error); - } - return wrapped; + if (typeof iterator.throw === 'function') return iterator.throw(error); + throw error; + }, + }; } diff --git a/unitTests/resources/replayLogs.test.js b/unitTests/resources/replayLogs.test.js index b61daecbd2..dbb6ccc44a 100644 --- a/unitTests/resources/replayLogs.test.js +++ b/unitTests/resources/replayLogs.test.js @@ -147,16 +147,26 @@ describe('endIteratorOnCorruptFrame', () => { assert.strictEqual(threwWith, boom); }); - it('does not synthesize return()/throw() when the underlying iterator lacks them', () => { - const wrapped = endIteratorOnCorruptFrame( - { - next() { - return { done: true, value: undefined }; - }, + it('return()/throw() fall back to protocol defaults and latch when the underlying lacks them', () => { + let nextCalls = 0; + const source = { + next() { + nextCalls++; + return { done: false, value: 1 }; }, - () => {} + }; + const wrapped = endIteratorOnCorruptFrame(source, () => {}); + + // return() defaults to done and latches without ever pulling the source again + assert.deepStrictEqual(wrapped.return('x'), { done: true, value: 'x' }); + assert.deepStrictEqual(wrapped.next(), { done: true, value: undefined }); + assert.strictEqual(nextCalls, 0); + + // throw() rethrows when the source can't handle it + const boom = new Error('boom'); + assert.throws( + () => endIteratorOnCorruptFrame({ next: source.next }, () => {}).throw(boom), + (error) => error === boom ); - assert.strictEqual(wrapped.return, undefined); - assert.strictEqual(wrapped.throw, undefined); }); }); From 9d563fe86d80d43c7cac310ab8b21f9317c8f72a Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Thu, 4 Jun 2026 13:13:56 -0700 Subject: [PATCH 4/4] tidy: trim verbose/redundant code comments De-duplicate the #1135/RangeError/end-of-log rationale that was repeated across the guard JSDoc, its inline comments, the stress-test helper/body, and the unit-test header. State each point once in its canonical spot; drop rocksdb-internals detail that isn't this module's concern. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) --- integrationTests/server/replay-stress.test.ts | 25 +++++-------- resources/replayLogsGuards.ts | 35 ++++++------------- unitTests/resources/replayLogs.test.js | 6 ++-- 3 files changed, 20 insertions(+), 46 deletions(-) diff --git a/integrationTests/server/replay-stress.test.ts b/integrationTests/server/replay-stress.test.ts index 9049662ffd..c559559c1c 100644 --- a/integrationTests/server/replay-stress.test.ts +++ b/integrationTests/server/replay-stress.test.ts @@ -135,12 +135,9 @@ function truncateTail(path: string, bytes: number) { } function corruptLastEntryLength(path: string): boolean { - // Walk the entry frames to the *last* well-framed entry, then force its declared length - // to overrun the log by setting the most-significant byte of its big-endian uint32 length - // to 0xff (≥ 4 GB). Targeting the last entry puts the corruption in the unflushed tail - // that replay actually reads (replay starts from the last-flushed position), rather than a - // flushed prefix it skips over. Deterministic — same frame, same corruption — unlike the - // old random byte flips, whose effect depended on what RocksDB had flushed before SIGKILL. + // Force the *last* well-framed entry's big-endian uint32 length to overrun the log (top + // byte → 0xff, ≥ 4 GB). The last entry sits in the unflushed tail that replay reads + // (replay starts from the last-flushed position), so a flushed prefix isn't skipped over. const buf = readFileSync(path); let pos = TRANSACTION_LOG_FILE_HEADER_SIZE; let lastLengthPos = -1; @@ -218,22 +215,16 @@ suite('Transaction log replay stress', (ctx: ContextWithHarper) => { } let corrupted = 0; await crashAndRestart(ctx, (dataRootDir) => { - // User-DB only: corrupting system/ txnlogs trips the version-tracking - // upgrade-abort path on next boot — a real but different failure mode. - // Here we want the framing-corruption case (#1135): a declared length that - // overruns the log used to throw an uncaught RangeError out of the txnlog - // iterator and abort startup; replay must now treat it as end-of-log. + // User-DB only: corrupting system/ txnlogs trips the upgrade-abort path on next + // boot — a different failure mode than the framing corruption (#1135) under test. for (const f of listTxnLogFiles(dataRootDir, { userOnly: true })) { if (corruptLastEntryLength(f)) corrupted++; } }); - // Fail loudly rather than vacuously pass if the framing ever changes and we corrupt - // nothing — otherwise this would silently stop being a regression test for #1135. + // Fail loudly, not vacuously, if the framing ever changes and nothing gets corrupted. ok(corrupted > 0, 'expected to corrupt at least one user-DB txnlog'); - // Behavioral assertion, not a wall-clock budget: crashAndRestart resolving above - // means the server came back up (startHarper saw 'successfully started'); a - // regression resurfaces as a failed/timed-out restart, not a slow one — so there - // is nothing to tune per-runner. Confirm every table is still queryable. + // Behavioral, not a wall-clock budget: crashAndRestart resolved → the server came back + // up; a regression shows up as a failed restart, not a slow one. Confirm tables queryable. for (const t of TABLES) { const c = await countRows(ctx.harper, t); ok(typeof c === 'number' && c >= 0, `count on ${t} should be a number, got ${c}`); diff --git a/resources/replayLogsGuards.ts b/resources/replayLogsGuards.ts index 3172669041..316738e74a 100644 --- a/resources/replayLogsGuards.ts +++ b/resources/replayLogsGuards.ts @@ -42,21 +42,12 @@ export function classifyAuditEntryForReplay( } /** - * Wraps a transaction-log query iterator so a framing-level corruption error ends - * iteration of that log cleanly instead of escaping as an uncaughtException. - * - * rocksdb-js's txnlog reader throws a bounded `RangeError` when an entry's declared - * length overruns the log or its header is truncated (intentional hardening — it would - * otherwise OOM on `allocUnsafe(bogusLength)` or deref an undefined buffer). A torn write - * at SIGKILL time, or a flipped byte in an unflushed/corrupt log, looks exactly like that. - * Once the framing is lost we can't locate the next entry, so the corrupt frame marks the - * usable end of this log (torn-write semantics): entries before it have already been - * yielded, and startup replay / replication broadcast must continue rather than abort the - * boot. The latch means a persistently-corrupt log is reported once, not on every re-poll. - * - * `onCorruptFrame` is invoked once, with the error, when a corrupt frame is hit — kept as - * a callback (rather than logging here) so this module stays free of the Harper module - * graph and the behavior is unit-testable. Non-`RangeError` failures propagate unchanged. + * Wraps a transaction-log query iterator so a corrupt/torn frame ends that log's iteration + * cleanly instead of escaping as an uncaughtException. rocksdb-js throws a bounded RangeError + * when an entry's framing is broken; framing loss means the next entry can't be located, so the + * frame marks end-of-log (entries before it were already yielded) and startup replay / + * replication broadcast continue. `onCorruptFrame` fires once, latched — kept a callback (not a + * direct log) so this module stays out of the Harper module graph and is unit-testable. */ export function endIteratorOnCorruptFrame( iterator: Iterator, @@ -72,22 +63,16 @@ export function endIteratorOnCorruptFrame( try { return iterator.next(); } catch (error) { - // rocksdb-js's txnlog reader signals frame corruption with a RangeError; the - // message wording is version-dependent (1.4.2 added hex offsets), so we key on - // the class, not the text. Anything else is unexpected and propagates. Treating - // a stray non-framing RangeError as end-of-log is the deliberate tradeoff: on - // this cold replay/boot path it still beats an uncaughtException aborting - // startup, and onCorruptFrame logs every occurrence so it is never silent. + // Key on the class, not the message: the framing RangeError's wording is + // version-dependent (1.4.2 added hex offsets). Anything else re-throws. if (!(error instanceof RangeError)) throw error; stopped = true; onCorruptFrame(error); return { done: true, value: undefined }; } }, - // Forward early termination so the source iterator's cleanup (e.g. releasing a - // rocksdb read handle / lock) still runs when a consumer exits a for-of early via - // break/return/throw. Mark stopped first so a later next() can't re-enter. The - // current rocksdb-js query iterator implements neither, hence the protocol defaults. + // Forward early termination (for-of break/return/throw) so the source's cleanup runs; + // mark stopped first. Current rocksdb-js implements neither — hence the protocol defaults. return(value?: any): IteratorResult { stopped = true; if (typeof iterator.return === 'function') return iterator.return(value); diff --git a/unitTests/resources/replayLogs.test.js b/unitTests/resources/replayLogs.test.js index dbb6ccc44a..a5bd8f930e 100644 --- a/unitTests/resources/replayLogs.test.js +++ b/unitTests/resources/replayLogs.test.js @@ -64,10 +64,8 @@ describe('classifyAuditEntryForReplay', () => { }); }); -// Regression tests for HarperFast/harper#1135: rocksdb-js's txnlog reader throws a bounded -// RangeError when an entry's declared length overruns the log (a torn/corrupt frame). That -// used to escape uncaught out of the replay/broadcast iterator and abort startup. The wrapper -// must turn it into a clean end-of-log instead, while leaving every other failure untouched. +// Regression tests for HarperFast/harper#1135: the wrapper must turn a framing RangeError into +// a clean end-of-log (so replay/broadcast don't abort the boot) and leave other errors alone. describe('endIteratorOnCorruptFrame', () => { it('yields entries up to a corrupt frame, then ends cleanly and reports it once', () => { let calls = 0;