Release the RocksDB transaction handle on abort and on a failed direct commit

[kriszyp]

Two paths could strand a RocksDB transaction handle, permanently pinning a read snapshot so the database could never discard obsolete row versions. Both now release it unconditionally, and system_information.metrics gains numSnapshots so the condition is visible.

Reported as #2107 — RocksDB read snapshot leaks permanently: on a production cluster a high-churn secondary index reached ~4 keys per live row in 5 days, numberReseeksIteration went 51 → 43,647, and bounded range scans degraded ~21x, with a process restart as the only recovery.

What was wrong

abort() released the native handle only through while (this.readTxnsUsed > 0) this.doneReadTxn(), and getReadTxn() is the only thing that sets readTxnsUsed. A write-first link — save() built the native transaction with no prior read — leaves it undefined, so undefined > 0 is false and the loop never runs. That shape is reachable: a write-only link in a multi-store chain never calls getReadTxn() (see the comment at chainStillActive), and abortDueToTimeout() walks the chain calling abort() on exactly those links.

directCommitSync() removed the transaction from tracking but never nulled this.transaction. On a throwing commitSync that left an open, untracked handle nothing could reach; on success it left the object serving an already-closed transaction from getReadTxn()'s early return.

doneReadTxn()'s native abort was unguarded. RocksTransaction.abort() throws on an already-released handle, and doneReadTxn() is the first statement of abort() — so a throw there skipped everything after it: CLOSED state, blob cleanup, context release. abortDueToTimeout() and abortChainAfterRetries() have no handler of their own. releaseReadTxn() was already guarded; this brings the other paths in line, and it is what makes the new unconditional release reachable on the read path at all.

Neither leak self-heals: rocksdb-js's descriptor registry holds a strong reference to the handle, so an unreleased one survives GC and holds its snapshot until the process exits. HarperFast/rocksdb-js is being fixed to release at GC as well; these are the paths that should never have depended on that.

What changed

The four detach-abort-swallow blocks are now one abortNativeTransaction() helper, so the reasoning behind swallowing the throw lives in one place instead of drifting across four call sites.

system_information.metrics gains numSnapshots per database. oldestSnapshotTime was the only snapshot signal and it stops moving once the oldest snapshot is pinned, so accrual was invisible. Note this reads rocksdb.num-snapshots, which lands in rocksdb-js 2.8; against the currently pinned 2.7.1 the field is simply absent, so this is inert until that ships and the pin moves.

Verification

npm run test:unit:resources — 1517 passing, 15 pending. Prettier and lint:required clean.

Four new cases in unitTests/resources/txn-tracking.test.js. Three are object-double checks of the release logic; the fourth builds the write-first shape on a real RocksDB store and asserts that both the registryStatus() transaction count and rocksdb.num-snapshots return to baseline after abort() — that one is the actual #2107 property. Fails-on-base: with origin/main's resources/DatabaseTransaction.ts restored, the write-first, real-snapshot, and direct-commit cases all fail; the read-created control passes on both, so it is not vacuous.

Not observable end-to-end beyond that: the leak's user-visible symptom is a multi-day scan degradation, so an integration test would have to age a store. The real-RocksDB unit assertion on num-snapshots is the closest honest proxy.

For the human reviewer

1. Unconditional release as a fallback after the refcount loop, rather than one unified release path. Two mechanisms now exist that must stay mutually exclusive — the loop owns the read-created handle, the fallback owns the write-first one — and the seam between them is exactly where the unguarded-throw gap lived. Chosen because collapsing them into a single mechanism means reworking the readTxnsUsed / readTxnRefCount pair and its deliberate off-by-one base, which is a larger change than this fix warrants. Reversible with a local refactor. Say no if you'd rather do the counter rework now.

2. directCommitSync() aborts and rethrows on a failed commitSync. That makes a failed sync commit terminal-rollback rather than leaving the handle for the caller to retry. Its two callers (resources/replayLogs.ts) already catch and log, and each version boundary builds a fresh transaction, so no caller loses a retry it was using. Changing this later alters observable write-atomicity behavior, so it is worth a second opinion.

3. numSnapshots ships ahead of the stat it reads. It is inert until rocksdb-js 2.8 lands and the pin moves. The alternative was holding this until the dependency ships; kept because it costs nothing and the metric is what makes the next occurrence a five-minute diagnosis rather than a multi-hour one.

Where to look hardest: abort() in resources/DatabaseTransaction.ts. The question worth the most scrutiny is whether zeroing readTxnsUsed in the fallback can strand an outstanding iterator that the refcount was tracking — I believe not, because the loop above has already drained any nonzero count and doneReadTxn() no-ops on a nulled handle, but that is the invariant the whole change rests on.

What the tests do not prove: that the write-only-next-link path actually reaches abort() with a live handle in a real multi-store request. The chain shape is asserted by main's own comments and by abortDueToTimeout()'s walk, not by a test that builds it end to end — a second database: in the unit suite resolves to the same store, so no next link forms there.

Coverage: Codex and Gemini ran on the implementation; Codex and Gemini again on the final artifact, with the Harper-domain leg on the middle revision. Findings acted on: the unguarded doneReadTxn() abort, the object-double-only test coverage, the duplicated release blocks, the missing throwing-abort case, an overstated numSnapshots comment (an in-flight read legitimately holds a snapshot — the signal is a count that stays nonzero while nothing is reading), and a helper that had been inserted between recordCommitLatency's doc comment and its function. Dropped after checking the code: Gemini's claim that trackedTxns.delete(this) is skipped for read-created handles (doneReadTxn() already deletes them) and its strict-null-check concern (tsconfig.json sets strict: false).

_{Human-Review-Need: 4 @ 3c5ead7}

[gemini-code-assist]

Code Review

This pull request addresses transaction handling and native handle cleanup in DatabaseTransaction.ts to prevent resource leaks and avoid pinning obsolete database versions. It introduces a safe abortNativeTransaction helper, ensures native handles are detached before committing or aborting, adds tracking for numSnapshots in system stats, and includes comprehensive unit tests. The review feedback suggests adding null/undefined guards in abortNativeTransaction and directCommitSync for defensive programming, and using assert.strictEqual instead of assert.equal in the test suite to comply with the repository's style guide.

[claude]

Reviewed; no blockers found.