Release a transaction dropped without commit or abort, instead of leaking its read snapshot for the life of the process

[kriszyp]

A transaction dropped without commit() or abort() used to live for the life of the process, holding a RocksDB read snapshot that stopped the database from ever discarding obsolete row versions. It now releases itself when V8 collects its JS wrapper.

Reported as HarperFast/harper#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.

The defect

DBDescriptor::transactionAdd stores a strong shared_ptr in the transactions map, while the closables entry written on the very next line is a weak_ptr. TransactionHandle::close() is the only path that calls ClearSnapshot(), and it is reached from ~TransactionHandle() — but the destructor can never run while the registry holds its reference. The napi finalizer only did (*txnHandle).reset(), which drops the JS-side reference and nothing else. So the destructor was correct and unreachable, and any caller that dropped a transaction leaked it permanently and silently.

The finalizer now calls onWrapperCollected() first. Once V8 has collected the wrapper, no JS code can commit, abort, retry, or read through that handle again, so it is closed. A commit in flight is the one exception — TransactionCommitState still owns the handle, and closing there would cancel the commit mid-flight — so the commit-completion paths close it instead: success already closed unconditionally, and the failure paths, which deliberately reset to Pending so a caller can retry, now check wrapperCollected because there is no caller left.

Why not make the registry reference weak_ptr. That was the first suggestion in the issue and it is not safe here: an async get holds a raw TransactionHandle* (AsyncGetState<TransactionHandle*>) and depends on close() running cancelAllAsyncWork() / waitForAsyncWorkCompletion() before the transaction is destroyed. Letting the last shared_ptr drop destroy the handle would race that. Going through close() from the finalizer gets the same self-healing with that machinery intact.

Observability

registryStatus() now reports transactionDetails — id and ageMs per live handle — and rocksdb.num-snapshots is exposed as a curated stat. A bare transaction count cannot distinguish a request in flight from a database that can never reclaim again; a nonzero snapshot count plus a handle age beyond any plausible request lifetime can. The registryStatus() read of the transactions map also now takes txnsMutex, which it was missing.

transactionDetails deliberately does not report snapshotSet or state. See the decision below.

Verification

pnpm test — 752 passing, 1 skipped, on Node. pnpm check clean. pnpm build:binding:debug compiles.

New test/transaction-orphan-gc.test.ts drives the real native stack (no mocks): a dropped transaction, a dropped transaction after a rejected commit, a still-referenced transaction (control — must not be disturbed), a caller that drops its reference before the commit settles, and an orphan that never read. The first two fail on main; the controls pass on both, so they are not vacuous.

The reproducer behind the issue, against main before this change:

baseline           num-snapshots= 0  oldest-snapshot-time= 0           registry.transactions= 0
after read in txn  num-snapshots= 1  oldest-snapshot-time= 1786236991  registry.transactions= 1
after GC x8        num-snapshots= 1  oldest-snapshot-time= 1786236991  registry.transactions= 1

Not fixed here

if (state == Committing) state = Pending in the commit-completion paths is a cross-thread compare-then-store that can overwrite an Aborted set concurrently by close(). Review surfaced it; it is pre-existing, independent of this change, and wants its own fix rather than being folded into a leak fix. Filed as #769.

For the human reviewer

1. Finalizer-driven close() rather than a weak registry reference. Chosen because an async get holds a raw TransactionHandle* and relies on close()'s cancel-and-wait; a destructor triggered by the last shared_ptr drop would skip it. The alternative — weak registry ref plus changing transactionRemove to take a raw pointer (close() currently calls shared_from_this(), which would throw bad_weak_ptr from a destructor) — is a larger lifetime change that also brushes against the open Worker-env teardown destroys transactions on the shared DBDescriptor, corrupting the heap under concurrent commits #741 and Pessimistic transactions still poison the environment on a drop-race commit #726. Reversible, but it would be a rewrite of this approach rather than a tweak. Say no if you'd rather take the lifetime change properly.

2. transactionDetails reports only id and ageMs. An earlier revision reported snapshotSet and state and made both std::atomic to do it safely. Review showed that was the wrong trade: it broke the debug build (DEBUG_LOG is variadic, std::atomic has no copy constructor), the implicit operators default to seq_cst so every state and snapshot check on the read/write paths grew a full barrier for an occasional diagnostic read, and it still didn't make the compare-then-store transitions atomic. id and createdAt are fixed before the handle is published to the registry, so they are race-free by construction. Cost: you cannot see per-handle "is this one holding a snapshot" — rocksdb.num-snapshots answers that per database instead. Cheap to revisit if the per-handle flag turns out to matter operationally.

3. The Committing deferral branch is unreachable from JS. A pending commit promise's executor still holds the wrapper, so V8 cannot collect it while the commit is in flight. The branch is kept as a correctness guard for any future path that can drop the wrapper mid-commit (a native-side caller, or a JS commit shape that doesn't retain), and the test that used to claim to exercise it has been renamed to what it actually proves. Alternative is deleting the branch and asserting the invariant instead; kept because the cost is three lines and the failure mode it guards is a cancelled in-flight commit.

