Skip to content

fix(core): graph-internal branch bases follow the opened root - #1235

Open
ragnorc wants to merge 6 commits into
ModernRelay:mainfrom
ragnorc:fix/1233-branch-base-follows-root
Open

ragnorc wants to merge 6 commits into
ModernRelay:mainfrom
ragnorc:fix/1233-branch-base-follows-root

Conversation

@ragnorc

@ragnorc ragnorc commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

This PR waits for the relocated graph roots RFC (#1234) to be accepted. It stays a draft until then. Fixes #1233. Upstream: lance-format/lance#10661.

Why

Lance 13 create_branch records the source branch's absolute URI as a dataset-root base. Inherited data, deletion, and index files take their path from that map, and their store from a cache built from the same URI. A whole-root copy opened at a new path therefore read a retained branch's inherited files at the path the branch was created at:

  • When that path was gone, the read failed with Not found.
  • When the original still existed, the copy silently read its files.

What changed

The rule. omnigraph_core::branch_base::follow_opened_root rebinds every base whose recorded path ends in the suffix its own name selects to find_branch(name) of the opened location:

  • the suffix is /<dir> for main (no name, or main) and /<dir>/tree/<name> for a branch;
  • <dir> is __manifest or a table's directory;
  • a recorded path loses its query and trailing / before the comparison, and on Windows \ reads as /.

Every other base refuses the dataset with a Permanent storage failure: a mismatched name, a data-only base, or any outside path. Persisted bytes, e-tags, branch identity, and the storage format are unchanged.

The base-store cache. Lance keeps a base's store in base_object_stores. That cache is built on first use, shared by clones (already_checked_out returns self.clone()), and cleared only by its private set_manifest. The door therefore installs the rebound map on a fresh clone with an empty cache (Dataset::with_object_store of the handle's own primary store), before any base resolves. It never rewrites the map of the handle that came in.

One door.

  • The rule runs in the instrumented opener and on the handle lance_clone::create_branch returns.
  • It also runs in one checkout, branch_base::checkout / checkout_native_branch. The catalog's layout::checkout_manifest / checkout_manifest_branch is that door for __manifest.
  • All 37 bare production checkouts now go through it.
  • forbidden_apis.rs::graph_dataset_checkouts_and_bases_have_one_door_issue_1233 refuses a bare checkout and Lance's base-adding writers. It also refuses any production read of base_paths, or assignment to a handle's manifest, outside branch_base.rs.

