Skip to content

feat(cli): build a branch's declared indexes with omnigraph index build - #1150

Merged
ragnorc merged 8 commits into
ModernRelay:mainfrom
ragnorc:claude/omnigraph-issue-840-98af0f
Oct 28, 2026
Merged

ragnorc merged 8 commits into
ModernRelay:mainfrom
ragnorc:claude/omnigraph-issue-840-98af0f

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Closes #840. No command could build a branch's indexes: optimize reconciles them on main only, and the full-text rebuild covers full text alone. A type whose first rows land on a branch had no identity, BTREE or vector index there, and rows added on a branch to an indexed type stayed outside its BTREE and vector indexes until the branch merged and main was optimized. Answers stayed correct; branch reads scanned instead.

omnigraph index build [--branch B] brings one branch's declared indexes up to its rows: it builds every declared index a table lacks and rebuilds whole every BTREE or vector index that leaves a current fragment uncovered, in one graph commit on that branch, reporting full-text coverage and vector properties without vectors as pending with their remedy. The engine operation, Omnigraph::ensure_indices_on_as, enforces Change on the branch before any effect; each table's work stages as one detached CreateIndex on its pin from the two builders optimize already chains after compaction, and optimize's own links are unchanged. omnigraph index rebuild --kind full-text replaces rebuild-full-text-indexes, which stays hidden for one release and warns. The full-text refusals named omnigraph … <URI>, which the CLI rejects; they now name constants a test parses through clap.

The engine test builds a first-rows type and refreshes a lagging identity index on a branch, leaves main unchanged, and publishes nothing on a second run; the CLI test shows the branch's explain move from sequential to index_probe on rank_idx, and fails on the missing refreshed Person.__id row when the refresh is disabled. The policy test refuses both operations and both no-actor wrappers before any effect. A fast-forward merge after index build on the source keeps the pure-insert proof: Lance applies any CreateIndex with every fragment of the version it read unchanged, so the proof now crosses one as an index-only commit instead of falling back to the ordered row diff (merge_fast_forward::fast_forward_merge_proof_crosses_an_index_build_on_the_source_issue_840 reads two links and scans nothing; before the fix it stopped after one). lance_surface_guards.rs::rfc_0067_create_index_keeps_every_fragment_of_its_read_version pins the Lance fact this relies on: a detached first build and a whole rebuild keep every fragment of the read version, ids, physical rows, data files and deletions. Pending work on edge tables is reported after the node types. The DST storage-cost golden is unchanged.

