Serialize database destruction with concurrent opens

AGENTS.md

@@ -210,6 +210,13 @@ sufficient (env teardown does not honor tsfn acquire counts); see
   ambiguous "disable the bound") falls back to the default like any malformed
   value. There is no opt-out: a deployment that would rather wait than fail a
   legitimately slow holder raises the value instead
+- `ROCKSDB_JS_DESTROY_DELAY_MS` - Test-only: delay after descriptor teardown and
+  before physical database destruction (widens same-path reopen races)
+- `ROCKSDB_JS_ITERATOR_NEXT_DELAY_MS` / `ROCKSDB_JS_COUNT_DELAY_MS` - Test-only: per-row delays in
+  `DBIterator::Next()` and `DBIteratorHandle::countRemaining()`. Both are read **once** in
+  `initializeTestSeams()` rather than per row: these are the two per-row native loops, and a
+  `getenv()` scan per row is a measurable share of their cost for a seam unset in production. Add
+  new per-row seams the same way.
 
 ## Test Structure
 
@@ -306,7 +313,42 @@ sufficient (env teardown does not honor tsfn acquire counts); see
    their own `shared_ptr` for the duration of a copy (backup, backup stream, checkpoint) make a
    racing close skip the purge (`use_count > 1`), so their state destructors re-run
    `PurgeIfUnreferenced` after releasing the ref — without that retry the skipped purge is permanent