One owner.

  • branch_base is the only reader of base_paths. The collector's borrowed origins, the full-text certificate, the range memo, the merge key, and the instrumentation retention checks all ask it.
  • The relocation bit (manifest_has_external_base_paths, SnapshotDataset::has_external_base_paths, the benchmark's relocation_self_contained) now reports only bases that are not graph-internal. A branched graph is relocation self-contained.
  • A table pin's manifest_path stays with metadata::full_manifest_object_store_path, and the docs name that split.

Tables and the collector. The rule applies to table datasets as well, so pre-v11 table forks follow the copy.

Docs.

  • docs/user/concepts/storage.md gains a new section, "Copying a graph root".
  • The upgrade rollback text says to restore at the original path for 0.14.0 and earlier.
  • Maintenance and troubleshooting cover the two refusals; a refresh does not bring back a missing copied object.
  • docs/dev/lance.md gets the "Branch bases" fence row, which includes the cache rule.
  • Skill text in commands.md and migrations.md, and changelog.d/relocated-graph-roots.fixed.md.

Evidence

Each test except the stock-Lance guard was also run with the rule, or the fresh cache, removed, and failed.

Test Covers Without the fix
branching.rs::relocated_root_serves_retained_branches_from_its_own_files_issue_1233 The issue's acceptance list on a copy with a divergent original, the original removed, a same-path restore, writes and branch creation on the copy, and the relocation bit false for every branch. A copy missing one inherited file fails and names a path under the copy. Not found under the original path. The no-fallback case returns 4 rows from the original.
branch_base::tests::a_base_store_resolved_before_the_rebind_is_not_read_through A stock handle resolves its base store in another shared-memory:// bucket before the rebind, then scans. With the map rewritten on the incoming handle (the reviewed behavior), Not found from the emptied source bucket.
s3_storage.rs::s3_relocated_root_…_issue_1233 and azure_relocated_root_…_issue_1233 Another prefix, then another bucket or container. Each copy reads, writes, and cleans up, and includes the same stale-store step on the never-written child branch. On RustFS without the fresh cache: Not found reading relocation-bucket/…/__manifest/tree/written.<id>/data/… from the old bucket.
maintenance.rs::cleanup_on_a_relocated_root_keeps_retained_branches_and_tagged_snapshots_issue_1233 --keep 1 on a relocated root with a retained branch and a __manifest tag on an older commit. Not found under the original path.
collector.rs::cleanup_keeps_legacy_native_origins_on_a_relocated_root_issue_1233 A pre-v11 table fork through cleanup and an indexed read. With the rule limited to __manifest, cleanup deletes a file the fork still borrows.
lance_surface_guards.rs::stock_branch_base_records_the_absolute_source_uri_issue_1233 Pins the stock behavior; goes red at the Lance bump that changes it. Not applicable.
branch_base rule tests Main, a main-named main, a branch, a nested branch name, a table fork, idempotence, six refused shapes, and a checkout on a copy whose original data is gone. Not applicable.

Local object-store runs, with the versions CI pins and two buckets or containers each:

  • RustFS 1.0.0-beta.12: s3_storage:: (8), bound_storage, and lance_surface_guards (72) all pass.
  • Azurite 3.35.0: the Azure relocation test passes.

Checks

Run locally on the Modal box (gVisor, as root), using the repository's own selections with RUST_MIN_STACK=16777216:

  • cargo fmt --all --check and cargo dev-clippy: clean.
  • cargo dev-test --no-fail-fast: 5,108 passed and 11 failed. One was mine, fts_compat_tests::certificate_file_is_bounded_and_follows_shallow_clone_ownership: the certificate path had started classifying bases. It is fixed in 6bba8f48, and the engine, core and catalog lib suites were re-run green after it. The other 10 also fail on main in this sandbox: eight omnigraph-bench tests (no Cpus_allowed_list; symlink refusals), cli_cluster::managed_lifecycle_capture_and_retry_records_refuse_symlinks_without_reading_targets, and db::upgrade::tests::interrupted::unreadable_legacy_object_asks_for_a_rerun (root reads a mode-0o000 file).
  • operator_transport::tests::http_connect_proxy_preserves_custom_ca_and_hides_origin_credentials failed in two of three isolated reruns, with ConnectionReset (sandbox networking). It passed in the full run.
  • The GQT corpus: 750 passed. DST: 112 passed; dst_bench_cost_count_golden passes and cost_table.txt is unchanged, because the door does no I/O.
  • scripts/check-docs.py: OK.

Not covered

  • CI wiring for the other-bucket and Azurite halves. The push token has no workflow scope, so the ci.yml change is not in this PR. That change creates a second RustFS bucket and a second Azurite container, sets OMNIGRAPH_S3_TEST_RELOCATION_BUCKET and OMNIGRAPH_AZURE_TEST_RELOCATION_CONTAINER, and makes both tests required cells. Without it, CI runs only the S3 copy into another prefix, and prints relocation into another bucket not run. A maintainer with workflow scope needs to push it; the patch is ready.
  • Handles returned by a Lance commit. These are not passed through the door again. A publish from a handle at a stale read version writes the recorded bases (as the RFC states), and Omnigraph reads after a publish through the door's checkout.
  • Live Azure stays under RFC 0029's preview.
  • A cluster-managed root at a new location is adoption and stays unsupported. External Blob URIs are unchanged.
  • Older binaries (0.14.0 and earlier) still fail on a relocated root, and the docs say so.

Lance 13 create_branch records the source branch's absolute URI as a
dataset-root base, so a whole-root copy opened elsewhere read a branch's
inherited files at the location the branch was created at, or silently
from the original root when it still existed (issue ModernRelay#1233).

branch_base rebinds, in the handle's in-memory manifest, every base whose
recorded path ends in the suffix its own name selects to find_branch(name)
of the opened location, and refuses every other base. It runs in the one
dataset opener, in lance_clone::create_branch, and in the one checkout
(branch_base::checkout; layout::checkout_manifest for __manifest), which
replaces every bare Lance checkout in engine, core and catalog. It applies
to table datasets too, so pre-v11 table forks follow the copy. Persisted
bytes and the storage format are unchanged.
- branching: the issue's acceptance list on a copied root, no fallback to
  a still-present original, writes and branch creation on the copy
- maintenance: cleanup on a relocated root with a retained branch and a
  tagged snapshot
- collector: a pre-v11 table fork on a relocated root keeps its borrowed
  files through cleanup
- s3_storage: copies into another prefix, then into another bucket
  (RustFS) or container (Azurite) when the second one is configured
- lance_surface_guards: stock Lance records the absolute source URI
- forbidden_apis: one checkout door, no Lance base-adding writer
Document that a whole-root copy opens at a new location on the same kind
of storage, the two refusals, rollback at the original path for older
binaries, and the Lance branch-base fence.
@ragnorc
ragnorc force-pushed the fix/1233-branch-base-follows-root branch from f42f95a to 25f7eed Compare November 3, 2026 04:32
@ragnorc
ragnorc marked this pull request as ready for review November 3, 2026 05:02
ragnorc added a commit to ragnorc/omnigraph that referenced this pull request Nov 3, 2026
State the base-store cache limit and that the door installs the rebound
map on a fresh, unshared cache before any base resolves; the storage
upgrade overwrites rather than clones; what a same-version publish, a
stale-handle publish and a new branch persist; the layout page's
shallow-clone port step; one owner each for table pin paths and Lance
bases, the relocation bit included. Also the builder's I/O, the NotFound
class and message path, separator normalization, the main-name residual,
and links to lance#10661 and draft ModernRelay#1235.
Lance 13 keeps a base's store in base_object_stores, built from the
base's URI on first use, shared by clones (an already_checked_out
checkout returns self.clone()) and cleared only by its private
set_manifest. Replacing the public manifest Arc left a store opened at
the recorded URI in place, so a handle could read the rebound path
through the old bucket or container.

follow_opened_root now installs the rebound map on a fresh clone with
an empty cache (Dataset::with_object_store of the handle's own primary
store) before any base resolves, and is async. branch_base becomes the
only reader of base_paths: the collector, the full-text certificate,
the range memo, the merge key and the instrumentation retention checks
ask it, and the relocation bit (manifest_has_external_base_paths,
SnapshotDataset::has_external_base_paths) reports only bases that are
not graph-internal.
- branch_base: a stock handle that resolved its base store in another
  shared-memory bucket reads the copy after the rebind (fails without
  the fresh cache)
- s3_storage journey: the same on RustFS and Azurite with the never
  written child branch, whose rows are all inherited
- branching: the relocation bit is false for every branch of a
  relocated root
- forbidden_apis: only branch_base reads base_paths or replaces a
  handle's manifest
The fence row states the cache limit and the fresh-cache install, names
branch_base as the only reader of base_paths and full_manifest_object_
store_path as the owner of table pin paths, and links lance#10661.
Troubleshooting says a refresh does not bring back a missing copied
object.

ragnorc commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Why

A retained branch stores the absolute path from the time Lance created it. A copy of the whole root then reads inherited files at that path. When the original root is still present, a command to the copy can return rows from the original. When the original path is gone, the read fails. That breaks the opened root as the one location a read uses (invariants 3 and 12). The class is every dataset Lance shallow-cloned: __manifest branches, and table forks from before storage format v11.

What

branch_base rewrites each graph-internal base to the location the opened root implies. The rewrite is in the handle's memory. Persisted bytes, e-tags, and branch identity stay the same. Every other base is refused. The new map is installed on a new handle whose base-store cache is empty.

How

The rewrite runs after every dataset open, after lance_clone::create_branch, and after the one checkout (branch_base::checkout, and layout::checkout_manifest for __manifest). A base is graph-internal when it is a dataset root and its recorded path ends with the suffix its name selects. has_external_bases is that same test. The relocation bit and the collector call it. forbidden_apis allows only this module to read base_paths or to replace Dataset::manifest.

Impact

A whole-root backup can stand alone on the same kind of storage. It serves every branch, tag, and retained snapshot, and it reads and writes only under the opened root. Omnigraph 0.14.0 and earlier still use the recorded path. A publish whose handle holds the read version stores the opened URIs for every base. A new branch stores the opened URI for its direct source and keeps ancestor URIs from disk. The reader rule covers both. A move between local, S3, and Azure stays export and import. A cluster still does not adopt a root at a new location.

Recommendation

Ready.

RFC #1234 at 4da4c206 is still a draft. This head implements that revised design: the fresh cache, one classifier for the relocation bit, and the fence text for the cache. The pull request says it waits for the RFC to be accepted. I see no code defect that should send the implementation back.

Optional

  1. The other bucket and the other container do not run in CI. On this head, RustFS job 125753017986 ran s3_relocated_root_serves_every_branch_from_its_own_objects_issue_1233 and it passed. The log then says relocation into another bucket not run. The job's skip check does not match that line, and the test is not a required cell. The Azure test lives in the same engine module. The RustFS job skips it because OMNIGRAPH_AZURE_TEST_CONTAINER is unset, and the Azurite job does not select it. The cross-authority case is covered locally by a_base_store_resolved_before_the_rebind_is_not_read_through (shared-memory buckets). Wire the second bucket and the Azurite owner when the workflow can change. The user text already names those locations.

  2. The shipped skill omits the old-binary limit. docs/user/concepts/storage.md and docs/user/operations/upgrade.md say Omnigraph 0.14.0 and earlier read some branch files at the create-time path. skills/omnigraph/references/migrations.md and commands.md say a whole-root copy opens at a new path, and they stop there. The skill ships with the binary. Add the same old-binary sentence.

Tradeoffs and liability

The stored URI was a second, frozen copy of a fact the opened location already holds. This change removes that second location for reads, including the base-store cache. Lance 13.0.0 builds that store from the base URI on first use and keeps it (dataset.rs base_object_store). Only the private set_manifest clears it, and only when base_paths change. Replacing the public manifest Arc leaves the old store in place. On S3 or Azure that store is the old bucket or container. follow_opened_root calls Dataset::with_object_store on the handle's own primary store, then installs the new map. store_params stay with the clone. A handle whose bases already name the opened root is returned as it is.

The new liability is a reader rule for one Lance shape, a fail-closed refusal, and one door on every open and checkout. The rule stays until a Lance release records a same-dataset location as relative. The fence row says to drop the rewrite for new bytes at that bump and to keep it for existing bytes. An on-disk rewrite of historical versions was set aside. Those versions would still need this reader, and new manifest versions would change e-tags and branch identity. That maintenance is the right cost. A format stamp would not repair the bytes that already exist.

branch_base is the right owner. A table pin's manifest_path stays with full_manifest_object_store_path. Those are two location facts and two owners. The relocation bit is not a third one: it asks has_external_bases. has_bases stays the separate question for callers that must stay on the primary store (the control image and the small-manifest reader). Five more absolute locations should join one of those two owners, not grow a new boolean each.

A same-version publish persists the rebound ancestor URIs. That matches Lance new_from_previous when the handle holds the read version. The storage upgrade does not clone. publish_conversion overwrites, and the predecessor bases come forward. The reader rule covers them. Storage Upgrade Compatibility and the V5 ↔ V10 format fence passed on this head.

Alignment

Validation

Merge base 36d3ea08c5289b40653a26752cd0e6c8ef85e6af. Head 04605cccc3e685b1b487f87c6ee357c779e1c21c. GQT cannot copy a root. The owner is integration/branching.rs.

Local, on the merge base, with the head's retained-branch test appended to branching.rs and then restored:

git rev-parse HEAD
# 36d3ea08c5289b40653a26752cd0e6c8ef85e6af
CARGO_TARGET_DIR=/agent/repos/omnigraph/target \
  cargo test -p omnigraph-engine --test integration \
  relocated_root_serves_retained_branches_from_its_own_files_issue_1233 \
  --locked -- --test-threads=1 --nocapture

Failed. person_rows_at on the retained branch of a copy that was missing an inherited file returned Ok(4) while the original root still held that file. Panic at branching.rs:5515: a read must not fall back to the create-time path: 4. Exit 101. One test ran.

Local, on this head:

CARGO_TARGET_DIR=/agent/repos/omnigraph/target \
  cargo test -p omnigraph-core --lib branch_base:: --locked -- --test-threads=8 --nocapture

Passed. 5 passed, 0 failed, including a_base_store_resolved_before_the_rebind_is_not_read_through. Exit 0.

Exact-commit CI, run https://github.com/ModernRelay/omnigraph/actions/runs/41586453451, head 04605ccc:

  • Test Workspace passed. branching::relocated_root_serves_retained_branches_from_its_own_files_issue_1233 PASS (0.347s). maintenance::cleanup_on_a_relocated_root_keeps_retained_branches_and_tagged_snapshots_issue_1233 PASS (0.189s).
  • RustFS S3 Integration (default) passed. The prefix relocation test passed. The other bucket did not run.
  • Storage Upgrade Compatibility passed. V5 ↔ V10 Format Fence passed.
  • Lint, rustfmt, GQT, and the DST pinned suite passed.

An earlier local run of the same integration filter on 25f7eedb also passed the file and cleanup owners. That commit is an ancestor of this head. The relocation-bit assertion was added after it. The Test Workspace result above is the pass of the current test.

Limitations

I did not run S3, Azure, or the predecessor CLIs locally. The cloud and predecessor results above are CI on this head. The other-bucket and other-container halves did not run there. I did not re-run the full workspace, Clippy, GQT, or DST locally. I did not measure performance. The local integration binary for this head was still compiling when I posted. The named test's pass is the Test Workspace result above.

Inline comment on crates/omnigraph/tests/integration/s3_storage.rs line 425:

The prefix copy runs in the RustFS job and passed on this head. This branch only prints, and the job's skip check does not match the line. The test is not a required cell. The Azure test in this file is skipped in that same job (OMNIGRAPH_AZURE_TEST_CONTAINER is unset) and the Azurite job does not select it. branch_base::a_base_store_resolved_before_the_rebind_is_not_read_through covers two authorities locally. A second bucket and a second container are still the locations the user text names, and CI does not run them. Wire those owners when the workflow can change.

Inline comment on skills/omnigraph/references/migrations.md line 77:

docs/user/concepts/storage.md and docs/user/operations/upgrade.md say Omnigraph 0.14.0 and earlier read some branch files at the create-time path. This sentence says a whole-root copy opens at a new path, and it stops there. commands.md has the same sentence. The skill ships with the binary. Add the old-binary limit here.

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of head 04605ccc (base 36d3ea08). I found no blocking defect in the code. Two process points block the merge (below).

Why, what, how, impact

  • Why. Lance 13 create_branch records the source branch's absolute URI as a base. A copy of a graph root, opened at a new path, then reads inherited files at the old path. It fails with Not found, or it silently reads the original.
  • What. A new module, branch_base, rebinds every graph-internal base to the opened root. It refuses any other base. It runs in the opener, in lance_clone::create_branch, and in one checkout door. A guard test in forbidden_apis.rs keeps it the only door.
  • Impact. A whole-root copy serves every branch from its own files. A dataset with a foreign base now fails to open. This follows invariant 8 (loud integrity failures).

Blocking

  1. The branch conflicts with main. git merge-tree reports a content conflict in crates/omnigraph-catalog/src/lib.rs (main 60bb6a8f). Rebase and rerun CI. The 37 moved checkout sites are where the next conflict will come from.
  2. The PR text says it waits for the RFC in #1234. That RFC is not accepted. The PR is not marked draft. Keep it unmerged until the RFC is accepted, or mark it draft.

Optional

  1. branch_base.rs cites docs/rfcs/2026-11-03-relocated-graph-roots.md. That file is not in this branch (it is in #1234), so the link dangles until the RFC lands.
  2. The design depends on two Lance internals. It assigns the public Dataset::manifest field, and it relies on with_object_store to reset base_object_stores. I checked this in lance-13.0.0/src/dataset.rs:2070-2082: the reset is there. Nothing documents it as a contract. The test a_base_store_resolved_before_the_rebind_is_not_read_through is the right tripwire for a Lance bump. Please add the removal condition (lance#10661 ships relative bases) to the "Branch bases" fence row in docs/dev/lance.md, so the shim has a documented end.
  3. The new refusal makes a dataset with a foreign base unopenable. The guard test shows that no writer adds such a base. State in the upgrade notes that a graph whose tables were forked by a tool outside Omnigraph is refused.

Tradeoffs and liability

  • Removed: every other reader of base_paths (the collector, the full-text certificate, the range memo, the merge key). The relocation bit now has one definition.
  • Added: one module, one guard test, and a dependence on Lance's cache behavior. After five similar changes, the guard keeps one owner. A sixth Lance-derived cache would be a silent gap, which the stale-store test would only catch if it covers that cache.
  • I judge this a net liability reduction. The alternative, a fix in Lance, is tracked in lance#10661. Treat this PR as a bridge.

Validation (local, Linux, 2 cores)

  • GQT cannot express a whole-root copy, so the cheapest existing owner is integration/branching.rs and maintenance.rs. That choice is right.
  • After (head 04605ccc).
    • cargo test -p omnigraph-core --lib branch_base: 5 passed.
    • cargo test -p omnigraph-engine --test integration relocated_root (the branching and maintenance cases): 4 passed.
    • cargo test -p omnigraph-engine --test forbidden_apis issue_1233: 1 passed.
  • Before (same head, follow_opened_root made a no-op, temporary edit, restored).
    • branching::relocated_root_serves_retained_branches_from_its_own_files_issue_1233 failed: "a read must not fall back to the create-time path: 4". It read 4 rows from the original.
    • maintenance::cleanup_on_a_relocated_root_keeps_retained_branches_and_tagged_snapshots_issue_1233 failed with Not found under original.omni/__manifest/data/….
    • Both are the predicted behavior, not a compile or setup failure.
  • Limits.
    • I did not run the "before" test on the base commit itself, because the tests do not exist there. The no-op edit stands in for it.
    • The S3 and Azure tests passed in 0.5 s locally, so they probably did not reach a store. I rely on the exact-head CI jobs RustFS S3 Integration and Azurite Azure Integration, which are green.
    • Exact-head CI is green, including Test Workspace, Lint (clippy) and CI Gate. One duplicate PR Title run shows CANCELLED beside two successes.
    • I did not review performance, because the PR makes no performance claim.

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.

bug: retained branch reads use the original path after whole-root relocation (0.14.0)

1 participant