Skip to content

Report a transaction log's corrupt-frame stop to getRange callers through an onCorruptFrame hook - #2463

Open
kriszyp wants to merge 8 commits into
mainfrom
fix/txnlog-midfile-break-callback
Open

Report a transaction log's corrupt-frame stop to getRange callers through an onCorruptFrame hook#2463
kriszyp wants to merge 8 commits into
mainfrom
fix/txnlog-midfile-break-callback

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 2, 2026

Copy link
Copy Markdown
Member

RocksTransactionLogStore.getRange now exposes an onCorruptFrame(error, logName) hook when a transaction-log iterator stops at a corrupt frame, giving replication consumers the signal they need to recover instead of parking on a dead iterator. The hook is composed with main's corruption stop tracking and telemetry at the existing latch, while aggregate delivery remains safe under re-entrancy and keeps mutable exclusions private to each range.

Independent review also identified a pre-existing addLog convergence issue: re-including an excluded peer does not itself invalidate latestUpdates, so that peer waits for another log-list update before receiving an iterator. It is outside this PR's change and is recorded as a follow-up finding.

Refs #2016, #2063. The paired engine work is HarperFast/rocksdb-js#817; harper-pro owns the policy that turns a mid-log report into a bounded base-copy resync.

For the human reviewer

  1. Core reports the gap; the consumer repairs it. The alternative is for core to seek past the break or initiate resync itself. Core cannot assign one recovery policy to boot replay, replication, and subscriptions, while harper-pro has the source and bounded-copy machinery. Reversing this later is invasive because recovery ownership crosses repositories; rejecting it now requires moving the policy and its end-to-end proof into core.
  2. The hook is synchronous inside next(). An event or microtask would be simpler, but the replication sender must learn why the drain stopped before it parks on a future transaction. Reports wait until the aggregate buffer is stable, and iterator splices wait for the outermost poll. Changing this later is behaviorally breaking; rejecting it now requires redesigning the sender notification contract.
  3. Promise-returning hooks are fire-and-forget. Rejections are contained and logged, but async recovery cannot gate a synchronous iterator. The alternatives are a synchronous-only signature or an asynchronous iterator contract. This is cheapest to change before adoption; rejecting it requires choosing one of those narrower APIs.
  4. Exclusions belong to the range, not the caller's options object. removeLog records the private exclusion immediately but defers structural mutation, so frozen options work and a hook's exclusion is visible to a nested refresh. The alternative is retaining the caller's array as live shared state. Reversal is local but restores hidden mutation and the frozen-options failure.

Verification

  • npm run build — passed.
  • npm run lint:required — passed.
  • npx mocha unitTests/resources/auditLog.test.js --grep "onCorruptFrame|removeLog remembers|hook exclusion" — 14 passing, including frozen options across refresh, a future peer excluded before it exists, hook-time exclusion before a nested refresh, sync/async failure containment, and a real on-disk torn-frame report.
  • npm run test:unit:resources — 2,055 passing, 27 pending on the final reviewed head.
  • Regression controls — the frozen-options test fails against the prior mutating implementation with TypeError: object is not extensible; the nested-refresh test fails before immediate exclusion registration because the excluded future peer is polled twice.
  • harper-pro integrationTests/cluster/txnlogTearReplication.test.mjs — prior paired-branch evidence: base parks at 39/60 rows; this hook plus the sender's bounded base copy converges to 60/60 and accepts a post-recovery row.
  • npm run test:integration:all — began successfully and passed numerous unrelated scenarios, then the runner saturated and multiple independent instances timed out together at 120 seconds; stopped after the synchronized infrastructure failures. npm run test:unit:main and npm run test:unit:windows likewise stopped producing a summary, with the Windows slice also exposing an unrelated ambient credential-socket failure.

Complexity: complicated

Review-Coverage: authored=codex; ran=claude,gemini; declined=cursor-grok,cursor-composer,domain; rounds=6 @ e6d26bd

Human-Review-Need: 4 @ e6d26bd

@kriszyp kriszyp added this to the v5.2 milestone Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Release cherry-pick v5.2: cancelled

Cherry-pick branch cherry-pick/v5.2/pr-2463 was deleted — this PR no longer targets v5.2 (milestone is now v5.3).

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

