Skip to content

Commit 9d563fe

Browse files
heskewclaude
andcommitted
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) <noreply@anthropic.com>
1 parent 8c04b88 commit 9d563fe

3 files changed

Lines changed: 20 additions & 46 deletions

File tree

integrationTests/server/replay-stress.test.ts

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -135,12 +135,9 @@ function truncateTail(path: string, bytes: number) {
135135
}
136136

137137
function corruptLastEntryLength(path: string): boolean {
138-
// Walk the entry frames to the *last* well-framed entry, then force its declared length
139-
// to overrun the log by setting the most-significant byte of its big-endian uint32 length
140-
// to 0xff (≥ 4 GB). Targeting the last entry puts the corruption in the unflushed tail
141-
// that replay actually reads (replay starts from the last-flushed position), rather than a
142-
// flushed prefix it skips over. Deterministic — same frame, same corruption — unlike the
143-
// old random byte flips, whose effect depended on what RocksDB had flushed before SIGKILL.
138+
// Force the *last* well-framed entry's big-endian uint32 length to overrun the log (top
139+
// byte → 0xff, ≥ 4 GB). The last entry sits in the unflushed tail that replay reads
140+
// (replay starts from the last-flushed position), so a flushed prefix isn't skipped over.
144141
const buf = readFileSync(path);
145142
let pos = TRANSACTION_LOG_FILE_HEADER_SIZE;
146143
let lastLengthPos = -1;
@@ -218,22 +215,16 @@ suite('Transaction log replay stress', (ctx: ContextWithHarper) => {
218215
}
219216
let corrupted = 0;
220217
await crashAndRestart(ctx, (dataRootDir) => {
221-
// User-DB only: corrupting system/ txnlogs trips the version-tracking
222-
// upgrade-abort path on next boot — a real but different failure mode.
223-
// Here we want the framing-corruption case (#1135): a declared length that
224-
// overruns the log used to throw an uncaught RangeError out of the txnlog
225-
// iterator and abort startup; replay must now treat it as end-of-log.
218+
// User-DB only: corrupting system/ txnlogs trips the upgrade-abort path on next
219+
// boot — a different failure mode than the framing corruption (#1135) under test.
226220
for (const f of listTxnLogFiles(dataRootDir, { userOnly: true })) {
227221
if (corruptLastEntryLength(f)) corrupted++;
228222
}
229223
});
230-
// Fail loudly rather than vacuously pass if the framing ever changes and we corrupt
231-
// nothing — otherwise this would silently stop being a regression test for #1135.
224+
// Fail loudly, not vacuously, if the framing ever changes and nothing gets corrupted.
232225
ok(corrupted > 0, 'expected to corrupt at least one user-DB txnlog');
233-
// Behavioral assertion, not a wall-clock budget: crashAndRestart resolving above
234-
// means the server came back up (startHarper saw 'successfully started'); a
235-
// regression resurfaces as a failed/timed-out restart, not a slow one — so there
236-
// is nothing to tune per-runner. Confirm every table is still queryable.
226+
// Behavioral, not a wall-clock budget: crashAndRestart resolved → the server came back
227+
// up; a regression shows up as a failed restart, not a slow one. Confirm tables queryable.
237228
for (const t of TABLES) {
238229
const c = await countRows(ctx.harper, t);
239230
ok(typeof c === 'number' && c >= 0, `count on ${t} should be a number, got ${c}`);

resources/replayLogsGuards.ts

Lines changed: 10 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -42,21 +42,12 @@ export function classifyAuditEntryForReplay(
4242
}
4343

4444
/**
45-
* Wraps a transaction-log query iterator so a framing-level corruption error ends
46-
* iteration of that log cleanly instead of escaping as an uncaughtException.
47-
*
48-
* rocksdb-js's txnlog reader throws a bounded `RangeError` when an entry's declared
49-
* length overruns the log or its header is truncated (intentional hardening — it would
50-
* otherwise OOM on `allocUnsafe(bogusLength)` or deref an undefined buffer). A torn write
51-
* at SIGKILL time, or a flipped byte in an unflushed/corrupt log, looks exactly like that.
52-
* Once the framing is lost we can't locate the next entry, so the corrupt frame marks the
53-
* usable end of this log (torn-write semantics): entries before it have already been
54-
* yielded, and startup replay / replication broadcast must continue rather than abort the
55-
* boot. The latch means a persistently-corrupt log is reported once, not on every re-poll.
56-
*
57-
* `onCorruptFrame` is invoked once, with the error, when a corrupt frame is hit — kept as
58-
* a callback (rather than logging here) so this module stays free of the Harper module
59-
* graph and the behavior is unit-testable. Non-`RangeError` failures propagate unchanged.
45+
* Wraps a transaction-log query iterator so a corrupt/torn frame ends that log's iteration
46+
* cleanly instead of escaping as an uncaughtException. rocksdb-js throws a bounded RangeError
47+
* when an entry's framing is broken; framing loss means the next entry can't be located, so the
48+
* frame marks end-of-log (entries before it were already yielded) and startup replay /
49+
* replication broadcast continue. `onCorruptFrame` fires once, latched — kept a callback (not a
50+
* direct log) so this module stays out of the Harper module graph and is unit-testable.
6051
*/
6152
export function endIteratorOnCorruptFrame<T>(
6253
iterator: Iterator<T>,
@@ -72,22 +63,16 @@ export function endIteratorOnCorruptFrame<T>(
7263
try {
7364
return iterator.next();
7465
} catch (error) {
75-
// rocksdb-js's txnlog reader signals frame corruption with a RangeError; the
76-
// message wording is version-dependent (1.4.2 added hex offsets), so we key on
77-
// the class, not the text. Anything else is unexpected and propagates. Treating
78-
// a stray non-framing RangeError as end-of-log is the deliberate tradeoff: on
79-
// this cold replay/boot path it still beats an uncaughtException aborting
80-
// startup, and onCorruptFrame logs every occurrence so it is never silent.
66+
// Key on the class, not the message: the framing RangeError's wording is
67+
// version-dependent (1.4.2 added hex offsets). Anything else re-throws.
8168
if (!(error instanceof RangeError)) throw error;
8269
stopped = true;
8370
onCorruptFrame(error);
8471
return { done: true, value: undefined };
8572
}
8673
},
87-
// Forward early termination so the source iterator's cleanup (e.g. releasing a
88-
// rocksdb read handle / lock) still runs when a consumer exits a for-of early via
89-
// break/return/throw. Mark stopped first so a later next() can't re-enter. The
90-
// current rocksdb-js query iterator implements neither, hence the protocol defaults.
74+
// Forward early termination (for-of break/return/throw) so the source's cleanup runs;
75+
// mark stopped first. Current rocksdb-js implements neither — hence the protocol defaults.
9176
return(value?: any): IteratorResult<T> {
9277
stopped = true;
9378
if (typeof iterator.return === 'function') return iterator.return(value);

unitTests/resources/replayLogs.test.js

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,8 @@ describe('classifyAuditEntryForReplay', () => {
6464
});
6565
});
6666

67-
// Regression tests for HarperFast/harper#1135: rocksdb-js's txnlog reader throws a bounded
68-
// RangeError when an entry's declared length overruns the log (a torn/corrupt frame). That
69-
// used to escape uncaught out of the replay/broadcast iterator and abort startup. The wrapper
70-
// must turn it into a clean end-of-log instead, while leaving every other failure untouched.
67+
// Regression tests for HarperFast/harper#1135: the wrapper must turn a framing RangeError into
68+
// a clean end-of-log (so replay/broadcast don't abort the boot) and leave other errors alone.
7169
describe('endIteratorOnCorruptFrame', () => {
7270
it('yields entries up to a corrupt frame, then ends cleanly and reports it once', () => {
7371
let calls = 0;

0 commit comments

Comments
 (0)