omnigraph/docs/dev/writes.md
Ragnor Comerford 446b46d548
Recovery liveness, storage fault-injection matrix, and one storage implementation over object_store (#203)
* test(engine): pin the long-lived-handle heal contract for sidecar-covered drift

A Phase B -> Phase C failure (commit_staged advanced Lance HEAD, manifest
publish did not land, recovery sidecar persists) currently wedges every
subsequent staged write on the same engine handle: the commit-time drift
guard rejects with 'run omnigraph repair', but repair itself refuses
while a recovery sidecar is pending, so a long-lived server can only
recover by restart. The documented contract (writes.md 'Long-running
servers', invariants.md invariant 5) says refresh-time roll-forward
closes this residual without restart -- but no write path runs it.

Two red tests pin the intended contract at the write entry points:
a follow-up load (the POST /ingest shape: shared handle, no reopen)
and a follow-up mutation must heal roll-forward-eligible sidecars
in-process and then succeed.

Currently failing with:
  table 'node:Company' has Lance HEAD version 2 ahead of manifest
  version 1; run `omnigraph repair` before writing

The fix lands in the next commit.

* fix(engine): heal pending recovery sidecars at the staged-write entry points

Close the long-lived-process gap in the recovery protocol: a Phase B ->
Phase C residual (per-table commit_staged landed, manifest publish did
not, sidecar persists) previously recovered only at the next ReadWrite
open or via an explicit refresh() that no production write path called,
so a long-lived server wedged every subsequent write on the commit-time
drift guard until restart.

New recovery::heal_pending_sidecars_roll_forward:
- one list_dir of __recovery/ at write entry (empty -> immediate
  return, the steady state), so the per-write cost is one storage list;
- per sidecar, acquires the same per-(table_key, table_branch) write
  queues every sidecar writer holds from before write_sidecar until
  after delete_sidecar, then re-checks sidecar existence -- this
  serializes the heal against live writers instead of rolling an
  in-flight sidecar forward from under its writer (which would fail
  that writer's publish CAS spuriously). Lock order queues ->
  coordinator matches every writer's commit->publish path. This is the
  queue-acquisition design recovery.rs and write_queue.rs already
  documented for in-process recovery;
- processes in RollForwardOnly mode: the common residual rolls forward
  in-process; rollback-eligible sidecars still defer to the next
  ReadWrite open (Dataset::restore is unsafe under concurrency).

Wire it into load_as and mutate_as (before the inline delete path can
advance any HEAD), and rebase Omnigraph::refresh onto the same helper
so refresh stops racing live writers' sidecars.

The maintenance entry points (apply_schema_as, branch_merge_as,
ensure_indices) intentionally keep their strict fail-loud preconditions
for now; wiring the same heal there is a follow-up with its own tests.

Turns the previous commit's two red tests green.

* fix(engine): name the right recovery path in the commit-time drift guard

The drift guard's 'run omnigraph repair before writing' advice is a
dead end when the drift is covered by a pending recovery sidecar:
repair refuses while a sidecar is pending. With the write-entry heal in
place, reaching this guard with sidecar-covered drift means the heal
deferred it (rollback-eligible), and the actual recovery path is a
read-write reopen. Distinguish the two classes on the error path only
(one sidecar list, after the conflict is already certain); a listing
failure falls back to the uncovered-drift wording rather than masking
the conflict.

Pinned by extending refresh_defers_rollback_eligible_sidecar_to_next_open
with a write attempt against the deferred sidecar.

* docs: write-entry in-process sidecar heal — contract and coverage

Update the recovery contract docs to match the previous two commits:
invariant 5 now states that the staged-write entry points and refresh
run in-process roll-forward recovery (long-lived processes converge on
the next write, not at restart); writes.md 'Long-running servers'
describes the heal's queue-acquisition concurrency contract, the
improved drift-guard error, and the entry points that intentionally do
not heal yet; testing.md indexes the new failpoint tests; AGENTS.md
capability matrix drops the claim that in-process recovery is entirely
future work (only the rollback path remains with the background
reconciler).

* test(engine): pin the entry heal contract for schema apply and branch merge

Without the write-entry heal, the two maintenance writers do worse than
wedge on sidecar-covered drift -- they proceed and decide its fate
implicitly:

- schema apply re-plans table rewrites from the manifest pin, orphaning
  the drifted Phase-B commit (its rows silently vanish from the
  rewritten table) while the stale sidecar lingers to misclassify
  against the post-apply pins;
- branch merge publishes over the drift, making the failed writer's
  commit visible as an unattributed side effect (no recovery audit
  row), and leaves the stale sidecar behind.

Two red tests pin the intended contract: both entry points heal the
sidecar first (attributed roll-forward), then run on the converged
state. Currently failing on the stale-sidecar / dropped-rows
assertions; the fix lands in the next commit.

* fix(engine): heal pending recovery sidecars at the schema-apply and branch-merge entries

Extend the write-entry heal to the remaining two write entry points.
Unlike load/mutate (which wedge on the drift guard), these proceeded
over sidecar-covered drift and decided its fate implicitly:

- schema apply re-planned table rewrites from the manifest pin,
  orphaning the drifted Phase-B commit -- its rows silently vanished
  from the rewritten table -- while the stale sidecar lingered to
  misclassify against the post-apply pins;
- branch merge published over the drift, making the failed writer's
  commit visible without a recovery audit row, and left the stale
  sidecar behind.

Both now run the same queue-serialized roll-forward heal at entry,
before their own sidecar exists, so recovery is attributed (audit row)
and deterministic. ensure_indices stays heal-free: it runs inside the
load / schema-apply flows after their entry heal.

Turns the previous commit's two red tests green. Docs updated in the
same change (invariant 5, writes.md, testing.md, AGENTS.md).

* test(engine): pin Phase A sidecar-write failure semantics

Storage fault-injection matrix, row 1: a sidecar PUT failure (S3
PutObject / fs write) in Phase A. New failpoint recovery.sidecar_write
at the top of write_sidecar -- the single choke point all five sidecar
writers go through -- models the storage error backend-generically.

Also adds the other three storage-fault failpoints used by the
following commits (recovery.sidecar_delete, recovery.sidecar_list,
recovery.record_audit); each is a no-op without the failpoints feature.

Pinned contract: every writer writes its sidecar BEFORE its first
HEAD-advancing commit, so a put failure aborts with zero drift (no
sidecar, Lance HEAD == manifest pin, no rows) and a transient fault
never wedges the graph -- the same handle writes/merges normally once
it clears. Covered for load (the staging writer) and branch_merge (the
multi-table writer, forced onto the RewriteMerged path by diverging
both sides).

* test(engine): pin Phase D delete, list, and audit-append storage-fault semantics

Storage fault-injection matrix, rows 2/3/5, plus the real-backend run:

- recovery.sidecar_delete: a Phase D delete failure (S3 DeleteObject)
  must NOT fail the user's write -- the manifest publish already
  landed, so the caller's data is durable. The swallowed failure
  leaves a stale sidecar; the next write's entry heal consumes it via
  the stale-sidecar audit-recovery path (RolledForward, attributed).

- recovery.sidecar_list: a __recovery/ list failure (S3 ListObjectsV2)
  is loud at every consumer -- the write-entry heal fails the write
  and the open-time sweep fails the open. Silently skipping recovery
  over a pending sidecar would be consumer tolerance of drift. Once
  the fault clears, open recovers the pending sidecar normally.

- recovery.record_audit: an audit write failure after the
  roll-forward's manifest publish aborts that recovery attempt and
  keeps the sidecar; re-entry detects the already-published manifest,
  records exactly ONE RolledForward audit row, and converges -- the
  retry tolerance documented on record_audit, exercised end-to-end.

- s3_load_recovers_after_publisher_failure_without_reopen: the
  same-handle heal scenario on a real bucket (gated on
  OMNIGRAPH_S3_TEST_BUCKET, skips locally), exercising sidecar
  put/list/delete through S3StorageAdapter instead of the local-FS
  adapter. CI wiring lands in a follow-up commit.

* test(engine): refuse corrupt recovery sidecars loudly

Storage fault-injection matrix, row 4 (no failpoint needed -- the
corrupt file is written by hand, sibling to the unknown-schema-version
refusal test): a truncated/garbage __recovery/{ulid}.json must be
refused loudly by both the write-entry heal (the write fails naming
the parse error) and the open-time sweep (ReadWrite open fails naming
the file), with the file left on disk for operator inspection.
Read-only opens still work -- the sweep is skipped there.

* test(engine): run the S3 sidecar-lifecycle coverage in CI + document the fault matrix

- ci.yml rustfs_integration: new step running the bucket-gated
  failpoints tests (name filter s3_) against the RustFS container, so
  sidecar put/list/delete are exercised through S3StorageAdapter on
  every storage-affecting PR.
- writes.md: sidecar I/O failure semantics -- Phase A put failure
  aborts with zero drift; Phase D delete failure is swallowed (write
  already durable) and healed by the next write; list failures are
  loud at heal and open; corrupt sidecars are refused with the file
  kept for inspection; audit-append failures are retried to exactly
  one audit row.
- testing.md: index the storage-fault matrix in the failpoints.rs row
  and the new RustFS CI line.

* test(engine): pin read-visibility of acknowledged local if-absent writes

The cluster lib test import_missing_state_creates_state_with_graph_-
observation flakes at ~50% under full-workspace load ('EOF while
parsing a value' reading back the state.json its own import just
acknowledged). Root cause is in the engine's local storage adapter:
write_text_if_absent writes through a buffered tokio::fs::File and
returns when write_all resolves -- which, per tokio's documented File
semantics, means the bytes reached tokio's internal buffer, not the
file. The actual write completes in a background blocking task after
drop, so a caller that acknowledges success and reads the object back
can see an empty or partial file. Under load the window widens; the
red run fails at iteration 0 with 0 of 8192 bytes on disk.

The regression test pins the contract at the adapter boundary: when
write_text_if_absent resolves, the full contents are visible to any
reader; a losing second claim leaves the winner's object untouched.

The fix lands in the next commit.

* fix(engine): publish local storage writes with atomic visibility

Close the class, not the instance. The local adapter admitted three
ways for a reader to observe a write that was acknowledged or visible
before its bytes were complete:

1. write_text_if_absent acknowledged success when the buffered
   tokio::fs::File write_all resolved -- i.e. when the bytes reached
   tokio's internal buffer, not the file. A caller reading back its own
   acknowledged write could see an empty object (the ~50% cluster
   import flake under full-workspace load; the regression test failed
   at iteration 0 with 0 of 8192 bytes visible).
2. The same call published its CLAIM (create_new) before its CONTENT,
   so concurrent readers saw an empty claimed file in the window.
3. write_text (plain tokio::fs::write) exposed truncated content
   mid-replace -- silently falsifying write_sidecar's 'readers either
   see the complete sidecar or none' contract on local FS (true on S3,
   where PutObject is atomic).

A flush in write_text_if_absent would have fixed only (1). Instead,
both local write paths now publish complete temp files atomically:
rename for replace (write_text -- the idiom write_text_if_match
already used) and hard_link for no-replace (write_text_if_absent --
link fails AlreadyExists, so exactly one of N concurrent claimants
wins and the winner's object is fully readable at the instant it
becomes visible). The local adapter now honors the same object-level
atomic-visibility contract as the S3 adapter, which is what every
caller (recovery sidecar protocol, cluster state CAS) was written
against. Crash-orphaned *.tmp.* files are inert: the sidecar sweep
filters to .json, and cluster state reads address state.json by name.

fsync/durability policy is unchanged (no fsync before, none now);
this fix is about visibility ordering, not power-loss durability.

Pre-existing on main (landed with the multi-graph server mode change,
PR #119); surfaced by this branch's heal work only because one extra
list_dir per write shifted test timing. Cluster lib suite: 12/25
failures before, 0/25 after. Turns the previous commit's red test
green.

* refactor(engine): one storage implementation over object_store for every backend

Collapse LocalStorageAdapter (hand-rolled tokio::fs) and
S3StorageAdapter into a single ObjectStorageAdapter backed by
Arc<dyn object_store::ObjectStore> -- LocalFileSystem for local URIs,
the existing AmazonS3 build for s3://, plus a pub in_memory()
constructor (full contract including TRUE conditional updates; the
in-memory test backend testing.md asked for at the adapter level).

Why: the acknowledged-before-visible bug showed the two-impl shape has
no referee -- one prose contract, two independent answers. Upstream
LocalFileSystem::put_opts is byte-for-byte the staged-temp+rename/
hard_link idiom that fix converged on, and Lance's own commit protocol
is built on the same primitives (put-if-not-exists / rename-if-not-
exists), so the substrate-aligned move is to stop hand-rolling it.
The per-backend residue shrinks to a UriCodec (URI <-> object path)
and one capability flag.

Semantics preserved by construction, with three deliberate deltas:
- exists() is now object-store-semantics everywhere (head + non-empty
  prefix fallback): an EMPTY local directory no longer 'exists'. The
  only dir-shaped caller (_graph_commits.lance probes) self-heals via
  ensure_commit_graph_initialized where it previously wedged loudly.
- A directory at an object path reads as NotFound, not as an IO error
  ('only objects exist'). The cluster unreadable-payload test used a
  same-named directory as a portable non-NotFound trigger; it now uses
  chmod 000, which still models genuine transient IO.
- write_text_if_match keeps content-token semantics on local
  (PutMode::Update is NotImplemented upstream for LocalFileSystem in
  0.12.5 and 0.13.2); the capability flag gates the token SOURCE in
  read_text_versioned too -- an ETag token with content-compare writes
  would lose every CAS.

delete_prefix keeps a local remove_dir_all branch: directories are a
local-FS concept, and list+delete would leave empty skeletons that
cluster graph_root_exists (raw Path::exists) reports as still present.

LocalStorageAdapter remains as a delegating shim so the pinned
contract tests gate this swap textually unchanged; the shim and the
test parameterization over local + in-memory land next. Cargo gains
the explicit 'fs' feature (already transitively enabled by lance).

* test(engine): one executable storage contract, run against every backend

Remove the LocalStorageAdapter delegation shim and migrate its
construction sites to ObjectStorageAdapter::local(). Replace the
per-backend duplicated tests with a single contract_suite asserting
the trait's promises (atomic replace, exists incl. the dataset-root
prefix probe, one-winner if_absent, versioned CAS with loud CAS-lost,
rename, list round-trip with no sibling-prefix bleed, idempotent
delete/delete_prefix), run against the local backend and the new
in-memory backend -- which implements true conditional updates, so the
strong-CAS path is exercised without a bucket. The bucket-gated S3
variant already exists (s3_adapter_conditional_writes_contract).

New local-specific pins for the deliberate semantic edges of the
collapse: empty directories are not objects (exists=false; the Lance
dataset-root probe shape is the non-empty case), file://-anchored and
spaces-in-path list output round-trips byte-identically into
read_text, dot-segment paths are lexically absolutized (the CLI's
./graph.omni shape), and upstream rename creating missing destination
parents. The acknowledged-write visibility regression test stays, now
documenting that the cross-API std::fs read-back is the point.

* refactor(cluster): drop put_json's per-backend atomicity branch

The local temp+rename dance predates the storage adapter guaranteeing
atomic visibility; now that write_text publishes via a staged temp +
rename on the filesystem (and a single atomic PUT on object stores) by
contract, the branch duplicated upstream behavior. One call, both
backends.

* docs: storage adapter collapse — contract, in-memory backend, local CAS gap

- testing.md: the 'no MemStorage backend' note is half-closed —
  ObjectStorageAdapter::in_memory() covers the text-object layer with
  the full contract (true conditional updates); Lance datasets bypass
  the adapter, so the engine substrate ask stays open.
- invariants.md: truth-matrix Tests row updated; new Known Gap for
  local write_text_if_match (upstream PutMode::Update is unimplemented
  for LocalFileSystem; content-token emulation is safe only under the
  cluster lock protocol — close before admitting a lock-free caller).
- writes.md: backend notes for the unified adapter (name#N staging
  residue invisible to the sweep, backend-wrapped error text with
  exists()-probing for missing-vs-error, loud permission failures).

* docs: finish renaming the storage adapters in user docs and test comments

storage.md's URI-scheme table and the S3 failpoint test's doc comment
still named the deleted LocalStorageAdapter/S3StorageAdapter; both now
describe the unified ObjectStorageAdapter over object_store, including
the relative-path absolutization note for local URIs.

* test(engine): pin branch-awareness of the drift guard's recovery advice

A pending sidecar on ANOTHER branch does not cover this branch's
drift: with a deferred feature-branch sidecar on disk and genuinely
uncovered drift on main, the main write's error must still point at
omnigraph repair -- a read-write reopen recovers the sidecar but
cannot repair main's uncovered drift. Currently red: the guard
matches sidecar pins by table_key only, so the feature sidecar flips
main's advice to the reopen path. Fix in the next commit.

Surfaced by external review of the drift-guard change.

* fix(engine): branch-aware sidecar matching in the drift guard's advice

The commit-time drift guard's sidecar-covered check matched pins by
table_key alone, so a pending sidecar on another branch flipped this
branch's uncovered-drift advice from 'run omnigraph repair' to the
reopen path -- and a reopen recovers that sidecar but cannot repair
this branch's drift. Compare the pin's table_branch too. Turns the
previous commit's red test green.

Surfaced by external review of the drift-guard change.

* test(engine): pin heal non-interference with a live schema apply

The write-entry heal's schema-staging reconcile runs before any queue
acquisition, so a load on the same handle, overlapping a schema apply
parked between its staging write and manifest commit, promotes the
apply's staging files (new catalog live against the old manifest),
classifies the LIVE apply's sidecar, and publishes its registrations
out from under it. The resumed apply then collides with its own stolen
commit. Currently red with:

  Lance("Concurrent modification: table version 3 already exists for
  node:Tag")

The fix (per-sidecar reconcile under the sidecar's write-queue guards,
plus a serialization key the schema-apply writer and the heal both
acquire) lands in the next commit.

Surfaced by external review of the write-entry heal.

* fix(engine): serialize the heal's schema-staging reconcile with live schema applies

The write-entry heal ran recover_schema_state_files up front, before
acquiring any queue guards. Overlapping a live schema apply parked
between its staging write and manifest commit, the heal promoted the
apply's staging files (new catalog live against the old manifest),
classified the LIVE apply's sidecar, and published its registrations —
the resumed apply then collided with its own stolen commit.

Correct by construction:

- New schema-apply serialization queue key, acquired by the schema-
  apply writer (alongside its per-table keys) from before write_sidecar
  until after delete_sidecar. Per-table keys alone don't cover a
  registration-only migration, which pins no existing tables but has a
  sidecar and staging files on disk.
- The heal reconciles schema staging lazily, PER SchemaApply sidecar,
  after acquiring that sidecar's guards (including the serialization
  key) and re-confirming the sidecar exists — a sidecar that survives
  the queue wait belongs to a dead writer, so the reconcile can no
  longer race a live apply. Recomputing per sidecar also removes the
  staleness of one up-front result across a multi-sidecar pass.
- Omnigraph::refresh drops its up-front reconcile-and-pass-through
  (same race, and a pre-promoted result would make the heal's guarded
  reconcile see clean staging and wrongly defer the sidecar): it now
  reconciles standalone only when NO sidecar exists — which cannot
  race a live apply, whose sidecar always precedes its staging files —
  and otherwise defers entirely to the heal.

The open-time sweep keeps its precomputed reconcile: open has no
concurrent writers. Turns the previous commit's red test green.

Surfaced by external review of the write-entry heal.

Self-audit addendum folded in: refresh's no-sidecar gate had a TOCTOU
(a live apply could write its sidecar + staging between the empty
check and the reconcile) — the standalone reconcile now holds the
serialization key across the list-then-reconcile pair. The remaining
residual is cross-process only (in-process queues cannot serialize
against a writer in another process; the open-time sweep has the same
pre-existing exposure) and is now an explicit Known Gap in
invariants.md rather than an implicit one.

* test(engine): pin catalog reload after the heal recovers a schema apply

When the write-entry heal rolls a crashed apply's SchemaApply sidecar
forward on the same handle, disk and manifest move to the new schema
(staging promoted, registrations published) but the handle's in-memory
schema_source/catalog do not. Subsequent writes then validate against
the stale catalog and reject rows of types the graph already has.
Currently red with:

  record 1: unknown node type 'Tag'

refresh() reloads after its heal; the write entry points must too.
Fix in the next commit.

Surfaced by external review of the write-entry heal.

* fix(engine): reload the in-memory catalog after the heal recovers a schema apply

heal_pending_recovery_sidecars refreshed the coordinator and
invalidated the runtime cache after processing sidecars, but never
reloaded schema_source/catalog — so a write whose entry heal rolled a
crashed SchemaApply sidecar forward proceeded to validate against the
OLD schema while disk and manifest were already on the new one.
reload_schema_if_source_changed is the same post-heal step refresh()
already runs; it no-ops on the (overwhelmingly common) non-schema heal
because the on-disk source is unchanged. Turns the previous commit's
red test green.

Surfaced by external review of the write-entry heal.

* test(engine): pin that a deleted-branch sidecar cannot wedge the graph

A rollback-eligible sidecar pinned to a branch is deferred by every
roll-forward-only pass; if the branch is then deleted, the sidecar
survives, referencing a branch with no manifest tree. The heal (every
write entry) and the open-time sweep (every ReadWrite open) both fail
opening the dead branch, and repair refuses while a sidecar is pending
-- a terminal read-only state with manual sidecar surgery as the only
exit. Currently red with:

  Lance("Not found: .../__manifest/tree/feature/_versions")

The branch's tree and forks are already reclaimed, so the pinned drift
is unreachable and the sidecar is provably moot; the fix classifies it
as an orphaned-branch terminal state (audit + discard) in both passes.

Surfaced by review (P1, verified by repro).

* fix(engine): classify deleted-branch sidecars as orphaned instead of wedging

A deferred (rollback-eligible) sidecar pinned to a branch survives
branch_delete; both the write-entry heal and the open-time sweep then
failed unconditionally opening the dead branch -- every write and
every ReadWrite open errored, and repair refuses while a sidecar
pends. Terminal state, manual sidecar surgery the only exit.

The branch's tree and per-table forks are already reclaimed at delete,
so the drift the sidecar pins is unreachable and the sidecar is
provably moot. Both passes now check the sidecar's branch against the
manifest's branch list (the authority -- deliberately NOT inferred
from a Not-found on open, which could be a transient storage error
masking real recovery intent) and discard orphans with an
OrphanedBranchDiscarded audit row, commit appended on main since the
sidecar's own branch no longer has a commit graph.

The open-time half is pre-existing; the write-entry heal made it hot.
Turns the previous commit's red test green.

Surfaced by review (P1, verified by repro).

* chore: harden review nits — vacuous CI filter, root-runner skip, liveness note

- ci.yml: the RustFS sidecar-lifecycle step now fails loudly if the
  's3_' name filter matches zero tests (cargo passes vacuously on an
  empty filter; the step exists specifically to prove S3 sidecar I/O
  coverage). The pre-existing CLI smoke step has the same shape and is
  left for a follow-up.
- cluster unreadable-payload test: cfg(unix) + a skip-with-log when
  running as root (mode 000 is still readable to root, common in
  container dev runners), so the test degrades instead of failing.
- refresh: document the one-pass-late convergence for legacy staging
  residue while non-SchemaApply sidecars pend, so nobody 'fixes' it by
  re-running the reconcile unserialized — the exact race the
  serialization key closes.

* test(engine): pin orphan-discard idempotency across a delete fault

discard_orphaned_branch_sidecar writes its audit row and main commit
before deleting the sidecar; a Phase D delete fault leaves the sidecar
on disk with the audit already durable, and the retry repeated the
whole path -- a second OrphanedBranchDiscarded audit row (and commit)
for the same operation. Currently red: 2 rows after one fault + retry.
The retry must only finish the delete. Fix next.

Also promotes the recovery-audit kinds reader into the shared test
helpers (it was recovery.rs-local).

Surfaced by external review of the orphan-discard fix.

* fix(engine): orphan-discard idempotency + heal reports acted-vs-deferred

Two review findings on the recovery surface:

- discard_orphaned_branch_sidecar now checks the audit table for an
  existing (operation_id, OrphanedBranchDiscarded) row before appending
  the commit + audit pair, so a Phase D delete fault retries ONLY the
  delete instead of duplicating audit rows and commit-graph entries.
  Cold path: the list scan runs only when an orphaned sidecar exists.
  Turns the previous commit's red test green (exactly one audit row
  across fault + retry).

- process_sidecar returns whether durable state changed; the heal sets
  processed_any only for sidecars that were actually rolled forward /
  rolled back / audit-recovered (orphan discards count). Deferred
  sidecars (rollback-eligible, invariant-violating, unpromoted
  SchemaApply) no longer trigger a per-write schema reload + full
  runtime-cache invalidation while they pend -- the cache is
  snapshot-keyed so this was waste, not corruption, but it was paid on
  every write until reopen. Acted-paths' processed=true remains pinned
  by load_after_schema_apply_phase_b_failure_uses_recovered_catalog
  (the reload depends on it).

Surfaced by external review.

* test(engine): pin the orphan-discard audit-append fault leg as documented tolerance

The orphan discard's commit append and audit append are two writes; a
failure between them leaves a recovery commit with no audit row, and
the retry (keyed on the audit row, the operator-facing record) appends
a second commit before the audit lands. This is the same
not-atomic-pair-write tolerance record_audit documents and the
manifest->commit-graph Known Gap covers for every publish: bounded
commit-graph noise, audit row exactly-once under clean failures.
Keying idempotency on commit rows instead would need an operation_id
column on _graph_commits, and audit-before-commit would dangle the
graph_commit_id join -- both worse than the documented residual.

Make the tolerance explicit instead of implicit: docstring names the
window, a failpoint sits inside it, and the new test pins convergence
across the fault (sidecar consumed, exactly one audit row), completing
the orphan-discard fault matrix alongside the delete-fault leg.

