Repository navigation
fix(engine): optimize survives a cross-process write race on the same table - #297
Merged
Merged
Conversation
Two regression tests for the prod bug: a direct `optimize` process racing a served write on the same table fails, because the in-process write queue does not serialize across processes and the data-table optimize path has no retry. - optimize_survives_concurrent_insert_advancing_manifest: a concurrent insert advances the manifest while optimize is paused between compact and publish; optimize's equality-CAS publish then fails "expected X but current Y". - optimize_survives_concurrent_delete_before_compaction: a concurrent delete commits before optimize compacts; Lance rebases the compaction past it cleanly, so optimize again fails the publish CAS (the genuine Lance Rewrite-vs-Rewrite overlap is rarer and shares the internal path's retry). Both fail today with ExpectedVersionMismatch. Adds the `optimize.before_compact` failpoint seam + a wait_for_sidecar helper; serializes the optimize failpoint tests (shared failpoint name). The fix lands next.
… table
The data-table optimize path trusted the in-process write queue and skipped a
retry, so a CLI `optimize` racing a served write (separate processes = separate
queues) failed: either the Lance Rewrite lost ("preempted by concurrent Update")
or the manifest publish lost the strict equality CAS ("expected X but current Y").
Unify both compaction paths on the internal path's reopen+replan shape, with a
two-level retry that matches the two failure points:
- Outer loop (reopen+replan): a genuine Lance Rewrite-vs-Update/Delete same-
fragment conflict means our compaction did not commit — reopen at the new HEAD
and re-plan. Lance rebases the common disjoint case (a concurrent insert/delete
on other fragments) for free, so this fires only on real overlap.
- Inner loop (Phase C, monotonic publish): the manifest advanced between our
compaction and our publish. The compaction is already committed at Lance HEAD N,
so we must NOT reopen (that trips the HEAD>manifest drift guard on our own work).
Re-read the current manifest version C: if C >= N the manifest already includes
our compaction (versions are linear) — no-op; else fast-forward to N. Monotonic,
not the strict equality CAS that manufactured the conflict.
The Phase-A sidecar is written once and reused across reopen attempts (every
Phase-B commit is content-preserving, so recovery rolls the observed HEAD forward
or safely rolls the compaction back). The in-process queue is kept — it is now an
in-process contention reducer, not the cross-process correctness guard. Shares the
COMPACTION_RETRY_BUDGET constant + is_retryable_lance_conflict with the internal
path; adds is_retryable_manifest_conflict for the publish loop. No writer_epoch.
Turns the prior commit's two race tests green.
…rite race §6.6 records the maintenance vs logical op-class distinction (maintenance commutes → Lance rebase + reopen/replan + monotonic manifest fast-forward, no writer_epoch; logical → strict cross-process OCC + epoch) and the prod optimize-vs-served-write race that motivated it, now landed. Adds the matching mechanic row to §4.2.
![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: 903ede2f95
ℹ️ 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
+524
to
+525
| Err(e) if attempt < COMPACTION_RETRY_BUDGET && is_retryable_lance_conflict(&e) => { | ||
| continue; |
There was a problem hiding this comment.
When optimize_indices hits a retryable Lance conflict after compact_files has already succeeded, this continue re-enters the outer loop with Lance HEAD advanced by our own compaction while the manifest is still at the old version. The next iteration then takes the lance_head_version > expected_version drift branch, deletes the recovery sidecar as if the drift were external, and returns skipped_for_drift, leaving an uncovered HEAD>manifest gap that can no longer be recovered automatically. This also applies to upgraded graphs if the auto-cleanup config scrub commits before a later retryable conflict, so the retry path needs to distinguish/publish or roll back its own Phase-B commits instead of looping into the generic drift skip.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Same finding as the cursor High at optimize.rs:443 — confirmed and fixed in 56d004e0. The retry now tracks head_advanced; once one of our Phase-B ops (auto_cleanup strip / compact_files) has committed, a reopened HEAD > manifest is treated as our own sidecar-covered work (the monotonic Phase-C publish fast-forwards it), not external drift, so the sidecar is not deleted and skipped_for_drift is not returned. Regression test optimize_retry_does_not_misclassify_own_head_drift injects exactly the "compact commits, reindex conflicts" sequence you describe.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review catch on the cross-process optimize fix: the outer retry loop re-ran the `lance_head > manifest` drift guard every iteration. After a partial Phase-B commit (the auto_cleanup strip or compaction commits, then a later op hits a retryable conflict), the reopened attempt saw HEAD ahead of the manifest — from OUR own sidecar-covered work, not an external writer — and deleted the sidecar + returned `skipped_for_drift`, stranding uncovered drift that then needs `repair`. Track `head_advanced` (did one of our Phase-B ops already commit). The drift guard now fires only when `!head_advanced` (genuine pre-existing external drift); once we have advanced HEAD, a reopened HEAD>manifest is our work that the monotonic publish fast-forwards. The no-op early-return likewise publishes prior committed work instead of dropping it when `head_advanced`. Regression test `optimize_retry_does_not_misclassify_own_head_drift` injects one retryable reindex conflict after the compaction commits (new `optimize.inject_ reindex_conflict` seam); red→green verified by negative control (reverting the gate reproduces `skipped_for_drift: Some(DriftNeedsRepair)`). Also de-flake `optimize_survives_concurrent_insert_advancing_manifest`: pause at `before_compact` (not post-compact) so the concurrent insert lands while HEAD== manifest — otherwise it could race optimize's committed-but-unpublished compaction and hit the write-path "HEAD ahead of manifest" guard.
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 56d004e. Configure here.
Review catch (greptile): the monotonic Phase-C publish loop returned an error on its
final iteration's retryable manifest conflict, even though that conflict can itself
mean a concurrent writer published a version that already includes our (content-
preserving) compaction — i.e. the postcondition ("the manifest reflects our
compaction") is already met. Recovery covered it (no data loss), but the operator
saw a spurious error and had to re-run.
Restructure the loop to re-read `current` on every retryable conflict and, on budget
exhaustion, do a final `current >= state.version` convergence check before surfacing
the error — the §6.6 "postcondition is the state, not winning the CAS" principle.
Factor the repeated current-version read into `current_manifest_version`.
ragnorc
added a commit
that referenced
this pull request
Jun 23, 2026
Folds in v0.7.1 (release #290 + optimize/write-path/internal-table-compaction fixes #288/#291/#297) under the MCP branch. Conflict resolutions (5 files): - crates/omnigraph-server/Cargo.toml: take main's 0.7.1 path-dep constraints; keep our omnigraph-mcp dep (bumped to 0.7.1) + http dep. - crates/omnigraph-server/src/handlers.rs: keep our server_list_queries doc-comment (exposed @mcp(expose) subset, invoke_query-gated) — it supersedes main's pre-@mcp(expose) text, since this branch adds the per-query expose flag. - docs/user/operations/server.md: keep our GET /queries description (invoke_query gate + @mcp(expose) exposure) over main's read-gated/list-all text. - docs/dev/index.md: keep both in-flight RFC rows; renumber this branch's tenancy RFC 013 -> 014 (rfc-014-tenancy-cells.md) since main now owns RFC-013 (rfc-013-write-path-latency.md). Title + index link updated; link-check green. - openapi.json: regenerated from merged source (OMNIGRAPH_UPDATE_OPENAPI=1) — now info.version 0.7.1 with our invoke_query/@mcp schema. Coherence: omnigraph-mcp bumped 0.7.0 -> 0.7.1 to match the workspace; Cargo.lock updated. cargo build --workspace green; server/mcp/api-types/compiler suites green (schema_routes.rs reopen-after-apply flakes under parallel IO on a near-full disk, passes single-threaded — a pre-existing main test, unchanged by the merge).
ragnorc
added a commit
that referenced
this pull request
Jun 23, 2026
Three findings this cycle share one root — the write base is a stale, un-probed, un-classified pin (the read path probes; the write path returns the warm coordinator snapshot): - #298 edge-@card stale-read regression (cursor High / codex P1, VALID): collapse #1 made the cardinality scan read txn.base instead of live HEAD, so a concurrent edge is uncounted and a max can be exceeded. Fix on #298: restore the live-HEAD read + deterministic test + correct the single-writer doc comment. - The structural liability underneath: no unified write-validation read-set — endpoint/cardinality/uniqueness each pick freshness ad hoc (warm/pinned/live), the same cardinality check forks mutation-vs-loader, none re-validated at commit. - The served-strict-write stale-view false-fail (validated on prod + a #[ignore] repro): a strict update/delete false-fails ExpectedVersionMismatch after an external optimize advance — the write-side mirror of #297/§6.6. The naive blanket probe is proven wrong (breaks the cross-process lost-update OCC contract). All three converge on Design A (step 5): open_txn's warm probe makes the base fresh, the op-class-aware precondition (derive maintenance vs logical from Lance per-version transaction metadata — no parallel marker) fast-forwards maintenance and fails logical, and §7.1's read-set-in-CAS unifies + re-validates the validation read-set. §8 records the #298 follow-up, the widened §7.1 scope, and the step-5 two-test acceptance contract.
ragnorc
added a commit
that referenced
this pull request
Jun 23, 2026
…e per write (#298) * docs(rfc-013): step-3b handoff + §4.1 corrections (validated) Add the RFC-013 write-path handoff doc, and correct §4.1's WriteTxn sketch from the 4-subagent validation against current code: - HandleCache → handle-threading (forward the commit-return handle; a version-keyed cache misses because HEAD walks N→N+1→N+2 across staging + index-build commits). - "re-resolution unrepresentable" softened to "pinned base for the pre-commit phase + named fresh re-reads at the commit/fork boundary" — three reads (commit-time OCC, the live-HEAD drift probe, fork authority) are irreducible correctness machinery. - WriteParams DOES carry a session field; the real constraint is "stage off an open Dataset," so attach the Session by opening read-style then staging off it. * test(engine): RED step-3b capture-once fitness asserts + open_count probe Two write-path cost gates, RED today, GREEN after the WriteTxn lands: - write_validates_schema_contract_once: a write must validate the schema contract once (3 read_text + 2 exists). Today re-validates at every resolve point — measured 12 read_text / 9 exists (~4 validations) via CountingStorageAdapter (zero production change; the write twin of the read-path schema-once test). - keyed_insert_opens_table_at_most_once: a keyed single-table write must open its table <=1x. Today measured 10 opens. Adds an exact open-CALL probe: open_count + record_open() on QueryIoProbes (mirroring probe_count/record_probe), called at both open chokepoints; surfaced as IoCounts.open_count. forbidden_apis guarantees every write open routes through them. * feat(engine): WriteTxn carrier + open_write_txn (3b scaffolding) The capture-once write transaction (RFC-013 step 3b): WriteTxn{branch, base: Snapshot, session} + Omnigraph::open_write_txn, which validates the schema contract once and pins the base snapshot + the shared per-graph Session. Landed as reviewed scaffolding (gated #[allow(dead_code)]); the next pass threads Option<&WriteTxn> through open_for_mutation_on_branch / staging on the non-strict bound-branch path — opening the base once from the pinned entry with the warm session (a session-aware pinned opener returning a SnapshotHandle) and skipping the per-table schema re-validation — to turn the two RED cost gates green. Strict ops / fork / the commit-time OCC re-read keep their fresh reads. * test(engine): scope write-path open_count to data tables (RFC-013 step 3b) The keyed_insert_opens_table_at_most_once gate asserted open_count <= 1, but open_count was a single unclassified counter: record_open() fires in both open chokepoints, and open_dataset_tracked also opens the internal/system tables (__manifest via layout.rs, _graph_commits/_graph_commit_actors via commit_graph.rs). So the count conflated data-table opens with the publisher CAS + commit-graph append opens — making the gate measure the wrong quantity and unreachable by threading alone (the manifest publish keeps it >1 regardless). Scope it by table class, mirroring the read-side counters (which already split by URI prefix via separate wrappers): record_open(uri) classifies the open's last path segment and feeds data_open_count vs internal_open_count. IoCounts exposes both; the gate now asserts data_open_count <= 1. Re-baselined: a single keyed insert is data_open_count=4 / internal_open_count=6 (sum 10, the old conflated value). The RED target for the WriteTxn threading is now the real data-table-open count (4 -> 1), with internal opens correctly out of scope. Pure test-harness/instrumentation; no production behavior change (classification runs only inside the probe closure, skipped when no probes are installed). Also marks #297 (optimize-vs-write race) as landed in the step-3b handoff — this branch is already stacked on origin/main after it merged. * feat(engine): validate the schema contract once per write (RFC-013 step 3b) A single mutate/load re-validated the schema contract ~4 times: at the entry (ensure_schema_state_valid), per-table in open_for_mutation_on_branch (resolved_branch_target), at the commit-time OCC re-read (fresh_snapshot_for_branch), and in the publisher's index-build snapshot (snapshot_for_branch). Each validation is 3 read_text + 2 exists on the storage adapter — O(touched resolve-points) of redundant contract I/O on every write. Thread the already-landed WriteTxn carrier through the write path: capture `txn = open_write_txn(branch)` once at the mutate/load entry (the single validation), then source the per-table entry and the commit/publish snapshots from `txn.base` instead of re-resolving. When `txn` is None (branch merge, schema apply, tests) every function is byte-identical to before. - mutate_with_current_actor / load_jsonl_reader capture txn once (replacing the entry-point ensure_schema_state_valid) and thread Some(&txn) through execute_*/open_table_for_mutation, commit_all, and commit_updates_on_branch_with_expected. - open_for_mutation_on_branch sources (snapshot, branch) from txn.base/txn.branch when present — skipping resolved_branch_target's re-validation. The OPEN itself is unchanged (still HEAD via open_dataset_head_for_write), and strict ops keep ensure_expected_version. Schema-once applies to strict and non-strict alike; the data-open collapse is a separate change. - commit_all uses fresh_snapshot_for_branch_unchecked (the OCC manifest re-read minus the schema re-validation) when txn is present; the drift guard is unchanged. - prepare_updates_for_commit uses txn.base for the publisher index-build snapshot. fresh_snapshot_for_branch{,_unchecked} now read the manifest directly via ManifestCoordinator instead of resolve_target. The OCC re-read consumes only the Snapshot (per-table location + version), which ManifestCoordinator::open().snapshot() produces identically — but resolve_target additionally opened the commit graph (a spurious _graph_commits.lance exists probe the OCC read never consults). Dropping that load is a pure read-cost reduction for every fresh-snapshot caller (commit_all's None arm, optimize, repair, fork reclaim); the returned Snapshot is unchanged and the read is a fresher cold manifest re-read, so the OCC freshness guarantee is preserved. Greens write_validates_schema_contract_once (3 read_text / 2 exists, was 12/9). keyed_insert_opens_table_at_most_once stays red (data_open_count=4) — the open collapse lands next. Full engine suite green otherwise. * feat(engine): open each data table once per write (RFC-013 step 3b) A single keyed-node mutate opened its data table 4 times: accumulation (to read .version()), staging (the real write base), the commit-time drift guard (to read live HEAD), and the publisher's index build (reopen at the just-committed version). Collapse three of the four — using the WriteTxn carrier threaded for schema-once — so a write opens each touched data table at most once. - #1 accumulation: open_for_mutation_on_branch now returns (Option<SnapshotHandle>, expected_version, full_path, table_branch). On the txn's own branch, a non-strict (Insert/Merge) op needs no open — the only thing the caller reads is .version() (the CAS fence), which is exactly the pinned base version (entry.table_version). So skip open_dataset_head_for_write and source the version from txn.base. The node insert path already discarded that handle; the edge path resolves a pinned read only when non-default cardinality needs it. STRICT ops and any write that must fork still open live HEAD + ensure_expected_version. - #3 commit drift guard: commit_all reads live HEAD via entry.dataset.dataset().latest_version_id() — a cheap manifest-pointer probe off the already-open staging handle (the same primitive ManifestCoordinator:: probe_latest_version uses) instead of a fresh open_dataset_head_for_write. The head<current / head>current drift classification is byte-identical. - #4 index build: commit_all now returns the per-table post-commit_staged SnapshotHandle map; commit_updates_on_branch_with_expected threads it into prepare_updates_for_commit, which builds indices on the threaded handle instead of reopening at the same just-committed version. Absent a handle (other writers, inline/delete tables) the reopen path is byte-identical. When txn is None (branch merge, schema apply, tests) every function opens and checks exactly as before. Greens keyed_insert_opens_table_at_most_once (data_open_count 4->1). Schema-once gate stays 3/2. Full engine suite + failpoints (recovery sidecar lifecycle) green. * refactor(engine): name the write-path open/commit returns (RFC-013 step 3b) The open collapse left two positional returns that are easy to mis-thread and carry an unwritten contract: open_for_mutation_on_branch's (Option<SnapshotHandle>, u64, String, Option<String>) and commit_all's 5-tuple (updates, expected_versions, sidecar_handle, guards, committed_handles). Replace both with named structs so each field reads at the call site and the Option's contract is documented, not folklore. - OpenedForMutation { handle, expected_version, full_path, table_branch } with a require_handle(ctx) helper for the callers that must have a handle (strict ops, the fork path, every no-txn caller — branch merge, the seed test). The handle is None only on the non-strict-txn open-skip path (collapse #1); require_handle panics with a named context if that contract is ever broken. - CommittedMutation { updates, expected_versions, sidecar_handle, guards, committed_handles } for commit_all; consumers destructure into the same local bindings they already used, so the publish/sidecar/guard-hold logic is unchanged. - A debug_assert in open_table_for_mutation pins the skip contract: a missing handle is legal only on the non-strict txn path, so a future strict arm returning None trips in debug builds instead of handing None to a require_handle consumer. Pure refactor — no behavior change. Both cost gates stay green (schema 3/2, data_open_count=1), full engine suite + lib (162) green. * refactor(engine): drop the unearned session field from WriteTxn (RFC-013 step 3b) The open collapse greens data_open_count<=1 by SKIPPING the accumulation open, PROBING live HEAD with latest_version_id, and REUSING the commit_staged handle — none of which consume a session. The captured WriteTxn.session was therefore dead (`#[allow(dead_code)]`): unearned surface a reviewer rightly flags. Remove it. The carrier is now {branch, base} — exactly what schema-once + the open collapse use. Step 5 (PublishPlan unification) makes WriteTxn the non-optional publish carrier and is the right home for session-aware base opens, where the warm-session benefit on the single remaining open — an object-store (S3) phenomenon, invisible on local FS — can be earned by its own cost gate rather than carried dead through this PR. No behavior change; both cost gates stay green (schema 3/2, data_open_count=1). * docs(rfc-013): mark step 3b DONE — schema-once + open-collapse shipped, session deferred to step 5 * docs(rfc-013): capture the write-base-staleness convergence (§1d) Three findings this cycle share one root — the write base is a stale, un-probed, un-classified pin (the read path probes; the write path returns the warm coordinator snapshot): - #298 edge-@card stale-read regression (cursor High / codex P1, VALID): collapse #1 made the cardinality scan read txn.base instead of live HEAD, so a concurrent edge is uncounted and a max can be exceeded. Fix on #298: restore the live-HEAD read + deterministic test + correct the single-writer doc comment. - The structural liability underneath: no unified write-validation read-set — endpoint/cardinality/uniqueness each pick freshness ad hoc (warm/pinned/live), the same cardinality check forks mutation-vs-loader, none re-validated at commit. - The served-strict-write stale-view false-fail (validated on prod + a #[ignore] repro): a strict update/delete false-fails ExpectedVersionMismatch after an external optimize advance — the write-side mirror of #297/§6.6. The naive blanket probe is proven wrong (breaks the cross-process lost-update OCC contract). All three converge on Design A (step 5): open_txn's warm probe makes the base fresh, the op-class-aware precondition (derive maintenance vs logical from Lance per-version transaction metadata — no parallel marker) fast-forwards maintenance and fails logical, and §7.1's read-set-in-CAS unifies + re-validates the validation read-set. §8 records the #298 follow-up, the widened §7.1 scope, and the step-5 two-test acceptance contract. * test(engine): RED — edge @card must scan live HEAD, not stale txn.base (#298) Regression guard for the cursor-High/codex-P1 finding on #298: 3b's collapse #1 made the non-strict edge-insert cardinality scan read the pinned txn.base instead of live HEAD (edge_cardinality_read_handle), so a concurrent edge committed after txn capture is uncounted and a @card max is silently exceeded (invariant 9). Deterministic two-handle test (no failpoint): handle A commits WorksAt(Alice->Acme) to the @card(0..1) max; stale handle B (never read since) inserts a second WorksAt for Alice. B's coordinator is stale by construction (the write path doesn't probe), so B scans txn.base (Alice has 0) and wrongly commits the 2nd edge. RED: the insert that must be rejected currently succeeds (panics at unwrap_err). Goes green when the scan reads live HEAD. * fix(engine): scan live HEAD for edge @card, not the pinned txn.base (#298) 3b's collapse #1 skips the non-strict edge accumulation open, so edge_cardinality_ read_handle reopened the edge table at the pinned txn.base for the @card scan. Since cardinality is validated once (never rechecked at commit), a concurrent edge committed after txn capture was uncounted and a @card max could be silently exceeded (invariant 9) — the cursor-High/codex-P1 regression on #298. Pre-3b the scan read live HEAD (the mutation's own open_dataset_head_for_write handle). Restore the live-HEAD read: take the table LOCATION from the pinned entry (stable across versions) and open the dataset at its current HEAD via open_dataset_head_for_ write. Gate-safe — the data_open_count / merge-insert-only gates are node inserts; the edge cardinality path (non-default @card only) is untouched by them, and the extra live-HEAD open is exactly the pre-3b shape. Also drops the dead None-fallback's schema re-validation (greptile P2, auto-resolved). The residual validate->commit TOCTOU is the pre-existing §7.1 gap (RFC-013 step 4), recorded in handoff §1d/§8. Turns cardinality_rejected_for_stale_handle_after_concurrent_edge_commit green; validators / write_cost / writes / consistency / end_to_end / branching all green. * docs(dev): link handoff docs from index * docs(engine): tighten 3b claims to match the code (#298 review) Review caught several comments/docs overclaiming what the code does (the session drop + the #298 cardinality fix left stale/too-strong wording). No logic change. - open_write_txn doc: drop the stale "shared per-graph Session" (WriteTxn no longer carries one); scope "once" to the table-touch hot path and note edge/load RI validation still re-resolves (→ step 4 §7.1) + the session-aware open is step 5. - edge cardinality call-site comment: it said the scan uses a "pinned txn.base" — it now opens LIVE HEAD (#298); corrected. - write_cost.rs: "opens the base once (with the shared Session)" → session-aware base open is deferred to step 5. - data_open_count completeness (instrumentation.rs + write_cost.rs): forbidden_apis only keeps engine code OUTSIDE the storage layer on the chokepoints; table_store.rs is allow-listed and holds direct Dataset::opens for branch-management ops (not the keyed-write hot path the gate measures). Narrowed the claim accordingly. - handoff §4: "schema once / open once" is the node hot path (the two gates); edge endpoint + loader RI/cardinality still re-validate and read warm — #298 un-regresses cardinality only, it does NOT close write-validation freshness (that's step 4 §1d/§7.1). build clean; write_cost / validators / forbidden_apis green.
ragnorc
added a commit
that referenced
this pull request
Jun 24, 2026
Reconciles the recovery roll-forward convergence fix + failpoint-harness hardening with main's #291 (compact internal manifest/commit-graph tables in optimize) and #297 (optimize survives a cross-process write race). The fix itself (recovery.rs convergence) merged cleanly; all conflicts were in the test-infra layer the two efforts both touched. Resolution: - optimize.rs / loader/mod.rs: took main's restructured Phase B/C and the new `commit_updates_on_branch_with_expected` signature, then re-applied the failpoint-name catalog conversion over the merged tree. - failpoints.rs: kept main's named serial groups (`#[serial(optimize)]`, `#[serial(branch_merge_phase_b)]`) at the conflict; the bare `#[serial]` / named-group mix runs clean across repeated full suite runs. - Catalog: added main's new failpoint names (`optimize.before_compact`, `optimize.inject_reindex_conflict`) and converted main's new literal call sites so `failpoint_names_guard` stays green. Verified: workspace build (no feature), engine failpoint suite 60/60 (x2), name guard, maintenance/recovery/consistency, cluster failpoints — all green.
Merged
ragnorc
added a commit
that referenced
this pull request
Jun 25, 2026
Patch release over v0.7.1: write-path latency reductions (#288 direct table opener, #298 schema-once + open-each-table-once) and three correctness fixes on the maintenance and recovery paths (#297 optimize survives a cross-process write race, #291 optimize compacts the internal metadata tables + non-destructive auto_cleanup strip, #296 recovery converges under a concurrent manifest advance). No breaking changes, no on-disk format change, no migration. Version coherence: all 7 crate manifests + path-dep constraints, Cargo.lock, openapi.json, and the AGENTS.md surveyed version bumped 0.7.1 -> 0.7.2. Build green under --locked; OpenAPI drift check green.
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
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.
What & why
A prod-discovered correctness bug (R2, while running #291's
optimize): a direct CLIoptimizeracing a served-server write on the same table fails, with two signatures:Rewrite … preempted by concurrent transaction Update at version N— optimize's Lancecompact_filescommit losing the OCC race.stale view of 'node:Media': expected manifest table version 14 but current is 15— optimize's manifest-publish equality CAS losing.Root cause (validated in code + Lance 7.0.0 source): the data-table optimize path (
optimize_one_table) trusted the in-process write queue and skipped a retry — but a CLI optimizer and the server are separate processes with separateWriteQueueManagers, so the queue serializes nothing between them. Lance cannot absorb the conflict either:commit_compactioncommits a fixedRewriteviaapply_commitdirect, there is no compactionRetryExecutor, and a semanticRetryableCommitConflictescapescommit_transaction's retry loop (io/commit.rs:979). So compaction fundamentally needs reopen + replan — which the internal-table path already did and the data path did not.This is the maintenance op-class face of the same process-boundary flaw as the logical-class corruption in RFC §6.5. It needs the opposite fix, because compaction commutes (content-preserving): not strict OCC + a lock, but Lance rebase + reopen/replan + a monotonic manifest fast-forward. No
writer_epoch.The fix
Unify both compaction paths on one reopen+replan shape, with a two-level retry matching the two failure points:
Rewrite-vs-Update/Deletesame-fragment overlap means our compaction did not commit — reopen at the new HEAD and re-plan. Lance rebases the common disjoint case (a concurrent insert/update/delete on other fragments) for free, so this fires only on real overlap.N, so we must NOT reopen (that would trip the HEAD>manifest drift guard on our own work). Re-read the current manifest versionC: ifC ≥ Nthe manifest already includes our compaction (versions are linear) → no-op; else fast-forward toN. Monotonic, never the strict equality CAS that manufactured the conflict.The
Optimizerecovery sidecar is written once and reused across attempts (every Phase-B commit is content-preserving, so recovery rolls the observed HEAD forward or safely rolls the compaction back). The in-process queue is kept — now an in-process contention reducer, not the cross-process guard. SharesCOMPACTION_RETRY_BUDGET+is_retryable_lance_conflictwith the internal path; addsis_retryable_manifest_conflictfor the publish loop.Design note (liability)
The lower-liability shape: the monotonic decision is kept local to
optimize.rsand reuses the existing equality-CAS publish unchanged — no new publisher mode, no signature ripple to load/mutate/recovery/repair (they keep strict equality). One engine file changed.This is the first correct instance of the maintenance-class commit model (RFC §6.6), not a parallel band-aid: when the planned step-5
PublishPlanunification lands, it relocates this (aTableAction::Rewritecarries the same commit model into the single publish authority) rather than reinventing it — the two regression tests are the contract that stays green across that refactor.Tests (test-first, red→green)
optimize_survives_concurrent_insert_advancing_manifestoptimize_survives_concurrent_delete_before_compactionBoth reproduce the prod manifest-CAS signature via a two-handle (cross-process-simulated) race + the
optimize.post_phase_b_pre_manifest_commit/ newoptimize.before_compactfailpoints. RED in the first commit, GREEN after the fix.Verification
cargo test --workspace --locked— green (65 suites, 0 failed).Out of scope (tracked separately)
node:Media drift_needs_repair(a narrower index-op recovery deferral).node:Artifact) — upstream Lance blob-v2 blocker.writer_epochfor the logical class — RFC step 5.Note
Medium Risk
Changes core optimize manifest publish and Lance retry behavior on a live maintenance path; regression tests and sidecar recovery mitigate, but incorrect drift classification could still strand HEAD/manifest gaps until repair.
Overview
Fixes a prod bug where CLI
optimizeand a served write in another process could fail on the same table: LanceRewriteconflicts or a manifest publish equality CAS (expected X but current Y) because the in-process write queue does not serialize across processes.optimize_one_tablenow uses a two-level retry: an outer reopen-and-replan loop on retryable Lance conflicts during Phase B (strip, compact, reindex, index build); an inner Phase C loop that monotonic fast-forwards the manifest (no-op whencurrent >=compacted version, else CAS against freshcurrent) instead of pinning to the pre-compaction version. Ahead_advancedflag keeps own Phase-B HEAD-ahead-of-manifest drift from being treated as external drift (sidecar retained, publish instead ofskipped_for_drift). The Optimize sidecar is written once and reused across reopen attempts.COMPACTION_RETRY_BUDGETis shared with the internal-table path;is_retryable_manifest_conflictandcurrent_manifest_versionsupport the publish loop.Tests add cross-process-simulated races (concurrent insert/delete before compact), own-HEAD drift on reindex retry, and
#[serial(optimize)]on related failpoint tests. RFC-013 §6.6 documents maintenance vs logical op-class commit models.Reviewed by Cursor Bugbot for commit e9d16a2. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
Fixes a prod correctness bug where
optimize(CLI) racing a served writer on the same table could fail with either a LanceRewrite-vs-UpdateOCC loss or a manifest equality-CAS mismatch — both caused by the in-process write queue not serialising across processes.optimize_one_tablegains a two-level retry: an outer reopen+replan loop for genuine LanceRewriteoverlap conflicts, and an inner Phase-C monotonic fast-forward replacing the strict equality-CAS publish (no-op when manifest≥ N, else CAS against freshly readcurrent). The recovery sidecar is written once and reused across attempts;head_advancedprevents the drift guard from misclassifying our own prior Phase-B commits as external drift.INTERNAL_COMPACTION_RETRY_BUDGET → COMPACTION_RETRY_BUDGETunifies the budget across both compaction paths; addsis_retryable_manifest_conflictandcurrent_manifest_versionhelpers.Confidence Score: 5/5
Safe to merge; the change is mechanically narrow (one engine file, one test file, one RFC doc), all three new tests reproduce the prod failure signatures with explicit RED→GREEN proof, and the previous two blocking review threads have been fully addressed in this head commit.
The root cause is accurately identified and closed by design: commutative maintenance ops now use a monotonic fast-forward instead of an equality CAS, so a concurrent writer that builds on the compaction causes a convergent no-op rather than a spurious error. The outer reopen+replan loop mirrors the already-correct internal-table path. The head_advanced flag correctly distinguishes our own prior Phase-B commits from external drift, preventing the sidecar from being prematurely deleted on retry. The post-budget convergence check (re-read current before surfacing an error) plugs the Phase-C exhaustion gap from the prior thread. Recovery semantics are preserved: the sidecar is written before any HEAD-advancing op and deleted after successful publish, and its loose-match classifier handles the stale expected_version across retries as documented.
No files require special attention; optimize.rs change is self-contained and the affected code paths are all exercised by the new failpoint tests.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant CLI as CLI optimize (separate process) participant SRV as Served writer (same table) participant Lance as Lance dataset participant Manifest as __manifest CLI->>CLI: acquire in-process write queue CLI->>Lance: "open_dataset_head_for_write (HEAD=V₀)" CLI->>Manifest: "fresh_snapshot → expected_version=V₀" CLI->>CLI: "Phase A — write sidecar (expected=V₀, head_advanced=false)" CLI->>CLI: failpoint: optimize.before_compact (pause) SRV->>Lance: "commit insert/delete → HEAD=V₀+1" SRV->>Manifest: publish V₀+1 CLI->>CLI: failpoint released CLI->>Lance: "compact_files (on ds@V₀, Lance rebases over V₀+1)" Lance-->>CLI: Ok(metrics) — HEAD now V₀+3 (reserve+rewrite) CLI->>CLI: "head_advanced=true" CLI->>Lance: optimize_indices, build_indices CLI->>Manifest: "Phase C: read current=V₀+1" Note over CLI,Manifest: current(V₀+1) < state.version(V₀+3) → fast-forward CLI->>Manifest: "CAS expected=V₀+1 → publish V₀+3" Manifest-->>CLI: Ok CLI->>CLI: Phase D — delete sidecar%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant CLI as CLI optimize (separate process) participant SRV as Served writer (same table) participant Lance as Lance dataset participant Manifest as __manifest CLI->>CLI: acquire in-process write queue CLI->>Lance: "open_dataset_head_for_write (HEAD=V₀)" CLI->>Manifest: "fresh_snapshot → expected_version=V₀" CLI->>CLI: "Phase A — write sidecar (expected=V₀, head_advanced=false)" CLI->>CLI: failpoint: optimize.before_compact (pause) SRV->>Lance: "commit insert/delete → HEAD=V₀+1" SRV->>Manifest: publish V₀+1 CLI->>CLI: failpoint released CLI->>Lance: "compact_files (on ds@V₀, Lance rebases over V₀+1)" Lance-->>CLI: Ok(metrics) — HEAD now V₀+3 (reserve+rewrite) CLI->>CLI: "head_advanced=true" CLI->>Lance: optimize_indices, build_indices CLI->>Manifest: "Phase C: read current=V₀+1" Note over CLI,Manifest: current(V₀+1) < state.version(V₀+3) → fast-forward CLI->>Manifest: "CAS expected=V₀+1 → publish V₀+3" Manifest-->>CLI: Ok CLI->>CLI: Phase D — delete sidecarReviews (3): Last reviewed commit: "fix(engine): optimize publish converges ..." | Re-trigger Greptile