Skip to content

Add a full-text derived-index backend - #2569

Draft
kylebernhardy wants to merge 10 commits into
mainfrom
codex/fulltext-derived-backend
Draft

Add a full-text derived-index backend#2569
kylebernhardy wants to merge 10 commits into
mainfrom
codex/fulltext-derived-backend

Conversation

@kylebernhardy

@kylebernhardy kylebernhardy commented Sep 11, 2026

Copy link
Copy Markdown
Member

Summary

  • add a package-independent FullTextDerivedIndexBackend that adapts Harper derived-index batches to an ordered asynchronous Fulltext engine lifecycle
  • add the optional owner-scoped backend acquisition hook and canonical record key needed to reopen a native writer safely across worker ownership changes
  • enforce bounded queueing, publication barriers, durable cursor validation, epoch fencing, recovery, shutdown, and fail-closed generation replacement
  • cover the backend and shared runtime with deterministic lifecycle tests plus a real Harper RocksDB storage route

This 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

  1. Acquisition failures are counted per runner. Eight consecutive failures by one runner publish process-wide 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.
  2. Reset may replace an old generation that cannot be opened. The production lifecycle must atomically select the new cursorless generation before dropping or mutating the old one.
  3. Queue bytes use Harper's source/projection estimate so deliver() never performs native encoding. This protects the write path but means the packed native allocation can exceed the nominal queue-byte estimate.
  4. The package boundary is structural until a versioned native Fulltext artifact is available. Real native callback, writer-lock, and RocksDB-generation behavior still require integration coverage before schema activation.
  5. Derived Fulltext storage writes currently emit root committed events 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 build
  • 128 passing focused tests across the full-text backend, shared derived-index runtime, native-backend runtime, real RocksDB route, and Rocks derived-index storage
  • changed files pass Prettier and git diff --check
  • repository lint reports only the 15 warnings already present on the stacked parent
  • full resource suite previously reached 2,477 passing and 31 pending; its one failure reproduces alone in untouched rangeReadActivity.test.js
  • exact committed code received Claude plus Harper-domain review; the final delta verdict is LGTM

Complexity: 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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread resources/FullTextDerivedIndexBackend.ts Outdated
Comment thread resources/FullTextDerivedIndexBackend.ts
Comment thread resources/FullTextDerivedIndexBackend.ts
Comment thread resources/FullTextDerivedIndexBackend.ts
@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kylebernhardy
kylebernhardy force-pushed the codex/fulltext-derived-index-slice branch from 3e65a8f to d292d54 Compare September 11, 2026 16:36
@kylebernhardy
kylebernhardy force-pushed the codex/fulltext-derived-backend branch from aba5a41 to 2cb7c4b Compare September 11, 2026 17:17
@kylebernhardy
kylebernhardy marked this pull request as draft September 11, 2026 17:37

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  • MmapDirectory at <index store path>/<name>.tantivy/; commit = meta.json rewrite; 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 from meta.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 before register(); getDurableCursor() is valid synchronously from the first owner acquisition. This integration requires no change to derivedIndexRuntime.ts."
  • Gone with it: acquire() and the ~190 runtime lines that serve it (#acquireBackend, #acquisitionFailed, the #acquiring gates in wake/drain/startRebuild/requestRebuild, release waiting on acquisition, the generation-preserving #discardProgress), the new readiness reason and attempt budget, RocksDerivedIndexStorage, the KvDirectory transport and its per-I/O NAPI callbacks, the root-wide flushSync barrier stalling unrelated tables, and the derived-index writes that emit root committed events 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 orderTransactionLogStore::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

@kylebernhardy
kylebernhardy force-pushed the codex/fulltext-derived-backend branch from 2cb7c4b to 20ad537 Compare September 11, 2026 18:35
@kylebernhardy
kylebernhardy changed the base branch from codex/fulltext-derived-index-slice to feat/derived-index-native-backend-runtime September 11, 2026 18:35
Base automatically changed from feat/derived-index-native-backend-runtime to main September 11, 2026 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants