Recovery liveness, storage fault-injection matrix, and one storage implementation over object_store - #203
Merged
Merged
Conversation
…ered 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.
… 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.
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.
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).
… 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.
…ranch-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).
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).
…t 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.
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.
…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.
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.
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.
…ery 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).
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.
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.
…AS 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).
…ents 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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7181eaaf70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+604
to
+608
| crate::db::schema_state::recover_schema_state_files( | ||
| root_uri, | ||
| std::sync::Arc::clone(&storage), | ||
| &snapshot, | ||
| ) |
There was a problem hiding this comment.
When a write-entry heal (e.g. the new load_as/mutate_as calls) overlaps a schema_apply that has written its sidecar and committed the manifest but has not yet renamed _schema*.staging, this call runs before acquiring any of the sidecar's write queues. It can promote the live schema apply's staging files, then the original schema_apply resumes and its own rename_text sees the staging source missing, returning an error even though the schema/manifest change is already durable. The queue serialization promised below only starts after this block, so schema-state recovery needs to be covered by the same serialization (or otherwise avoid acting on live sidecars).
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Valid — and the reproduced failure was one layer worse than described: with the apply parked between its staging write and manifest commit, the heal's up-front reconcile not only promoted the staging files, it then classified the LIVE apply's sidecar and published its registrations, so the resumed apply collided with its own stolen commit (Concurrent modification: table version 3 already exists). Fixed correct-by-construction: (1) a schema-apply serialization queue key held by the writer from before write_sidecar until after delete_sidecar (per-table keys alone don't cover registration-only migrations), (2) the heal reconciles lazily per SchemaApply sidecar, after acquiring that sidecar's guards and re-confirming it still exists, (3) refresh drops its up-front reconcile-and-pass-through and reconciles standalone only when no sidecar exists (race-free: a live apply's sidecar always precedes its staging files). Pinned by heal_does_not_promote_live_schema_apply_staging (red→green pair in history).
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…sign main's aabb3dc added flush() to the old LocalStorageAdapter's write_text_if_absent — an independent diagnosis of the same acknowledged-before-visible bug this branch fixed (3d26638 red test, d7a0144 fix). The flush closes the return-ordering window; the redesign on this branch closes the whole class (claim-before-content visibility to concurrent readers, and write_text's truncation window) and then collapses the adapter onto object_store, deleting the implementation the flush patched. Resolution: keep the unified ObjectStorageAdapter; port main's regression test (write_text_if_absent_is_read_consistent_immediately) into the contract test module — it exercises a surface ours doesn't (storage_for_uri construction, file:// URIs, multi-thread runtime).
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.
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.
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.
…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.
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.
…chema 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.
aaltshuler
reviewed
Jun 12, 2026
aaltshuler
left a comment
aaltshuler
left a comment
Collaborator
There was a problem hiding this comment.
Full review pass with verification. Summary of what I checked and found — inline comments below for the actionable items.
Verified and holds:
- The heal's safety precondition is real: all four sidecar writers acquire their write queues before
write_sidecarand hold them pastdelete_sidecar(staging.rs:515→674, merge.rs:1334→1415, optimize.rs:317→399, table_ops.rs:170→184), andacquire_manysorts+dedupes so the heal can't ABBA-deadlock a live writer. - Cedar
enforceprecedes the heal at bothload_asandmutate_as, so unauthorized callers can't trigger recovery publishes. - Red→green commit pairing throughout; docs updated in the same PR; the two new Known Gaps are made explicit rather than hidden.
One verified P1 (inline on recovery.rs): a deferred (rollback-eligible) branch-scoped sidecar plus a branch delete wedges every write and every ReadWrite open. Reproduced with a test against this branch's head.
Non-blocking notes without a good inline anchor:
- Every write now pays a real S3 LIST (
__recovery/) and gains a hard availability dependency on ListObjectsV2 — a transient LIST failure now fails writes that previously succeeded. Loud-over-silent is right per invariant 13, but for the single-writer-process server shape an in-process "no sidecar can exist" fast path could skip the LIST in steady state. Suggest a tracking issue. optimizedoesn't heal at entry (it refuses on a pending sidecar) while the other maintenance writers do. Invisible for the CLI (fresh open sweeps first); only bites an embedded long-lived handle. Worth a pointer in the refusal message.- Question: at
load_as/mutate_asthe heal runs afterensure_schema_state_valid. The Phase-B crash test proves the known shapes pass that check first — but if heal-before-validate is equally safe, it's the more defensive order; if validate-first is load-bearing, a comment would help. - Suggest a quick
rg 'ErrorKind::NotFound'audit overread_textcallers in engine/cluster — error text for missing objects changed from io-error-shaped to backend-wrapped, and error-path matches are what the green suite is least likely to catch.
Verdict: approve once the P1 is addressed; everything else is follow-up material.
| let branch_snapshot = match sidecar.branch.as_deref() { | ||
| Some(b) => { | ||
| let mut branch_coord = | ||
| GraphCoordinator::open_branch(root_uri, b, std::sync::Arc::clone(&storage)) |
Collaborator
There was a problem hiding this comment.
P1 — verified by repro: a deleted-branch sidecar wedges every write and every ReadWrite open.
This open_branch is unconditional and ?-propagated, and the open-time sweep (line 737) has the identical pattern. If a sidecar the heal defers (rollback-eligible / invariant-violating shapes) carries branch: "feature" and that branch is then deleted, checkout_branch on __manifest fails here on every subsequent write entry, the sweep fails every ReadWrite open, and repair refuses while a sidecar is pending (ensure_no_pending_recovery_sidecars, repair.rs:131/159). Terminal state: read-only graph; only remediation is manual __recovery/{ulid}.json surgery.
The state is reachable: ensure_branch_delete_safe has no sidecar awareness, and while branch_delete_as's internal refresh() does heal roll-forward-eligible sidecars before the authority flip (I verified the benign Phase-D-swallow case self-heals), it defers rollback-eligible ones — which then survive the delete. Reproduced on this branch's head: hand-shaped deferred sidecar on feature (same shape as tests/recovery.rs::drift_guard_advice_ignores_other_branch_sidecars) → branch_delete succeeds, sidecar survives → next main write and Omnigraph::open both fail with storage: Not found: …/__manifest/tree/feature/_versions.
Note the reopen half is pre-existing (the sweep's open_branch is already on main), but this PR promotes the pattern to the hot write path, so it's worth closing here.
Suggested fix: treat a sidecar whose branch is missing as an explicit orphaned-branch terminal state — the branch's tree and forks are already reclaimed, so the pinned drift is unreachable and the sidecar is provably moot: record an audit row (e.g. OrphanedBranch) and delete it, in both heal and sweep. Caution: key the orphan classification off the manifest's branch list (the authority), not off a Not found error from the open — swallowing a transient storage error as "orphaned" would silently discard recovery intent. A belt-and-braces companion: refuse branch_delete while a deferred sidecar pins the branch. Regression test: branch-scoped deferred sidecar → branch_delete → subsequent write + ReadWrite reopen must both succeed.
Contributor
Author
There was a problem hiding this comment.
Valid — great catch, and your repro matched exactly (Not found: …/__manifest/tree/feature/_versions on the next write). Fixed as suggested: both the heal and the open-time sweep now check the sidecar's branch against the manifest's branch list (the authority — deliberately not inferred from a Not-found on open, per your caution) before opening, and discard orphans with an OrphanedBranchDiscarded audit row (commit appended on main, since the dead branch has no commit graph). Pinned by deleted_branch_sidecar_does_not_wedge_writes_or_open — branch-scoped deferred sidecar → branch_delete → next write and ReadWrite reopen both succeed, sidecar consumed (red→green pair in history). I left the belt-and-braces branch_delete refusal out: the orphan classification makes the state benign, and refusing the delete would add operator friction for a sidecar that becomes provably moot the moment the branch dies.
| # bucket (the failpoint only wedges the publisher; the sidecar | ||
| # I/O is exercised for real). Name filter `s3_` matches the | ||
| # bucket-gated tests in the failpoints target only. | ||
| run: cargo test --locked -p omnigraph-engine --features failpoints --test failpoints s3_ -- --nocapture |
Collaborator
There was a problem hiding this comment.
Substring filter footgun: if the bucket-gated test is ever renamed away from the s3_ prefix, this step goes vacuously green (cargo passes with 0 tests matched). Since the step exists specifically to prove S3 sidecar I/O coverage, consider --exact with the full test name, or asserting non-zero tests ran from the output. (The existing CLI smoke step has the same shape — fine to fix both in a follow-up.)
Contributor
Author
There was a problem hiding this comment.
Valid — fixed for this step: it now fails loudly if the filter matches zero tests (captures output, requires test result: ok. [1-9]… passed). Left the pre-existing CLI smoke step for the follow-up as you suggested.
| .write_queue | ||
| .acquire(&crate::db::manifest::schema_apply_serial_queue_key()) | ||
| .await; | ||
| if crate::db::manifest::list_sidecars(&self.root_uri, self.storage.as_ref()) |
Collaborator
There was a problem hiding this comment.
Minor liveness wrinkle worth a comment: with a pending non-SchemaApply sidecar (e.g. a Mutation residual), this gate skips the standalone schema-staging reconcile, and the heal below only reconciles per-SchemaApply-sidecar — so pre-sidecar-era orphaned staging residue waits for the next refresh after the sidecars are consumed. Convergence holds, just one pass late. Suggest noting that here so nobody later "fixes" it by re-running the reconcile unserialized — the exact race this block exists to close.
Contributor
Author
There was a problem hiding this comment.
Agreed — comment added verbatim in spirit: the one-pass-late convergence for legacy staging residue while non-SchemaApply sidecars pend, and the explicit warning that re-running the reconcile unserialized here reintroduces the live-apply race the serialization key closes.
| // unreadable one.) Requires a non-root test runner, which is what | ||
| // CI and dev machines use. | ||
| let mut perms = fs::metadata(&blob).unwrap().permissions(); | ||
| std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o000); |
Collaborator
There was a problem hiding this comment.
Two portability nits: (1) this fails when the test runner is root (chmod 0o000 doesn't stop root reads — Docker dev containers commonly run as root); a euid guard with a logged skip would degrade gracefully instead of failing. (2) std::os::unix::fs::PermissionsExt is unix-only — fine today, but a #[cfg(unix)] would keep a hypothetical Windows test build compiling.
Contributor
Author
There was a problem hiding this comment.
Both fixed: #[cfg(unix)] on the test, plus a probe-based skip-with-log when mode 000 is still readable (root runners) — the contract under test needs a genuine permission error, so degrading beats failing.
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).
…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).
…ness 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.
…s-symptomatic-fixes
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.
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.
…nted 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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f2e902e. Configure here.
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.
…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.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three units of work, each motivated by the previous one's findings. The through-line: correct by design over symptomatic patches — every fix closes the bug class, not the instance, with a red regression test committed immediately before each fix so the red → green pair is visible in history.
1. Write-entry sidecar heal (recovery liveness for long-lived processes)
A Phase B → Phase C residual (per-table
commit_stagedlanded, manifest publish did not, recovery sidecar persists) previously recovered only at the nextOmnigraph::open— but a long-lived server never reopens, so every subsequent write wedged on the commit-time drift guard with dead-end advice (omnigraph repairrefuses while a sidecar is pending). Worse, the two maintenance writers proceeded over sidecar-covered drift and decided its fate implicitly: schema apply silently dropped the drifted rows (re-planned table rewrites from the manifest pin, orphaning the Phase-B commit) and branch merge silently published them with no recovery audit row.All four write entry points (
load_as,mutate_as,apply_schema_as,branch_merge_as) now run a roll-forward-only heal at entry: one__recovery/list in the steady state; when sidecars exist, the heal acquires the same per-(table, branch)write queues every sidecar writer holds across its sidecar's lifetime, so it serializes against live writers instead of rolling an in-flight sidecar forward from under its writer.refresh()was rebased onto the same helper (fixing its pre-existing race against live writers). Rollback-eligible sidecars still defer to a read-write open — Lancerestoreis unsafe under concurrency — and the drift-guard error now names the correct recovery path per drift class.2. Storage fault-injection matrix for the sidecar lifecycle
Four new failpoints at the sidecar I/O choke point (
recovery.sidecar_{write,delete,list},recovery.record_audit) model backend-generic storage failures. Pinned contracts:RolledForwardaudit row.3. One storage implementation over
object_storeRunning the suite under load surfaced a pre-existing bug (landed with the multi-graph server change): the local adapter's
write_text_if_absentacknowledged success whentokio::fs::File::write_allresolved — 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 (~50% flake in the cluster import test under full-workspace load; the red test fails at iteration 0 with 0 of 8192 bytes visible).The fix closes the class, not the instance: local writes now publish complete-or-invisible. And since the resulting idiom is byte-for-byte what upstream
object_store::LocalFileSystemalready implements — and Lance's own commit protocol is built on the same primitives (put-if-not-exists/rename-if-not-exists) — the two hand-maintained adapters (LocalStorageAdapterover rawtokio::fs,S3StorageAdapter) are collapsed into a singleObjectStorageAdapteroverArc<dyn ObjectStore>:UriCodec(URI ↔ object path) and one capability flag (PutMode::Updateis unimplemented upstream forLocalFileSystem, so localif_matchkeeps the content-token emulation — documented as a Known Gap, safe under the cluster lock protocol).in_memory()backend implements the full contract including true conditional updates, partially closing the long-standing "no in-memory test backend" note.contract_suiteparameterized over local + in-memory (the bucket-gated S3 leg already existed), plus pins for the deliberate semantic edges (object-storeexists()semantics for directories, byte-identicallist_dirround-trips forfile://and spaces-in-path anchors, lexical absolutization of relative/dot-segment paths, rename creating destination parents).put_jsonatomicity branch.Test plan
cargo test --workspace --locked— greencargo test -p omnigraph-engine --features failpoints --test failpoints— 43 passed (6 new storage-fault tests + 4 entry-heal tests)cargo test -p omnigraph-cluster --features failpoints --test failpoints— greentest(engine):commit and reproduce the failureDocs
invariants.md (invariant 5, truth matrix, two Known Gap entries), writes.md (long-running servers, sidecar I/O failure semantics, backend notes), testing.md (failpoints index, RustFS section, in-memory backend note), storage.md (URI-scheme table), AGENTS.md capability matrix.
Note
High Risk
Changes commit/recovery concurrency, multi-table write entry behavior, and all text-object storage semantics; incorrect locking or heal ordering could corrupt manifest/schema state or wedge writes, though the PR adds extensive failpoint coverage.
Overview
Long-lived servers no longer depend on reopen to finish Phase B → Phase C recovery:
load,mutate,schema apply, andbranch mergecall a roll-forward-onlyheal_pending_sidecars_roll_forwardat entry (same path as reworkedrefresh), using per-table write queues and a__schema_apply__key so heals do not race live writers or promote in-flight schema staging. Rollback-eligible sidecars still defer to read-write open; orphaned sidecars on deleted branches getOrphanedBranchDiscardedaudit + discard; commit-time drift errors distinguish sidecar-covered vs uncovered drift.Recovery sidecar I/O gains failpoints (
recovery.sidecar_{write,delete,list},recovery.record_audit, orphan audit) with broad failpoint/integration coverage and a RustFS CI step for bucket-gateds3_tests.Storage collapses
LocalStorageAdapter/S3StorageAdapterintoObjectStorageAdapteroverobject_store(local, S3, in-memory), fixingwrite_text_if_absentvisibility viaPutMode::Create; clusterput_jsondrops per-backend branching. Docs and invariants updated for in-process recovery and known local-CAS / multi-process heal gaps.Reviewed by Cursor Bugbot for commit 974a27e. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR delivers three related improvements: in-process recovery liveness for long-lived servers (all four write entry points now heal Phase B→C sidecars before executing, removing the restart requirement), a storage fault-injection matrix with six new failpoint tests that pin the sidecar I/O contract end-to-end, and a storage-layer consolidation that collapses two hand-maintained adapters into a single
ObjectStorageAdapteroverobject_store(fixing thewrite_text_if_absentflush-visibility bug that caused ~50% test flakes under load).heal_pending_sidecars_roll_forwardruns at every write entry point andrefresh, serializing against live writers via per-(table_key, branch)queue acquisition + existence re-check; rollback-eligible sidecars and orphaned-branch sidecars are both handled;schema_apply_serial_queue_keycloses the staging-file-steal race.ObjectStorageAdapterprovides atomic visibility on all backends, adds a true-CASin_memory()backend for tests, and removes the per-backendput_jsonbranch in the cluster store.Confidence Score: 5/5
Safe to merge — the recovery and storage changes are well-tested with red-to-green regression pairs, the concurrency design is clearly documented and follows a consistent lock order, and no correctness defects were found.
Every new code path is covered by a matching failpoint or integration test, the lock ordering (queues to coordinator) is invariant across all new call sites, and the documented Known Gaps (local CAS TOCTOU, audit-before-commit window) are pre-existing and explicitly bounded. Two minor observations — a stale one-list_dir cost claim in the refresh doc, and a redundant second coordinator refresh for SchemaApply sidecars — are documentation and efficiency nits with no runtime impact.
No files require special attention. The most complex new logic is in crates/omnigraph/src/db/manifest/recovery.rs and is well-documented and well-tested.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD WE[Write Entry load_as / mutate_as / apply_schema_as / branch_merge_as] --> H[heal_pending_recovery_sidecars] R[refresh] --> SL{list_sidecars empty?} SL -- yes --> SR[standalone schema state reconcile] SL -- no --> SK[skip reconcile, heal owns it] SR --> H SK --> H H --> HPSRF[heal_pending_sidecars_roll_forward] HPSRF --> LS2[list_sidecars] LS2 -- empty --> DONE[return false] LS2 -- sidecars --> LOOP[for each sidecar] LOOP --> AQ[acquire per-table queue keys + serial key if SchemaApply] AQ --> RC{sidecar still exists?} RC -- no --> SKIP[skip] RC -- yes --> ORPHAN{branch exists?} ORPHAN -- no --> DISC[discard_orphaned_branch_sidecar audit + delete] ORPHAN -- yes or null --> PS[process_sidecar RollForwardOnly] PS -- rolled forward --> RA[record_audit + delete_sidecar] PS -- deferred --> DEF[defer to next ReadWrite open] RA --> DONE2[return true] LOOP --> DONE2Comments Outside Diff (1)
crates/omnigraph/src/storage.rs, line 459-475 (link)write_text_if_matchlocal emulation: intermediateputis an unconditional overwriteAfter the content-token check passes,
self.store.put(&location, ...)is an unconditional overwrite — no CAS between thegetand theput. A concurrent writer that wins between those two calls will have its content silently replaced. This is the same TOCTOU gap as the previoustokio::fs::read+ temp-write + rename implementation, and it is correctly documented as a Known Gap. However, the new code doesn't include the explicittmpcleanup path that the old code had for the rename-failure case; sincestore.putonLocalFileSystemis internally staged, that path disappears naturally — worth a one-line note confirming the old cleanup path is superseded.Reviews (8): Last reviewed commit: "fix(engine): admit ambiguity in the drif..." | Re-trigger Greptile