fix(table): make evict() return a Promise on RocksDB

[ldt1996]

Summary

• Wrap the RocksDB return in Promise.resolve(...) in evict()'s finally so it always returns a thenable, matching the LMDB branch.
• Drop the caller-side (resolution as any).catch band-aid from the cleanup scanner now that the source contract is honored.

Context

39c3a24 ("storage-engine-aware commit in evict()") changed the RocksDB path to return transaction?.commit(), which is synchronous (the LMDB wrapper returned a Promise). The cleanup scanner at Table.ts:4549 chains .catch on the return, which now throws TypeError: resolution.catch is not a function every cycle on RocksDB. The scanner crashes, eviction stops draining records, and peer counts hold at the full count instead of shrinking when expiresAt is set.

Affects v5.0.22 and v5.0.23. main masks the crash with if (resolution && (resolution as any).catch), which silences the TypeError but silently skips the concurrency control and never awaits the eviction's commit, so eviction is still broken on main, just quieter.

This restores the Promise contract at the source so both engines behave the same. The cleanup loop goes back to the simpler if (resolution) { ... resolution.catch(...) }.

closes #1089

[github-actions]

Patch cherry-pick: conflict

Cherry-pick onto v5.0 produced conflicts on commit(s): ceeb570567910dea9c962fb7b7b673b9a1c07581 7c6df5de9e4408e0d5614a7eb40ec378ccce7961 3284fe47e3690963b795689e64d105fe3799d63f 378d55eb54a85979e0428c2ce8c77d7c05cad101 680992b9972da256a0fa701126dc52cbd558707c

The conflict markers are committed on branch cherry-pick/v5.0/pr-890.
A pull request has been opened to land this patch: #1093

[gemini-code-assist]

Code Review

This pull request ensures that the transaction commit in the evict function always returns a Promise by wrapping the RocksDB commit in Promise.resolve, allowing the cleanup loop to safely call .catch on the resolution. However, the reviewer identified a critical issue where returning a value from a finally block can swallow errors and override early returns. A refactoring of the evict function was suggested to return the commit promises from the try block and use the finally block solely for transaction cleanup.

[gemini-code-assist]

Returning a value from a finally block is an unsafe pattern in JavaScript/TypeScript (often flagged by ESLint's no-unsafe-finally rule). It has two major unintended side effects:

1. Swallows Errors: Any exception thrown in the try block (e.g., during removeEntry or updateIndices) will be silently discarded, and the value returned by the finally block will be returned instead. This makes debugging eviction failures extremely difficult.
2. Overrides Early Returns: Any early return; statement in the try block (which is intended to return undefined when eviction is skipped) is overridden by the finally block's return value. As a result, evict() always returns a Promise even when no eviction was performed, which causes the cleanup loop to unnecessarily track and await these skipped operations.

To fix this, we should return the commit promises directly from the try block, and use the finally block only to abort/cleanup the transaction if it wasn't committed.

Here is the suggested refactoring for the evict function:

		static evict(id, existingRecord, existingVersion) {
			let entry;
			const lmdbTransaction = txnForContext({ transaction: new DatabaseTransaction() });
			let transaction = lmdbTransaction.getReadTxn();
			let options = { transaction };
			let committed = false;
			try {
				if (hasSourceGet || audit) {
					if (!existingRecord) return;
					entry = primaryStore.getEntry(id, options);
					if (!entry || !existingRecord) return;
					if (entry.version !== existingVersion) return;
				}
				if (hasSourceGet) {
					// if there is a resolution in-progress, abandon the eviction
					if (primaryStore.hasLock(id, entry.version)) return;
				}
				// 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
				if (primaryStore.ifVersion) {
					// lmdb
					primaryStore.ifVersion?.(id, existingVersion, () => {
						updateIndices(id, existingRecord, null);
					});
					removeEntry(primaryStore, entry ?? primaryStore.getEntry(id), existingVersion);
				} else {
					updateIndices(id, existingRecord, null, options);
					removeEntry(primaryStore, entry ?? primaryStore.getEntry(id), options);
				}

				committed = true;
				if (primaryStore.ifVersion) {
					return (lmdbTransaction as any).commit();
				}
				return Promise.resolve((transaction as any)?.commit?.());
			} finally {
				if (!committed) {
					if (primaryStore.ifVersion) {
						lmdbTransaction.abort();
					} else {
						(transaction as any)?.abort?.();
					}
				}
			}
		}

This ensures that errors are properly propagated, and early returns correctly return undefined.

[ldt1996]

both failure modes are real as far as I can tell, and predate this PR. Probably best for another PR to be opened for it and let this one keep the focus on this specific fix

[kriszyp]

Table.ts line 4549 throws an error trying to call a function? I don't see any call on line 4549: https://github.com/HarperFast/harper/blob/main/resources/Table.ts#L4549
So you are saying that there is an error thrown by evict? It seems like we should move the commit() call out of the finally (should only execute if removeEntry doesn't throw), and possibly (synchronously) try/catch the evict call. The code generally looks like it is designed to handle a MaybePromise , which should be the expected return type of the evict (except for the missing try/catch if are expecting only rejections, no throws, I guess). Is there a GH issue that documents the error this is fixing?

[claude]

Reviewed; no blockers found.

[ldt1996]

@kriszyp You're right on the line number, should have been 4551

I pushed changes, commit moves into the try block with a committed flag, finally only aborts when !committed

No GH issue, came direct fro diagnosis on the stage cluster, happy to file one if you want it for tracking.

Stack:

  TypeError: resolution.catch is not a function
    at <anonymous> (resources/Table.ts:4473:73)
    at runNextTicks (node:internal/process/task_queues:65:5)
    at process.processTimers (node:internal/timers:538:7)

(line 4473 from 5.0.22, claude ran with that, but on main is 4551)

cause: evict() returned the synchronous transaction?.commit() on
rocksDB rather than a Promise

Repro: any 5.0.22 rocksDB node with a TTL evicted table. Cleanup scan
throws every cycle, eviction stops working entirely, confirmed on the
v4 to v5 stage cluster where us-lax-1 held the full 25k record count
instead of draining like it did on 5.0.21

[kriszyp]

Good, I think appropriate fixes are now in place.