Reproduce and fix hnsw race #386

[kriszyp]

Reproduction and fix for #386
Depends on HarperFast/rocksdb-js#531
This is marked as ready for review, although it is not ready to merge.

[claude]

Missing worker file — test will always fail.

vectorIndex-thread.js is referenced here but does not exist in the PR (or anywhere in the repo). new Worker with a nonexistent path causes each worker to emit an error event; the test collects those via the worker.once('error', ...) handler and the final assert.deepEqual(errors, []) will always fail.

The existing analog for this pattern is unitTests/resources/create-thread.js. A vectorIndex-thread.js counterpart needs to be added — it should:

• Import setupTestDBPath + setMainIsWorker, call both at startup
• Set up the HNSWConcurrentTest table
• Listen on parentPort for { type: 'insert', start, count, dims } messages
• Perform the PUTs and reply with { type: 'done' } (or { type: 'error', ... } on failure)

[claude]

Review: PR #415 — HNSW concurrent PUT / coordinated RocksDB retry

Summary

This PR has two independent parts:

1. DatabaseTransaction.ts / RecordEncoder.ts — switches RocksDB conflict handling from an error-rejection model (ERR_BUSY thrown) to a coordinated-retry model (RETRY_NOW_VALUE resolved). Both new RocksTransaction(…, { coordinatedRetry: true }) call sites are updated, and RecordEncoder.saveStructures gains { retryOnBusy: true } on its sync transaction.
2. vectorIndex.test.js — adds a multi-worker HNSW concurrent PUT test to reproduce issue HNSW concurrent PUT race — 500 'Cannot read properties of undefined (reading 0)' on harperfast/harper:5.0.1 #386.

---

1. Missing vectorIndex-thread.js (blocker)

File: unitTests/resources/vectorIndex.test.js:316

What: The new describe block spawns workers with new Worker(__dirname + '/vectorIndex-thread.js'), but that file is not in this PR or anywhere in the repo.

Why it matters: Each worker emits an error event immediately (module not found). The test catches those errors via worker.once('error', resolve), collects them, and then hits assert.deepEqual(errors, []) — which always fails. The test that's meant to prove issue #386 is fixed is itself structurally broken.

Suggested fix: Add unitTests/resources/vectorIndex-thread.js modeled on the existing create-thread.js. It should call setupTestDBPath() + setMainIsWorker(true) at startup, open the HNSWConcurrentTest table, and on { type: 'insert', start, count, dims } messages, perform the PUTs and reply { type: 'done' } (or { type: 'error', … } on failure).

---

What I traced (no other blockers found)

• Coordinated-retry logic (DatabaseTransaction.ts:265-313): The commitResult === RETRY_NOW_VALUE branch correctly mirrors the old ERR_BUSY rejection path — same exponential back-off, same MAX_RETRIES guard, same transaction reuse. outstandingCommit tracking is safe: .finally() clears the pointer before .then() fires, so the retry call sees a clean slot.
• RecordEncoder.saveStructures: Adding { retryOnBusy: true } to the sync transaction is independent of the async coordinated-retry path and addresses the busy conflict for structure saves. Logic inside the callback is unchanged.
• const RETRY_NOW_VALUE placement (DatabaseTransaction.ts:12): Sits between two import statements. Imports are hoisted so this is semantically correct, and TypeStrip (erasableSyntaxOnly) handles it fine — no erasability concern.
• LMDB guard: Both describe blocks correctly skip when HARPER_STORAGE_ENGINE=lmdb; test infrastructure (setupTestDBPath, setMainIsWorker) calls match the existing pattern in validation.test.js and resource-operation.test.js.

[dawsontoth]

The integration and unit tests seem proper miffed with the changes...

[cb1kenobi]

The integration and unit tests seem proper miffed with the changes...

Yeah, that's because this PR is dependent on changes in rocksdb-js that haven't been merged/published.

[cb1kenobi]

The fix is much simpler than introducing this RETRY_NOW logic: #423.