Fix RocksDB @expiresAt eviction sweeps - #2155
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the record expiration sweep by introducing RocksDB index value enumeration and batching the eviction process in groups of 100 records. It also adds a fallback mechanism to clean up dangling index entries when a record is already gone. The review feedback correctly identifies a redundant database read in this fallback path when primaryStore.ifVersion is undefined, suggesting a simplification to avoid unnecessary I/O overhead.
|
Found 1 blocker: |
9426905 to
9f7e742
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Patch cherry-pick: cancelledThe |
9f7e742 to
c54d4b6
Compare
|
Patch? Really? |
|
Good catch. I removed the Update: at Kyle’s request, I rewrote the five PR commits so both author and committer use Comment generated by kAIle (GPT-5.6) |
c54d4b6 to
409138c
Compare
cb1kenobi
left a comment
cb1kenobi
left a comment
There was a problem hiding this comment.
About a year ago, I tried to add support for getValues() and we ended up abandoning it in rocksdb-js on June 25, 2025 due to "key sort order" challenges.
I created an IndexStore that extends a Store and would handle the encoding of the keys: https://github.com/HarperFast/rocksdb-js/blob/index-store/src/index-store.ts. I don't remember the specifics, but I remember naively creating an array as the key won't work. If you wanted "hello", it gave you "hello world" too. I tried adding markers around the values, but even those could cause matches. I don't think I tested fixed width keys, but that might yield some success.
We ended up dropping dupSort, getValues(), and getValuesCount() from rocksdb-js.
It's a year later now, the dust has settled, things have stabilized, LLMs have gotten smarter, and maybe this is solvable. I think the next step for this PR is to add way more tests, specifically with similar values.
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
I guess additional concerns I have: I think http://github.com/HarperFast/rocksdb-js/issues/312 is the real solution here. This is would be kind of a temporary solution for... who? I thought the only place we had ever used this was for the EKV project that didn't go through and this received little attention after that. But sure, I guess there is an immediate need for this, we can keep going with this. I think the ticket is marked P1. We haven't been automatically prioritizing P1 tickets into 5.2 patch (while we still have open P0s, or at least P1s with active customer connections).
🤖 Reviewed with Claude
Ok, so you cool with this being milestone 5.3, then? (I assigned that). |
|
Yes, v5.3 timing is acceptable. Engineering Metrics is no longer blocked on this change, and keeping it in v5.3 gives the remaining transaction-lifecycle correction and rollout behavior the right review space. Thanks for moving the milestone. Comment generated by kAIle (Codex GPT-5.6) |
| export function completeSchemaQuiesce(message: SchemaQuiesceMessage): void | Promise<void> { | ||
| recoveringSchemaQuiescences.delete(message.quiesceId); | ||
| const state = schemaQuiescence.get(message.quiesceId); | ||
| if (state && message.phase === 'reconcile-quiesce') return reconcileSchemaQuiesce(state); |
There was a problem hiding this comment.
Medium: a terminal reconcile that throws can never be retried — it permanently answers reconciled: false
Routing reconcile-quiesce into reconcileSchemaQuiesce() here is the right fix, but it changes the retry semantics of the terminal handler. reconcileSchemaQuiesce() sets state.abortRequested = true as its first statement and only clears the state at the very end, so anything that throws in between (resetDatabases(), resetQuiescedDatabase(), state.table.abortDropQuiesce()) leaves the state in the map with abortRequested set. applySchemaChange's catch then calls failSchemaQuiesceFinalization(), which resets finalizing and re-arms the lease but leaves abortRequested alone — and finishSchemaQuiesce() rejects on exactly that:
const state = schemaQuiescence.get(message.quiesceId);
if (!state || state.abortRequested) return false;So every later attempt returns { reconciled: false }. completeSchemaChange()'s three retries can't recover, the local leg throws on the first failure, and dropTable()'s finally rejects with Could not reconciled schema quiesce … on every worker, masking the original drop error while the target stays fail-closed until the 120s lease backstop fires.
This is new in 879f243c: before it, the terminal reconcile never entered reconcileSchemaQuiesce(), so a transient resetDatabases() failure left abortRequested clear and the retry path worked (the behavior does not acknowledge finalization when the strict reset fails covers).
Suggested fix — in failSchemaQuiesceFinalization (line 1637), clear the flag the interrupted reconcile set. It is already false on the finalize path, so this is a no-op there:
| if (state && message.phase === 'reconcile-quiesce') return reconcileSchemaQuiesce(state); | |
| state.finalizing = false; | |
| // A reconcile that threw mid-restore already set abortRequested; leaving it set makes | |
| // finishSchemaQuiesce reject every retry of the very operation we are re-arming for. | |
| state.abortRequested = false; |
(apply at line 1637, not here)
—
Generated by Barber AI
| await assert.rejects(() => abortSchemaQuiesce(message), /commit boundary/); | ||
| const terminal = { ...message, phase: 'reconcile-quiesce' }; | ||
| assert.strictEqual(finishSchemaQuiesce(terminal), true); | ||
| await completeSchemaQuiesce(terminal); |
There was a problem hiding this comment.
Nit: one more completeSchemaQuiesce() call site needs the await this line just gained — line 516
completeSchemaQuiesce() now returns a promise for the reconcile-quiesce phase, and this test correctly awaits it. The teardown in retains the origin unavailable fence when local terminal state is unresolved does not:
// line 516
if (terminalMessage) completeSchemaQuiesce({ ...terminalMessage, phase: 'reconcile-quiesce' });That is a drop_schema state that is still present, so the reconcile branch is taken and its resetQuiescedDatabase() → resetDatabases() runs unawaited into whichever test starts next. Given how much stabilization this suite has already needed, worth closing. The enclosing function is async, so await completeSchemaQuiesce(...) is a one-word fix.
—
Generated by Barber AI
| const state = schemaQuiescence.get(message.quiesceId); | ||
| if (!state) return; | ||
| state.finalizing = false; | ||
| armSchemaQuiesceLease(state); |
There was a problem hiding this comment.
What: failSchemaQuiesceFinalization clears state.finalizing and re-arms the lease, but never clears state.abortRequested. reconcileSchemaQuiesce (line 1467) sets abortRequested = true as its first statement, before any of resetDatabases(), resetQuiescedDatabase(), or state.table.abortDropQuiesce() run. If any of those throw, completeSchemaQuiesce's reconcile-quiesce branch (line 1638, return reconcileSchemaQuiesce(state)) rejects, and whatever caller routes that failure into failSchemaQuiesceFinalization leaves abortRequested permanently set.
Why it matters: finishSchemaQuiesce (line 1622) unconditionally rejects when state.abortRequested is true: if (!state || state.abortRequested) return false;. Every subsequent retry of the terminal handler for this quiesce now returns false forever — the operation can never complete locally again until the 120s lease backstop fires. This is a regression introduced by this PR (879f243c): before routing the terminal reconcile phase through reconcileSchemaQuiesce, a transient failure here left abortRequested clear and retries could still succeed.
Suggested fix: clear the flag the interrupted reconcile set, since it's already false on the normal finalize path (replacing this line):
| armSchemaQuiesceLease(state); | |
| state.abortRequested = false; | |
| armSchemaQuiesceLease(state); |
| ); | ||
| } finally { | ||
| signalling.finalizeSchemaChange = originalFinalize; | ||
| if (terminalMessage) completeSchemaQuiesce({ ...terminalMessage, phase: 'reconcile-quiesce' }); |
There was a problem hiding this comment.
Suggestion (non-blocking): completeSchemaQuiesce() now returns a promise for the reconcile-quiesce phase (other tests in this file already await it, e.g. line 422). This finally block doesn't, so resetQuiescedDatabase()/resetDatabases() can still be running when the next test starts. The enclosing function is async, so await completeSchemaQuiesce(...) closes the gap.
| if (terminalMessage) completeSchemaQuiesce({ ...terminalMessage, phase: 'reconcile-quiesce' }); | |
| if (terminalMessage) await completeSchemaQuiesce({ ...terminalMessage, phase: 'reconcile-quiesce' }); |
| if (state.committed) { | ||
| // A synchronous RocksDB drop can outlive the timer that its blocked origin would renew. | ||
| // A connected owner may still be destructing storage, so peers must remain fail-closed. | ||
| if (!state.localOwner && manageThreads.isThreadConnected(state.message.originator)) { |
There was a problem hiding this comment.
Medium: this fence bypasses the committed-recovery attempt cap, so a wedged peer never recovers
The guard returns before recoverCommittedSchemaQuiesce() runs, so it never consumes a recoveryAttempts slot — MAX_COMMITTED_QUIESCE_RECOVERY_ATTEMPTS no longer bounds anything here, and armSchemaQuiesceLease() re-arms every 120s for as long as the origin thread exists. Harper worker threads are long-lived, so "the origin thread is alive" is not a bound on "the drop is still running".
The concrete consequence is that it makes the still-open finding at line 1638 permanent instead of self-healing. reconcileSchemaQuiesce() sets state.abortRequested as its first statement, so once the terminal reconcile throws once, finishSchemaQuiesce() rejects every retry and completeSchemaQuiesce() never clears the state. Before this commit the 120s lease was the escape hatch; now there isn't one.
Executed against this head (peer drop_table, injected throw in the terminal reconcile's restore work, then repeated lease expiry):
originator= a connected thread id — still fail-closed after 5 expiries, table still missing fromdatabases[schema].originator= a disconnected id (control) — recovers on the first expiry.
Suggested fix: bound the fence so it tracks the operation rather than the thread's lifetime — consume a recovery attempt (or a separate max-hold counter) on each origin-live re-arm, and fall through to recoverCommittedSchemaQuiesce() once it is exhausted.
—
Generated by Barber AI
| function expireSchemaWorkerBarrierLease(message) { | ||
| // The origin can legitimately block its event loop during synchronous RocksDB destruction. | ||
| // Keep replacement workers fenced until that owner disconnects or sends a terminal message. | ||
| if (isThreadConnected(message.originator)) { |
There was a problem hiding this comment.
Low: this recheck loop has no cap, so an unreleased barrier can block worker replacement for the life of the process
expireSchemaWorkerBarrierLease() re-arms a 1s timer indefinitely while the origin thread is connected. holdWorkerStartsForSchema() gates startAfterSchemaWorkerBarrier(), which is how a crashed worker's replacement is started (server/threads/manageThreads.js:670) — so an origin that stays alive but never sends release-worker-starts (stuck in a synchronous drop, or a releaseSchemaWorkerBarrier() broadcast that never lands; both failures are warn-only in utility/signalling.ts) permanently prevents worker replacement. The previous unconditional expiry bounded that at leaseUntil.
Suggested fix: cap the rechecks the way MAX_COMMITTED_QUIESCE_RECOVERY_ATTEMPTS caps the quiesce lease, and log loudly when the cap is hit, so a lost release degrades to a bounded window instead of a permanent one.
—
Generated by Barber AI
| } | ||
|
|
||
| function isThreadConnected(ownerThreadId) { | ||
| if (ownerThreadId === threadId || ownerThreadId === 0) return true; |
There was a problem hiding this comment.
Low: a second thread-liveness predicate with weaker semantics than the isThreadRunning() already in this file
isThreadRunning() (line 1188) has the identical first line and is the authoritative check: it queries the main thread for thread info and awaits any in-flight process-group termination before reporting an owner gone — an invariant that unitTests/server/threads/fixtures/processGroupReclaimHarness.js explicitly pins. isThreadConnected() instead reads the local connectedPorts array, which lags REMOVE_PORT and knows nothing about process-group teardown, so the two now answer the same question differently depending on which one a caller reaches for.
Separately, originator is optional on SchemaQuiesceMessage, and an undefined owner id falls through to connectedPorts.some(...) and returns false — releasing the fence. That is the opposite of the fail-closed default the new comment states, and the release path republishes the target while the origin may still be destructing storage.
Suggested fix: reuse isThreadRunning() if the callers can afford its 1s query; otherwise document why the local view is deliberate here, and make an unknown owner id hold the fence rather than release it.
—
Generated by Barber AI
| leaseUntil: Date.now(), | ||
| }; | ||
| await schema_handler({ type: 'schema', message }); | ||
| await waitForSchemaWorkerStarts(); |
There was a problem hiding this comment.
Nit: this test hangs instead of failing when the release regresses
.mocharc.json sets timeout: 0, so a regression that keeps the barrier held makes this await block forever and the whole unit-test job time out rather than reporting a failed assertion. Verified: with isThreadConnected() forced to return true, this test hung and only reported a failure once run with an explicit --timeout.
Suggested fix: race the wait against a short deadline and assert on the winner, e.g. assert.strictEqual(await Promise.race([waitForSchemaWorkerStarts().then(() => 'released'), new Promise((r) => setTimeout(() => r('held'), 2000))]), 'released');
—
Generated by Barber AI
3 participants
Summary
@expiresAtsweeps workcontext.expiresAt(or table fallback), never from a returned@expiresAtfieldFixes #1481. The broader native index-store work remains tracked in HarperFast/rocksdb-js#312.
Rollout behavior
This intentionally activates physical reclamation for expired RocksDB rows that reads already treat as absent. The prior Rocks sweep was effectively inert, so an upgraded node may have an existing backlog. The sweep drains that backlog without an audit or replication event, matching eviction semantics; it yields every 10 candidates and limits transaction commits to four in flight, but it does not impose the previously reviewed 10k/min ceiling. This behavior change needs explicit release-note and human rollout sign-off.
Verification
npm run buildnpm run typechecknpm run test:typesnpm run lint:requirednpm run format:checkComment generated by kAIle (GPT-5.6)