-   and the entry (plus the open RocksDB) leaks (HarperFast/rocksdb-js#672).
+   and the entry (plus the open RocksDB) leaks (HarperFast/rocksdb-js#672). Once `beginClose()` wins,
+   `DBHandle::opened()` must report false even while the native DB still exists. Any synchronous N-API
+   path that dereferences `descriptor->db` or the handle's column family must take an `OperationGuard`
+   immediately after `UNWRAP_DB_HANDLE_AND_OPEN()`; `finishClose()` can reset the column-family pointer
+   from another env after the in-flight count drains. The VT-only `verifyVersion` / `populateVersion`
+   fast paths narrow, but do not remove, that requirement: both still start with
+   `UNWRAP_DB_HANDLE_AND_OPEN()`, so they still gate on `descriptor`/`isClosing()`. What
+   `DBHandle::open()`'s snapshotted `verificationTableDbId` / `verificationTableColumnFamilyId` actually
+   avoids is the `getColumnFamilyHandle()` dereference and the `OperationGuard`'s in-flight
+   registration — the two things that are unsafe to skip everywhere else. `PutSync`/`RemoveSync`/
+   `TransactionHandle` still compute the VT address as `descriptor->vtEpoch` +
+   `getColumnFamilyHandle()->GetID()` rather than reading the cached fields, so there are two
+   spellings of the same address computation that must stay in agreement.
+   Async N-API setup must hold the guard until it hands off to `DBHandle::registerAsyncWork()`. Iterators
+   take the guard through construction/descriptor attachment, then serialize each native iterator call
+   against foreign forced close with their per-iterator mutex. `DBHandle::close()` itself is cross-env and must
+   serialize mutation of its `shared_ptr` members. A close-time flush failure keeps the native DB
+   quarantined so `shutdown()` can retry without losing `disableWAL` writes; an explicit destroy may
+   force teardown because the caller requested deletion. A failed physical destroy leaves a registry
+   tombstone, but `shutdown()` is deliberately non-destructive: it reports the tombstone and only an
+   explicit `destroy()` retries path deletion.
+   Because `finishClose()` drains `operationsInFlight` with an **untimed** wait, any operation that can
+   run unboundedly while holding an `OperationGuard` must abort itself once `closing` is published, or
+   it blocks teardown — and, since the blocked closer holds the path gate, times out every concurrent
+   `OpenDB()` for that path. There are two such operations and they cancel differently: the whole-range
+   count scan (`DBIteratorHandle::countRemaining`, behind `getKeysCount()` on both the database and
+   transaction paths) polls `isClosing()` per row and reports the abort to its caller rather than a
+   partial count; a manual `compactRange()` cannot poll from inside RocksDB, so it gets an explicit
+   cancel token (`DBDescriptor::compactCancelRequested` → `CompactRangeOptions::canceled`).
+   Everything the four registry teardown paths do _after_ claiming a descriptor —
+   `finishClose()`, erase-or-quarantine, notify, emit `database:closeFailed` — is one helper,
+   `closeClaimedDescriptors` in `db_registry.cpp`; only the claim predicate differs per caller. Its
+   `failOnCompletedWithError` option is the one deliberate asymmetry: a close that finished native
+   teardown but reported an error (a failed close-time flush) is fatal for `shutdown()`/`PurgeAll()`
+   because dropping it silently would hide possible data loss, and non-fatal for `destroy()`, whose
+   caller asked for the data to be deleted anyway.
 7. **One writable BackupEngine per backup directory (kernel advisory lock)**: each backup op opens its
    own short-lived `rocksdb::BackupEngine`/`BackupEngineReadOnly` (`src/binding/database/backup.cpp`), and
    RocksDB only serializes work _within_ a single engine — it has no cross-engine lock on the directory.
@@ -587,6 +629,27 @@ sufficient (env teardown does not honor tsfn acquire counts); see
     is the descriptor's single `CommitWorker` thread (see "Commit execution" above), which dispatches
     every `Transaction.commit()` in order — so opting a flush into a stall queues up every commit
     behind it, including ones from callers that never touched flush.
+17. **Async-work admission and cancellation share one mutex; the drain that follows must never time
+    out**: `AsyncWorkHandle` (`napi/async.h`) tracks in-flight async work per `DBHandle`/
+    `TransactionHandle`. `registerAsyncWork()` and `cancelAllAsyncWork()` both take `waitMutex`, so a
+    registration that races a close either lands (and is counted) before cancellation publishes, or
+    is refused — there is no window where it is admitted after `waitForAsyncWorkCompletion()` has
+    already observed the count at zero. Refusal returns `false`; every call site (the shared
+    `admitAsyncWorkOrReject()` helper, or `ScopedAsyncWorkRegistration::ok()` for the RAII
+    cross-handle case in `transaction_handle.cpp`) must fail the operation — reject the
+    already-constructed promise and touch no native state — rather than proceed with work nothing is
+    tracking anymore. `waitForAsyncWorkCompletion()` itself has no timeout: `DBHandle::close()` /
+    `TransactionHandle::close()` call it immediately before releasing the `rocksdb::DB`, column
+    family, or transaction that admitted work may still be using, and a flush legitimately waiting
+    out a write stall (invariant 16) can run far longer than any fixed bound. A bounded wait that
+    gives up anyway — the previous 5-second default — let `finishClose()` reach `this->db.reset()`
+    while a flush was still executing against it, a genuine use-after-free. `database.cpp`'s
+    `Flush`/`Compact`/`Clear`/async `Get` rely entirely on this drain for safety: their
+    `OperationGuard` from `ACQUIRE_OPERATIONS_LOCK()` covers only the synchronous setup, not the
+    queued execute callback. Backup/checkpoint/backup-stream additionally hold the descriptor's
+    `operationsInFlight` claim through their whole async execution (the pinning pattern from
+    invariant 9), so for those this drain is defense in depth rather than the only thing preventing
+    a use-after-free.
 
 17. **An env's pending transactions are reaped by its cleanup hook — never by `DBHandle::close()`**:
     `transactionAdd` stores a strong `shared_ptr<TransactionHandle>` in the process-global

README.md

@@ -141,7 +141,11 @@ Creates a new database instance.
 ### `db.close()`
 
 Closes a database. This function can be called multiple times and will only close an opened
-database. A database instance can be reopened once its closed.
+database. A database instance can be reopened once it is closed. A flush failure leaves the native
+database quarantined so `shutdown()` can retry without losing unflushed data; an explicit
+`destroy()` can instead delete it. A compaction failure is reported after native teardown
+completes. All native close errors emit `database:closeFailed`. The quarantine applies to both
+writable and read-only opens because both modes share the physical path lifecycle.
 
 ```typescript
 const db = RocksDatabase.open('foo');
@@ -183,6 +187,12 @@ Sets global database settings.
     Defaults to 32MB. Set to `0` (zero) disables block cache for future opened databases. Existing
     block cache for any opened databases is resized immediately. Negative values throw an error.
   - `compactOnClose: boolean` When `true`, compacts the database on close. Defaults to `false`.
+  - `lifecycleWaitSeconds: number` How long a synchronous open, destroy, or shutdown waits for a
+    _conflicting_ lifecycle operation already in progress on the same path (e.g. another open or
+    close) before throwing a retryable timeout error. It does not bound the separate, intentionally
+    unbounded wait that `destroy()`/`shutdown()` make for in-flight backups, checkpoints, or other
+    async work still using the database — see [`db.destroy()`](#dbdestroy-void). Defaults to `30`
+    seconds and must be a positive integer.
   - `verificationTableEntries: number` The number of slots in the process-global
     [Verification Table](#verification-table). Each slot is 8 bytes, so the default of `131072`
     (128K) slots is 1 MB. Set to `0` to disable the verification table. This must be configured
@@ -349,7 +359,16 @@ db.compactSync({ bottommost: true });
 ### `db.destroy(): void`
 
 Completely removes a database based on the `db` instance's path including all data, column families,
-and files on disk.
+and files on disk. Destruction owns the physical path for the process: it closes every writable and
+read-only handle for that path, waits for registered backups and checkpoints to stop using the
+native database, and prevents another handle from reopening the path until removal finishes. Those
+waits are synchronous and can outlive `lifecycleWaitSeconds` once destruction has claimed the path,
+because releasing the native database beneath an active copy would be unsafe.
+
+A previously opened instance does not need to remain open, which allows an explicit `destroy()`
+retry after failed physical cleanup. A never-opened or read-only instance cannot destroy the
+database. `shutdown()` reports a pending cleanup tombstone but never retries deletion; only an
+explicit `destroy()` can remove the path.
 
 ```typescript
 db.destroy();
@@ -1898,6 +1917,9 @@ console.log(currentThreadId());
 Returns an array containing that status of all active RocksDB instances.
 
 - `path: string` The database path.
+- `closeError?: string` The native lifecycle error retaining this registry entry.
+- `destroyCleanupPending?: boolean` The native database is closed, but physical path cleanup must
+  finish before the next open. Call `destroy()` to retry cleanup.
 - `refCount: number` The number of JavaScript database instances plus the registry's reference.
 - `columnFamiles: object` A map of column family names and their their info.
   - `userSharedBuffers: number` The count of active user shared buffers.
@@ -1915,11 +1937,20 @@ console.log(registryStatus());
 
 The `shutdown()` will flush all in-memory data to disk and wait for any outstanding compactions to
 finish, for all open databases. It is highly recommended to call this in a `process` `exit` event
-listener (on the main thread), to ensure that all data is flushed to disk before the process exits:
+listener (on the main thread), to ensure that all data is flushed to disk before the process exits.
+It throws the first close failure after attempting every claimed database; call it again to retry
+any descriptor whose native teardown did not complete. It reports pending destroy-cleanup
+tombstones without deleting their paths; retry those with an explicit `destroy()`:
 
 ```typescript
 import { shutdown } from '@harperfast/rocksdb-js';
-process.on('exit', shutdown);
+process.on('exit', () => {
+	try {
+		shutdown();
+	} catch (error) {
+		console.error('rocksdb-js shutdown failed', error);
+	}
+});
 ```
 
 ### `versions: { 'rocksdb': string; 'rocksdb-js': string }`

benchmark/setup.ts

@@ -235,6 +235,7 @@ interface WorkerState {
 	benchPromise: ReturnType<typeof withResolvers<void>>;
 	exitPromise: ReturnType<typeof withResolvers<void>>;
 	teardownPromise: ReturnType<typeof withResolvers<void>>;
+	teardownError?: Error;
 }
 
 interface WorkerBenchmarkOptions extends BenchmarkOptions<any, any> {
@@ -381,6 +382,7 @@ export function workerBenchmark(type: string, options: any): void {
 	}
 
 	const workerState: WorkerState[] = [];
+	const dbPath = join('benchmark', 'data', `rocksdb-benchmark-${randomBytes(8).toString('hex')}`);
 	const workerPayload = {
 		suites: workerCurrentSuites.map((suite) => suite.name),
 		benchmark: benchmarkName,
@@ -408,12 +410,6 @@ export function workerBenchmark(type: string, options: any): void {
 				if (mode === 'run') {
 					return;
 				}
-				const path = join(
-					'benchmark',
-					'data',
-					`rocksdb-benchmark-${randomBytes(8).toString('hex')}`
-				);
-
 				let teardownTimeoutId: NodeJS.Timeout;
 				await Promise.race([
 					activeBenchmark,
@@ -436,19 +432,24 @@ export function workerBenchmark(type: string, options: any): void {
 								benchmarkFile: pathToFileURL(benchmarkFile).toString(),
 								benchmarkWorkerId: i + 1,
 								mode,
-								path,
+								path: dbPath,
 							});
 							// important! these promises need to be referenced as
 							// properties of `state` because they are reset by reference
-							const state = {
+							const state: WorkerState = {
 								worker,
 								benchPromise: withResolvers<void>(),
 								exitPromise: withResolvers<void>(),
 								teardownPromise: withResolvers<void>(),
 							};
 							workerState[i] = state;
 							worker.on('error', reject);
-							worker.on('exit', () => {
+							worker.on('exit', (code) => {
+								if (code !== 0 && !state.teardownError) {
+									state.teardownError = new Error(
+										`Benchmark worker ${i + 1} exited with code ${code}`
+									);
+								}
 								state.benchPromise.resolve();
 								state.teardownPromise.resolve();
 								state.exitPromise.resolve();
@@ -461,6 +462,9 @@ export function workerBenchmark(type: string, options: any): void {
 									state.benchPromise.resolve();
 								} else if (event.teardownDone) {
 									state.teardownPromise.resolve();
+								} else if (event.teardownError) {
+									state.teardownError = new Error(event.teardownError);
+									state.teardownPromise.resolve();
 								} else if (event.timeout) {
 									state.teardownPromise.resolve();
 									state.benchPromise.reject(new Error('Benchmark timed out'));
@@ -492,8 +496,19 @@ export function workerBenchmark(type: string, options: any): void {
 						return workerState[i].exitPromise.promise;
 					})
 				);
+				const teardownError = workerState.find((state) => state.teardownError)?.teardownError;
+				if (!teardownError) {
+					try {
+						rmSync(dbPath, { force: true, recursive: true, maxRetries: 3 });
+					} catch (err) {
+						console.warn(`Benchmark teardown failed to delete db path: ${err}`);
+					}
+				} else {
+					console.warn(`Benchmark teardown failed; retaining ${dbPath} for inspection`);
+				}
 
 				resolve();
+				if (teardownError) throw teardownError;
 			},
 		}
 	);
@@ -529,12 +544,14 @@ export async function workerInit(): Promise<void> {
 				await teardown(ctx);
 			}
 			if (ctx.db) {
-				// console.log('workerTeardown', workerData.benchmarkWorkerId, workerData.mode, type, path);
-				ctx.db.close();
 				try {
-					rmSync(path, { force: true, recursive: true, maxRetries: 3 });
-				} catch (err) {
-					console.warn(`Benchmark teardown failed to delete db path: ${err}`);
+					await ctx.db.close();
+				} catch (error) {
+					parentPort!.postMessage({
+						teardownError: error instanceof Error ? error.message : String(error),
+						benchmarkWorkerId,
+					});
+					process.exit(1);
 				}
 			}
 			parentPort!.postMessage({ teardownDone: true, benchmarkWorkerId });

docs/backups.md

@@ -202,8 +202,9 @@ The archive contains the standard RocksDB files for the snapshot (`CURRENT`, a `
   consumer therefore lets obsolete SST files accumulate (transient disk pressure) until the stream
   finishes. This is the cost of streaming with no scratch copy; a fast consumer is unaffected.
 - **The database must stay open for the whole stream.** Closing or destroying the database while a
-  stream is in flight aborts it: the backup promise rejects, and `destroy()` throws rather than
-  tearing the database down underneath the copy.
+  stream is in flight aborts the stream and makes the backup promise reject. `destroy()` waits for
+  the native producer to release its in-flight claim, then closes every handle for the path and
+  removes the database; it never tears the native database down underneath the copy.
 - **A consumer error aborts the backup.** If `stream.write()` rejects (or the stream is aborted), the
   backup promise rejects and the native producer stops.
 

src/binding/binding.cpp

@@ -39,8 +39,21 @@ namespace rocksdb_js {
  * Shutdown function to ensure that we write in-memory data from all databases.
  */
 napi_value Shutdown(napi_env env, napi_callback_info info) {
+	std::string error;
+	try {
+		DBRegistry::Shutdown();
+	} catch (const std::exception& exception) {
+		error = exception.what();
+	} catch (...) {
+		error = "Unknown native database shutdown failure";
+	}
+	// Release global listener threadsafe functions on every path, including a
+	// failed shutdown -- otherwise they outlive this N-API environment.
 	GlobalEvents::Shutdown();
-	DBRegistry::Shutdown();
+	if (!error.empty()) {
+		::napi_throw_error(env, nullptr, error.c_str());
+		return nullptr;
+	}
 	napi_value result;
 	NAPI_STATUS_THROWS(::napi_get_undefined(env, &result));
 	return result;
@@ -154,6 +167,7 @@ napi_value TransactionLogMapCount(napi_env env, napi_callback_info info) {
 static std::atomic<int32_t> moduleRefCount{0};
 
 NAPI_MODULE_INIT() {
+	initializeTestSeams();
 #ifdef DEBUG
 	// disable buffering for stderr to ensure messages are written immediately
 	::setvbuf(stderr, nullptr, _IONBF, 0);
@@ -216,9 +230,18 @@ NAPI_MODULE_INIT() {
 		int32_t newRefCount = --moduleRefCount;
 		if (newRefCount == 0) {
 			DEBUG_LOG("Binding::Init Cleaning up last instance, shutting down all databases\n");
-			rocksdb_js::GlobalEvents::Shutdown();
-			rocksdb_js::TransactionLogStoreRegistry::Shutdown();
-			rocksdb_js::DBRegistry::Shutdown();
+			auto cleanup = [](const char* name, auto shutdown) {
+				try {
+					shutdown();
+				} catch (const std::exception& error) {
+					::fprintf(stderr, "rocksdb-js %s cleanup failed: %s\n", name, error.what());
+				} catch (...) {
+					::fprintf(stderr, "rocksdb-js %s cleanup failed: unknown native error\n", name);
+				}
+			};
+			cleanup("database registry", []() { rocksdb_js::DBRegistry::Shutdown(); });
+			cleanup("transaction logs", []() { rocksdb_js::TransactionLogStoreRegistry::Shutdown(); });
+			cleanup("global events", []() { rocksdb_js::GlobalEvents::Shutdown(); });
 			DEBUG_LOG("Binding::Init env cleanup done\n");
 		} else if (newRefCount < 0) {
 			DEBUG_LOG("Binding::Init WARNING: Module ref count went negative!\n");