HarperFast / HarperFast/rocksdb-js

Store.getRange() silently ignores options.transaction: range reads never see the transaction's staged writes or snapshot

Open
#830 0 comments 0 reactions 1 assignee Claimed by @kriszyp View on GitHub
Dominant language
C++
Stars
21
Forks
2
Avg merge
2d 9h
Merged PRs (30d)
36

Description

## Summary

`Store.getRange()` builds its native iterator from `this._context` and never reads
`options.transaction`. `get`/`getSync`/`remove` route the same option through `getTxnId(options)`.
Because `RocksDatabase extends DBI`, `db.getRange({ transaction })` type-checks —
so the option is accepted, silently dropped, and the iteration runs against the database instead of
the transaction.

Consequences for any caller that passes it:

- the transaction's own staged writes are invisible to the iteration (they are in its write batch,
which only `Transaction::GetIterator` merges);
- the iteration is not on the transaction's snapshot either, so it observes writes other
transactions commit while it is open.

There is no error and no warning. A point read and a range read on the same handle, in the same
transaction, disagree about the contents of the database.

## Reproduction

```js
import { RocksDatabase, Transaction } from '@harperfast/rocksdb-js';

const db = RocksDatabase.open(path);
await db.put('committed', 'before');

const txn = new Transaction(db.store);
txn.put('staged', 'in-batch');

const viaOptions = [...db.getRange({ start: 'a', end: 'z', transaction: txn })].map((e) => e.key);
const viaContext = [...txn.getRange({ start: 'a', end: 'z' })].map((e) => e.key);
```

```
get(key, {transaction}) sees staged : true
getRange({transaction}) sees staged : false ← ['committed']
txn.getRange() sees staged : true ← ['committed', 'staged']
```

The correct path exists and works: iterating with the transaction AS the context returns a
`BaseDeltaIterator` that merges the write batch. Only the `options.transaction` spelling is inert.

## Where it is

- [`src/store.ts`](../../blob/main/src/store.ts) — `getRange()` passes `context` to `NativeIterator`
and sets `ITERATOR_CONTEXT_IS_TRANSACTION_FLAG` from `context !== this.db`. `options.transaction`
is never consulted. `getKeys()`/`getKeysCount()`/`getCount()` delegate here, so they inherit it.
- [`src/binding/iterator/db_iterator.cpp`](../../blob/main/src/binding/iterator/db_iterator.cpp) —
the native constructor takes its `TransactionHandle` only from the context argument, trusting the
JS layer's flag; there is no options path for it to read.
- [`src/dbi.ts`](../../blob/main/src/dbi.ts) — `getRange(options?: IteratorOptions & T)` with
`RocksDatabase extends DBI` is what makes the dropped option type-check.

## Impact

This is the root cause of HarperFast/harper#2506: Harper's query engine passes
`{ transaction }` to every index and primary-store range read, so no `search()`/`query()` in Harper
runs inside the request transaction. Reported from a live 5.2.7 deployment as "indexed `search()`
cannot see the row this request just wrote", with an application-level consequence (a per-account
row cap enforced by write-then-count overshooting under concurrency).

## Fix shape

Route `options.transaction` into the iterator context, so it means what `get()` means:

```ts
const iterContext = (options as DBITransactional)?.transaction?._context ?? context;
```

with the `ITERATOR_CONTEXT_IS_TRANSACTION_FLAG` derived from `iterContext`.

Not a one-liner in practice: `DBIteratorHandle` calls `txnHandle->registerIterator()`, so honouring
the option makes every such range read pin the transaction for as long as the iterator is open.
That interacts directly with Harper's read-transaction lifetime accounting
(`DatabaseTransaction.readTxnsUsed`, and the "read iterators held a committed transaction's
snapshot past the open-transaction limit" path), which is why the Harper-side issue is tracked
separately rather than folded in here.

Whatever the resolution, a silently-ignored option in the public type surface should not survive
it: if the transaction is deliberately not supported on range reads, `getRange` should reject the
option rather than accept and drop it.

## Versions

Present on `main` (`86757a84`). Nothing in the git history suggests it was ever otherwise; the
`get`/`getRange` asymmetry predates the transaction-log work.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.