Surfaced by external review of the orphan-discard idempotency.

* test(engine): pin honest drift-guard advice when sidecar listing fails

The guard's unwrap_or(false) conflated 'classified as uncovered' with
'could not classify': a transient list fault on the guard's second
list (the entry heal's first list having succeeded) confidently routed
the operator to omnigraph repair even when the heal had just deferred
a rollback-eligible sidecar -- and repair refuses while a sidecar is
pending. Currently red: the error says 'run omnigraph repair' with no
mention of the reopen path. The fix names both paths plus the failure
cause when classification is impossible.

Surfaced by external review of the drift-guard fallback.

* fix(engine): admit ambiguity in the drift guard when sidecar listing fails

Replace the unwrap_or(false) fallback with a tri-state: covered ->
reopen advice; uncovered -> repair advice; listing FAILED -> say the
drift could not be classified, name the cause, and give both paths in
order ('run repair, or reopen read-write if repair reports a pending
sidecar'). The old fallback confidently routed a transient list fault
to repair, which refuses while a sidecar is pending -- a self-
correcting but pointless detour. The conflict itself is still always
raised; only the advice degrades honestly. Turns the previous commit's
red test green.

Surfaced by external review of the drift-guard fallback.
2026-06-13 11:20:08 +02:00

20 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 __manifest by the v2→v3 internal-schema migration on first read-write open. (The inert _graph_runs.lance bytes remain until a delete_prefix primitive lands.)
  • Cancelled mutation futures leave no graph-level state — only orphaned Lance fragments, which the existing omnigraph cleanup pipe 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 RecordBatch into the per-table pending accumulator. Lance HEAD does not advance during op execution.
  • Read sites (validation, predicate matching for update) consume TableStore::scan_with_pending, which scans committed via Lance and applies the same SQL filter to the pending batches via DataFusion MemTable. Same-query writes are visible to subsequent reads.
  • At end-of-query, MutationStaging::finalize issues exactly one stage_* + commit_staged per touched table (concatenating accumulated batches; merge-mode dedupes by id, last-write-wins), and the publisher publishes the manifest atomically across all touched sub-tables. Cross-table conflicts surface as ManifestConflictDetails::ExpectedVersionMismatch.
  • Deletes still inline-commit. Lance's Dataset::delete is not exposed as a two-phase op in 6.0.1; deletes go through delete_where immediately and record their post-write state in MutationStaging.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 use stage_create_*_index + commit_staged. Vector indices stay inline (residual — Lance build_index_metadata_from_segments is pub(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 uses stage_merge_insert + commit_staged. Deletes stay inline (Lance #6658 residual).
  • schema_apply rewritten_tables (db/omnigraph/schema_apply.rs) — rewrites use stage_overwrite + commit_staged, including empty-table rewrites via a zero-fragment Lance Operation::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):

  1. Phase A: writer writes a sidecar JSON to __recovery/{ulid}.json BEFORE its first HEAD-advancing commit (commit_staged, or compact_files for optimize_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.
  2. Phase B: writer's per-table commit_staged loop runs.
  3. Phase C: publisher commits the manifest.
  4. 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_* then commit_staged, two steps) is referred to by those API verbs — never by phase letters — so a reader of recovery.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 single ManifestBatchPublisher::publish call extends every pin atomically. SchemaApply sidecars 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::restore to the manifest-pinned table version, then a single ManifestBatchPublisher::publish of the restored HEAD — symmetric with roll-forward, so manifest == HEAD after 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's to_version records 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.lance carries a commit tagged actor_id = "omnigraph:recovery", and a sibling _graph_commit_recoveries.lance row carries recovery_kind, recovery_for_actor (the original sidecar's actor), operation_id, per-table outcomes. Operators run omnigraph commit list --filter actor=omnigraph:recovery to 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: the write entry points (load_as, mutate_as, apply_schema_as, branch_merge_as) and Omnigraph::refresh run roll-forward-only recovery in-process (recovery::heal_pending_sidecars_roll_forward) — the common Phase B → Phase C residual closes on the next write, without a restart and without an explicit refresh. The heal lists __recovery/ (one list_dir; empty in the steady state) and, per sidecar, acquires the same per-(table_key, table_branch) write queues every sidecar writer holds from before write_sidecar until after delete_sidecar — so it serializes against a live writer instead of rolling its in-flight sidecar forward from under it (a sidecar whose queues can be acquired belongs to a writer that finished or died; an existence re-check after the wait skips the finished case). Lock order is queues → coordinator, matching every writer's commit→publish path. Pinned by the four tests/failpoints.rs::*_after_finalize_publisher_failure_heals_without_reopen tests (load, mutation, schema apply, branch merge). The maintenance entries need the heal for more than liveness: without it, a schema apply re-plans rewrites from the manifest pin and orphans the drifted Phase-B commit (dropping its rows), and a branch merge publishes the drift as an unattributed side effect — both while the stale sidecar lingers to misclassify later. 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). When such a deferred sidecar blocks a write, the commit-time drift guard says so explicitly ("a pending recovery sidecar requires rollback — reopen the graph read-write") instead of pointing at omnigraph repair, which refuses while a sidecar is pending. Continuous in-process recovery for the rollback path is the goal of a future background reconciler. ensure_indices does not heal at entry itself — it runs inside the load / schema-apply flows after their entry heal, and its strict preconditions still fail loudly on drift when invoked directly.

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.

