Fix concurrent auto-increment id-range allocation on RocksDB overwriting a sibling thread's range

[kriszyp]

⊙ Problem

On RocksDB, two worker threads that run out of their auto-increment id range at the same time can both write the shared id_allocation record, and the losing write is silently overwritten. createNewAllocation and updateEnd in resources/Table.ts were meant to be compare-and-replace operations, but on RocksDB they did no compare:

• The version token is always missing. The record is written with a bare putSync, without Harper's record metadata, so on RocksDB getEntry(Symbol.for('id_allocation')) returns no version. The check (stored?.version ?? null) == expectedVersion therefore compared null with null and passed for every caller, even with no concurrency.
• The read and the write were outside the transaction. rocksdb-js transactionSync(cb) passes the transaction to the callback, and an operation joins it only when given { transaction: txn }. Neither the getEntry nor the put passed it, so both ran directly against the database and the transaction committed empty.
• The extension's conditional write was unconditional. updateEnd called put(key, value, Date.now(), version) with LMDB's positional ifVersion; on RocksDB put is putSync(key, value, options), so the guard was dropped.

The effect: a thread's id limit (maxSafeId, which is per thread) and the shared counter could name different ranges. That thread could then hand out ids past the range it had checked for existing keys, and an automatic create overwrites a record that already has that id. LMDB is not affected: its write transactions serialize the read and the write, and its put honors ifVersion.

❓ Your call: Is the fix warranted at this size? I think so: the check was a no-op on the default engine, and the failure mode is an overwritten record. The alternative is to wait for #2170, which would add a version to this record. That fixes only the first bullet; the read and write would still be outside the transaction.

💡 Solution

The two allocation paths now share one compare-and-replace helper, replaceIdAllocation:

• Compare by value. The comparison token is the allocation value itself (start, end, nodeName, pid) rather than entry.version. A matching value means the same reservation, so there is no ABA risk (a different write leaving an identical-looking value).
• Read and write inside the transaction. Both run inside transactionSync, with { transaction } on RocksDB and retryOnBusy. When a sibling thread commits between the read and the commit, the commit hits an optimistic-concurrency conflict. The retry then reads the sibling's allocation and leaves it in place.
• The loser follows the stored range. A thread whose extension loses now bounds its ids by the stored range instead of the range it proposed.
• LMDB writes are unchanged on disk. The callback receives no transaction on LMDB, so the helper writes Date.now() as the version, as before.

⚖️ Alternatives

• Add a version to the RocksDB record and keep the version compare. Rejected for now: Harper's RocksDB metadata comes from the record encoder's next-encoding state, which a bare write does not set. #2170 may add it; the value compare stays correct either way.
• Keep the LMDB versioned put and branch by engine. Rejected for one code path.

❓ Your call: On LMDB, the background (setImmediate) range extension used to be an async conditional put. It is now a synchronous write transaction: one per range extension, which is every 0x3ff ids for Int keys and every 0x3fffff ids for Long. That is the only LMDB behavior change. The alternative is engine-specific code that keeps the versioned put on LMDB, and switching to it later is a small change.

❓ Your call: Out of scope here: the shared counter is still moved by an unconditional Atomics.store after the winner commits. For the few statements between that commit and the store, a losing thread's limit already names the new range while the counter is still in the old one. The same window exists on LMDB today, and this PR makes it reachable on RocksDB, because a losing thread now exists there. A correct fix needs the limit shared across threads; a compare-and-exchange on the counter alone can double-move it. This is tracked separately in Auto-increment ids: a thread losing a concurrent range re-allocation can issue an existing id before the winner moves the shared counter (HarperFast/harper#3022).

🔧 Changes

