Implement the shared derived-index transaction-log runtime - #2533
kylebernhardy wants to merge 5 commits into
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 implements Stage 1 of the derived-index runtime, establishing committed log delivery and exact replay foundation. It introduces derivedIndexRuntime.ts and derivedIndexRegistry.ts to coordinate lock-elected, cursor-based delivery of committed RocksDB log mutations to derived-index backends. Additionally, it updates RocksTransactionLogStore.ts to support exact-start resume, tracks failed logs, and includes log names on returned audit records, while Table.ts is updated to stage local-only eviction markers. A critical bug was identified in RocksTransactionLogStore.ts where expectedExactStarts.splice(i--, 1) uses a post-decrement operator i-- inside a loop that already decrements i, which will cause an index to be skipped during log removal.
| logs.splice(i, 1); | ||
| iterators.splice(i--, 1); | ||
| iterators.splice(i, 1); | ||
| expectedExactStarts.splice(i--, 1); |
There was a problem hiding this comment.
There appears to be a bug in this loop. The post-decrement operator i-- in splice(i--, 1) will cause the loop to skip an element after a removal. Since you are iterating backwards, the loop's own i-- is sufficient to correctly move to the next element. The extra decrement will cause an index to be skipped.
You've correctly fixed this for the iterators array on the previous line, and the same fix should be applied here.
| expectedExactStarts.splice(i--, 1); | |
| expectedExactStarts.splice(i, 1); |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| | { state: 'idle' | 'running' | 'deferred' | 'waiting-durable' | 'stopped'; ownerEpoch?: bigint } | ||
| | { state: 'needs-rebuild'; reason: string; ownerEpoch?: bigint }; | ||
|
|
||
| const ELIGIBLE_ACTIONS = new Set(['put', 'patch', 'delete', 'invalidate', 'relocate', 'evict']); |
There was a problem hiding this comment.
Blocker: two of six ELIGIBLE_ACTIONS are untested
ELIGIBLE_ACTIONS gates which audit entries turn into derived-index mutations. It includes 'invalidate' and 'relocate' alongside 'put' / 'patch' / 'delete' / 'evict', but none of the new or modified test files exercise either of those two:
unitTests/resources/derivedIndexRuntime.test.jsonly ever builds entries via aput-defaultedaudit()helper.unitTests/resources/derivedIndexRuntimeRocks.test.jsexercises put/patch/delete/evict end-to-end, but never invalidate/relocate.unitTests/resources/transactionLogRangeMetadata.test.jsdoesn't touch this set at all.
Both strings are live production audit-action types — Table.ts writes 'invalidate' on cache/data-loader invalidation and 'relocate' on record migration (matching EVENT_TYPES bytes INVALIDATE=4 / RELOCATE=6 in auditStore.ts). Since this is a plain string-literal allowlist iterated by value, a typo or an accidental drop of either entry would silently stop a derived index from ever seeing invalidate/relocate writes, with nothing in the suite catching the regression.
Suggested fix: add a case (e.g. in derivedIndexRuntime.test.js, using the existing fake-log harness) that emits an 'invalidate' and a 'relocate' audit entry and asserts the runtime still delivers a mutation for each, the same way put/delete/evict are already covered.
| logs.splice(i, 1); | ||
| iterators.splice(i--, 1); | ||
| iterators.splice(i, 1); | ||
| expectedExactStarts.splice(i--, 1); |
There was a problem hiding this comment.
Correction of the review comment above (gemini-code-assist): this is not a bug, and the suggested patch would introduce one.
The enclosing loop (line 423) iterates forward — for (let i = 0; i < logs.length; i++) — not backwards. When a forward loop removes the current element via splice(i, 1), the next surviving element shifts down into index i, so the loop's own i++ would skip over it unless compensated. expectedExactStarts.splice(i--, 1) splices at the original i (post-decrement returns the pre-decrement value) and then decrements, cancelling the next i++. That's the standard fix for "remove while iterating forward," applied correctly here.
This is confirmable from the diff itself: before this PR the same single decrement lived on iterators.splice(i--, 1), which was then the last splice in the group. This PR added the new expectedExactStarts array and correctly relocated the decrement onto the new last splice call — exactly one decrement per removal is needed, and it still has exactly one. Applying the suggested change (dropping the decrement) would reintroduce the skip: whenever two logs needing removal are adjacent, the second would be missed and left stranded in logs/iterators/expectedExactStarts.
Recommend not applying the suggested change.
|
Reviewed the derived-index runtime, RocksDB log-range changes, eviction-marker plumbing, and all new/modified tests. One blocker found (inline): Also traced through gemini-code-assist's "critical" comment on |
| @@ -0,0 +1,520 @@ | |||
| # Derived-index runtime: committed log delivery and exact replay foundation | |||
There was a problem hiding this comment.
Putting design docs for a PR into the repository goes against our coding conventions. Design information that needs to be persisted for future agents should go in DESIGN.md files (with tight budgeting, more words is extremely costly and degrading for future agent's context), or for the temporary purpose of PR design, it should go in the PR description (never lost, all PRs histories are retained). Most of this is the latter.
Ordinary input could take a file-primary index down for hours. A stored vector whose length differs from the plane's create-time dims, a component finite as a double but not as the f32 the plane stores, one whose square overflows f32 even though it does not (storing invMag 0, which makes every distance involving that node NaN), a decoded BigInt, or a value with no length at all: each reached the crate or Float32Array.from as an error neither the search path nor reconstruction can attribute to the input. A record aborted every rebuild attempt at the same entry; a query was read as plane corruption and unlinked a healthy file, costing a reconstruction the design measures in hours at 16M nodes. Validate the invariant once, in assertPlaneVector, and apply it at both entry points: the committed projection and the search target. The dimension check runs again before the native insert, where a worker that had no plane to compare against finally has one, and before the old node is removed. Rejecting bad input is not enough on its own: only the rebuild scan skipped records the backend cannot index, while replay rethrew them, and rebuild finishes by replaying from the oldest retained entry — so every record the scan skipped was met again and reconstruction could never finish. Give replay the same skip-and-count guard. Retiring a runtime now fences its in-flight rebuild and replay: neither checked `closed` at any await, so on the table() redefine path the retired runtime's final `isIndexing = false` landed after its replacement had set it and this worker served searches from the half-built generation; a retired runtime no longer advances a durable cursor either. A failure in the id-mapping reads that follow a successful traversal also no longer counts as a plane failure, so a transient RocksDB read or a store closed under an in-flight search cannot unlink the file. hnsw-native-plane.md §13 records an integrated ingest benchmark and the convergence plan it argues for: adopt #2533's runtime, add the coalesced delivery view, bounded collection and delivery, an explicit durability cadence, a rebuild phase, generation fencing and shared readiness, and reduce this PR to an HNSW backend. §13.6 retires the rebuild-anchor question the first draft deferred to the storage layer: a committed read is bounded at one physical offset and walks every byte up to it, so nothing sits behind the tail it yields and the tail is already a safe anchor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VhMkk4iZBE8zEQcnvu1syc
Ordinary input could take a file-primary index down for hours. A stored vector whose length differs from the plane's create-time dims, a component finite as a double but not as the f32 the plane stores, one whose square overflows f32 even though it does not (storing invMag 0, which makes every distance involving that node NaN), a decoded BigInt, or a value with no length at all: each reached the crate or Float32Array.from as an error neither the search path nor reconstruction can attribute to the input. A record aborted every rebuild attempt at the same entry; a query was read as plane corruption and unlinked a healthy file, costing a reconstruction the design measures in hours at 16M nodes. Validate the invariant once, in assertPlaneVector, and apply it at both entry points: the committed projection and the search target. The dimension check runs again before the native insert, where a worker that had no plane to compare against finally has one, and before the old node is removed. Rejecting bad input is not enough on its own: only the rebuild scan skipped records the backend cannot index, while replay rethrew them, and rebuild finishes by replaying from the oldest retained entry — so every record the scan skipped was met again and reconstruction could never finish. Give replay the same skip-and-count guard. Retiring a runtime now fences its in-flight rebuild and replay: neither checked `closed` at any await, so on the table() redefine path the retired runtime's final `isIndexing = false` landed after its replacement had set it and this worker served searches from the half-built generation; a retired runtime no longer advances a durable cursor either. A failure in the id-mapping reads that follow a successful traversal also no longer counts as a plane failure, so a transient RocksDB read or a store closed under an in-flight search cannot unlink the file. hnsw-native-plane.md §13 records an integrated ingest benchmark and the convergence plan it argues for: adopt #2533's runtime, add the coalesced delivery view, bounded collection and delivery, an explicit durability cadence, a rebuild phase, generation fencing and shared readiness, and reduce this PR to an HNSW backend. §13.6 retires the rebuild-anchor question the first draft deferred to the storage layer: a committed read is bounded at one physical offset and walks every byte up to it, so nothing sits behind the tail it yields and the tail is already a safe anchor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VhMkk4iZBE8zEQcnvu1syc
Ordinary input could take a file-primary index down for hours. A stored vector whose length differs from the plane's create-time dims, a component finite as a double but not as the f32 the plane stores, one whose square overflows f32 even though it does not (storing invMag 0, which makes every distance involving that node NaN), a decoded BigInt, or a value with no length at all: each reached the crate or Float32Array.from as an error neither the search path nor reconstruction can attribute to the input. A record aborted every rebuild attempt at the same entry; a query was read as plane corruption and unlinked a healthy file, costing a reconstruction the design measures in hours at 16M nodes. Validate the invariant once, in assertPlaneVector, and apply it at both entry points: the committed projection and the search target. The dimension check runs again before the native insert, where a worker that had no plane to compare against finally has one, and before the old node is removed. Rejecting bad input is not enough on its own: only the rebuild scan skipped records the backend cannot index, while replay rethrew them, and rebuild finishes by replaying from the oldest retained entry — so every record the scan skipped was met again and reconstruction could never finish. Give replay the same skip-and-count guard. Retiring a runtime now fences its in-flight rebuild and replay: neither checked `closed` at any await, so on the table() redefine path the retired runtime's final `isIndexing = false` landed after its replacement had set it and this worker served searches from the half-built generation; a retired runtime no longer advances a durable cursor either. A failure in the id-mapping reads that follow a successful traversal also no longer counts as a plane failure, so a transient RocksDB read or a store closed under an in-flight search cannot unlink the file. hnsw-native-plane.md §13 records an integrated ingest benchmark and the convergence plan it argues for: adopt #2533's runtime, add the coalesced delivery view, bounded collection and delivery, an explicit durability cadence, a rebuild phase, generation fencing and shared readiness, and reduce this PR to an HNSW backend. §13.6 retires the rebuild-anchor question the first draft deferred to the storage layer: a committed read is bounded at one physical offset and walks every byte up to it, so nothing sits behind the tail it yields and the tail is already a safe anchor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VhMkk4iZBE8zEQcnvu1syc
|
@kylebernhardy I am trying to optimize this runtime to be safe, efficient, and maximize simplicity with #2548. However, #2430 exists as the only consumer of the runtime to verify its functionality. Without any Tantivy consumer, the simplification work in 2548 is likely to make changes to this PR that will significantly impact (or break) Tantivy use of this. It seems like you would probably want to have a real consumer drive the contract of this PR rather than just self-assertion of the PR itself? |
|
Superseded by #2548, which carries this branch's commits and retargets the whole derived-index stack at |
2 participants
Outcome
Adds Harper’s shared post-commit delivery and exact-replay foundation for non-transactional derived indexes. Each backend has an independent lock-elected runner, cursor vector, bounded backlog, and failure state; record commits never await derived-index work.
What changed
DerivedIndexRuntimedelivery, cursor, ownership, and backpressure contractCloses #2489
Validation
npm run buildnpm run lint:requiredAudit cleanup retirement, reproduced on unmodifiedmainComment generated by kAIle (GPT-5)