Sidecar I/O failure semantics (all sidecar I/O goes through the backend-generic StorageAdapter; the contracts below are pinned by the storage-fault failpoints recovery.sidecar_{write,delete,list} / recovery.record_audit and their tests in tests/failpoints.rs and tests/recovery.rs):

  • Phase A put fails (S3 PutObject / fs write): the writer aborts before its first HEAD-advancing commit — no sidecar, no drift, nothing to recover; a transient fault never wedges later writes.
  • Phase D delete fails (S3 DeleteObject): swallowed with a warning — the write already published, so failing the caller would report an error for a durable write. The stale sidecar is consumed by the next write's entry heal (or the next open) via the stale-sidecar audit-recovery path, recorded as RolledForward.
  • __recovery/ list fails (S3 ListObjectsV2): loud at every consumer — the write-entry heal fails the write, the open-time sweep fails the open. Silently skipping recovery would be consumer tolerance of drift.
  • Corrupt / unparseable sidecar: refused loudly by heal and open alike; the file stays on disk for operator inspection (read-only opens still work — the sweep is skipped there).
  • Audit append fails after a roll-forward publish: that recovery attempt errors and keeps the sidecar; re-entry sees the already-published manifest, records exactly one RolledForward audit row, and deletes the sidecar (the retry tolerance documented on record_audit).

Backend notes (the adapter is one implementation over object_store for every backend): local writes stage through name#<digits> temp files that the backend filters from listings and refuses to address — crash residue of that shape is invisible to the sweep, harmless, and reclaimed by delete_prefix/manual cleanup. Storage errors are backend-wrapped text without a typed NotFound discriminant — callers that need missing-vs-error (the cluster store) probe exists() first. exists() itself is object-store semantics everywhere: only objects (or non-empty prefixes) exist, and a permission failure is a loud error, not a silent false.

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.