Where to look hardest: onWrapperCollected() in src/binding/transaction/transaction_handle.cpp and the two wrapperCollected checks in src/binding/transaction/transaction.cpp. The question worth the most scrutiny is whether state == Committing is a sufficient test for "a commit still owns this handle" on every path that can reach the finalizer.

What the tests do not prove: cross-worker registry safety under concurrent envs, and finalization during an active commit (unreachable, above). The pnpm test:bun and pnpm test:deno routes are handled by the GC adapter in the new test file but were not run here.

Coverage: Codex and Gemini both ran on the final artifact; the Harper-domain leg ran on the previous revision. Findings acted on: the cross-environment field race, the debug-build break, the seq_cst hot-path regression, the Bun GC adapter, the overclaiming mid-commit test, and the missing never-read orphan case. Findings dropped after checking the code: Gemini's null-dereference on state->handle (the enclosing condition already requires a non-null handle) and the extra steady_clock read per transaction (a vDSO read next to BeginTransaction(), and steady_clock is the correct source for an age that must survive wall-clock adjustments).

_{Human-Review-Need: 4 @ 095e6db}

[gemini-code-assist]

Code Review

This pull request addresses a critical issue where orphaned transactions (dropped without being committed or aborted) pinned RocksDB snapshots and prevented the reclamation of obsolete versions. It introduces a mechanism to release these transactions when their JS wrappers are garbage collected, exposes a new rocksdb.num-snapshots statistic, and adds transaction details to the registry status. The review feedback highlights a thread-safety concern regarding concurrent access to TransactionHandle::state across threads, suggesting making it atomic to prevent data races. Additionally, it recommends using process.versions.bun for Bun detection in tests to maintain consistency with repository conventions.

[gemini-code-assist]

Style Guide Adherence: Bun Detection Convention

According to the repository's general rules, we should prefer accessing process.versions.bun directly to check for Bun's presence in tests to maintain consistency with the repository's established conventions.

Suggested change

	const forceGC: () => void =
	typeof globalThis.gc === 'function'
	? globalThis.gc
	: typeof (globalThis as { Bun?: { gc?: (sync: boolean) => void } }).Bun?.gc === 'function'
	? () => (globalThis as unknown as { Bun: { gc: (sync: boolean) => void } }).Bun.gc(true)
	: () => {
	throw new Error('these tests need an exposed GC (node/deno --expose-gc, or Bun.gc)');
	};
	const forceGC: () => void =
	typeof globalThis.gc === 'function'
	? globalThis.gc
	: process.versions.bun
	? () => (globalThis as unknown as { Bun: { gc: (sync: boolean) => void } }).Bun.gc(true)
	: () => {
	throw new Error('these tests need an exposed GC (node/deno --expose-gc, or Bun.gc)');
	};

References

1. Prefer accessing process.versions.bun directly to check for Bun's presence in tests to maintain consistency with the repository's established conventions, as long as the repository's tsconfig configuration allows it without compilation errors.

[github-actions]

📊 Benchmark Results

get-sync.bench.ts

getSync() > random keys - small key size (100 records)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	24.44K ops/sec	40.92	39.79	503.64	0.070	122,185
🥈 rocksdb	2	10.17K ops/sec	98.36	94.27	31,491.379	1.24	50,832

getSync() > sequential keys - small key size (100 records)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	28.34K ops/sec	35.29	34.25	530.322	0.101	141,691
🥈 rocksdb	2	10.57K ops/sec	94.61	92.35	579.17	0.056	52,848

ranges.bench.ts

getRange() > small range (100 records, 50 range)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	25.44K ops/sec	39.30	36.01	1,880.714	0.299	127,224
🥈 rocksdb	2	16.81K ops/sec	59.49	51.57	1,082.964	0.124	84,049

realistic-load.bench.ts

Realistic write load with workers > write variable records with transaction log

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	397.54 ops/sec	2,515.47	96.16	39,247.395	12.96	801
🥈 lmdb	2	27.46 ops/sec	36,421.894	456.068	1,144,217.065	136.658	64.00

transaction-log.bench.ts

Transaction log > read 100 iterators while write log with 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	36.55K ops/sec	27.36	13.08	437.651	0.241	182,743
🥈 lmdb	2	436.13 ops/sec	2,292.912	111.735	11,455.747	1.39	2,181

Transaction log > read one entry from random position from log with 1000 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	743.34K ops/sec	1.35	1.17	5,140.052	0.214	3,716,698
🥈 lmdb	2	461.62K ops/sec	2.17	1.09	7,629.167	0.587	2,308,096

worker-put-sync.bench.ts

putSync() > random keys - small key size (100 records, 10 workers)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	851.56 ops/sec	1,174.319	996.918	1,815.364	0.322	1,704
🥈 lmdb	2	1.16 ops/sec	865,180.616	818,275.159	941,427.729	3.08	10.00

worker-transaction-log.bench.ts

Transaction log with workers > write log with 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	22.88K ops/sec	43.70	29.32	580.584	0.590	45,767
🥈 lmdb	2	827.64 ops/sec	1,208.253	306.389	12,492.8	5.23	1,658

---

Results from commit 85444c0