Repository navigation
Adopt rocksdb-js 2.12's transaction-log retention: pin the txn.state witness in QA-822 and anchor emptied logs by floor in derived indexes - #3265
Merged
Conversation
…witness in QA-822 and anchor emptied logs by floor in derived indexes rocksdb-js#911 lets retention purge every fully flushed file of a log, the one txn.state names and the current segment included. The QA-822 integration test pinned the retired #799 floor (`fileCount > 0`, floor file on disk) and went red on main with 2.12.0; invariants 1 and 4 now pin the witness contract instead: a non-sentinel position, the writer above it when no files remain, no backwards move across a restart, txn.state's eight bytes on disk while the process is down, and no fully flushed file left behind. The derived-index runtime read `fileCount === 0` as "never written", which an emptied log now also reports. The one-line predicate #911 asked for makes every rebuild over such a log fail (no committed tail, beginning "lost"), so the cursor gains an optional `floors` map: for a log with no committed transaction at the boundary, the sequence its next commit lands in. A log is read from its beginning only while every file from its floor on is retained, proven at install, before its first exact anchor, and before each caught-up idle pass; floors are cursor identity and travel through the full-text payload codec. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ub3YgzNx9s7KVfKJT4Kx7t Dispatch-Task: main-red-harper-3193-retention-floor-20261013
…ery catch-up publication, and floor an uncommitted tail at its oldest file Review round 1: the full-text backend's cloneCursor dropped floors on the barrier and durable paths, so a floor-bearing cursor was published without them and the runtime condemned it as unoffered; the unanchored-log proof ran only on empty idle passes, so a nonempty pass that reached the tail could publish coverage and ready over a crossed floor; a log whose retained files held only uncommitted entries took the writer's sequence as its floor, below which that transaction would later complete. QA-822 now samples txn.state while the process is down and checks that no file above the position was deleted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ub3YgzNx9s7KVfKJT4Kx7t Dispatch-Task: main-red-harper-3193-retention-floor-20261013
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ub3YgzNx9s7KVfKJT4Kx7t Dispatch-Task: main-red-harper-3193-retention-floor-20261013
…being reconciled Review round 3: the pre-delivery check read the previous offered cursor, so a log anchored in the batch under delivery could still be condemned by a concurrent purge; the idle-pass check duplicated the probe the durable reconciliation already makes. Both are gone; #reconcileDurableCursor proves the floors of the cursor it reconciles before any catch-up publication. QA-822's witness check with files kept allows the position one sequence behind the oldest file (a flush ending exactly at a sealed file's end lets the purge delete that file and keep its unflushed successor), and its on-disk oracle no longer claims a keep-half this no-write arm cannot exercise. Floor docs name the oldest-file rule and the three proof sites. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ub3YgzNx9s7KVfKJT4Kx7t Dispatch-Task: main-red-harper-3193-retention-floor-20261013
… suffix in QA-822 Review round 4: durable reconciliation re-proved the floors of the older durable cursor, so a floor file purged after the runner had read and anchored past it condemned the generation for nothing; the proof now takes the newest accepted vector. QA-822's data arm writes markers after the purge cutoff and before the purge, so the keep half of the rule — the file holding them survives — is asserted on real files, and the floor docs state the oldest-retained-file rule. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ub3YgzNx9s7KVfKJT4Kx7t Dispatch-Task: main-red-harper-3193-retention-floor-20261013
…empty the store Review round 5: writing the protected rows before the only purge kept the store from being emptied, so the first witness sample and the same-process rows no longer followed the path rocksdb-js#911 changed. The arm now purges once with rows newer than its cutoff (the keep half, asserted on the file count and the rows), flushes fully, and purges again into an emptied store before the same-process rows. One purge helper serves both arms. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ub3YgzNx9s7KVfKJT4Kx7t Dispatch-Task: main-red-harper-3193-retention-floor-20261013
Review round 6: the emptied-store samples were only probably emptied. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ub3YgzNx9s7KVfKJT4Kx7t Dispatch-Task: main-red-harper-3193-retention-floor-20261013
…ws, and trim comments Review round 8 (closing full): with the rows unflushed, rocksdb-js refused the file on flush state alone and the keep half never exercised the cutoff. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ub3YgzNx9s7KVfKJT4Kx7t Dispatch-Task: main-red-harper-3193-retention-floor-20261013
…assert the reopened writer sequence, fix a stale comment Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ub3YgzNx9s7KVfKJT4Kx7t Dispatch-Task: main-red-harper-3193-retention-floor-20261013
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the transaction log purging and indexing mechanism to align with rocksdb-js#911 (v2.12.0), where fully flushed logs can be completely emptied of files. It introduces a 'floors' property to the DerivedIndexCursor to anchor logs that do not have a completed transaction when a rebuild boundary is captured, ensuring log sequences are tracked correctly even when no files remain on disk. The changes span the runtime implementation, full-text index serialization/deserialization, design documentation, and comprehensive integration and unit tests. There are no review comments provided, so I have no feedback to evaluate.
kriszyp
marked this pull request as ready for review
October 19, 2026 00:07
This was referenced Oct 19, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
⊙ Problem
Main's Integration Tests shard 6 is red since chore(deps): Update rocksdb-js to 2.12.0 (#3193): QA-822 fails at invariants 1 and 4.
Not a defect. rocksdb-js 2.12.0 ships Let transaction-log retention purge the flushed-position and current segments (rocksdb-js#911): a purge may now delete every fully flushed file. QA-822 required the old #799 floor. The derived-index runtime also reads
fileCount === 0as "never written", which an emptied log now reports too, so an index could skip commits.🤔 Is the requested fix the right one?
The task asked to keep the referenced file and not weaken the test; under #911 that file is gone by design. The owner ruled on 2026-10-18 that Harper pins #911. The invariants now assert the witness itself and fail on 2.11.0, where the floor file survived.
💡 Solution
QA-822 asserts the
txn.statewitness: never the sentinel, writer above it when no files remain, never moves back, exact bytes on disk while down, every fully flushed file gone, files holding newer rows kept.The derived-index cursor gains
floors: { <log>: <sequence> }for a log with no completed transaction at rebuild. Such a log is read from its beginning only while every file from its floor on is retained, checked before every anchor and catch-up.🤔 Why not the one-line predicate #911 suggested?
Measured here: with
fileCount === 0 ? currentSequenceNumber <= 1 : oldestSequenceNumber === 1alone, an index over a database with one emptied peer log goes permanentlyunavailable. The rebuild rejected the emptied log, and nothing recorded whether it was emptied before or after the boundary. The floor is that record; the predicate is its floor-1 case.🤔 One rebuild after upgrade for indexes over an emptied log
A pre-existing cursor has no floor, so its first resume rejects it and rebuilds once; later resumes are quiet. Correct for integrity, but a large HNSW rebuild is long. Release-note item, with #911's downgrade warning (remove
txn.statebefore downgrading below 2.12.0 once a store's last file was purged).🤔 Anchored idle logs still rebuild once per purge cycle
Floors cover logs with no committed transaction at the boundary. A log anchored by timestamp whose last transaction's file is later emptied still fails
exactStarton the next acquisition and rebuilds once; the new cursor then floors it. Fixing that needs a per-anchor seal (writer position at a caught-up point, accepted when equal tolastFlushedPosition). Follow-up.⚖️ Alternatives
Rejected options
verifyTablesand prebuild fixes, blocks the harper-pro consumers Extract Fastify routes into a standalone component #911 serves, and the test needs the same rewrite next bump.cursor.coverage: optional, published after catch-up, dropped by the runtime's clone.format: 2: an older full-text decoder then fails closed instead of rebuilding once; an older runtime ignoresfloorseither way.🔧 Changes
integrationTests/database/qa822-purge-log-invariants.test.ts: invariant 1 and invariant 4 restated; invariant 4 samplestxn.state(LE uint32 offset, uint32 sequence) and the file list in the shutdown callback, while the process is down, and requires the reopened writer to sit where the purge retired it; arm 1 purges twice through one helper, keeping rows newer than the first cutoff (the keep half) and then emptying the store, asserted, before the same-process rows.resources/derivedIndexRuntime.ts:DerivedIndexCursor.floors;#captureBoundaryrecords a no-tail log's oldest retained file (or the writer's sequence with no files) when past 1;#retainsFrom(log, floor)isfileCount === 0 ? currentSequenceNumber <= floor : oldestSequenceNumber <= floor;#validateLogSetcovers floor-only logs and judges unanchored ones by their floor;#anchorre-proves the floor before the first exact anchor;#checkUnanchoredLogsruns inside#reconcileDurableCursor, the one path every catch-up publication takes, against the newest accepted vector;isValidCursor,cloneCursorandsameCursortreat floors as identity. Look hardest at the three proof sites: a missed one is a silently stale index.resources/indexes/fullTextDerivedIndex.ts: the payload codec validates, sorts and acceptsfloors;cloneCursorandsameCursorkeep and compare them, with no extra work for floors when neither cursor has any.getStats(), once per caught-up pass on the background runner; anchored logs pay nothing new.resources/indexes/DESIGN.md: cursor shape; retention rule, proof sites, rollback behaviour.✅ Verification
Planning review
Framing-Verdict: chosen-approach-sound.npm run test:unit:resourceson the final head: 4304 passing, 0 failing, 54 pending.origin/maindist.npm run lint:requiredand Prettier clean.New tests
derivedIndexRuntimeNativeBackend.test.js(its fake store reports per-log retention stats): legacy cursor over an emptied log condemns once and anchors by floor; intact floor resumes without rebuild (three layouts); crossed floor condemns (two layouts); floor-only log gone condemns; floor re-proven before the first exact anchor, and before catch-up under sustained local traffic with no remote transaction to anchor; first commit retires the floor; a floor the accepted batch already replaced is not re-proven against the older durable cursor; uncommitted entries floor at the oldest file; tampered floors read as unoffered.derivedIndexRuntimeRocks.test.js: the lifecycle on a real RocksDB store with a peer log emptied bypurgeLogsunder 2.12.0, including a pre-floors cursor.fullTextDerivedIndex.test.js: floors survive deliver, flush and coverage publication; sorted round-trip;0,1.5,2^32, an array and an empty name rejected.Refs #3193
— Claude Fable 5.1
🤖 Generated with Claude Code
https://claude.ai/code/session_01Ub3YgzNx9s7KVfKJT4Kx7t
Release target: main — adopts the rocksdb-js 2.12.0 retention contract that only main pins (#3193); the release lines stay on 2.11.x
Related PRs: #2743 independent, #2338 independent
Complexity: complicated
Deferred findings
b39ba5bde0c9a4a2major reliability —resources/derivedIndexRuntime.ts— anchored log emptied by 2.12 retention fails exactStart on reacquire and forces a full rebuild — NOT FILEDf70e81c290b13b80minor correctness —resources/derivedIndexRuntime.ts— installCursor ignores a synchronous rebuild started by validateLogSet and publishes ready over rebuilding — NOT FILEDOrigin — the dispatch brief this PR was written from
Investigate transaction-log purge retention-floor failures after rocksdb-js 2.12.0
Investigate and fix the Harper main Integration Tests regression associated with harper#3193. At integrationTests/database/qa822-purge-log-invariants.test.ts:573 and :736, transaction-log purge deletes every log file while lastFlushedPosition still names seq 2 / offset 7213886, violating the retained-floor and clean-reopen invariants. Reproduce the failure, trace the behavior across the Harper purge and rocksdb-js dependency boundary, then implement the smallest correct fix. If the fix is not obvious, recommend reverting harper#3193 rather than making a speculative change.
Acceptance
The retention floor's referenced log remains available after purge and after a clean write-free reopen; the QA-822 invariants at :573 and :736 pass on affected Node.js v22/v24/v26 Integration legs. Add focused regression coverage if needed. Report the established cause and why the chosen fix is correct. Keep the change scoped to Harper/rocksdb-js transaction-log retention; do not alter the test to weaken the invariant.
Dispatch: task
main-red-harper-3193-retention-floor-20261013· queued by ci-health-monitor · ran by claude/fable/high · worker kzyp-xps-1Review-Coverage: authored=claude; ran=gemini,muse,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=10; full=6 @ 9d84a36
Review-Attention: deep ~30m (critical: derivedIndexRuntime.ts, fullTextDerivedIndex.ts; decisions: floors-in-cursor, floor-proof-sites, floors-as-identity, scope-unanchored-only, txn-state-byte-layout, do-less-alternative) @ 9d84a36