Add a full-text derived-index backend - #2569
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a package-independent full-text derived-index backend adapter and integrates it with the shared derived-index runtime, introducing an asynchronous acquire hook to manage owner acquisition before reading durable cursors. The review feedback focuses on aligning with the repository style guide by using loose equality checks (== null) for null-or-undefined checks, and optimizing hot-path performance in functions like fullTextFields, normalizedCursor, and cursorAtOrAfter by replacing array-allocating operations with for...in loops and Object.hasOwn checks.
|
Reviewed; no blockers found. |
3e65a8f to
d292d54
Compare
aba5a41 to
2cb7c4b
Compare
kriszyp
left a comment
There was a problem hiding this comment.
@kylebernhardy Ok, I think I see how I got confused here. This also kind of a response to #2568 (comment), but this architectural commentary, so belongs here.
Basically: I think Tantivy should actually be using its own persistence/storage capabilities, and not trying to push its segments into RocksDB (into a column family). I think I understand your rationale; unifying backup and stuff, but HNSW is already going down the road of owning its own persistence, and I think that is the intent I had for the derived indexes and how they should be interacting with Harper. I think it is perfectly reasonable to say that (external) indexes have to be re-built on database restore. And/or plan to create fancier checkpoint/coordination mechanisms for built-in backups down the road.
WDYT, is that reasonable?
Anyway, here lengthier version in Claudish, if your agent is interested:
Read against the runtime as it stands on #2567 (one backend contract, committed-tail rebuild, plain-word readiness) and the HNSW backend on #2430. The main point is about where the index lives; the rest follows from it.
1. Tantivy should persist the way the HNSW plane does: on its own, next to the index store, with no RocksDB round trip. Nothing in the derived-index runtime needs a backend's state in RocksDB. The contract already makes the cursor backend-owned — getDurableCursor() returns whatever the backend made durable with its own barrier — and the runtime's only RocksDB footprint is the transaction log it reads plus a one-byte condemnation marker. Restore, copy-db and replica seed rebuild derived state from records + log regardless of where it lived; that is what the rebuild phase exists for, so "one storage engine for backup" buys nothing the rebuild does not already provide, and a CF snapshot taken mid-generation is not a consistent index either.
HNSW keeps its graph in a .hnsw mmap file with its own msync barrier and its generation in the file header; only the pk ↔ nodeId mappings and the cursor sit in the index CF, and the mappings are there only because the plane addresses nodes by numeric id. Tantivy needs less of RocksDB than that, not more: the document carries <tableId>.<recordKey>, so there is no mapping to keep, and Tantivy already has an atomic barrier (meta.json rewrite) and a commit payload — which is exactly what this backend's committedPayload is.
The consistent shape, which Tantivy supports natively (MmapDirectory) and the runtime already permits:
MmapDirectoryat<index store path>/<name>.tantivy/; commit =meta.jsonrewrite; the cursor vector is the commit payload, as here. Generation = a directory name or a payload field; rebuild = build a new directory, swap, delete the old.getDurableCursor()reads the payload frommeta.json: a read-only open, no writer, no lock, cheap enough to do at backend construction. That is what #2568's slice doc said the plan was — "the backend is constructed and its asynchronous Fulltext open is awaited beforeregister();getDurableCursor()is valid synchronously from the first owner acquisition. This integration requires no change toderivedIndexRuntime.ts."- Gone with it:
acquire()and the ~190 runtime lines that serve it (#acquireBackend,#acquisitionFailed, the#acquiringgates in wake/drain/startRebuild/requestRebuild, release waiting on acquisition, the generation-preserving#discardProgress), the new readiness reason and attempt budget,RocksDerivedIndexStorage, theKvDirectorytransport and its per-I/O NAPI callbacks, the root-wideflushSyncbarrier stalling unrelated tables, and the derived-index writes that emit rootcommittedevents and wake the runners themselves (reviewer note 5). #2568's benchmark then measures a path we would not ship; the number that matters becomes Tantivy's own commit cost, which is already known.
The trade is the one DESIGN.md § Native HNSW plane already states for HNSW: node-local derived state on disk that backup includes after a barrier or marks rebuild-on-restore. The two backends should make the same statement.
2. cursorAtOrAfter fails the backend closed on a legitimate cursor. deliver() rejects a through whose per-log timestamp is numerically lower than the last accepted one ("cursor moved backward" → DERIVED_INDEX_FAILED → condemnation). Transaction timestamps are unique per log but not monotone in physical order — TransactionLogStore::writeBatch only advances latestTimestamp when the batch's is greater — so a later physical transaction with a lower timestamp is normal, and the runtime's cursor design deliberately never compares timestamps (exact-start resume, repeat detection by set). The runtime already guarantees through is an offered vector delivered in order; the backend should not re-derive order from the numbers. This will condemn a healthy index under ordinary concurrent writes. Independent of point 1.
3. recordKey on every mutation. Fine and cheap (it is the key the collector already computes), and the collection-time filter on non-string keys matches the scan rule.
4. Docs. docs/fulltext-derived-index-backend.md here and docs/fulltext-derived-index-vertical-slice.md on #2568 describe the PR and its stages. Repo docs carry only durable design (the engine/lifecycle interfaces, the document-id scheme, the ordered command state machine) as a section next to DESIGN.md § Derived-index runtime; alternatives, verification and staging belong in the PR body.
5. Base. The diff of #2568's branch against feat/derived-index-native-backend-runtime removes integrationTests/.../choose-operation-authz.test.ts and ~160 lines of serverUtilities.ts that exist on both main and the runtime branch. That looks like a base mismatch rather than intent — worth a rebase and a look before either PR merges.
Two facts from the runtime work this backend depends on: a committed transaction-log read is a contiguous physical prefix (commitFinished() advances lastCommittedPosition to the earliest still-uncommitted write), which is why the committed tail is a safe rebuild anchor; and a log that has never written a file reports oldestSequenceNumber: 0, so "retains its beginning" is fileCount === 0 || oldestSequenceNumber === 1.
— Claude Fable 5.1
2cb7c4b to
20ad537
Compare
Summary
FullTextDerivedIndexBackendthat adapts Harper derived-index batches to an ordered asynchronous Fulltext engine lifecycleThis is the backend/lifecycle slice only. It does not add the unpublished Fulltext package as a dependency or activate schema and query surfaces yet.
Closes #2510 in part; schema activation, native artifact wiring, backup integration, and query exposure remain follow-up units.
For the human reviewer
unavailable; this bounds retry storms and avoids an implicit destructive rebuild, but a worker-local failure can therefore stop healthy contenders until an explicit rebuild request.deliver()never performs native encoding. This protects the write path but means the packed native allocation can exceed the nominal queue-byte estimate.committedevents and can wake derived-index runners. The Rocks test records the exact amplification; the benchmark gate must be evaluated before activation, with any mitigation implemented generically in the transaction-log notification path.Verification
npm run buildgit diff --checkrangeReadActivity.test.jsComplexity: complicated
Comment generated by kAIle (GPT-5)
Review-Coverage: authored=codex; ran=claude; adjudicated=domain; declined=gemini,cursor-grok,cursor-composer; rounds=9 @ aba5a41
Human-Review-Need: 3 (decisions: async-owner-acquisition-hook, shared-acquisition-failure-terminal-state, package-independent-integration-scope, schema-drift-field-omission, replace-after-unopenable-generation) @ aba5a41