Serialize database destruction with concurrent opens

[kriszyp]

Summary

Harper currently works around a rocksdb-js lifecycle race by locking root opens in JavaScript. This moves that invariant into the native registry: physical destruction owns a database path across read-write/read-only descriptors, concurrent opens wait, and shutdown is serialized with both operations.

The change also makes teardown failures observable and recoverable, prevents destroy/shutdown from releasing the native database beneath backups or checkpoints, and makes unbounded async work drain before its native owner is released. This is the rocksdb-js root-cause fix for Harper PR #2169.

The reported shutdown-cleanup failure is fixed and the suite is green. It was a genuine crash introduced by this PR's quarantine behaviour, and the root cause is a process-exit ordering rule that is now written down as AGENTS.md invariant 19: no rocksdb::DB may outlive the module's env-cleanup hook. A close-time flush failure deliberately quarantines the descriptor so shutdown()/destroy() can retry it — but at process exit there is no later retry, and DBRegistry::instance is a namespace-scope static, so the surviving entry was destroyed from an atexit handler. Closing a RocksDB database there runs CancelAllBackgroundWork() → PeriodicTaskScheduler::Unregister() after RocksDB's own function-local statics are gone, so port::Mutex::Lock() gets EINVAL and RocksDB's PthreadCall aborts with pthread lock: Invalid argument. DBRegistry::Teardown() now releases whatever Shutdown() left behind, from the cleanup hook while RocksDB is still usable; the failure is still reported, the process just exits cleanly. It reproduces from test/background-error.test.ts alone, which chmods a database directory read-only so RocksDB records a sticky background error — after which every close-time flush fails. That file's fixtures now tear down with destroy(), so they stop leaving a broken database and a leaked temp dir behind.

