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).

_{Review-Coverage: authored=unknown; ran=none; rounds=1 @ 4db2a0b}

_{Human-Review-Need: 4 @ 4db2a0b}

[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.

[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.65K ops/sec	40.57	39.09	640.681	0.116	123,256
🥈 rocksdb	2	10.75K ops/sec	93.02	90.48	4,917.526	0.196	53,754

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

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	28.87K ops/sec	34.63	33.58	536.384	0.098	144,370
🥈 rocksdb	2	11.16K ops/sec	89.62	86.54	3,135.034	0.144	55,791

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.87K ops/sec	38.66	35.90	3,674.415	0.314	129,339
🥈 rocksdb	2	16.21K ops/sec	61.67	53.76	1,086.955	0.118	81,075

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	384.42 ops/sec	2,601.323	95.18	32,723.637	10.16	770
🥈 lmdb	2	26.18 ops/sec	38,194.815	453.571	1,216,511.755	136.136	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	38.36K ops/sec	26.07	11.99	769.287	0.261	191,794
🥈 lmdb	2	439.66 ops/sec	2,274.461	170.966	25,931.734	1.51	2,199

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	741.36K ops/sec	1.35	1.15	478.224	0.064	3,706,820
🥈 lmdb	2	430.93K ops/sec	2.32	1.20	8,826.853	0.991	2,154,628

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	857.36 ops/sec	1,166.373	990.108	1,831.624	0.343	1,715
🥈 lmdb	2	1.16 ops/sec	865,318.755	797,411.129	951,413.924	4.73	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	23.88K ops/sec	41.87	30.12	546.78	0.591	47,769
🥈 lmdb	2	825.75 ops/sec	1,211.016	70.87	11,316.072	5.37	1,652

---

Results from commit 8c7390c

[cb1kenobi]

Medium: GC-time close() can free the handle out from under a still-running async get, because the wait it relies on is allowed to time out silently

This is a new interaction with the rebased base rather than a change in this PR's own code — the diff is byte-identical to the previously reviewed revision, but the base moved underneath it.

The safety argument for calling close() from a finalizer is that close() cancels and waits for in-flight async work before destroying the transaction. That wait does not actually guarantee it:

// src/binding/napi/async.h:162-179
void waitForAsyncWorkCompletion(std::chrono::milliseconds timeout = std::chrono::milliseconds(5000)) {
    ...
    if (elapsed >= timeout) {
        DEBUG_LOG(... "timeout waiting for async work completion, %u items remaining" ...);
        return;   // gives up, no signal to the caller
    }

close() calls it with the default and then proceeds unconditionally to this->txn->ClearSnapshot(); delete this->txn; (transaction_handle.cpp:342-344). It has already dropped the registry's strong reference via transactionRemove at the top, so when control returns here and the finalizer runs (*txnHandle).reset() (transaction.cpp:117), that is the last reference and the handle is freed.

What makes this newly reachable is the pairing of two independent changes:

1. This PR makes an orphaned TransactionHandle destroyable at all. Before it, the registry's strong reference kept the object alive forever, so a timed-out close() left a live-but-cancelled object and the execute handler's state->handle->isCancelled() guard read valid memory.
2. The new base added worker-thread dereferences inside the same execute handler:

// transaction_handle.cpp:515-519 (base-added)
vtCheckAsyncGet(state, state->handle->dbHandle->descriptor->db.get(), ...);

plus vtCheckLatest reading state->readOptions.snapshot on the worker (database.h:440 → database.h:118 readSnapshot->GetSequenceNumber()) — a raw snapshot pointer captured at queue time, which ClearSnapshot() has now released.

state->handle is a bare TransactionHandle* (AsyncGetState<TransactionHandle*>), so past the timeout even the isCancelled() guard on line 503 is itself a use-after-free.

Reachability is narrow but not exotic: the async-work registration is claimed at queue time, so the 5 s budget includes libuv queueing delay (default UV_THREADPOOL_SIZE=4). It also needs a directly-constructed Transaction rather than db.transaction(), whose async-function context pins the wrapper for the whole body — but Transaction is public API (src/index.ts:56) and this PR's own test constructs one that way.

The invariant worth enforcing rather than working around is in waitForAsyncWorkCompletion itself: a wait that is permitted to return without the condition holding is not a wait. Having it report the timeout, and having onWrapperCollected() re-arm/defer instead of destroying when it did not drain, fixes this at the root. Note the same pattern is relied on by DBHandle::close() (db_handle.cpp:132-135), so the fix likely belongs in async.h rather than in this PR alone — reasonable to split it out if you'd rather not grow a leak fix.

—
Generated by Barber AI

[cb1kenobi]

Agreed — close() from the finalizer is not safe while an async get still holds the handle, because waitForAsyncWorkCompletion can time out and then delete this->txn under the worker.

Stacked fix in #789:

• onWrapperCollected still sets wrapperCollected and still defers if Committing.
• For a pending txn it cancels in-flight work, and does not close() while activeAsyncWorkCount > 0.
• AsyncGetState now pins the handle with a shared_ptr (same shape as async Database::Get).
• The execute handler sees cancelled/collected and rejects with Transaction is closed instead of calling txn->Get.
• The complete callback close()s once execute has finished, so the snapshot is still released (harper#2107).

That way a get that races GC throws rather than UAFing. In-flight commit is unchanged (completeCommitWork still owns close).

[cb1kenobi]

This method seems to have been inserted between the description of the TransactionHandle::close() method and the actual TransactionHandle::close() code. Please move TransactionHandle::close()'s docblock next to it's implementation.

[cb1kenobi]

Agreed — close() from the finalizer is not safe while an async get still holds the handle, because waitForAsyncWorkCompletion can time out and then delete this->txn under the worker.

Stacked fix in #789:

• onWrapperCollected still sets wrapperCollected and still defers if Committing.
• For a pending txn it cancels in-flight work, and does not close() while activeAsyncWorkCount > 0.
• AsyncGetState now pins the handle with a shared_ptr (same shape as async Database::Get).
• The execute handler sees cancelled/collected and rejects with Transaction is closed instead of calling txn->Get.
• The complete callback close()s once execute has finished, so the snapshot is still released (harper#2107).

That way a get that races GC throws rather than UAFing. In-flight commit is unchanged (completeCommitWork still owns close).