fix(table): make TTL eviction path errors non-fatal on both storage engines (#1287)

resources/Table.ts

@@ -1667,25 +1667,50 @@ export function makeTable(options) {
 				// evictions never go in the audit log, so we can not record a deletion entry for the eviction
 				// as there is no corresponding audit entry and it would never get cleaned up. So we must simply
 				// removed the entry entirely, but first cleanup indices
+				let lmdbCompletion: MaybePromise<unknown>;
 				if (primaryStore.ifVersion) {
-					// lmdb
-					primaryStore.ifVersion?.(id, existingVersion, () => {
+					// lmdb: the index cleanup and the record removal are both version-guarded optimistic writes.
+					// Capture both promises so a real write failure on either resolves through evict()'s catch
+					// below rather than escaping as an unhandled rejection from the fire-and-forget callers.
+					const indexCleanup = primaryStore.ifVersion(id, existingVersion, () => {
 						updateIndices(id, existingRecord, null);
 					});
-					removeEntry(primaryStore, entry ?? primaryStore.getEntry(id), existingVersion);
+					const removal = removeEntry(primaryStore, entry ?? primaryStore.getEntry(id), existingVersion);
+					lmdbCompletion = Promise.all([indexCleanup, removal]);
 				} else {
 					updateIndices(id, existingRecord, null, options);
 					removeEntry(primaryStore, entry ?? primaryStore.getEntry(id), options);
 				}
 				committed = true;
+				// Eviction is best-effort cleanup, run fire-and-forget from the record-expiration sweep and the
+				// read path as well as the concurrency-limited cleanup scan. A concurrent write to the same record
+				// makes the commit conflict — that is expected, not a failure: lazy-expiry-on-read keeps queries
+				// correct and the active writer resets the record's expiry/version. So evict() must (a) always
+				// return a thenable (the cleanup scan awaits it for backpressure) and (b) never reject, so a
+				// conflict can't escape as an unhandledRejection from the fire-and-forget callers.
 				if (primaryStore.ifVersion) {
-					// LMDB: committing the wrapper calls doneReadTxn(), removing it from trackedTxns
-					return (lmdbTransaction as any).commit();
+					// LMDB: committing the wrapper calls doneReadTxn(), removing it from trackedTxns. It has no
+					// tracked writes (the writes went straight to the store via optimistic ifVersion), so it returns
+					// a plain resolution object rather than a promise — return the store's write promises instead, so
+					// the caller gets a real thenable that resolves once the removal is durable.
+					(lmdbTransaction as any).commit();
+					return Promise.resolve(lmdbCompletion).catch((error) => {
+						logger.warn?.('Error evicting record', id, error);
+					});
 				}
-				// RocksDB: eviction writes went directly into the raw transaction via options;
-				// commit it directly, as DatabaseTransaction.commit() would abort it (no tracked writes).
-				// Wrap in Promise.resolve so callers can rely on a thenable return regardless of engine.
-				return (transaction as any).commit();
+				// RocksDB: eviction writes went directly into the raw transaction via options; commit it directly,
+				// as DatabaseTransaction.commit() would abort it (no tracked writes). The raw commit bypasses
+				// DatabaseTransaction's ERR_BUSY retry, so a concurrent-write conflict rejects here — swallow it
+				// (abandon the eviction) and log anything unexpected, rather than letting it crash the process.
+				return (transaction as any).commit().catch((error) => {
+					// The commit failed, so the read-snapshot/transaction handle is still open — release it, as the
+					// batched-eviction path does on its own commit failures. committed===true skips the finally abort.
+					try {
+						(transaction as any).abort();
+					} catch {}
+					if (error?.code === 'ERR_BUSY') logger.trace?.('Abandoned eviction of busy record', id);
+					else logger.warn?.('Error evicting record', id, error);
+				});
 			} finally {
 				if (!committed) {
 					// Skip path or thrown error: abort instead of committing so we don't apply

unitTests/resources/evictionBusy.test.js

@@ -0,0 +1,55 @@
+require('../testUtils');
+const assert = require('assert');
+const { setupTestDBPath } = require('../testUtils');
+const { table } = require('#src/resources/databases');
+const { setMainIsWorker } = require('#js/server/threads/manageThreads');
+
+// Regression for #1287 (RocksDB leg). The cleanup scan's batcher retries ERR_BUSY, but the two
+// fire-and-forget evict() call sites (the read-path expiry check and runRecordExpirationEviction on
+// worker 0) commit the raw transaction directly, bypassing DatabaseTransaction's ERR_BUSY retry. A
+// concurrent write to the same record makes that raw commit reject with TransactionIsBusyError, which
+// escaped as an unhandledRejection on [main/0] and could exit the process. evict() must swallow the
+// conflict (eviction is best-effort; lazy-expiry keeps reads correct) so it can never reject.
+describe('evict() swallows a commit conflict instead of rejecting (#1287)', () => {
+	if (process.env.HARPER_STORAGE_ENGINE === 'lmdb') return;
+	let BusyTable;
+
+	before(async function () {
+		setupTestDBPath();
+		setMainIsWorker(true);
+		BusyTable = table({
+			table: 'EvictBusyTable',
+			database: 'test',
+			attributes: [{ name: 'id', isPrimaryKey: true }],
+		});
+	});
+
+	it('returns a thenable that resolves when the eviction commit hits ERR_BUSY', async function () {
+		const { Transaction } = require('@harperfast/rocksdb-js');
+		const originalCommit = Transaction.prototype.commit;
+		await BusyTable.put('b1', { id: 'b1' });
+		const entry = BusyTable.primaryStore.getEntry('b1');
+
+		let injected = false;
+		// Arm a single ERR_BUSY into the next commit — the eviction commit issued synchronously by evict().
+		Transaction.prototype.commit = async function (...args) {
+			if (!injected) {
+				injected = true;
+				const error = new Error('Resource busy');
+				error.code = 'ERR_BUSY';
+				throw error;
+			}
+			return originalCommit.apply(this, args);
+		};
+
+		try {
+			const resolution = BusyTable.evict('b1', entry.value, entry.version);
+			assert.equal(typeof resolution?.then, 'function', 'evict() must return a thenable');
+			// Must resolve, not reject: a rejection here is the unhandledRejection that crashed [main/0].
+			await resolution;
+			assert.ok(injected, 'the injected ERR_BUSY conflict should have fired on the eviction commit');
+		} finally {
+			Transaction.prototype.commit = originalCommit;
+		}
+	});
+});

unitTests/resources/evictionLmdb.test.js

@@ -0,0 +1,53 @@
+require('../testUtils');
+const assert = require('assert');
+const { setupTestDBPath } = require('../testUtils');
+const { table } = require('#src/resources/databases');
+const { setMainIsWorker } = require('#js/server/threads/manageThreads');
+
+// Regression for #1287 (LMDB leg). The cleanup scan keeps the per-record TableResource.evict() path
+// for LMDB (RocksDB coalesces into a batcher instead). evict()'s LMDB branch returned
+// DatabaseTransaction.commit()'s plain resolution object — the optimistic remove goes straight to the
+// store, so the wrapper transaction has no tracked writes and commit() returns `{ txnTime }`, not a
+// promise. The scan then did `resolution.catch(...)` and threw `TypeError: resolution.catch is not a
+// function` every cycle, degrading eviction to roughly one record per scan and growing the store.
+//
+// evict() must return a thenable that resolves once the removal is durable (RocksDB has its own
+// coverage in evictionBatch.test.js, so this leg is LMDB-only).
+describe('evict() returns a thenable on the LMDB path (#1287)', () => {
+	if (process.env.HARPER_STORAGE_ENGINE !== 'lmdb') return;
+	let EvictTable;
+
+	before(async function () {
+		setupTestDBPath();
+		setMainIsWorker(true);
+		EvictTable = table({
+			table: 'EvictLmdbThenableTable',
+			database: 'test',
+			attributes: [
+				{ name: 'id', isPrimaryKey: true },
+				{ name: 'name', indexed: true },
+			],
+		});
+	});
+
+	it('evict() resolves a thenable and physically removes the record + its index entry', async function () {
+		await EvictTable.put('a1', { id: 'a1', name: 'group 7' });
+		const entry = EvictTable.primaryStore.getEntry('a1');
+		assert.ok(entry, 'record should be resident before eviction');
+		assert.equal(EvictTable.indices['name'].getValuesCount('group 7'), 1, 'index should be populated');
+
+		const resolution = EvictTable.evict('a1', entry.value, entry.version);
+		// The core regression: the cleanup scan does `resolution.catch(...)`, so a non-thenable return
+		// throws and aborts the scan. evict() must hand back a real promise.
+		assert.equal(typeof resolution?.then, 'function', 'evict() must return a thenable');
+		assert.equal(typeof resolution?.catch, 'function', 'evict() thenable must expose catch()');
+		await resolution;
+
+		assert.equal(EvictTable.primaryStore.getEntry('a1'), undefined, 'record should be physically evicted');
+		assert.equal(
+			EvictTable.indices['name'].getValuesCount('group 7'),
+			0,
+			'index entry must be removed with the record'
+		);
+	});
+});