Manual compaction cancellation is now complete on both close paths, which took three rounds to get right and is the part worth reading closely. RocksDB abandons a manual compaction only through the CompactRangeOptions::canceled pointer it was handed, so a token armed after teardown has already blocked on that compaction does nothing. Sync compactSync()/clearSync() hold an OperationGuard and are awaited by finishClose()'s in-flight wait, so they use the descriptor token armed by beginClose(). Async compact()/clear() released that guard at setup handoff and are awaited by DBHandle::close()'s async-work drain, so they get a per-handle token — which has two arming sites, one per closer, and the second is the fix in the newest commit: a foreign destroy()/shutdown() reaches a handle only through the closables sweep, the last step of teardown, and three earlier steps each block on that compaction (compactOnClose takes compactMutex, WaitForCompact() does not return under a manual compaction, then the sweep's own drain). finishClose() therefore publishes cancellation on every attached closable up front, before its first blocking step. Arming another thread's handle is safe precisely because the per-handle token is cleared by DBRegistry::OpenDB() after it adopts the new descriptor (invariant 20); the descriptor token, which is never cleared, could not be used this way. The whole contract is invariant 6.

The final open/attach race is fixed. DBRegistry::OpenDB() now receives the handle and publishes its descriptor-backed state and attaches it while databasesMutex still owns the lifecycle gate. A forced destroy therefore either precedes the open or sees the fully adopted handle in the descriptor's closables sweep; there is no return/adopt/attach gap in which RocksDB can be deleted beneath an invisible column-family handle. The worker-thread regression fixture asserts both handles are attached before destroy, verifies the racing handle is force-closed, then reopens that same instance and completes async I/O. Failed opens still retain their requested path, and the quarantine fault-injection fixture recovers by calling destroy() on that same failed-open handle.

For the human reviewer

Look hardest at four places, all teardown ordering: DBRegistry::Teardown()'s placement in the cleanup hook, the per-wait deadline split in DestroyDB/Shutdown, the up-front cancellation pass at the top of finishClose(), and the new registry-locked handle publication in OpenDB().

1. Resolved (compaction cancel token). The dedicated token is retained rather than aliasing RocksDB's cancellation pointer onto closing: RocksDB writes through the pointer it is given (DisableManualCompaction() sets the caller's atomic), and closing means the registry has an owner committed to running finishClose(), which RocksDB must not be able to publish. Honest limit on the coverage: the three fixtures fail if options.canceled is unwired, the sync one additionally fails if arming moves past the in-flight drain, and fork-compact-cancel-destroy.mts fails if the foreign-close arm moves back to the sweep — but none distinguishes arming in beginClose() from arming at the top of finishClose(), because for a single descriptor those are equivalent. beginClose() is still the right home for the descriptor token (one mutation site tied to the transition), but that is a design argument, not a tested one.
2. Resolved (open/adopt/attach gap). The descriptor and column family are now selected, cancellation is reset, all descriptor-backed handle fields are published, and the handle is attached before databasesMutex is released. The deterministic fixture proves the pre-fix layout fails: moving attachment back after the seam produced a native SIGSEGV, while the fixed build reports exactly two closables and force-closes the racing handle.
3. waitForAsyncWorkCompletion() now waits without a timeout, rather than letting teardown free a native database still used by a slow flush. The alternative is a bounded wait that leaks or needs new ownership machinery; changing this after release would alter availability semantics. Every operation that can run unboundedly under that wait now has a cancellation path — which is what items 1 and the count-scan abort exist for.
4. Destroy synchronously waits for admitted native backups, streams, and checkpoints before teardown; rejecting destroy while copies are active would avoid blocking but changes the operation contract. The end-to-end fixture covers a directory backup racing destroy.
5. A failed native close is quarantined for explicit shutdown()/destroy() recovery rather than erased or automatically deleted. This preserves data and exposes the failure, but an unrecovered path remains unavailable in-process. Teardown() is the terminal owner of that quarantine at process exit. The same failure also makes shutdown() throw, rather than reporting only through database:closeFailed; used directly as a Node 'exit' listener, that can stop later exit listeners, so the failure-delivery contract needs an explicit human ruling before release.
6. Declined here, real (raw-path aliases). Registry identity still compares raw path strings, so ../symlink aliases can bypass the destroy/open gate. Not introduced by this PR, and narrower than it reads — RocksDB's own directory LOCK already blocks a second read-write open through an alias, leaving a read-only alias during destroy as the live hazard. Canonicalizing changes a user-visible identity contract (registryStatus().path, every lifecycle error message, TransactionLogStoreRegistry keys, per-path lock/backup files) and wants its own PR and tests.
7. Declined here, real (commits admitted against a quarantined descriptor). After a quarantining close, commitCompletionsClosed sends Transaction::Commit down the legacy libuv path, and the closables sweep never ran, so pending transaction handles still admit. Two corrections to how it was reported: the same window exists on the first close pass (finishClose() flushes, then sweeps closables), so it is not specific to quarantine; and in the quarantined case there is no durable flush for the write to land after, since that flush is what failed. Rejecting on descriptor->isClosing() is what AGENTS.md invariant 18 and test/txn-close-commit-uaf.test.ts exist to prevent. The fix that closes it without rejecting anything is to run the closables sweep before the flush in finishClose() — a material reordering of the most delicate path here, which wants its own change.
8. Declined here, real, and the most serious of the three (CloseDB detaches before it closes). DBRegistry::CloseDB calls descriptor->detach(handle) before handle->close(), so for the duration of that close the handle is in no descriptor's closables. A concurrent DestroyDB/Shutdown/PurgeAll claim then runs finishClose() whose in-flight wait returns immediately (an async flush/compact/get released its OperationGuard at setup, invariant 17) and whose closables sweep — the only thing that would block on this handle's closeMutex and drain its async work — does not see it, reaching this->db.reset() while a libuv worker is still inside descriptor->flush(). That is a use-after-free on the rocksdb::DB. It is pre-existing (origin/main has the same ordering) and the same shape as the NativeIterator finalizer, which also detaches before close(); this PR does widen the window, by making the drain untimed and by having destroy() force-close foreign handles at all. The fix is small — close first, then detach, holding a local shared_ptr across it and releasing it before PurgeIfUnreferenced so the refcount check still sees an unreferenced descriptor — but it changes the serialization of the most delicate path in a PR that already reworks it, and it needs its own fixtures. Filed for triage rather than folded in.
9. Open review concern (foreign-handle N-API cleanup). DBHandle::close() currently treats a matching std::thread::id as proof that it owns the handle's N-API environment. Thread IDs may be reused after a worker exits, so a later worker could pass that guard and delete references against a dead environment. Releasing log references by napi_env from the environment cleanup hook would avoid that ambiguity, but it adds new cross-environment cleanup ownership and needs a dedicated worker-reuse regression rather than a mechanical edit here.