• resources/Table.ts
  • Module-level ID_ALLOCATION_KEY, the hoisted RocksDB retryOnBusy options, and isSameIdAllocation, the value comparison.
  • At startup, the stored allocation value is passed as the expected allocation.
  • updateEnd's extension goes through the compare-and-replace and adopts the stored end. Before, a thread that lost kept the end it had proposed; after a sibling re-allocated into a lower range, it issued ids past the stored end without scanning for existing keys.
  • The re-allocation path passes the stored allocation it read.
  • createNewAllocation returns its own allocation, or the stored one marked alreadyUpdated.
  • replaceIdAllocation:
    • Reads and writes with { transaction }, so a sibling's commit in between is an optimistic-concurrency conflict.
    • Writes when no allocation is stored, for example after a table clear.
    • Throws if the transaction was aborted. Before, an aborted transaction left maxSafeId undefined, so that thread stopped checking for existing ids altogether.
• resources/DESIGN.md design note, with an index line in DESIGN.md. The note records why the value is the token, why the transaction option and retryOnBusy are required on RocksDB, and the residual counter-reset window.
• Rebased onto main (one textual conflict in the constants block above getNewId, resolved by keeping both sides' additions). Follow-up commits: hoisted replaceIdAllocation out of getNewId()'s body — a nested function declaration there is instantiated on every call (even the String/ID early return), so every insert on every table paid for allocating its closure; now it's created once per table. Trimmed two comments that only narrated what the code/DESIGN.md already say.

❓ Your call: The independent review also surfaced two narrow failure-path gaps in replaceIdAllocation/updateEnd that aren't covered by the existing notes above or by #3022: (1) updateEnd raises idIncrementer.maxSafeId to the proposed value before the claim commits; a thrown/aborted claim (resources/Table.ts:1672) or a failed LMDB outer commit leaves that uncommitted bound in place until the next expansion cycle or a restart's idAfter rescan. (2) On LMDB with multiple processes sharing one table, a losing extension adopts whatever end is on the sibling's record without checking its pid/nodeName, even though this process's shared counter doesn't serve that range. (3) primaryStore.put(...) (resources/Table.ts:1669) is RocksDB's async put(key, value, options) { return this.store.putSync(...) } — putSync itself runs synchronously, but a throw inside it becomes a rejected promise instead of a synchronous exception, and the call here is neither awaited nor caught, so a failed native write could leave replaceIdAllocation returning nextAllocation as if it had committed. I confirmed the wrapper shape in the local rocksdb-js checkout; I did not confirm whether putSync can actually throw mid-transaction in practice, so reach is unverified, likely low. All three are rare (a hard transaction failure; multi-process LMDB; a native write fault), reachable only inside or alongside the existing counter-reset window already tracked by #3022, and self-correct on restart. Worth a line in #3022 or a follow-up issue, or fine to leave as-is — your call.

✅ Verification

End-to-end route: a new unit test drives the production getNewId path against a native RocksDB store. A same-thread hook commits a sibling worker's allocation (and then its counter reset) between this thread's in-transaction read and its commit. No integration test can produce a cross-thread interleaving deterministically.

• unitTests/resources/idAllocation.test.js (RocksDB-only; skipped on LMDB, where the interleaving cannot occur). The fixture commits the sibling's allocation, then its counter reset, just before this thread's allocation write.

  • New range: re-allocation adopts a sibling's range that was committed mid-allocation, and the next id falls inside that range.
  • Extension: an extension does not overwrite a sibling's extension.
  • Lost extension: an extension that loses to a lower sibling range still extends when ids approach that range's end.

• Fails on base 33bea3057: all 3 fail, with the sibling's allocation overwritten.

• Mutations:

  Mutation	Result
  Drop { transaction }	2 fail (sibling overwritten)
  Drop retryOnBusy	2 fail (Transaction commit failed: Resource busy)
  Drop the adoption of the stored end	the lost-extension test fails

• Existing coverage: unitTests/resources/create.test.js, including the cross-thread increment test, passes on RocksDB and on LMDB.

• Gates:

  • test:unit:resources on RocksDB: 3950 passing, 3 failing. All 3 are in sourceApplyConflictRetry.test.js (a premise assertion that a memtable flush strands the snapshot) and fail the same way on base 33bea3057 with the same rocksdb-js 2.10.0, so they are a local baseline.
  • test:unit:resources with HARPER_STORAGE_ENGINE=lmdb: 3029 passing, 0 failing.
  • test:unit:main: hung locally after 528 passing in an unrelated process-group (server/threads) suite and was killed at the 30-minute background limit, matching known local suite contention; left to CI
  • test:integration:all: not run locally; left to CI.

• Format and lint: prettier, oxlint and check:design-docs are clean.

_{🤖 Generated by Claude (Anthropic Claude Code, Opus); posted via @kriszyp.}

Related PRs: #2170 overlaps (would version this record; the value compare stays correct), #3020 overlaps (changes RocksDB commit validation; this fix relies on a write-write conflict surfacing as a retryable busy error), #562 independent, #1835 independent, #2155 independent, #2446 independent, #2463 independent, #2575 independent, #2649 independent, #2752 independent, #2763 independent, #2775 independent, #2901 independent, #2906 independent, #2918 independent, #2930 independent, #2939 independent, #2946 independent, #2962 independent, #2981 independent, #2986 independent, #2990 independent, #2994 independent, #3011 independent, #3014 independent, #3015 independent, #3000 independent, #2998 independent, #3029 independent, #3030 independent (touch Table.ts or DESIGN.md, not id allocation)
Complexity: complicated

Framing-Verdict: chosen-approach-sound

Three independent review rounds (this task's two, plus the original authoring session's) found no superior alternative to the value-compared, transaction-joined compare-and-replace. The one accepted tradeoff (the counter-reset window) is deliberately scoped and tracked separately in #3022, the alternatives considered are recorded above, and the narrow failure-path gaps this round found are flagged as a ❓ **Your call:** above rather than folded into this verdict.

🤖 Generated with Claude Code

_{Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-kimi,cursor-muse; rounds=3; full=2 @ 964263b}

_{Review-Attention: study ~20m (critical: Table.ts; decisions: comparison-token, loser-adopts-stored-range, engine-split-call-shape, retry-budget) @ 964263b}

[gemini-code-assist]

Code Review

This pull request introduces a compare-and-replace mechanism (replaceIdAllocation) in resources/Table.ts to claim auto-increment ID ranges atomically across concurrent worker threads, with corresponding design documentation and unit tests added. The review feedback suggests renaming a shadowed variable (storedAllocation) inside the transaction callback to improve readability, and recommends using loose assertions (assert.deepEqual) instead of strict ones in the test suite where strict semantics are unnecessary.

[gemini-code-assist]

The variable storedAllocation is declared in the outer scope of replaceIdAllocation (line 2662) and shadowed in the inner transaction callback scope (line 2665). Renaming the inner variable to currentAllocation avoids shadowing and improves code readability.

				const storedAllocation = primaryStore.transactionSync(
					(transaction) => {
						const options = transaction && { transaction };
						const currentAllocation = primaryStore.getEntry(ID_ALLOCATION_KEY, options)?.value;
						if (currentAllocation && !isSameIdAllocation(currentAllocation, expectedAllocation)) return currentAllocation;
						primaryStore.put(ID_ALLOCATION_KEY, nextAllocation, options ?? Date.now());
						return nextAllocation;
					},

[gemini-code-assist]

In accordance with the repository's general rules, plain assert or loose assertions like assert.deepEqual should be used as the default style for tests, reserving strict assertions (assert.deepStrictEqual) only for cases that genuinely require strict semantics.

Since we are comparing plain objects with simple numeric and string properties here, strict semantics are not required. Please use assert.deepEqual instead.

Affected locations:

• Line 80
• Line 97
• Line 117

Suggested change

	assert.deepStrictEqual(readAllocation(store), sibling);
	assert.deepEqual(readAllocation(store), sibling);

References

1. Use plain assert (or loose assertions like assert.equal and assert.deepEqual) as the default style for tests, reserving strict assertions (assert.strictEqual / assert.deepStrictEqual) only for cases that genuinely require strict semantics (e.g., where type coercion could mask bugs).