Range reads honor the transaction option: getRange(), getKeys(), and getKeysCount() see a transaction's staged writes and snapshot - #837
Conversation
Route range iterators through the transaction supplied in options while preserving the caller's column family, snapshot, bounds, and iterator lifetime. Add coverage across range APIs and transaction modes.\n\nCo-Authored-By: GPT-5 Codex <noreply@openai.com>
Follow-up to the range routing change: register a transaction iterator only after its registry insert succeeds, drain the registry without allocating, admit counts through the same pending-state check as ranges and surface the error instead of reading a write batch a commit is consuming, close iterators before the coordinated-retry reset deletes the transaction, always step off a key equal to the exclusive end bound in reverse, and skip the bound compare on plain iterators except the reverse exclusive-start case RocksDB's inclusive lower bound cannot express. Document the option and tailing semantics; cover staged overwrites/deletes, empty bases, range-first and disabled snapshots, pessimistic mode, in-flight commits, foreign registries, commitSync, and a routed orphan iterator. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018QjjjDsTgnSnsHc3sBUk3t
Round-1 review fixes: `getTxnId` rejects a transaction whose store path is not this database's, since ids are allocated per database and another database's id resolves to an unrelated transaction; native `Return`/`Throw` tolerate an iterator a commit or abort already closed so loop cleanup cannot throw; `closeIterators` waits for a handle mid-destruction on another thread instead of freeing the transaction under it; the far-bound compare on transaction iterators is gated at compile time on the linked RocksDB (bounds on the write batch since 8.10.0), so the pinned build only pays it for the reverse exclusive-start case. Document count admission and cleanup semantics. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018QjjjDsTgnSnsHc3sBUk3t
Header docs now describe createIterator/closeIterators instead of the removed register/unregister pair and no longer claim init() registers with the descriptor; README states that a transaction context takes precedence over a transaction option; AGENTS.md says the iterator registry does not serialize a cross-environment close against an in-flight next(). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018QjjjDsTgnSnsHc3sBUk3t
`txn.getRange()` after abort or commit dereferenced the transaction's cleared DBHandle while resolving the key buffer, ahead of the pending-state check; it now throws like a range opened during commit does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018QjjjDsTgnSnsHc3sBUk3t
A transaction whose own database handle closed while the descriptor lived
on crashed in the iterator constructor; both admission paths now check the
target handle is open. `getTxnId` requires a real Transaction (one that
carries its store), so a bare `{ id }` can no longer resolve another
caller's transaction in the same database.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018QjjjDsTgnSnsHc3sBUk3t
There was a problem hiding this comment.
Code Review
This pull request implements support for transactional ranges and iterators, allowing operations like getRange(), getKeys(), and getKeysCount() to run within a transaction context. It updates documentation, handles cross-column-family scans, ensures proper iterator lifetime management (closing iterators before transaction commit/abort), and adds comprehensive tests. The review feedback suggests wrapping the non-transactional branch of Database::GetCount in a try/catch block to prevent potential crashes, avoiding unused structured bindings in TransactionHandle::closeIterators to prevent compiler warnings, and explicitly throwing an error if a transaction is passed to a read-only database instead of silently ignoring it.
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit e06a67d |
`Database::GetCount` wrapped only its transactional branch; the plain branch constructs a `DBIteratorHandle` the same way and now reports a failure as a JS error instead of letting it escape the N-API callback. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018QjjjDsTgnSnsHc3sBUk3t
db.getRange({ transaction }),getKeys(), andgetKeysCount()now read through the given transaction exactly asdb.get(key, { transaction })does: the caller's column family, the transaction's staged writes, and its snapshot. Before, the typed option was accepted and silently dropped, so a range read and a point read on the same handle in the same transaction disagreed about the database (the root cause of HarperFast/harper#2506). Refs #830.The transaction is routed by id into the native iterator (a seventh constructor argument), where the caller database's descriptor resolves it, so an index column family's range read stays on the index column family; substituting the transaction's own context would have scanned the column family the transaction was created on. Transaction iterators register with the transaction, and commit, commitSync, abort, the coordinated-retry reset, and teardown close them before touching the RocksDB transaction, so a later
next()throws "Iterator not initialized" whilereturn()/throw()stay idempotent for loop cleanup; a range or count on a transaction that already started committing, or that has finished, throws "Transaction is not in pending state" (the latter crashed onmain). A transaction handed to a database at another path is rejected, since ids are allocated per database and would otherwise resolve to an unrelated transaction, and so is a bare{ id }object, which used to resolve by id alone; a range on a transaction whose own handle was closed while the descriptor lived on also crashed and now throws "Database not open". The native implementation was authored by a Codex session on this task; this Claude session rebased it onto #829, added the lifetime hardening and provenance check, the docs, and most of the tests.Left as they are, all pre-existing and recorded as findings for separate issues: a forced cross-environment teardown (
shutdown()/destroy()) is not serialized against anext()in flight on another environment's thread (the descriptor's closables sweep never was); a RocksDB iterator that hits a read error still reports exhaustion rather than throwing (plain ranges and counts too); andtxn.getSync()afterabort()has the same cleared-handle crash that this PR closes for ranges, in code this PR does not touch.For the human reviewer
{ transaction }on every primary-store and index scan (resources/search.ts), so on its next rocksdb-js bump everysearch()/query()becomes transactional and each scan pins its read transaction until the iterator is drained or closed. Harper's read-transaction lifetime accounting (DatabaseTransaction.readTxnsUsed/doneReadTxn) is the queued companion task. Chosen: no gate, because Harper pins@harperfast/rocksdb-jsto an exact version (2.8.0), so the flip lands only in Harper's own dependency-bump PR, where the companion change ships with it; an opt-in flag would keep today's default of a typed option that is accepted and ignored, and a major bump would label a bug fix as a contract change. Reversible: an opt-in open option can still be added before release. Cost of a "no": another release cycle and a flag to remove later. The planning review's framing verdict was chosen-approach-sound; ship the JS and native binaries together, since the seventh constructor argument cannot be mixed with an older binding.txn.getRange()now establishes and reads on the transaction snapshot, astxn.get()andtxn.getKeysCount()already did; before, a direct transactional range saw the latest committed state plus the write batch and pinned no snapshot. This is what makes range and point reads agree, but a transaction that only ever scanned now holds a snapshot for its lifetime (a lifetime and optimistic-conflict-rate change).disableSnapshottransactions andtailing: truestill read the latest committed state (tested and documented in the README; the invariant is AGENTS.md 19).next()after commit now throws deterministically where it previously read freed memory.getTxnIdnow requires a realTransaction(one that carries its store) and rejects one whose store path differs from the caller's; column families of one database share the path, so cross-column-family reads still work. This is stricter than before for callers that passed a bare{ id }(none in Harper, which passesRocksTransactioninstances). The alternative, native descriptor identity, would need new surface across the binding.exclusiveStartcase, like plain iterators (RocksDB's lower bound is inclusive). AROCKSDB_VERSION/ROCKSDB_PATHbuild against an older release checks both bounds on transaction iterators; that path cannot be exercised by the pinned build.Verification
Route (a), extended vitest suites:
test/ranges.test.ts(with the foreign-database case),test/transaction-cross-column-family.test.ts,test/transactions.test.ts, and the orphan-GC fixture (routed iterator mode). Fails-on-base: with the new tests applied to a build of the merge base (7ab102c), 15 tests fail withexpected [ 'b', 'd' ] to deeply equal [ 'a', 'b', 'c', 'd' ],expected [ { key: 'committed', … } ] to deeply equal [ …, { key: 'later', … } ],expected [Function] to throw an error,expected [] to deeply equal [ 'b' ], and (cross column family)expected [ …(25) ] to have a length of 26 but got 25; all pass on the branch. Disabling the explicit bound check on the branch makes the reverse bounds tests yield the lower-bound key (expected [ 'e', 'd', 'c', 'b', 'a' ] to deeply equal [ 'e', 'd', 'c', 'b' ]), the measurement behind entry 5.pnpm build:bindingpreceded every run.pnpm teston the final head (d3dfdc9): 63 files passed, 1 skipped; 884 tests passed, 9 skipped (the skips are the pre-existing GC- and platform-gated cases).pnpm checkclean. The native GoogleTest target is unaffected (N-API code only).benchmark/ranges.bench.ts(rocksdb only, base and branch interleaved twice on an idle machine, ops/s): small range 19.7k/18.6k then 18.6k/18.6k, keys only 28.7k/27.3k then 26.5k/27.3k, forward 20.2k/19.5k then 19.7k/19.3k, reverse 223k/246k then 224k/240k, sparse prefix 80.7k/79.0k then 79.7k/79.2k. Plain forward scans are within run-to-run noise; reverse scans gained 7 to 10 percent from the explicitSeekForPrevand the removal of the peek-ahead inNext().Complexity: complicated
Review-Coverage: authored=codex; ran=claude,gemini; declined=cursor-grok,cursor-composer,domain; rounds=5 @ 594c8a2
Human-Review-Need: 4 @ 594c8a2