fix: close pending transactions with their owning DBHandle; napi-free TransactionHandle::close (#741)

[cb1kenobi]

Root cause: pending transactions leak past their env and get walked at shutdown

A worker env that exits while a transaction is still pending leaks its TransactionHandle:

• Transaction::Constructor → transactionAdd stores a strong shared_ptr<TransactionHandle> in the process-global DBDescriptor::transactions.
• The JS wrap finalizer only drops the JS-side ref — it never closes the handle. Only commit/abort call transactionRemove.
• So the handle — holding a live RocksDB transaction + snapshot and an env that dangles once the worker dies — survives in the shared descriptor until the last env's DBRegistry::Shutdown → finishClose → close() walks it. That is exactly the Worker-env teardown destroys transactions on the shared DBDescriptor, corrupting the heap under concurrent commits #741 production stack (Shutdown → finishClose → TransactionHandle::close → ~OptimisticTransaction → ~TransactionBaseImpl → _int_free).

Besides the crash, this is a real resource leak: every worker recycled mid-request leaks an open RocksDB transaction and snapshot until process-wide shutdown.

The fix

1. Env-cleanup-hook reap — DBRegistry::CloseTransactionsByEnv → DBDescriptor::closeTransactionsByEnv, wired into the module's per-env cleanup hook: every transaction owned by the dying env's handles is closed on that env's own thread while the env is still valid. The invariant (AGENTS.md item 12): an env's pending transactions are reaped by its cleanup hook.

   Deliberately NOT in DBHandle::close(). A first iteration reaped there and it broke the legitimate close-then-commit overlap: db.transaction() awaits its callback before committing, so a commit is routinely one microtask behind db.close() and must still run — reaping rejected those commits with "Database not open", and under Deno's scheduling stranded the caller's commit promise (txn-close-commit-uaf timed out on all three Deno CI platforms; back to main-parity after the rework). At env teardown no such continuation can exist, so the hook is the only safe reap point. Between a user db.close() and env death, an open transaction's handle intentionally lingers (bounded, reaped at env exit).

2. TransactionHandle::close() is now napi-free — env/jsDatabaseRef/envThreadId are gone. The JS database is passed to UseLog per-call by the TS layer (its only consumer), so close is safe from any thread and any teardown phase, removing the cross-thread napi_delete_reference hazard class entirely.

Evidence

Repro: test/lingering-txn-shutdown.test.ts + test/fixtures/fork-lingering-txn-shutdown.mts — N workers exit with a pending transaction while an anchor pins the descriptor; the last worker's shutdown walks the leaked handles.

Linux/glibc (node:24 arm64, Debian bookworm, native — no sanitizers), 10 leakers per run:

build	leaker runs	committed-txn control	no-txn control
main (3ea9a0f)	0/10 pass — aborts: corrupted size vs. prev_size, corrupted double-linked list, free(): invalid pointer, malloc_consolidate(): invalid chunk size, 1× SIGSEGV	3/3	2/2
main + MALLOC_PERTURB_	0/4 pass	—	—
this branch (owner-reap iteration)	10/10 pass (incl. MALLOC_PERTURB_)	3/3	2/2
this branch (final env-hook reap)	10/10 pass	3/3	—

Those abort strings are the #741 family verbatim (incl. the exact corrupted size vs. prev_size from the still-skipped vt-lock-tracker-churn residual).

Which change carries the fix?

Isolation arms (same container and method; main plus exactly one change; 8 leaker runs each):

arm	result
releaseByOwner only	8/8 pass
napi-free close() only	8/8 pass

Each change is independently sufficient on Linux (arms measured on the first iteration; the leaker choreography exercises the same teardown reap as the final env-hook shape), which pins the corrupting write precisely: Shutdown walks the leaked handles of dead envs, close()'s thread-identity guard passes when the closing thread has recycled a dead worker's pthread (std::thread::id collision), and napi_delete_reference(dead env, dead ref) corrupts the heap. Removing the napi call breaks the chain at the write; reaping the env's transactions at env death breaks it at the source. Both are kept: the reap also fixes the resource leak (a live RocksDB transaction + snapshot per abandoned worker transaction), and napi-free close() removes the hazard class structurally.

The repro test runs on Linux (red on main, green here). On macOS it is skipIf(darwin): a Guard-Malloc-only fault in Node's second-pass napi finalizer drain persists even with the fix — it never reproduces natively or on glibc, and looks like a Node-core teardown artifact worth a separate minimal repro (no rocksdb involved).

Full Vitest suite (final tree, release build): 757 passed / 2 skipped / 0 failures locally. Deno txn-close-commit-uaf verified back to main-parity locally (12.5s pass, was a 60s timeout on all three Deno platforms with the first iteration); the new repro test is gated to Linux+Node per the repo's existing teardown-repro convention (notify-teardown-uaf).

Relationship to #745

The dominant #741 crash reproduces on main without any of #745's guarded paths engaging (verified: zero napi_delete_reference calls, zero TransactionHandle::close calls at fault time on the pre-fix repro), and #745's head does not prevent this repro (5/6 crash under the macOS harness). This PR fixes the lifecycle hole those guards work around; #745's stateMutex remains relevant for the same-env close-vs-commit-thread TOCTOU (AGENTS item 12a gaps untouched here).

Review coverage

Cross-model pre-push review (prepush-review.mjs, --author claude, risk=high/scope=full) at 675bfd5e:

leg	model family	result
graded	Codex	ok — 1 blocker, 1 major, 1 nit
findings	Gemini (agy)	ok — 1 major
domain adjudication	Claude	FAILED (--permission-mode dontAsk rejected by the installed CLI)

Two outside families produced coverage; the Harper domain leg failed, so the findings below were never adjudicated (no false-positive filter) — each was verified by hand against the code instead. Receipt published Human-Review-Need: 4 partly for that degradation. Cursor Grok/Composer were pruned by auto-policy.

What it changed, all in e9345cce:

• Codex corroborated the review blocker independently and scoped it the same way — narrow timeout hardening here, full admission-and-drain contract in Make TransactionHandle close exclude new work and fully drain in-flight operations #784. TransactionHandle::close() now drains before mutating anything and, on drain timeout, returns without touching state, log/VT state, or txn.
• Both outside models caught a leak the human review and I both missed: close() can only self-remove from DBDescriptor::transactions while it can still reach the descriptor through its DBHandle, and a user-closed handle has already reset that pointer — precisely the case this reap exists to catch, so the entry leaked for the process lifetime. closeTransactionsByEnv now removes it directly.

Follow-up worth noting for anyone re-running this: the prepush-review.mjs domain leg is currently broken against the installed claude CLI (dontAsk is no longer a valid --permission-mode).

Refs #741

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request addresses a critical native heap corruption issue (#741) caused by pending transactions outliving their owning DBHandle during worker environment teardown. The fix introduces DBDescriptor::releaseByOwner to clean up all registered resources in one sweep and refactors TransactionHandle to be napi-free by passing the JS database per-call to UseLog. Feedback on these changes highlights a potential null pointer dereference in Transaction::UseLog when called on a closed transaction, which should be guarded against. Additionally, the jsDatabase parameter in NativeDatabase::useLog should be made optional to preserve backward compatibility for existing TypeScript callers.

[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.45K ops/sec	40.90	39.29	638.786	0.116	122,258
🥈 rocksdb	2	10.67K ops/sec	93.70	90.22	31,409.542	1.24	53,363

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

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	28.75K ops/sec	34.78	33.69	489.821	0.100	143,760
🥈 rocksdb	2	11.39K ops/sec	87.80	86.13	580.35	0.051	56,945

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.79K ops/sec	38.78	35.43	1,917.035	0.288	128,945
🥈 rocksdb	2	15.40K ops/sec	64.94	56.76	1,069.791	0.118	76,992

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	374.47 ops/sec	2,670.469	96.62	49,334.33	18.08	749
🥈 lmdb	2	27.14 ops/sec	36,846.764	55.86	1,155,630.697	136.822	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	40.11K ops/sec	24.93	10.94	13,853.486	0.599	200,560
🥈 lmdb	2	443.85 ops/sec	2,252.993	93.67	22,273.006	1.39	2,220

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	724.40K ops/sec	1.38	1.21	5,163.229	0.214	3,622,023
🥈 lmdb	2	488.38K ops/sec	2.05	1.13	8,031.45	0.520	2,441,893

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	832.98 ops/sec	1,200.503	1,018.804	3,890.923	0.396	1,666
🥈 lmdb	2	1.16 ops/sec	859,733.09	809,935.263	914,224.616	2.80	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.70K ops/sec	44.06	29.74	512.162	0.561	45,394
🥈 lmdb	2	831.20 ops/sec	1,203.081	125.445	14,649.713	5.32	1,670

---

Results from commit 125de3e

[kriszyp]

Review outcome: changes needed before merge

#780 is the right narrow replacement for the dominant reproduced #741 failure: it reaps pending transactions at environment exit and removes the transaction-owned N-API reference. At head 630a3efb, two correctness problems still block that fix:

1. Environment cleanup can destroy a transaction while native work is active. The cleanup path calls close(), which mutates state before waiting and, after a five-second timeout, releases log/VT state and deletes txn even if RocksDB still uses it. The inline comment now states the required ordering: prevent new acquisition, drain already-acquired native work definitively, then mutate or destroy state. #784 tracks extending that invariant across every entry point.
2. useLog() can bind through a foreign database object. Because the new argument is checked only as an object, a transaction from database B can be paired with database A; descriptor-local transaction IDs commonly collide, so a log entry can silently attach to an unrelated transaction. The binding must validate exact DBHandle ownership before changing log state.

The native type declarations, committed-worker control, and unreferenced VT churn fixtures also need the inline corrections so the API contract and regression evidence match the implementation.

The GC-retained pending-transaction problem is real but can remain outside this PR now that it is tracked in #785. The omitted #745 lifecycle scope is durable under #741:

• #783 — parked coordinated-retry TSFNs outliving their environment
• #784 — complete TransactionHandle close/work synchronization
• #785 — GC-abandoned pending transactions
• #786 — remaining DBHandle N-API references reachable from shutdown
• related: #746 — Deno/macOS commit-teardown mutex crash

Accordingly, #780 should continue to use Refs #741: merging it must not close #741 or be described as fully superseding #745. #745 can stay closed as the obsolete implementation vehicle.

Reviewed head: 630a3efbc67d7705638204dfdafffc02cfca9d78. git diff --check passes.

— KrAIs (Codex)

[kriszyp]

Another thing I will note: I don't see any info in the PR description about which models were used to review this before pushing (should be at least two outside models). Just a note for future PRs.

[cb1kenobi]

Good call on the model coverage — I'd skipped it, and running it was worth the detour. Ran prepush-review.mjs (--author claude, risk=high/scope=full) at 675bfd5e: Codex (graded) + Gemini (agy) both produced coverage; the Harper domain adjudication leg failed (--permission-mode dontAsk is no longer valid for the installed claude CLI), so those findings are unadjudicated and I verified each by hand. Details now in the PR description.

Two outcomes, both in e9345cce:

1. Your close-during-active-work blocker — fixed, and Codex reached it independently. It also scoped it exactly as you did: narrow timeout hardening here, the full admission-and-drain contract in #784. waitForAsyncWorkCompletion() now reports whether it actually drained; close() drains before touching anything and, on timeout, returns without mutating state, releasing log/VT state, or deleting txn — the in-flight commit owns its own cleanup. A leaked transaction is recoverable; a use-after-free and a log/data disagreement are not.

The regression test took two attempts and the first one was worthless — worth recording. ROCKSDB_JS_COMMIT_DELAY_MS fires after execute completes, so it can't reach the drain at all. My first new seam sat at the end of executeCommitWork, which held the work registered but left txn unused during the stall: it passed identically with and without the fix (5/5 both arms on glibc). Moving it to immediately before rocksdb::Transaction::Commit() gives a real red/green — 3/3 SIGSEGV before the hardening, clean after, and since it's a plain dereference rather than silent heap corruption it reproduces natively on macOS with no allocator instrumentation, so that test isn't platform-gated.

2. A leak both outside models flagged that neither of us caught. close() can only self-remove from DBDescriptor::transactions while it can still reach the descriptor through its DBHandle — and a handle the user closed earlier has already reset that pointer. That's exactly the case this reap exists to catch, so the entry (a strong ref to the handle, and through it the DBHandle) leaked for the life of the process. closeTransactionsByEnv now removes it directly.

The other six inline comments are addressed in 675bfd5e, with one deliberate deviation: on the AGENTS.md indentation, adding the four spaces puts them inside an inline code span that straddles the line break (corrupted double-linked` / `list), changing what renders — and oxfmt strips it right back out, which is how it got that way. I rewrapped so the spans sit on one line instead: uniform indentation, correct rendering, formatter-stable.

Also filed separately: prepush-review.mjs's domain leg is currently broken against the installed claude CLI.

—
Generated by Barber AI

[kriszyp]

Also filed separately: prepush-review.mjs's domain leg is currently broken against the installed claude CLI. (--permission-mode dontAsk is no longer valid for the installed claude CLI),

What version of the CLI are you using? I think the latest should support dontAsk. But I posted https://github.com/HarperFast/skills-internal/pull/126 to tolerate this. And did you say you filed an issue (that I should link)?

Anyway, codex thinks there is still a pointer to fix.

_{🤖 Reviewed with Codex}

[kriszyp]

Making TransactionHandle::close() N-API-free solves the teardown constraint, but the unreachable-transaction root remains: the NativeTransaction finalizer only resets the wrapper’s shared pointer, while DBDescriptor::transactions retains a strong pointer. If an uncommitted Transaction is garbage-collected while its database remains open, its snapshot, locks/VT intents, log pending count, and DB handle remain alive until database close. After resolving the in-flight-close synchronization above, please close/abort from the finalizer or change the registry ownership model, and add a forced-GC regression against a long-lived database.

— KrAIs (Codex)