Also carried, unchanged from the previous round: getRange() pays a std::mutex per row for a window that only opens under a concurrent foreign forced close (deliberate, recorded rather than treated as free), and the last-env cleanup still keys off an unsynchronized --moduleRefCount == 0.

Verification

• End-to-end route: the full Node/Vitest suite exercises spawned-process and worker-thread destroy/open, backup, iterator, count-scan, compaction-cancel, shutdown, quarantine, and retry fixtures against the built addon.
• Coverage gap: validation of lifecycleWaitSeconds is tested, but the new lifecycle timeout branches do not yet have a fault-injected timeout-and-retry fixture proving each gate is released after the throw.
• pnpm build / pnpm check — passed.
• pnpm test:native — 159 passed.
• pnpm test — 857 passed, 3 skipped, exit 0. The previously reported exit-1 during process cleanup (.../000010.log: Permission denied followed by pthread lock: Invalid argument) no longer occurs.
• Post-review fixture hardening at d082b425: pnpm check passed, and node --expose-gc ./node_modules/vitest/vitest.mjs test/destroy.test.ts --run passed all 25 tests after replacing the scheduler-sensitive fixed sleep with a bounded closables === 2 poll.
• Fails-on-base checks, each built and run rather than reasoned about:
  • removing the Teardown() call → fork-quarantined-exit.mts hangs in WaitForFlushMemTables, and the chmod repro aborts with pthread lock: Invalid argument (gdb backtrace through ~unique_ptr<DBRegistry> → DBDeleter → PeriodicTaskScheduler::Unregister);
  • unwiring CompactRangeOptions::canceled → the compaction fixtures fail;
  • moving the descriptor-token arm past the in-flight drain → fork-compact-cancel-sync.mts fails (the compaction runs the full seam, then succeeds);
  • removing the up-front arming loop from finishClose() → fork-compact-cancel-destroy.mts fails, with the foreign destroy taking 7770ms instead of ~490ms.
• Independent pre-push review on d082b425: Codex confirmed the scheduler-independent poll fixes the final fixture-barrier finding and introduced no new findings. The preceding 85caaac7 round also completed Harper-domain adjudication; Gemini remained unavailable because the local CLI is unauthenticated, while Cursor was policy-pruned because this diff edits AGENTS.md.

Complexity: complicated

Refs #787

— Claude Opus

_{Review-Coverage: authored=claude; ran=codex; blocked=gemini(auth); declined=cursor-grok,cursor-composer,domain; rounds=14 @ d082b42}

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

[gemini-code-assist]

Code Review

This pull request introduces robust database lifecycle management for RocksDB JS bindings. It implements a timed-wait mechanism (lifecycleWaitSeconds) for open, destroy, and shutdown operations to prevent concurrent lifecycle conflicts. It also introduces a "quarantine" state for database paths when a native close, flush, compaction, or physical directory cleanup fails, preventing subsequent opens until the cleanup is retried via destroy() or shutdown(). Additionally, it ensures that in-flight operations (like backups and checkpoints) are safely awaited before destruction, and that thread-affine N-API references are cleaned up safely. There are no review comments, so I have no feedback to provide.