For review: ensure_indices changes meaning. It now also rebuilds lagging BTREE and vector indexes, whole, since OmniGraph adds no delta segments (lance#9817), and with a policy installed it refuses without an actor. Three seeded pins moved with it, each because an earlier ensure_indices now has refresh work: a branching test now builds the branch's indexes before forking so its no-work premise holds; dst_maintenance_commit_uses_execution_branch seed 228317 moves from write 84 to 95 (with the refresh disabled, 84 lands in op 9 again; 95 is the same kind of write, a nodes-table transaction put, as the other seed's pin); and dst_setup_crash_needs_its_aftermath moves from hunt cell seed 2 to 5, the first of four scanned seeds whose first ensure_indices still has nothing to build. Full-text coverage stays explicit as RFC 0043 decided, though its original reason no longer applies since RFC 0067; that is a separate decision. openapi.json changes one description. Not run locally: the GQT corpus, whose harness builds indexes once on freshly seeded tables.

`omnigraph index build [--branch B]` brings one branch's declared indexes up
to its rows: it builds every declared index a table lacks and rebuilds whole
every BTREE or vector index that leaves a current fragment uncovered, in one
graph commit on that branch. Full-text coverage and vector properties without
vectors are reported as pending with their remedy. Optimize does the same on
main with compaction; both use the same index builders.

`omnigraph index rebuild --kind full-text` replaces
`rebuild-full-text-indexes`, which stays one release as a hidden alias that
warns. `--kind` is required.

Engine: `Omnigraph::ensure_indices_on_as` enforces Change on the branch before
any effect and records the actor; `ensure_indices` and `ensure_indices_on`
delegate to it with no actor, so with a policy installed they refuse instead
of bypassing the gate. One table's work stages as one detached CreateIndex
(`TableStore::stage_index_reconciliation`).

The full-text refusals and optimize's pending reason named commands with a
positional address the CLI rejects; they now name parseable commands, held as
constants that a CLI test parses.
…e-840-98af0f

# Conflicts:
#	crates/omnigraph-cli/src/planes.rs
#	docs/user/operations/upgrade.md
An index build now rebuilds an index whose coverage lags, so in hunt cell
seed 2 the first EnsureIndices has work after rows reach an indexed table,
and a setup crash at its pre-manifest window strands staging. Seed 5's first
EnsureIndices has nothing to build, which is what the test needs.

ragnorc commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Why

A branch can hold rows whose declared indexes are missing or no longer cover those rows. Queries still return the right rows. Filters and search then scan more data than the schema declares. Operators need one command that closes that cost gap on the branch they name.

What

omnigraph index build [--branch B] publishes one graph commit on that branch. The commit builds each declared index the table lacks. That set is the identity BTREE, a declared BTREE, a trainable vector index, and a full-text index that has no postings yet. The commit also rebuilds each lagging BTREE or vector index over every current fragment. Full-text coverage that already has postings stays pending. The operator refreshes that coverage with index rebuild --kind full-text. A second run publishes nothing. optimize on main still compacts, then reconciles indexes, and still writes the traversal artifact.

How

ensure_indices_on_as checks Change on the selected branch, then plans work on the pinned table versions. It stages one Lance CreateIndex per table that has work. It takes the same schema, branch, and table gates as other writers. It reads the branch again. A changed pin returns manifest_read_set_changed and publishes nothing. The publication sets published_dataset_version to the pinned version plus one and stores the detached tip, including a chained untrained full-text declaration when one is required. That logical step matches other writers.

The new segment's fragment bitmap is the dataset bitmap. Lance 13 CreateIndexBuilder::execute_uncommitted sets that bitmap when the build trains and the caller passes no fragment list. OmniGraph lists the old segments of the same name in removed_indices. The Lance 13 manifest apply drops those UUIDs and appends the new segment. Fragments stay as they are. replace(true) only lets the uncommitted build keep the name. The committing execute path is the one that fills removed_indices, and this code supplies the removals in the batch.

That whole rebuild is required on this Lance version. On stable row ids, a delta scalar or full-text segment does not own its rows. Older segments can still match. Vector search already has per-segment ownership. The Lance alignment guide says to drop the whole-rebuild rule at the bump whose scalar and full-text search apply segment ownership. Unknown fragment coverage stays untouched, which is the existing fold rule.

Impact

After the commit, the branch can probe the new indexes. The CLI test shows by_rank use index_probe on rank_idx after the command. Before the command the same query uses sequential. main and every other branch keep their pins and their indexes. A later merge still returns the right rows. See the optional note on the pure-insert proof.

Verdict

Approve. Blocking findings: 0. Optional findings: 4.

Optional findings

  1. docs/user/operations/maintenance.md lines 35-36 say index build does the index part of optimize on any other branch. The command also runs on main. main is the default. On main the command builds and refreshes indexes. Compaction, dropped-value rewrite, and the CSR artifact stay on optimize. The Build indexes section already says the default branch is main. Make the Optimize sentence match.

  2. The Build indexes section (lines 110-117) does not state the read-set rule. The Optimize section does (lines 54-57). The protocol is the same. A write that arrives while the batch is staged fails the run. The operator runs the command again. Add that sentence to the new section.

  3. proven_chain_fragments treats only an untrained full-text declaration as a commit that moves no row. A trained CreateIndex from this command has removals, so the pure-insert proof stops. The next merge of that interval uses the ordered row diff. The rows stay correct. This is the same class as index rebuild --kind full-text. A later change can treat a CreateIndex that adds no fragments as row-neutral, the way is_full_text_declaration already does for an untrained segment.

  4. A skipped edge vector fold is stored on the edge key and then left out of the result. The final pending list walks node types only (table_ops.rs around lines 627-633). Optimize reports the same skip on the table stat. OmniGraph does not declare edge vector indexes. An external edge vector that cannot train would be absent from this report.

Tradeoff and liability

A separate index command is the right primitive. optimize owns fragment rewrite, blob batch size, and the CSR artifact, and those effects stay on main. Index catch-up is the reconciliation ensure_indices already owns. The branch path calls the same index_fold_batch and create_index_batch. It publishes them as one CreateIndex per table. optimize still publishes compaction, fold, and create as up to three detached commits. One planner, two compositions.

The new surface is the index command group, ensure_indices_on_as, and IndexBuildResult. ensure_indices and ensure_indices_on still return the pending list. They now also refresh lagging BTREE and vector indexes. With a policy installed and no actor they refuse before any effect. The changelog records that refusal. The policy test checks a refused call leaves the tree unchanged and an allowed call records the actor.

Five similar commands fit this group: status, rebuild of one kind, and drop. --kind is already required on rebuild, so a new kind does not need a new top-level command. A background scheduler would add a queue. The invariants forbid that queue. Index work stays an explicit operator run.

This change spends efficiency to keep coverage correct on Lance 13. A whole rebuild writes more index data than a delta segment. That cost removes the stale-match hazard. The pinned search behavior makes the delta unsafe for scalar and full-text indexes. A refresh keeps the recorded partition count and trains L2, which is the metric OmniGraph uses for the vector indexes it creates. That choice already exists on the optimize fold.

The deprecated spelling rebuild-full-text-indexes remains for one release and warns. Remedies name omnigraph index build --target <target> --branch <branch> and omnigraph index rebuild --kind full-text with the same flags. The CLI rejects a positional graph address, so the old remedy text could not be copied and run.

Other open pull requests

#1151 fixes RUSTSEC-2026-0387 and does not change this command.

#1139 (6e527c16) still lists rebuild-full-text-indexes in CLI_ONLY_OPERATIONS. This head lists index build and index rebuild (crates/omnigraph-api-types/src/operations.rs lines 620-621). The pull request that lands second must keep the new names out of the MCP tool list.

#1130 overlaps cli_data.rs, docs/dev/execution.md, the search docs, and openapi.json. This PR's CLI test reads explain fields access and index. A merge must keep that assertion aligned with the explain shape.

#951 and #963 are direction for permissions and schema apply with live branches. This command adds no scheduler and no second catalog. It stays compatible with that direction.

Validation

Reviewed head: e6668bc443ab8256392ff37bf1ca372a0ffad301. State: open. Reviews: none. Review comments: none. Mergeable: yes.

Isolated checkout: /tmp/pr1150/checkout at that commit. Merge-base with origin/main: 7711e4a3def5c179cf95ec2525a6263819155916.

Local runs, debug profile, CARGO_TARGET_DIR=/workspace/target, from that checkout:

  • cargo test -p omnigraph-engine --offline --test integration maintenance::index_build_on_a_branch_builds_missing_and_lagging_indexes_issue_840 -- --exact --test-threads=8 — 1 passed, 0.10s.
  • cargo test -p omnigraph-engine --offline --test integration maintenance::optimize_reindexes_fragments_appended_after_index_build -- --exact --test-threads=8 — 1 passed, 0.06s.
  • cargo test -p omnigraph-engine --offline --test integration maintenance::index_build_tolerates_null_vector_rows -- --exact --test-threads=8 — 1 passed, 0.03s.
  • cargo test -p omnigraph-engine --offline --test policy_engine_chassis index_maintenance_enforces_selected_branch_before_effects_and_records_actor_issue_840 -- --exact --test-threads=8 — 1 passed, 0.12s.
  • cargo test -p omnigraph-cli --offline --lib commands::maintenance::tests::index_remedies_name_commands_that_parse_issue_840 -- --exact --test-threads=8 — 1 passed.
  • cargo test -p omnigraph-cli --offline --test integration cli_data::index_build_builds_missing_and_lagging_indexes_on_a_branch_issue_840 -- --exact --test-threads=8 — 1 passed, 0.33s.

An earlier engine filter omitted the maintenance:: module and matched 0 tests. That run is not evidence.

CI on this head: Test Workspace, GQ Logic Tests, GQT ordinary, GQT dst, GQT served, DST pinned suite, rustfmt, clippy, S3, Azurite, and Storage Upgrade succeeded. Dependency Guard failed on crypto-bigint 0.5.5, advisory RUSTSEC-2026-0387 (job log of run 40122585886). CI Gate failed because that job failed. #1151 addresses the advisory. It is not a defect in this change.

Regression proof

No GQT before/after pair is required. The new contract is a maintenance publication and the plan that follows it. The GQT runner calls ensure_indices once during seed, on a fresh load, and has no index-build step. The testing guide places cleanup and optimization in GQ until GQT can exercise them. The CLI integration test is the owner that can run the command and read sequential, then index_probe.

The .fixed note changes remedy text. FullTextIndexRequired needs a snapshot with no full-text segment. Current writers install an untrained segment, so a fresh GQT schema cannot reach that error. FullTextIndexRebuildRequired needs a forged index with no analyzer certificate. The CLI unit test fills <target> and <branch> and parses both remedies with clap, and it checks the error text contains those commands. No temporary GQT case was added. The checkout has no review edits.

Limitations

I did not merge a branch after an index build in this run. The pure-insert conclusion is from proven_chain_fragments and is_full_text_declaration. I did not measure rebuild time or index bytes. I did not run two processes on one graph. The new branch test covers a lagging identity BTREE, new BTREE indexes, and a new full-text index. A lagging vector refresh uses the same index_fold_batch as optimize. I did not run a separate vector-on-branch case. Local tests use small graphs in the debug profile.

Inline comments

docs/user/operations/maintenance.md:36 (RIGHT):

The sentence says index build does the index part on any other branch. The command also runs on main, and main is the default. On main the command builds and refreshes indexes. Compaction, dropped-value rewrite, and the CSR artifact stay on optimize. State that in this sentence. The Build indexes section already says the default branch is main.

docs/user/operations/maintenance.md:114 (RIGHT):

Add the read-set sentence from the Optimize section. A write that arrives on a table while its index batch is staged fails the run with a read-set conflict. The next run plans again from the new pin. maintain_indices_for_branch checks the pin again after the gates and before the manifest publication.

ragnorc added a commit to ragnorc/omnigraph that referenced this pull request Oct 26, 2026
State the full-text contract change, add the mixed fresh-and-moved BTREE case, name the identity and endpoint BTREEs, record lance#9785 and lance#9872 and mark the moved-row half as the Lance 13.0.0 fence, state the moved-row costs, and block on PR ModernRelay#1150.
An index build or fold commits a Lance CreateIndex with removals, which
the pure-insert merge proof treated as a row change, so the next merge
of that interval fell back to the ordered row diff. Any CreateIndex read
at the expected version keeps every fragment, so the proof now crosses
it as an index-only commit.

Pending index work on edge tables is now reported after the node types,
and the maintenance guide states that index build also runs on main and
that a write landing during the batch fails the run with a read-set
conflict.

ragnorc commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Why

A fast-forward merge can adopt a source branch when every commit after the base only inserts rows. An index build commits a Lance CreateIndex. The previous proof treated only an untrained full-text declaration as a commit that moves no row. A trained index removes the old segments, so the proof stopped. The merge then compared both sides by ordered rows. The rows stayed correct. The same stop applied to every index writer that commits a trained CreateIndex.

What

8be4900e replaces that special case with one check. A transaction whose operation is CreateIndex, read at the parent version recorded in its transaction file name, moves no row. The pure-insert proof continues past it. The same commit reports pending index work on edge tables after the node types. The maintenance guide now says index build runs on main as well as on other branches, and that a write during the staged batch fails the run with a read-set conflict.

How

Lance 13 applies CreateIndex by copying every fragment of the version it read, then replacing index metadata. The operation has new_indices and removed_indices only. It has no fragment list. See lance-table 13.0.0 transaction/manifest_build.rs, the CreateIndex arm, and the transaction format page for that message.

OmniGraph commits every index effect as that operation, through commit_detached:

  • index build and a lagging BTREE or vector refresh, one CreateIndex per table (IndexBatch::stage, from stage_index_reconciliation)
  • the full-text rebuild (stage_create_indices)
  • optimize's fold and its missing-index create, each a later link after the compaction Rewrite
  • the untrained full-text declaration that commit_staged_detached chains when a declared column has no segment

The check sits on the operation, in is_index_only_commit. Every path above is covered. A compaction Rewrite is not a CreateIndex, so the proof still stops there. That stop is required, because a rewrite replaces fragments.

The proof then requires the source fragments that the base lacks to hold the same physical row count as the certified inserts. A CreateIndex that added or removed a fragment id fails that check. A body whose read_version disagrees with the transaction file name is not index-only, and the insert certificate refuses it.

Impact

A fast-forward after an index build on the source stays a pointer switch. main takes the source pin, indexes included. The chain walk reads the index commit and the insert. It does not open an ordered scan of either side. Other branches stay on their own pins until a merge. A second index build still publishes nothing. A missing index still does not change query rows. This commit removes the ordered scan on a proven insert interval that also contains index commits.

Verdict

Ready. New blocking findings: 0. New optional findings: 1.

Earlier optional findings

All four notes on e6668bc4 are addressed.

  1. docs/user/operations/maintenance.md now says index build does only the index part, on any branch including main. Compaction, the dropped-value rewrite, and the traversal artifact stay on optimize.
  2. The Build indexes section now states the read-set conflict, and it tells the operator to run the command again.
  3. 8be4900e covers every CreateIndex, not only the tested index build. See How.
  4. Skipped edge work is stored under the edge: key, and the result walk appends those entries after the node types. OmniGraph still declares no edge vector index. The walk uses the key the skip already writes.

Optional finding

  1. Pin the Lance fact this proof now trusts. is_index_only_commit accepts any CreateIndex. That is safe on Lance 13 because apply copies the parent fragments. No lance_surface_guards case fails if a later Lance rewrites fragment bytes inside CreateIndex while ids and physical row counts stay the same. The merge test would still pass, and the pointer switch would adopt those bytes. Add a guard that commits a CreateIndex and requires the same fragment ids, physical row counts, and data-file paths as the parent. The comment on the function should point at that guard.

Inline on crates/omnigraph/src/table_store.rs line 580 (RIGHT): This accepts any CreateIndex whose read_version matches the transaction file name. That is safe on Lance 13: manifest_build copies the parent fragments and this operation has no fragment list. No lance_surface_guards case fails if a later Lance rewrites fragment bytes here while ids and physical row counts stay the same. The merge proof would still succeed, and the pointer switch would adopt those bytes. Add a guard that commits a CreateIndex and requires the same fragment ids, physical row counts, and data-file paths as the parent. Point this comment at that guard.

Tradeoff and liability

The old predicate named one safe CreateIndex shape that OmniGraph itself builds. The new predicate names the operation Lance applies without a fragment change. That is one rule for every index writer, so a new builder cannot forget a special case. The new dependency is the Lance 13 apply rule. The optional guard is the pin for that dependency.

The change feed still rejects CreateIndex in operation_is_row_set_preserving. An index commit inside a change-feed interval still uses the ordered comparison. The rows stay correct. That path does not use this proof. A shared classifier would need a zero-row plan for CreateIndex in the candidate scan. That cleanup can wait.

The new surface is unchanged from the previous review: the index command group, ensure_indices_on_as, and IndexBuildResult. ensure_indices still returns the pending list, and with a policy installed it still requires Change on the branch before any effect. Five more index commands belong in this group. A background scheduler remains outside the invariants.

This spends index bytes to keep coverage correct on Lance 13. A whole rebuild writes more than a delta segment. That cost removes the stale-match hazard the alignment guide pins for scalar and full-text indexes. #1156 plans incremental segments later and says the index build contract does not change. This commit does not add delta segments.

#1175 will move maintenance from Change to a maintain action. This head still checks Change, which is the current writer rule. #1175's statement that ensure_indices takes no actor is stale against this head. #1168 keeps standalone maintenance on the direct CLI. index build and index rebuild remain in CLI_ONLY_OPERATIONS, so the MCP catalog from #1139 does not expose them. The #1158 CSV overwrite path does not call ensure_indices. Load still publishes rows. Index build remains a separate commit.

Validation

Reviewed head: 8be4900e1f96df3713e5acce7b5b53f045074f51. State: open. Pull-request reviews: none. Inline review comments: none. The prior review is the issue comment on e6668bc4. Base: da1c46368c33876d61615add0ceb6778f9ce2b58.

Local runs used an isolated checkout of that head and CARGO_TARGET_DIR=/workspace/target. Debug profile. The checkout was restored after the parent run. Nothing was pushed.

Parent behavior, files merge.rs and table_store.rs taken from 41e15739639c (the commit before the fix), with the new test left in place:

cargo test -p omnigraph-engine --offline --locked --test integration merge_fast_forward::fast_forward_merge_proof_crosses_an_index_build_on_the_source_issue_840 -- --exact --test-threads=8

Failed. 0 passed, 1 failed, 642 filtered out, 0.14s. The assertion at the history-read count was left: 1, right: 2. The proof read the index commit and stopped before the insert. The fast-forward and pointer-switch assertions had already passed, so the ordered-scan assertion did not run. After a failed proof, classify_adopt calls compute_adopt_delta, which opens ordered cursors on both sides.

Same test at the head, after restoring those two files:

cargo test -p omnigraph-engine --offline --locked --test integration -- --exact --test-threads=8 merge_fast_forward::fast_forward_merge_proof_crosses_an_index_build_on_the_source_issue_840 maintenance::index_build_on_a_branch_builds_missing_and_lagging_indexes_issue_840

Passed. 2 passed, 0 failed, 641 filtered out, 0.09s.

CI on this head (run 40276555402) passed Test Workspace, GQ Logic Tests, GQT ordinary, GQT dst, GQT served, DST pinned suite, rustfmt, clippy, RustFS, Azurite, Storage Upgrade, and the CI Gate. That is CI evidence, not a local rerun.

Limitations

I did not rerun the workspace suite, the GQT corpus, or the CLI integration test locally. I did not run a separate fast-forward after index rebuild --kind full-text, after optimize's fold, or after a vector refresh. Those writers commit CreateIndex through the same commit_detached path, and the check does not name the caller. I did not measure rebuild time or index bytes. Local tests use small graphs in the debug profile.

…e-840-98af0f

# Conflicts:
#	crates/omnigraph/src/storage_layer.rs
…erging main

Main's csv overwrite sink and the index reconciliation each forward one
pinned dataset through TableStorage, so storage_layer.rs now holds 35.
The pure-insert merge proof crosses any CreateIndex link as moving no
row. A surface guard now commits a first build and a whole rebuild
detached, as the engine does, and requires every fragment of the read
version unchanged: ids, physical rows, data files and deletions. It goes
red if a Lance apply rewrites fragments inside CreateIndex. The proof's
comment and the Lance compatibility table point at it.

ragnorc commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Why

The pure-insert merge proof treats every CreateIndex as a commit that moves no row. That is safe when Lance copies the fragments of the version the transaction read. The review on 8be4900e asked for a surface guard that fails when a later Lance changes those fragments. After that review the branch merged origin/main. This review checks the guard, the merge, and the registry count that the merge left behind.

What

Head c83bcbe5145e50eeecaae4e6dc68002bb1baee8c adds two commits after the merge 8b728c8a.

0769e14c registers 35 sealed .dataset() forwards in storage_layer.rs. The merge had left the count at 34.

c83bcbe5 adds rfc_0067_create_index_keeps_every_fragment_of_its_read_version. The comment on is_index_only_commit and the detached-commit row in docs/dev/lance.md name that guard.

The merge 8b728c8a brings #1178 and #1182. Git records one conflict, in crates/omnigraph/src/storage_layer.rs. The resolution keeps OverwriteChunk from #1178 and RefreshedIndex from this branch (lines 58-60).

How

Operation::CreateIndex in Lance 13.0.0 lance-table transaction/manifest_build.rs (the CreateIndex arm) copies every fragment of the parent manifest, then replaces index metadata. The operation has new_indices and removed_indices. The transaction format page gives the same message and no fragment list.

The guard builds a BTREE with execute_uncommitted, then commits CreateIndex detached, with skip_auto_cleanup, on a stable-row-id V2_2 table that has two fragments. It does this for a first build and for a rebuild that passes the old segment in removed_indices. It reopens each result by version and compares Fragment values. Fragment equality includes the id, physical_rows, files (path, field ids, and file_size_bytes), overlays, deletion_file, row-id metadata, and the version metadata. The row-id scan returns the same ids. The rebuild leaves one segment, and that segment has a new uuid.

The sealed-forward count is an AST count of zero-argument .dataset() calls. The walker skips #[cfg(test)]. stage_append at line 1298 is the test-only call. The production forwards include stage_index_reconciliation (line 1208) and stage_overwrite_stream (line 1482). At the merge, both calls are present and the registry still says 34. The test then reports one drift: observed 35, registered 34. 0769e14c sets 35. At the head the same test reports no drift.

git diff 8be4900e HEAD is empty for exec/merge.rs, db/omnigraph/table_ops.rs, and db/omnigraph/optimize.rs. The table_store.rs diff in the merge is stage_overwrite_stream and the overwrite helpers from #1178. is_index_only_commit changes only in the comment that names the guard.

Impact

index build still publishes one graph commit. A fast-forward after an index build still follows the pure-insert proof. A later Lance that rewrites fragments inside CreateIndex turns this guard red before that proof can adopt the new bytes.

Verdict

Ready. New blocking findings: 0. New optional findings: 0.

Earlier findings

The review on e6668bc4 had 0 blocking findings and 4 optional findings. The review on 8be4900e had 0 new blocking findings and 1 optional finding. All five are addressed on this head.

  1. docs/user/operations/maintenance.md lines 35-36 say index build does only the index part, on any branch including main.
  2. Lines 117-118 state the read-set conflict and tell the operator to run the command again.
  3. is_index_only_commit accepts every CreateIndex whose read_version matches the transaction file name. exec/merge.rs line 1013 uses that check.
  4. table_ops.rs lines 635-642 append edge pending entries after the node types.
  5. The surface guard now commits a first build and a rebuild and compares fragment identity, physical rows, and data-file paths. The comment on is_index_only_commit (lines 573-579) names the guard. The detached-commit row in docs/dev/lance.md names it too.

Tradeoff and liability

The proof depends on one Lance apply rule. The guard is that dependency, in the suite that already pins detached commits. A Lance bump that changes the apply fails this test. The engine gains no new type, no new state, and no second catalog.

The registry edit is the existing chokepoint list. It records the forward #1178 added beside the forward this branch added. A second unregistered .dataset() in storage_layer.rs still fails the same test.

The whole-rebuild cost is unchanged from the review on 8be4900e. This delta does not add delta segments. #1156 still says the index build contract for BTREE and vector indexes stays the same, and that design uses this CreateIndex apply.

Five more pins of this kind belong in lance_surface_guards. They do not need a new module. The concept these commits share is "a CreateIndex link moves no row." is_index_only_commit is the owner. The guard is the substrate fact that owner trusts. A second classifier for the change feed can wait, as the previous review said.

Other open pull requests

origin/main is 790174c7 (#1129), one commit after the merge base bdddcbb (#1178). That commit changes the release attestation Node version. It does not touch this contract. This head does not contain it.

#1156 (updated after this head) keeps the BTREE and vector index build contract and cites the CreateIndex apply. This guard is the pin for the fragment half of that cite.

#1175, #1177, and #1181 also edit table_ops.rs, storage_layer.rs, table_store.rs, forbidden_apis.rs, or docs/user/operations/maintenance.md. The pull request that lands second must keep the sealed .dataset() count equal to the AST count, and must keep index build as one publication. The note from the 8be4900e review still holds: #1175's statement that ensure_indices takes no actor is stale against this head, which checks Change before effects.

Validation

Reviewed head: c83bcbe5145e50eeecaae4e6dc68002bb1baee8c. State: open. Pull-request reviews: none. Inline review comments: none. Issue comments: the two earlier reviews, unchanged. Mergeable: yes. Merge base with origin/main: bdddcbb153415d575711d7aa7eaf5999af27a8a2.

Local runs used /tmp/pr1150 and CARGO_TARGET_DIR=/workspace/target. Debug profile. --offline --locked. The worktree was restored to this head. Nothing was pushed.

At the head:

cargo test -p omnigraph-engine --offline --locked --test lance_surface_guards --test forbidden_apis --test integration -- --test-threads=8 rfc_0067_create_index_keeps_every_fragment_of_its_read_version graph_visible_write_chokepoints_are_registered fast_forward_merge_proof_crosses_an_index_build_on_the_source_issue_840 index_build_on_a_branch_builds_missing_and_lagging_indexes_issue_840

Passed. forbidden_apis: 1 passed, 35 filtered, 1.48s. integration: 2 passed, 641 filtered, 0.11s. lance_surface_guards: 1 passed, 71 filtered, 0.02s.

CI on this head, run 40444314162, passed Test Workspace, GQ Logic Tests, GQT ordinary, GQT dst, GQT served, DST pinned suite, rustfmt, clippy, RustFS, Azurite, Storage Upgrade, and the CI Gate. The workspace log shows the same four tests passed. That is CI evidence.

Regression proof

The failure starts at the merge commit 8b728c8a, the first commit that contains both forwards. The merge base bdddcbb contains the overwrite forward only.

At 8b728c8af959850d13eee9f70551a9a961c54d87:

cargo test -p omnigraph-engine --offline --locked --test forbidden_apis -- --test-threads=8 graph_visible_write_chokepoints_are_registered --exact

Failed. 0 passed, 1 failed, 35 filtered, 1.14s. The panic text is storage_layer.rs .dataset(): observed 35, registered 34.

The same command at the head passed, as listed above.

The Lance guard records behavior that already holds. With only lance_surface_guards.rs taken from the head, on 8b728c8a, the guard passed: 1 passed, 0.02s. The same guard passed at the head. The temporary file was restored. Nothing was pushed.

No GQT case was added. This delta does not change query rows, counts, column types, or error text. The maintenance publication remains owned by the engine and CLI tests from the earlier reviews.

Limitations

The fixture has no deletion file. Fragment equality includes deletion_file, and in this fixture both sides are absent. A Lance change that drops a deletion file only when one is present stays green here.

DataFile equality includes path and file_size_bytes. A rewrite that keeps the same path and the same size stays green. Lance 13's CreateIndex arm clones the fragment list and writes no data file.

I did not rerun the workspace suite, the GQT corpus, or the CLI integration test locally. CI on this head passed those jobs. I did not measure rebuild time or index bytes. Local tests use small graphs in the debug profile.

@ragnorc
ragnorc added this pull request to the merge queue Oct 27, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 27, 2026
@ragnorc
ragnorc added this pull request to the merge queue Oct 28, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 28, 2026
@ragnorc
ragnorc merged commit be90423 into ModernRelay:main Oct 28, 2026
53 of 55 checks passed
ragnorc added a commit to blarghmatey/omnigraph that referenced this pull request Oct 29, 2026
The merge of ModernRelay#1150 made `rebuild-full-text-indexes` a hidden spelling of
`index rebuild --kind full-text` and added `index build`, both direct-only
on a cluster root. The S3 allow-list still named only the old variant, so
the current commands refused an S3 root and the skill pointed agents at
the deprecated one. Admit `Command::Index` and test both leaves.

Say in the skill, command reference and guide that each direct writer
keeps its admission lock, which must be force-unlocked before the next
writer or the server starts. Move the S3 section to its own page: it put
the maintenance guide over the 350-line docs limit.
ragnorc added a commit that referenced this pull request Oct 29, 2026
…ry and maintenance (#1189)

* feat(cli): accept an inline s3:// cluster root for deployment, recovery and maintenance

Since #1115 an inline target is local-only, so the 0.14 CLI cannot address
an S3 cluster root. With the server stopped there is then no way to run
optimize or cleanup, force-unlock a retained lock, or reconcile an
outstanding deployment on S3 (#1163, #1082).

This admits an inline s3:// root for cluster plan, apply, status,
force-unlock and upgrade-ledger, and for optimize, cleanup, repair and
rebuild-full-text-indexes with --graph. The destination is captured once
from the AWS environment through StorageProvider::capture_from. Every other
command, az://, and an s3:// graph URI still refuse before any I/O.

Claude-Session: https://claude.ai/code/session_01FGZebF1Mp287NfGK4u4qbc

* fix(cli): admit index build and rebuild on an inline s3:// root

The merge of #1150 made `rebuild-full-text-indexes` a hidden spelling of
`index rebuild --kind full-text` and added `index build`, both direct-only
on a cluster root. The S3 allow-list still named only the old variant, so
the current commands refused an S3 root and the skill pointed agents at
the deprecated one. Admit `Command::Index` and test both leaves.

Say in the skill, command reference and guide that each direct writer
keeps its admission lock, which must be force-unlocked before the next
writer or the server starts. Move the S3 section to its own page: it put
the maintenance guide over the 350-line docs limit.

---------

Co-authored-by: Ragnor Comerford <ragnor.comerford@gmail.com>
Co-authored-by: Ragnor Comerford <hello@ragnor.co>
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.

No CLI command builds a branch's missing declared indexes

1 participant