Repository navigation
Conversation
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.
f42f95a to
25f7eed
Compare
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.
WhyA 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: What
HowThe rewrite runs after every dataset open, after ImpactA 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. RecommendationReady. RFC #1234 at Optional
Tradeoffs and liabilityThe 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 ( 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.
A same-version publish persists the rebound ancestor URIs. That matches Lance Alignment
ValidationMerge base Local, on the merge base, with the head's retained-branch test appended to Failed. Local, on this head: Passed. 5 passed, 0 failed, including Exact-commit CI, run https://github.com/ModernRelay/omnigraph/actions/runs/41586453451, head
An earlier local run of the same integration filter on LimitationsI 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 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 ( Inline comment on
|
ragnorc
left a comment
There was a problem hiding this comment.
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_branchrecords 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 withNot 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, inlance_clone::create_branch, and in one checkout door. A guard test inforbidden_apis.rskeeps 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
- The branch conflicts with
main.git merge-treereports a content conflict incrates/omnigraph-catalog/src/lib.rs(main60bb6a8f). Rebase and rerun CI. The 37 moved checkout sites are where the next conflict will come from. - 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
branch_base.rscitesdocs/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.- The design depends on two Lance internals. It assigns the public
Dataset::manifestfield, and it relies onwith_object_storeto resetbase_object_stores. I checked this inlance-13.0.0/src/dataset.rs:2070-2082: the reset is there. Nothing documents it as a contract. The testa_base_store_resolved_before_the_rebind_is_not_read_throughis the right tripwire for a Lance bump. Please add the removal condition (lance#10661 ships relative bases) to the "Branch bases" fence row indocs/dev/lance.md, so the shim has a documented end. - 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.rsandmaintenance.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(thebranchingandmaintenancecases): 4 passed.cargo test -p omnigraph-engine --test forbidden_apis issue_1233: 1 passed.
- Before (same head,
follow_opened_rootmade a no-op, temporary edit, restored).branching::relocated_root_serves_retained_branches_from_its_own_files_issue_1233failed: "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_1233failed withNot foundunderoriginal.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 IntegrationandAzurite Azure Integration, which are green. - Exact-head CI is green, including
Test Workspace,Lint (clippy)andCI Gate. One duplicatePR Titlerun shows CANCELLED beside two successes. - I did not review performance, because the PR makes no performance claim.
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_branchrecords 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:Not found.What changed
The rule.
omnigraph_core::branch_base::follow_opened_rootrebinds every base whose recorded path ends in the suffix its own name selects tofind_branch(name)of the opened location:/<dir>for main (no name, ormain) and/<dir>/tree/<name>for a branch;<dir>is__manifestor a table's directory;/before the comparison, and on Windows\reads as/.Every other base refuses the dataset with a
Permanentstorage 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_outreturnsself.clone()), and cleared only by its privateset_manifest. The door therefore installs the rebound map on a fresh clone with an empty cache (Dataset::with_object_storeof the handle's own primary store), before any base resolves. It never rewrites the map of the handle that came in.One door.
lance_clone::create_branchreturns.branch_base::checkout/checkout_native_branch. The catalog'slayout::checkout_manifest/checkout_manifest_branchis that door for__manifest.forbidden_apis.rs::graph_dataset_checkouts_and_bases_have_one_door_issue_1233refuses a bare checkout and Lance's base-adding writers. It also refuses any production read ofbase_paths, or assignment to a handle'smanifest, outsidebranch_base.rs.One owner.
branch_baseis the only reader ofbase_paths. The collector's borrowed origins, the full-text certificate, the range memo, the merge key, and the instrumentation retention checks all ask it.manifest_has_external_base_paths,SnapshotDataset::has_external_base_paths, the benchmark'srelocation_self_contained) now reports only bases that are not graph-internal. A branched graph is relocation self-contained.manifest_pathstays withmetadata::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.mdgains a new section, "Copying a graph root".docs/dev/lance.mdgets the "Branch bases" fence row, which includes the cache rule.commands.mdandmigrations.md, andchangelog.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.
branching.rs::relocated_root_serves_retained_branches_from_its_own_files_issue_1233Not foundunder 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_throughshared-memory://bucket before the rebind, then scans.Not foundfrom the emptied source bucket.s3_storage.rs::s3_relocated_root_…_issue_1233andazure_relocated_root_…_issue_1233childbranch.Not foundreadingrelocation-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 1on a relocated root with a retained branch and a__manifesttag on an older commit.Not foundunder the original path.collector.rs::cleanup_keeps_legacy_native_origins_on_a_relocated_root_issue_1233__manifest, cleanup deletes a file the fork still borrows.lance_surface_guards.rs::stock_branch_base_records_the_absolute_source_uri_issue_1233branch_baserule testsmain-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.Local object-store runs, with the versions CI pins and two buckets or containers each:
s3_storage::(8),bound_storage, andlance_surface_guards(72) all pass.Checks
Run locally on the Modal box (gVisor, as root), using the repository's own selections with
RUST_MIN_STACK=16777216:cargo fmt --all --checkandcargo 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 in6bba8f48, and the engine, core and catalog lib suites were re-run green after it. The other 10 also fail onmainin this sandbox: eightomnigraph-benchtests (noCpus_allowed_list; symlink refusals),cli_cluster::managed_lifecycle_capture_and_retry_records_refuse_symlinks_without_reading_targets, anddb::upgrade::tests::interrupted::unreadable_legacy_object_asks_for_a_rerun(root reads a mode-0o000file).operator_transport::tests::http_connect_proxy_preserves_custom_ca_and_hides_origin_credentialsfailed in two of three isolated reruns, withConnectionReset(sandbox networking). It passed in the full run.dst_bench_cost_count_goldenpasses andcost_table.txtis unchanged, because the door does no I/O.scripts/check-docs.py: OK.Not covered
workflowscope, so theci.ymlchange is not in this PR. That change creates a second RustFS bucket and a second Azurite container, setsOMNIGRAPH_S3_TEST_RELOCATION_BUCKETandOMNIGRAPH_AZURE_TEST_RELOCATION_CONTAINER, and makes both tests required cells. Without it, CI runs only the S3 copy into another prefix, and printsrelocation into another bucket not run. A maintainer withworkflowscope needs to push it; the patch is ready.