fix(schema-apply): concurrent optimize must not false-fail a schema apply (iss-schema-apply-optimize-occ)

[ragnorc]

What

The follow-up correctness-by-design fix to the iss-848 index-reconciler work (#246). It instantiates the same governing principle — logical contract over physical state: a physical operation must never fail a logical one.

The bug

A periodic optimize (Lance compaction) running concurrently with a schema apply false-failed the apply. optimize advances a table's manifest version; schema apply captured its snapshot first, then a graph-global "write lease" check compared the whole manifest version against the pre-apply version and aborted (schema apply lost its write lease: main advanced from vN to vM …) whenever any table moved — even a table the apply never touched. Operators had to wrap cluster refresh + retry around apply.

The fix (mirror what optimize already does)

1. classify_plan_steps — derive the touched-table sets from the plan snapshot-free (pure schema_table_key), so the write queues can be acquired before the snapshot.
2. Acquire the touched-existing-table write queues (rewritten ∪ rename-source ∪ dropped) BEFORE the snapshot, then snapshot under the held queues — the same queue-before-read order optimize uses. No in-process optimize/mutate can move a touched table during the apply, so the per-table preconditions pass by construction.
3. Remove the graph-global write-lease check. A compaction of an untouched table is a disjoint manifest row the publisher rebases onto cleanly; it no longer fails the apply. A compaction of a touched table serializes behind it.
4. Publish with a per-table expected-version CAS (commit_changes_with_expected_with_actor). The held queues make this pass in-process; the CAS is the loud cross-process backstop (typed ExpectedVersionMismatch instead of a silent clobber).

Operations that don't conflict semantically no longer conflict mechanically.

Proof (deterministic failpoint tests)

Via the new schema_apply.after_queue_acquire_pre_snapshot / schema_apply.post_snapshot_pre_commit failpoints:

• schema_apply_succeeds_under_concurrent_write_to_untouched_table — lease-removal proof: an untouched table's version advancing mid-apply no longer false-fails it.
• concurrent_optimize_on_touched_table_serializes_behind_schema_apply — queue-ordering proof: optimize of a touched table blocks behind the apply, then runs over the post-apply layout.

Commits

1. fix(schema-apply) — engine (schema_apply.rs reorder + lease removal + CAS; graph_coordinator.rs CAS wrappers).
2. test(schema-apply) — the two failpoint proofs.
3. docs(schema-apply) — writes.md concurrency subsection, invariants.md truth-matrix row + narrowed gap, testing.md.

Notes

• Branched off current main (post feat(server)!: cluster-only server — remove single-graph serving (RFC-011) #250–252 RFC-011 config work); engine files untouched since Index materialization is derived state: defer off the write path, reconcile via optimize (iss-848) #246, so the design held against live code.
• Verified via CI (cold local cache; following the Index materialization is derived state: defer off the write path, reconcile via optimize (iss-848) #246 "let CI build" workflow). Full suite + failpoints job exercise the proofs.

---

Note

Medium Risk
Changes schema-apply serialization and manifest publish preconditions on a critical migration path; design mirrors optimize and adds typed per-table CAS, but cross-process recovery/staging races remain documented.

Overview
Fixes iss-schema-apply-optimize-occ: concurrent optimize could abort schema apply with a graph-global “write lease” error when any table’s manifest version moved, even tables the migration never touched.

Engine: classify_plan_steps derives touched-table sets from the plan without a snapshot. apply_schema_with_lock then acquires write queues for existing tables (rewritten ∪ rename-source ∪ dropped, plus the schema-apply serial key when a sidecar is written) before snapshot—matching optimize’s queue-before-read order. Hard-drop cleanup URIs are resolved after the snapshot. The pre-publish global manifest version lease check is removed; publish uses commit_changes_with_expected_with_actor with per-table expected versions (graph_coordinator adds the manifest CAS wrappers).

CI: RustFS S3 integration job timeout 75 → 120 minutes so cold runs can finish all suites and save cache (avoids a timeout loop).

Docs: writes.md, invariants.md, and testing.md document the concurrency model and why an in-process failpoint race test is largely unreachable while __schema_apply_lock__ blocks new writers.

^{Reviewed by Cursor Bugbot for commit 508f13d. Bugbot is set up for automated code reviews on this repo. Configure here.}

Greptile Summary

This PR fixes a false-fail in schema apply caused by a concurrent optimize advancing the global manifest version for a table the migration never touched. The root cause was the old graph-global "write lease" check comparing the whole manifest version before and after the apply — any manifest advance (even on an unrelated table) would abort the apply with a spurious conflict.

• classify_plan_steps is extracted to derive touched-table sets (added / renamed / rewritten / dropped) snapshot-free, enabling write-queue acquisition before the snapshot — the same queue-before-read order optimize already uses.
• The global write-lease check is removed and replaced with a per-table expected-version CAS (commit_changes_with_expected_with_actor) at publish time. Compaction of an untouched table now advances the global manifest version without failing the apply; compaction of a touched table serializes behind the apply via the held write queues.
• Two new pub(crate) helpers on GraphCoordinator mirror the existing commit_changes_with_actor pattern; a CI timeout bump (75 → 120 min) addresses a cold-cache timeout loop on the RustFS S3 job.

Confidence Score: 5/5

Safe to merge: the fix correctly narrows the concurrency conflict check from a graph-global manifest-version comparison to a per-table expected-version CAS, which removes the false-fail without introducing new clobber risk.

The queue-before-snapshot reordering, removal of the global write-lease check, and introduction of per-table CAS are all mechanically sound. The per-table queues prevent any in-process writer from moving a touched table between snapshot and publish, and the CAS is the correct loud backstop for cross-process movers. The two new GraphCoordinator helpers follow the established layering exactly. The writes_sidecar predicate is equivalent to the old guard. The acknowledged absence of a deterministic regression test is documented clearly and does not introduce any undetected defect path.

schema_apply.rs carries all the ordering and CAS logic and deserves the closest read; the GraphCoordinator additions are straightforward wrappers.

Important Files Changed

Filename	Overview
crates/omnigraph/src/db/omnigraph/schema_apply.rs	Core fix: extracts ClassifiedSteps (snapshot-free), acquires touched-table write queues before the snapshot, removes graph-global write-lease check, and switches to per-table expected-version CAS publish. Logic is sound; no regression test (documented as infeasible under the schema-apply lock).
crates/omnigraph/src/db/graph_coordinator.rs	Adds two pub(crate) methods — commit_manifest_changes_with_expected and commit_changes_with_expected_with_actor — mirroring the existing commit_manifest_changes / commit_changes_with_actor pair. Pattern is correct; failpoint hook preserved.
.github/workflows/ci.yml	RustFS S3 job timeout bumped 75 to 120 minutes with a detailed justification comment; addresses a cold-cache timeout loop.
docs/dev/writes.md	Adds subsection documenting queue-before-snapshot, per-table CAS, and removal of the global write-lease check; explains why in-process failpoint tests are not feasible under the schema-apply lock.
docs/dev/invariants.md	Adds Schema apply concurrency invariant row and narrows the multi-process OCC gap to the recovery-heal path.
docs/dev/testing.md	Documents that the bug-4 fix has no new in-process failpoint tests and explains why every spawnable concurrent writer is excluded by the schema-apply lock.

Comments Outside Diff (2)

1. crates/omnigraph/tests/failpoints.rs, line 662-674 (link)

   Timing-sensitive synchronization before optimize spawn

   The 300ms sleep is the only mechanism ensuring the apply task has actually acquired the node:Person write queue before the optimize task is spawned. If plan_schema_for_apply, validate_catalog, or queue acquisition take longer than 300ms (e.g., on a cold or loaded CI runner), the optimize task could acquire the queue first, run to completion, and make the !optimize.is_finished() assertion spuriously fail.

   The first test in this pair (schema_apply_succeeds_under_concurrent_write_to_untouched_table) uses a more robust polling loop — it waits for the __recovery sidecar file to appear, which is written just after the snapshot and immediately before the post_snapshot_pre_commit park. For this test the park is after_queue_acquire_pre_snapshot, which fires before the sidecar is written, so the same file-existence signal isn't available. A comment acknowledging this timing assumption and the acceptable flake risk — or a tokio::sync::Notify / atomic set inside the failpoint hook if the infrastructure supports it — would make the constraint explicit.

   

2. crates/omnigraph/tests/failpoints.rs, line 519 (link)

   Commit ordering inverts the AGENTS.md rule-12 test-first contract

   AGENTS.md rule 12 requires that for bug fixes the test commit lands before the fix commit ("the red → green pair is visible in git log"). The PR commits are ordered fix → test → docs, which means a reviewer checking out the test commit alone cannot reproduce the failure against the pre-fix engine. Reordering to test → fix → docs would make the regression proof self-contained in git history, as the rule intends.

   Context Used: CLAUDE.md (source)

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

   

_{Reviews (8): Last reviewed commit: "Merge branch 'main' into ragnorc/bug-4-s..." | Re-trigger Greptile}

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 7da260b. Configure here.}