Skip to content

fix(merge): preserve state when source table versions lag target - #630

Merged
azimafroozeh merged 3 commits into
mainfrom
codex/repro-untouched-table-merge
Sep 7, 2026
Merged

fix(merge): preserve state when source table versions lag target#630
azimafroozeh merged 3 commits into
mainfrom
codex/repro-untouched-table-merge

Conversation

@aaltshuler

@aaltshuler aaltshuler commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

What & why

A fast-forward merge can report success while retaining stale rows when the source table's native Lance version is lower than or equal to the target's. In the reproduced round trip, main has three edges at version 5 and feature has two at version 20. Merging main into feature, editing only a node, and merging back silently reduces main to two edges. A second regression returns age 47 after successfully merging age 50.

Native version counters belong to individual branches, but the current manifest projection selects the greatest registered version per table identity. Route these adoptions through the existing target-lineage delta writer; retain the complete target entry when row comparison proves the delta empty. Blob preflight uses the same routing rule, and existing recovery owns every required table effect before the single graph publication.

Backing issue / RFC

Checklist

  • Change is focused on adoption with equal/lower source table versions.
  • Tests cover edge loss, scalar rollback, equal/lower versions, owned/lazy targets, empty deltas, recovery, and changed-only Blob copying.
  • Public merge documentation, developer guidance, and release notes updated.
  • Reviewed against architectural invariants; reuses the existing publication/recovery protocol with no storage-format change.

Rebase and GQT verification — 2026-09-07

Rebased onto current main at 5f94a7419f574d05f87b9ee593a7102f6fe14491; #662 and #668 retain their stacked bases.

  • Added branch_round_trip_preserves_newer_values.gqt and branch_round_trip_preserves_untouched_edges.gqt, with observed red_on provenance. On the clean main runner they fail with age 47 instead of 50 and two edges instead of three, respectively.
  • Clean existing branch baseline: 2 passed. Freshly rebuilt fix(merge): preserve state when source table versions lag target #630 runner at 47cb4c02: all 50 GQT cases passed. Each new case completes in under 0.5 s locally.
  • Rust native-version, native-ref, recovery, Blob, and physical-head assertions remain in their existing owners.
  • Documentation and metadata checks passed; formatting and whitespace checks passed. Current-head CI passed the full workspace/failpoint test graph, AWS-server feature tests, both Clippy graphs, and the genuine V5/V6 format fence. The dedicated GQT and DST workflows also passed; DST ran 78 tests with 30 ignored instruments. Live RustFS/Azurite integration jobs were skipped by the PR workflow.

Reported GQT results use separately rebuilt and hashed runners for each stack revision; a stale shared-target attempt was discarded and the engine/runner rebuilt. Earlier verification below belongs to its original source revisions, not this rebase.

Original implementation verification (before this rebase)

Cargo commands used a shared CARGO_TARGET_DIR.

  • Before the fix: cargo test -p omnigraph-engine --locked --test branching branch_merge_ -- --nocapture — 20 passed, two new regressions failed as predicted. The clean owning baseline passed before test edits.
  • After the fix: branching 45 passed; merge fast-forward 20 passed; merge cost 2 passed; merge truth table 1 passed; merge failpoints 32 passed. The extended Blob and recovery cases also passed in the workspace run.
  • cargo test --workspace --locked --no-fail-fast --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints — 2,939 passed, 21 failed, 24 ignored on the initial run. Twenty CLI parity failures came from the harness looking in this worktree's target/debug while builds used the shared cache. After connecting the ignored target path, the complete same-binary parity rerun passed (21 tests). The remaining benchmark process-cleanup test passed on an isolated same-binary rerun; its worker does not invoke merge code.
  • cargo clippy --workspace --all-targets --locked -- -D warnings -W clippy::dbg_macro — passed.
  • cargo clippy --workspace --all-targets --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints -- -D warnings -W clippy::dbg_macro — passed.
  • cargo fmt --all --check, git diff --check, bash scripts/check-agents-md.sh, python3 scripts/check-docs.py, and python3 scripts/check-workflow-action-pins.py — passed.

Notes for reviewers

The new .gqt cases cover scalar rollback and untouched-edge preservation through branch round trips and reopen. Rust owners retain the native-version matrix, native pins, exact stored-edge identities, and recovery checks. Empty-delta assertions check both the retained table registration and unchanged physical HEAD.

The fresh-fork-after-insert control passes before this fix. This PR fixes the reproduced existing-branch round trip; it does not establish the exact cause of the original fresh-fork report. The indexed lazy-target cases use first-generation forks; the separate clone-of-clone index-path defect remains covered by second_generation_branch_index_reads_fail_upstream (Lance #7840).

@aaltshuler
aaltshuler force-pushed the codex/repro-untouched-table-merge branch from a8aa5ea to 47cb4c0 Compare September 7, 2026 12:49
@azimafroozeh
azimafroozeh merged commit ad425c6 into main Sep 7, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants