* MR-854: convert engine call sites to &dyn TableStorage; demote legacy methods
Phase 1b: every db.table_store.X(...) call site converts to
db.storage().X(...), reaching the storage layer through the sealed
TableStorage trait (returns &dyn TableStorage). Opaque SnapshotHandle
and StagedHandle replace bare lance::Dataset and Transaction in the
threaded values.
Phase 9: the inherent inline-commit methods on TableStore
(append_batch, merge_insert_batch{,es}, overwrite_batch,
create_btree_index, create_inverted_index) demote from pub to
pub(crate). Their only remaining direct users are table_store.rs
itself and the bulk loader's LoadMode::{Append, Overwrite, Merge}
concurrent fast-paths in loader::write_batch_to_dataset (no
two-phase shape in Lance 4.0.0 — closes after lance#6658 and #6666).
Docs:
- invariants.md \u00a7VI.23: drop "at the writer-trait surface"
qualifier; staged primitives are now the only engine surface.
- runs.md: residual matrix shrinks to delete_where and
create_vector_index (the two upstream-blocked residuals).
- forbidden_apis.rs: replace transitional language with the
current allow-list shape (table_store.rs + loader concurrent
fast-path only).
Files touched:
- changes/mod.rs, db/omnigraph.rs (+export/optimize/schema_apply/
table_ops.rs), exec/{merge,mod,mutation,staging}.rs,
loader/mod.rs, storage_layer.rs, table_store.rs,
tests/forbidden_apis.rs, docs/{invariants,runs}.md.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* MR-854: replace test-only inline-commit append callers with local Lance helpers
After demoting TableStore::append_batch from pub to pub(crate), the
integration tests in tests/recovery.rs and tests/staged_writes.rs
that previously called store.append_batch(...) directly to simulate
HEAD-ahead-of-manifest drift can no longer access the inherent
method. Replace those calls with small in-test helpers that do a raw
Dataset::append (the same body the inherent method runs).
- tests/helpers/mod.rs gains lance_append_inline (shared helper).
- tests/staged_writes.rs gets a file-local lance_append_inline_local
(staged_writes.rs does not import helpers::).
- tests/recovery.rs drops the unused TableStore import in the one
function whose store binding became unused after the conversion.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* MR-854: retrigger CI for flaky Test Workspace job
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* MR-854: convert remaining table_store call sites in export.rs / read_blob
Two leftover `self.table_store.X` / `db.table_store.X` call sites were
missed in the initial sweep — flagged by Devin Review on PR #86. Both
now go through the trait surface:
- `entity_from_snapshot` (db/omnigraph/export.rs): switch from
`db.table_store.open_snapshot_table` + `db.table_store.scan` to
`db.storage().open_snapshot_at_table` + `db.storage().scan`.
- `read_blob` (db/omnigraph.rs): replace
`snapshot.open(table_key)` + `self.table_store.first_row_id_for_filter`
with `self.storage().open_snapshot_at_table` +
`self.storage().first_row_id_for_filter`. The follow-up
`take_blobs` call still needs an `Arc<Dataset>` (it's a Lance blob
accessor not surfaced through the trait), so we hand off via
`SnapshotHandle::into_arc()` with a comment.
After this commit, no engine code outside `table_store.rs` reaches the
inherent `TableStore` API — the docs/runs.md and docs/invariants.md
claim is now uniformly true.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* MR-854: post-rebase doc fixes (Lance 6.0.1, MR-A framing, into_dataset note)
Reviewer feedback on the rebased PR:
* docs/dev/writes.md residuals matrix: drop demoted methods from the trait-surface table (now `pub(crate)`); keep only the two genuine trait-surface residuals (`delete_where`, `create_vector_index`); reframe under MR-A (Lance v7.x bump) per docs/dev/lance.md.
* tests/forbidden_apis.rs: update transitional allow-list header to (a) drop the truncate_table mislabel (truncate_table is a Lance Dataset method, not a TableStore method — overwrite_batch's internal call), (b) reframe trait-surface residuals under MR-A / Lance #6666.
* crates/omnigraph/src/storage_layer.rs::SnapshotHandle::{into_arc, into_dataset}: add single-ref invariant doc — both consume Arc via try_unwrap-or-clone; sibling SnapshotHandle clones across an await point force a deep Dataset clone.
* Replace lance-4.0.0 version refs with lance-6.0.1 in active source/test/dev-doc comments (storage_layer.rs, table_store.rs, table_ops.rs, schema_apply.rs, merge.rs, recovery.rs, staged_writes.rs, consistency.rs, docs/dev/execution.md, docs/user/query-language.md). Historical refs in docs/releases/v0.4.1.md and the canonical "Lance 4.0.0 → 6.0.1 migration" line in docs/dev/lance.md left intact.
No engine code changes.
* MR-854: update docs/dev/invariants.md Storage trait row + gap entry
Reviewer feedback: the docs reorg landed; the invariant row now lives in
docs/dev/invariants.md with stable headings (no more numbered §VI.23).
Update two pieces to reflect MR-854 completion:
* Status table 'Storage trait' row: was 'full call-site migration ... incomplete';
now 'engine call sites all route through db.storage() (MR-854); inline-commit
inherent methods are pub(crate)-demoted; capability/stat surfaces are roadmap'.
* 'Known Gaps' 'Storage abstraction' entry: was 'older inherent TableStore call
sites and inline residuals remain'; now names the closed scope (MR-854 — call
sites migrated, methods demoted, loader fast-paths) and the remaining
trait-surface residuals under MR-A (Lance v7.x bump) and Lance #6666.
Cross-links to docs/dev/lance.md and docs/dev/writes.md so the framing stays
co-located with the canonical Lance surface tracking.
* MR-854: remove dead inline-commit methods from the storage surface
The loader concurrent fast-path (write_batch_to_dataset) is only reached
for LoadMode::Overwrite — Append/Merge route through MutationStaging — so
its Append/Merge arms were unreachable. Collapse it to overwrite-only and
drop the now-unused mode params, which removes the only callers of:
- TableStorage::append_batch + TableStorage::merge_insert_batches (trait)
- TableStore::merge_insert_batch + merge_insert_batches (inherent)
create_btree_index / create_inverted_index had zero callers anywhere
(scalar index builds use the stage_* primitives). Remove both from the
trait and the inherent impl.
Inherent append_batch stays pub(crate): overwrite_batch and recovery
tests use it. Migrate the one trait-append_batch test caller
(seed_person_row) to stage_append + commit_staged. The merge_insert
FirstSeen-workaround rationale moves from the deleted merge_insert_batch
into stage_merge_insert (now the sole merge path). No behavior change.
Also corrects the inaccurate loader residual comment (the prior text
blamed Lance #6658/#6666, which are the delete and vector-index issues,
for keeping overwrite inline; a stage_overwrite primitive already exists
and schema_apply uses it).
* MR-854: seal db.storage() to staged-only; move residuals to InlineCommitResidual
Split the three remaining inline-commit writes (overwrite_batch,
delete_where, create_vector_index) off the TableStorage trait onto a new
sealed InlineCommitResidual trait, reachable only via the explicit
Omnigraph::storage_inline_residual() accessor. db.storage() now exposes
only staged primitives + reads, so engine code cannot couple a write
with a Lance HEAD advance through the default surface — MR-793 acceptance
§1 ("no public method commits as a side effect of writing") now holds by
construction, not by review + naming.
Call sites moved to storage_inline_residual(): loader overwrite
fast-path, the three mutation delete_where paths, the branch-merge
delete, and the vector-index build. Impl bodies are unchanged (same
delegation to the pub(crate) inherent methods); this is a pure surface
reshape with no behavior change.
The residual trait holds two genuinely upstream-blocked methods
(delete_where -> Lance #6658/v7.x, create_vector_index -> Lance #6666)
plus overwrite_batch, kept for the loader's cross-table bulk-overwrite
concurrency until its staged migration lands (tracked follow-up).
* MR-854 docs: describe the staged-only seal; fix stale Lance index URLs
- writes.md / invariants.md / AGENTS.md: the inline-commit residuals now
live on InlineCommitResidual behind db.storage_inline_residual(), so
acceptance §1 holds by construction rather than 'option (b)' per-method
enumeration. Drop the inaccurate 'until Lance exposes
Operation::Overwrite { fragments }' claim (that op exists; stage_overwrite
already builds it) and reframe overwrite_batch as a removable legacy
residual gated on the loader's bulk-overwrite concurrency.
- forbidden_apis.rs: rewrite the allow-list doc for the split surface.
- lance.md: the index spec pages moved from /format/table/index/ to
/format/index/ in Lance 6.x (the old paths 404). Fix all 13 URLs.
* MR-854: fix stale lance-4.0.0 comment refs flagged in review
Addresses greptile (exec/merge.rs) and aaltshuler's stale-version blocker:
update lance-4.0.0 -> 6.0.1 in the comment/doc refs within this PR's
footprint (exec/merge.rs, exec/mutation.rs, docs/dev/writes.md). Also
corrects exec/merge.rs to cite lance#6666 (not #6658) for
build_index_metadata_from_segments — that is the vector-index segment-commit
API; #6658 is the two-phase delete. (Pre-existing 4.0.0 refs in untouched
files like architecture.md/storage.md are main's incomplete migration
cleanup, left out of scope.)
* fix(storage): stage loader overwrites
* fix(storage): stage empty schema rewrites
---------
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: Ragnor Comerford <ragnor.comerford@gmail.com>
Co-authored-by: Ragnor Comerford <hello@ragnor.co>
17 KiB
Direct-Publish Write Path
History: the Run state machine and
__run__<id>staging branches were removed in MR-771 (shipped v0.4.0). Writes now go directly to the target table; this document specifies that direct-publish path.
mutate_as and load write directly to the target table
and call ManifestBatchPublisher::publish once at the end with
expected_table_versions (the per-table manifest versions captured before
the first write). Cross-table OCC is enforced inside the publisher; the
publisher's row-level CAS on __manifest is the single fence.
What this means in practice
- No
RunRecord, no_graph_runs.lance, no_graph_run_actors.lance. - No
omnigraph run *CLI subcommands and no/runs/*HTTP endpoints. - No
__run__<id>staging branches;__run__*is no longer a reserved name. The branch-name guard was removed in MR-770, and any stale__run__*branch on an upgraded graph is swept off__manifestby the v2→v3 internal-schema migration on first read-write open. (The inert_graph_runs.lancebytes remain until adelete_prefixprimitive lands.) - Cancelled mutation futures leave no graph-level state — only orphaned
Lance fragments, which the existing
omnigraph cleanuppipe reclaims.
Read-your-writes within a multi-statement mutation
A .gq query with multiple ops (e.g. insert Person … insert Knows …)
must observe earlier ops' writes when validating later ops (referential
integrity, edge cardinality). After MR-794 step 2+ this is implemented
via an in-memory MutationStaging accumulator in
crates/omnigraph/src/exec/staging.rs,
shared by both mutate_as and the bulk loader:
- On the first touch of each table, the pre-write manifest version is
captured into
expected_versions[table_key](the publisher's CAS fence at end-of-query). - Each insert/update op pushes a
RecordBatchinto the per-table pending accumulator. Lance HEAD does not advance during op execution. - Read sites (validation, predicate matching for
update) consumeTableStore::scan_with_pending, which scans committed via Lance and applies the same SQL filter to the pending batches via DataFusionMemTable. Same-query writes are visible to subsequent reads. - At end-of-query,
MutationStaging::finalizeissues exactly onestage_*+commit_stagedper touched table (concatenating accumulated batches; merge-mode dedupes byid, last-write-wins), and the publisher publishes the manifest atomically across all touched sub-tables. Cross-table conflicts surface asManifestConflictDetails::ExpectedVersionMismatch. - Deletes still inline-commit. Lance's
Dataset::deleteis not exposed as a two-phase op in 6.0.1; deletes go throughdelete_whereimmediately and record their post-write state inMutationStaging.inline_committed. The parse-time D₂ rule (below) prevents inserts/updates from coexisting with deletes in one query, so the inline path is safe for delete-only mutations.
This upholds the manifest-atomic mutation and read-your-writes invariants tracked in docs/dev/invariants.md.
D₂ — parse-time mixed-mode rejection
A single mutation query is either insert/update-only or delete-only. Mixed → rejected at parse time with a clear error directing the user to split the query. Reason: mixing creates ordering hazards (insert→delete on the same row would silently no-op because the staged insert isn't visible to delete; cascading deletes of just-inserted edges break referential integrity). Until Lance exposes a two-phase delete API, the parse-time rejection keeps both paths atomic and correct. Tracked: MR-793, plus a Lance-upstream ticket.
MR-793 status (storage trait two-phase invariant) — partial
MR-793 hoists the staged-write pattern into a TableStorage trait
surface with sealed-trait enforcement and opaque SnapshotHandle /
StagedHandle types — see crates/omnigraph/src/storage_layer.rs.
The trait is the canonical surface for new engine code; existing call
sites still use the inherent TableStore methods (mechanical migration
deferred to a follow-up cycle — tracked).
Three writers have been migrated onto staged primitives:
ensure_indices(db/omnigraph/table_ops.rs::build_indices_on_dataset_for_catalog) — scalar indices (BTree, Inverted) now usestage_create_*_index+commit_staged. Vector indices stay inline (residual — Lancebuild_index_metadata_from_segmentsispub(crate)in 6.0.1; companion ticket to lance-format/lance#6658 needed).branch_merge::publish_rewritten_merge_table(exec/merge.rs) — merge_insert now usesstage_merge_insert+commit_staged. Deletes stay inline (Lance #6658 residual).schema_applyrewritten_tables (db/omnigraph/schema_apply.rs) — rewrites usestage_overwrite+commit_staged, including empty-table rewrites via a zero-fragment LanceOperation::Overwrite.
A defense-in-depth integration test (tests/forbidden_apis.rs) walks
engine source and fails if non-allow-listed code calls Lance's
inline-commit APIs directly. The trait surface itself is the primary
enforcement (sealed + only-callable-via-trait once call sites land);
the grep test catches type-system bypass attempts.
The "finalize → publisher residual" described below applies equally to
the migrated writers — Lance has no multi-dataset atomic commit
primitive, so the per-table commit_staged → manifest publish gap is
the same drift class. Closing it requires either upstream Lance
multi-dataset commit OR the omnigraph-side recovery-on-open reconciler
described in .context/mr-793-design.md §15 (deferred to MR-795).
Inline-commit residuals live on InlineCommitResidual, not db.storage() (MR-793 acceptance §1, by construction)
MR-793's acceptance criterion §1 ("TableStore (or successor) public API has no method that performs a manifest commit as a side effect of writing") holds by construction after MR-854. db.storage() (&dyn TableStorage) exposes only staged primitives + reads; the inline-commit writes Lance cannot yet stage live on a separate InlineCommitResidual trait reached via Omnigraph::storage_inline_residual(). A new engine writer cannot couple a write with a Lance HEAD advance through the default surface — it would have to name the residual accessor explicitly. The dead legacy methods (trait append_batch / merge_insert_batches, inherent merge_insert_batch{,es}, create_{btree,inverted}_index) were removed; appends/merges and scalar index builds all use the stage_* primitives.
Two methods remain on InlineCommitResidual, each named honestly at its call site:
| Residual method | Inline-commit reason | Closes when |
|---|---|---|
delete_where |
DeleteBuilder::execute_uncommitted is not in Lance v6.0.1 (closed upstream as #6658 but first ships in v7.0.0-beta.10); see docs/dev/lance.md |
MR-A: Lance v7.x bump migrates delete_where to staged, retires the parse-time D₂ mutation rule, and extends recovery sidecar coverage |
create_vector_index |
Vector indices take Lance's "segment commit path"; build_index_metadata_from_segments is pub(crate) (Lance #6666 still open) |
Lance #6666 lands and stage_create_vector_index joins the staged surface |
The tests/forbidden_apis.rs guard still catches direct lance::* inline-commit misuse outside the storage layer; the trait split makes the staged-only default a type-system guarantee on top of it.
LoadMode::Overwrite uses staged Lance Overwrite
The bulk loader's Append, Merge, and Overwrite modes all use the
staged-write path described above. LoadMode::Overwrite accumulates
replacement batches in memory, validates node/edge constraints, referential
integrity, and edge cardinality before any Lance HEAD movement, stages
each touched table with Lance Operation::Overwrite, then runs
commit_staged under the normal SidecarKind::Load recovery sidecar
before publishing __manifest. OMNIGRAPH_LOAD_CONCURRENCY applies to the
fragment-writing stage only; the commit and manifest publish still run
under the per-table write queues. Empty-table overwrite is represented as
a valid zero-fragment Lance Overwrite transaction, not as
truncate-then-append.
Open-time recovery sweep
The staged-write rewire eliminates one drift class by construction at
the writer layer: an op that fails before pushing to the in-memory
accumulator (validation errors, missing endpoints, parse-time D₂
rejection) leaves Lance HEAD untouched on every staged table. This is
the case the partial_failure_leaves_target_queryable_and_unblocks_next_mutation
test pins.
A second, narrower drift class — the finalize → publisher window — is closed across one open cycle by the open-time recovery sweep:
MutationStaging::finalize runs stage_* + commit_staged per touched
table sequentially, then the publisher commits the manifest. Lance has
no multi-dataset atomic commit, so the per-table commit_staged calls
are independent operations: if commit_staged on table N+1 fails after
commit_staged on tables 1..N succeeded, or if the publisher's CAS
pre-check rejects after every commit_staged succeeded, tables 1..N
are left at Lance HEAD = manifest_pinned + 1.
Recovery protocol (lifecycle of every staged-write writer —
MutationStaging::finalize, schema_apply::apply_schema_with_lock,
branch_merge_on_current_target, ensure_indices_for_branch,
optimize_all_tables):
- Phase A: writer writes a sidecar JSON to
__recovery/{ulid}.jsonBEFORE its first HEAD-advancing commit (commit_staged, orcompact_filesforoptimize_all_tables, which advances the Lance HEAD via a reserve-fragments + rewrite commit rather than a staged write). The sidecar names every(table_key, table_path, expected_version, post_commit_pin)it intends to commit + the writer kind + actor_id. - Phase B: writer's per-table
commit_stagedloop runs. - Phase C: publisher commits the manifest.
- Phase D: writer deletes the sidecar.
Phase letter convention. Throughout the recovery code, log messages, failpoint names (e.g.
branch_merge.post_phase_b_pre_manifest_commit), and the per-writer integration tests, "Phase A/B/C/D" refers exclusively to the four-step lifecycle above. The per-table staged-write contract (stage_*thencommit_staged, two steps) is referred to by those API verbs — never by phase letters — so a reader ofrecovery.rs,failpoints.rs, or this document only encounters phase letters in the per-writer context.
A failure between Phase A and Phase D leaves the sidecar on disk. The
next Omnigraph::open (gated on OpenMode::ReadWrite) runs the
recovery sweep in crates/omnigraph/src/db/manifest/recovery.rs:
- For each sidecar in
__recovery/, compare every named table's Lance HEAD to the manifest pin. Classify per the all-or-nothing decision tree (RolledPastExpected / NoMovement / UnexpectedAtP1 / UnexpectedMultistep / InvariantViolation). - If any table is
InvariantViolation(Lance HEAD < manifest pinned — should be impossible), abort with a loud error and leave the sidecar on disk for operator review. - Otherwise, if every table is
RolledPastExpected, roll forward: a singleManifestBatchPublisher::publishcall extends every pin atomically.SchemaApplysidecars are eligible only when schema-state recovery promoted the matching staging files in the same recovery pass; otherwise full open-time recovery rolls them back and refresh-time recovery leaves them for the next read-write open. - Otherwise roll back: per-table
Dataset::restoreto the manifest-pinned table version, then a singleManifestBatchPublisher::publishof the restored HEAD — symmetric with roll-forward, somanifest == HEADafter recovery (no residual drift). This convergence is what lets a failed-then-retried schema apply succeed instead of failing one version higher each iteration. The audit row'sto_versionrecords the logical rolled-back-to version (manifest_pinned); the manifest is published at the restore commit (manifest_pinned + 1, same content). - After a successful roll-forward or roll-back, an audit row is
recorded —
_graph_commits.lancecarries a commit taggedactor_id = "omnigraph:recovery", and a sibling_graph_commit_recoveries.lancerow carriesrecovery_kind,recovery_for_actor(the original sidecar's actor),operation_id, per-table outcomes. Operators runomnigraph commit list --filter actor=omnigraph:recoveryto find recoveries. - Sidecar deleted as the final step.
Triggers for the residual: transient Lance write errors during finalize
(object-store retry budget exhaustion, disk full); persistent publisher
contention exceeding PUBLISHER_RETRY_BUDGET = 5 retries.
Long-running servers: Omnigraph::refresh runs roll-forward-only
recovery in-process — the common Phase B → Phase C residual closes
without a restart. The next mutation on the same handle (after refresh)
no longer surfaces ExpectedVersionMismatch for the failed table.
Sidecars that would require a Dataset::restore (mixed / unexpected
state) are deferred to the next OpenMode::ReadWrite open: restore is
unsafe under concurrency because Lance's check_restore_txn accepts
the restore against in-flight Append/Update/Delete commits and
silently orphans them (pinned by
tests/staged_writes.rs::lance_restore_loses_to_concurrent_append_via_orphaning).
Continuous in-process recovery for the rollback path is the goal of a
future background reconciler with per-(table, branch) writer-queue
acquisition.
The publisher-CAS contract is unchanged: a concurrent writer that advances any of our touched tables between snapshot capture and publisher commit produces exactly one winner. The residual above is about our abandoned commits in the failure path, not about concurrency races.
Conflict shape
Concurrent writers to the same (table, branch) produce exactly one
success and one failure. The losing writer's error is
OmniError::Manifest with kind Conflict and details
ManifestConflictDetails::ExpectedVersionMismatch { table_key, expected, actual }. The HTTP server maps this to 409 Conflict with body
{"error": "...", "code": "conflict", "manifest_conflict": { "table_key": "...", "expected": N, "actual": M }} — see docs/user/server.md.
Audit
actor_id lands in _graph_commits.lance via record_graph_commit (no
intermediate run record). Audit history is queried via omnigraph commit list.
Migration code
db/manifest/migrations.rs carries the v2→v3 internal-schema step (MR-770):
a one-time sweep that deletes legacy __run__* staging branches off
__manifest. It runs in Omnigraph::open(ReadWrite) (via
manifest::migrate_on_open, before the coordinator reads branch state) and
again on the publisher's write path; both are idempotent once the stamp is at
v3. Deleting the inert _graph_runs.lance / _graph_run_actors.lance dataset
bytes is still deferred — it needs a StorageAdapter::delete_prefix
primitive — but those bytes are invisible to graph-level state.
Mid-query partial failure: closed by MR-794
The pre-MR-794 design had a known limitation: a multi-statement .gq
mutation where op-N inline-committed a Lance fragment and op-N+1 then
failed left the touched table at Lance HEAD = manifest_version + 1,
blocking the next mutation with ExpectedVersionMismatch.
MR-794 (step 1 + step 2+) closed this for inserts/updates by
construction at the writer layer: insert and update batches accumulate
in memory; no Lance HEAD advance happens during op execution; one
stage_* + commit_staged per touched table runs at end-of-query, and
only after every op succeeded. A failed op leaves Lance HEAD untouched
on the staged tables, so the next mutation proceeds normally with no
drift to reconcile.
The cancellation case (future drop mid-mutation) inherits the same guarantee — the in-memory accumulator evaporates with the dropped task and no Lance write was ever issued.
For delete-touching mutations the legacy inline-commit shape is
preserved (Lance has no public two-phase delete in 6.0.1) — the same
narrow window remains. The parse-time D₂ rule prevents inserts/updates
from coexisting with deletes in one query, so a pure-delete failure
cannot drift any staged-table state. If a delete-only multi-table
mutation fails mid-cascade, the same workaround as before applies
(retry; rely on omnigraph cleanup once a later successful commit
moves HEAD past the orphan version). Closing this requires Lance to
expose DeleteJob::execute_uncommitted; tracked in MR-793 and a
Lance-upstream ticket.