[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.34K ops/sec	41.08	39.67	585.287	0.120	121,722
🥈 rocksdb	2	10.22K ops/sec	97.87	93.58	3,814.19	0.155	51,091

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

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	28.51K ops/sec	35.07	33.98	2,139.257	0.131	142,566
🥈 rocksdb	2	10.85K ops/sec	92.21	89.35	576.322	0.049	54,226

ranges.bench.ts

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

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	24.42K ops/sec	40.94	36.50	1,842.85	0.289	122,125
🥈 rocksdb	2	15.52K ops/sec	64.44	57.32	1,121.048	0.122	77,590

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	357.94 ops/sec	2,793.767	61.61	61,671.718	16.69	716
🥈 lmdb	2	26.60 ops/sec	37,597.739	382.569	1,173,403.203	136.582	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	39.61K ops/sec	25.25	11.48	26,637.034	1.07	198,033
🥈 lmdb	2	444.59 ops/sec	2,249.279	193.817	24,364.103	1.52	2,223

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	687.69K ops/sec	1.45	1.26	460.429	0.065	3,438,460
🥈 lmdb	2	469.77K ops/sec	2.13	1.16	8,433.588	0.571	2,348,870

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	810.90 ops/sec	1,233.193	1,058.168	1,995.066	0.394	1,622
🥈 lmdb	2	1.16 ops/sec	863,530.401	831,357.626	919,384.119	2.14	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.61	20,316.362	2.07	45,769
🥈 lmdb	2	821.13 ops/sec	1,217.839	104.715	14,753.943	5.63	1,643

---

Results from commit 35e8d64

[cb1kenobi]

Reviewed f24ef7a5 — no issues found. This PR looks good, nice job!

Re-review of the one new commit since b35ad3f7 ("Address remaining lifecycle review threads"). Both previously-open findings are confirmed fixed in the code, not just marked resolved:

• db_iterator.cpp:289 (Medium, per-row getenv) — fixed. The lookup is hoisted into initializeTestSeams(), which runs as the first statement of NAPI_MODULE_INIT, and Next() now does a relaxed atomic load. Verified at the object-code level: ROCKSDB_JS_ITERATOR_NEXT_DELAY_MS no longer appears in db_iterator.o (only in binding.o), and the compiled DBIterator::Next contains zero getenv calls. The new ROCKSDB_JS_COUNT_DELAY_MS seam got the same treatment up front.
• db_registry.cpp:960/961 (Medium, closeError asymmetry) — fixed. The four copies of the finishClose() → erase-or-quarantine → notify → emit tail are collapsed into closeClaimedDescriptors(), and the policy that had drifted is now one named, documented option (failOnCompletedWithError). The gate reduces exactly to the old DestroyDB behavior when false and the old unconditional behavior when true, so the refactor is behavior-preserving while making the remaining asymmetry deliberate rather than accidental.
• The earlier getCount Low is also addressed: countRemaining() polls isClosing() per row and reports the abort instead of a partial count, on both the database and transaction paths, and the inaccurate "compaction is the only unbounded in-flight op" comment is corrected.

Also checked and cleared: the dropped if (condition) null guard in PurgeIfUnreferenced is safe (both DBRegistryEntry constructors make_shared the condition; the old guard only mattered because the notify used to sit outside the if (descriptor) block); the newly-added closeRetrying = false on the PurgeAll/PurgeIfUnreferenced quarantine paths is a no-op, since only DestroyDB/Shutdown ever latch it and beginClose() is single-shot.

Verification: full suite 55 files, 773 passed / 1 skipped / 0 failed; targeted destroy + ranges 64/64 including the new aborts an in-flight getCount() when a foreign destroy begins fixture. CI green on the head (Windows jobs still pending at review time).

One merge-ordering note, not a defect in this PR: finishClose() still holds txnsMutex across cancelForDB(), which takes writerMutex_, and this PR makes that a routine path because destroy() now force-closes every descriptor. If #744 lands with PurgeIfUnreferenced still on the wake callback path, its lock-order inversion becomes materially more likely — worth sequencing #744's fix before or with this.

—
Generated by Barber AI

[cb1kenobi]

Reviewed 542b058a — no issues found. This PR looks good, nice job!

Re-review of the one new commit since f24ef7a5 (the branch was rebased onto main after rocksdb-js#744 merged; verified via git range-diff that all 35 prior commits carried forward unchanged modulo rebase context, with commit 542b058a new at the tip).

542b058a fixes a real race: compactCancelRequested now stays armed for finishClose()'s whole duration (an async compact-on-close pass opts out via a new cancellable param instead), and Transaction::GetCount now takes an OperationGuard + isClosing() check so finishClose()'s drain can't return early and let the closables sweep roll back the transaction mid-scan. Both changes are consistent with the existing OperationGuard/ACQUIRE_OPERATIONS_LOCK pattern elsewhere in the codebase.

Also re-verified at object-code level:

• DBIterator::Next() has zero getenv calls in its compiled disassembly (ROCKSDB_JS_ITERATOR_NEXT_DELAY_MS is absent from db_iterator.o's string table); the seam is now a relaxed atomic load, set once in initializeTestSeams().
• closeClaimedDescriptors() remains the single teardown tail for all four callers (CloseDB, DestroyDB, PurgeAll, Shutdown), with the completed-but-errored policy as the named ClaimedCloseOptions.failOnCompletedWithError option — false only for destroy(), true (fatal) everywhere else.
• finishClose() still takes txnsMutex and holds it across cancelForDB(), which itself takes VT's writerMutex_ — the txnsMutex → writerMutex_ ordering is unchanged by rocksdb-js#744 merging.

pnpm test (destroy.test.ts lifecycle suite: 19/19), pnpm test:native (148/148, 3 expected macOS skips), and pnpm check all pass clean at this head.

—
Generated by Barber AI

[cb1kenobi]

Please rebase with main and resolve the merge conflicts.

[cb1kenobi]

Barbarian reviewed ba0e1c0 and found no blocking issues. No new blocking defects were confirmed on changed lines. Existing findings were not repeated.

—
Generated by Barber AI

[cb1kenobi]

High — Raw path aliases bypass destruction ownership

Registry identity compares the caller’s raw path string, so equivalent .., relative, or symlink paths can create separate entries and bypass destroyingPaths. An alias can open the same physical database while this loop closes only exact-string entries and then removes their shared files, producing live-handle I/O failures or a failed/corrupt destroy. Canonicalize the path once at every registry boundary and use that canonical identity for DBKey and destroyingPaths.

—
Generated by Barber AI

[kriszyp]

Real, but out of scope for this PR — leaving open for a human decision rather than resolving.

Registry identity has always been the raw path string; destroyingPaths inherits that, it does not introduce it. The practical blast radius is also narrower than it reads: RocksDB takes its own directory LOCK, so a second read-write open of the same physical database through an alias fails at the RocksDB layer regardless of what our map key says. What genuinely survives is a read-only alias (no write lock) that DestroyDB will not close before rocksdb::DestroyDB deletes the files.

Canonicalizing is the right fix and it is not a one-liner: DBKey identity is user-visible in registryStatus().path and in every lifecycle error message, TransactionLogStoreRegistry keys off descriptor->path, and the per-path lock/backup files derive from it — so it needs its own change with its own tests, plus a decision on weakly_canonical failure modes on a path whose tail does not exist yet. Doing it inside a PR that is already reworking teardown ordering would make both harder to review.

Filed as a follow-up for triage; carried in the PR body under ## For the human reviewer.

— Claude Opus

[cb1kenobi]

High — Quarantined descriptors still admit transaction commits

After a close-time flush failure, the descriptor is closing and its commit workers are stopped, but pending transaction handles are not cancelled yet. During shutdown() retry this registration can therefore succeed and the fallback async commit can run after the retry’s flush, violating close durability—especially with WAL disabled. Admit commits under a descriptor OperationGuard, reject when descriptor->isClosing(), and ensure close drains the admission before stopping workers and flushing.

—
Generated by Barber AI

[kriszyp]

Leaving open — the analysis holds but the suggested fix collides with a documented invariant, so this is a human call rather than something to change under bot feedback.

Confirming the window: after a quarantining close, finishClose() has already set commitCompletionsClosed, so Transaction::Commit falls through to the legacy libuv path, which admits on the transaction handle. The closables sweep never ran (the flush threw first), so those handles are not cancelled and admission succeeds. Two corrections to the framing, though: the same window exists on the first close pass too — finishClose() flushes and only then sweeps closables — so it is not specific to quarantine or to the shutdown() retry; and there is no durable flush for the write to land after in the quarantined case, since that flush is precisely what failed.

The suggested fix — reject admission when descriptor->isClosing() — is what AGENTS.md invariant 18 exists to prevent. db.transaction() awaits its callback before committing, so a legitimate commit is routinely one microtask behind a db.close(); rejecting there is what stranded commit promises under Deno and is what test/txn-close-commit-uaf.test.ts guards. (Today that overlap is protected by refcount, not by admission policy: a live transaction pins the descriptor so PurgeIfUnreferenced skips the claim.)

The fix that would actually close it without rejecting anything is to run the closables sweep before the flush in finishClose() — every transaction handle is then closed and drained, so nothing can commit after the flush. That is a material reordering of the most delicate path in this PR and wants its own change and its own review.

Carried in the PR body under ## For the human reviewer.

— Claude Opus