Copy link
Copy Markdown
Contributor

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 new onCorruptFrame hook option to RocksTransactionLogStore.getRange(), allowing callers to be notified synchronously when a transaction log iterator stops early due to a corrupt frame. The implementation safely handles hook execution, defers reports for construction-time corrupt frames, and queues log removals requested during polling to prevent iterator disruption. Comprehensive unit tests have been added to verify these behaviors under various scenarios, including aggregate/single-log paths, throwing/async hooks, and on-disk torn frame detection. No review comments were provided, so there is no feedback to address.

@kriszyp kriszyp modified the milestones: v5.2, v5.3 Sep 2, 2026
@kriszyp
kriszyp requested review from cb1kenobi and removed request for Devin-Holland and heskew September 2, 2026 17:08
@kriszyp
kriszyp marked this pull request as ready for review September 2, 2026 17:08
@kriszyp
kriszyp force-pushed the fix/txnlog-midfile-break-callback branch from d1bd8b6 to 85b5848 Compare September 2, 2026 19:06
@kriszyp
kriszyp marked this pull request as draft September 2, 2026 19:07
@claude

claude Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Comment thread resources/RocksTransactionLogStore.ts Outdated
@kriszyp
kriszyp requested a review from cb1kenobi September 4, 2026 04:56
@kriszyp
kriszyp marked this pull request as ready for review September 4, 2026 04:56
kriszyp and others added 8 commits September 3, 2026 23:31
…hrough an onCorruptFrame hook

A corrupt frame ends a log's query iterator early and latches it dead
(endIteratorOnCorruptFrame), but the store only warned; a consumer had no
way to learn that its stream had stopped, or whether intact entries follow
the break. The replication sender therefore parks on a latched iterator
forever after a mid-log tear (harper-pro txnlogTearReplication red:
follower holds 39/60 rows).

getRange now takes an optional onCorruptFrame(error, logName) hook, fired
once per log at the latch point on both the single-log and the aggregate
path, synchronously from inside the iterable's next(). The error is the
engine's CorruptFrameError, typed against the pinned engine: resyncPosition
present means a mid-log break with entries lost to the stream, absent means
a torn tail. Core reports only; the recovery policy belongs to the caller.
A throwing or rejecting hook is contained so the end-of-log stop stays a
stop.

The unit tests drive the hook through fakes throwing the engine's own
CorruptFrameError, and through the real engine over a log torn on disk:
the hook receives the engine error with the resume offset and the drain
stops at the break.

Refs #2016, #2063. Engine half: HarperFast/rocksdb-js#817.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LMb1MzgF9DR2MC8X7Fz3uj
…e the hook's per-iterable scope

The polling flag was cleared by the innermost guarded next(), so a hook
that re-entered the same iterable left the outer advance() unguarded and a
second break in that pass could splice a slot it was about to write. The
flag is now saved and restored, and deferred removals are applied only by
the outermost call. The hook doc now says once per log per iterable: the
latch lives on the iterable, so a still-broken log reports again on the
next getRange.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LMb1MzgF9DR2MC8X7Fz3uj
… framing breaks

A hook that re-entered next() from inside the poll was given the entry
the outer next() was in the middle of returning, because the slot was
replaced only after the poll that fired the hook. With a hook configured
the slot is marked done before the poll. The hook doc now says it covers
framing breaks only (a reader that dies with another error ends its log
through safeNext without a report) and that removeLog from the hook is an
aggregate-path lever.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LMb1MzgF9DR2MC8X7Fz3uj
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Clone caller exclusions into mutable iterable state so corruption hooks can remove logs even when getRange options are frozen. Extend the regression coverage across refreshes and align logger-failure assertions with core corruption telemetry.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Record removeLog exclusions before a peer exists, allocate iterable-private state lazily, and reuse the existing corruption error type. Document and test the future-peer refresh contract identified during independent review.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Register excluded log names immediately while continuing to defer iterator-array splices until polling completes. Cover a hook that excludes a future peer and re-enters next during the same callback.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
@kriszyp
kriszyp force-pushed the fix/txnlog-midfile-break-callback branch from 85b5848 to e6d26bd Compare September 4, 2026 06:36
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.

2 participants