feat(branching): add unattached graph branches

[azimafroozeh]

What & why

This PR adds unattached graph branches for benchmark workloads that write, inspect, extend and discard results without advancing a named branch.

• Create and extend: as unattached branch, optionally on $base or on "<id>", returns immutable snapshots containing cumulative changes.
• Inspect: query snapshots by id and discover live snapshots with commit list --unattached.
• Discard: dropping a snapshot removes it and its descendants; unattached snapshots cannot publish.

Backing issue / RFC

• Unattached graph branches: included as a draft; acceptance remains outstanding.

Checklist

• Focused: unattached snapshot creation, extension, inspection and lifecycle.
• Tests: 14 unattached GQT cases plus catalog, collector, compatibility, policy and CLI coverage; 4,336 workspace tests and 433 GQT tests passed.
• Public docs: branching, CLI, policy and upgrade guides updated.
• Release note: changelog.d/unattached-graph-branches.added.md.
• Invariants: invariant 5 extends its snapshot-addressability exception to unattached snapshots; the backing RFC remains draft.

Local verification

• cargo run -q -p omnigraph-vocabulary-guard -- check --surface rust-string --base 72b7d060752f20e537e8df94e158e59ce40543b8 --inventory tools/omnigraph-vocabulary-guard/graph-vocabulary-inventory.tsv --openapi openapi.json: red because the unchanged merge-base inventory is invalid; current-tree evaluation did not complete. This audit is disabled in CI.
• RFC named-branch baseline and comparative cost record: not run and remain outstanding. No performance claim is made.

Notes for reviewers

• Unattached writes require storage v16. Upgrade all binaries before stamping v14/v15 stores, either through an ordinary publication or offline omnigraph upgrade.
• CommitOutput gains kind and root_commit_id; clients infer legacy receipts' kind from published when kind is absent.
• Snapshots expire when their recorded branch head advances or its incarnation ends. Dropping snapshots removes addressability without promising immediate storage reclamation.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-12T12:17:46.253583Z	4b8590c	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[ragnorc]

I recommend request changes at 18b114ff6cecf114d9ffaf7f64305671ac91ddbf. Two local probes expose contract failures: cleanup can break an admitted read, and unrelated unattached snapshots add reads to proposal listing. The inline comments describe both causes and the required test coverage.

This PR serves a benchmark that writes, inspects, extends, then discards temporary results. Each effectful write creates immutable table versions and a detached graph manifest. A native tag makes that snapshot addressable. A child starts from its parent's complete snapshot. Named branches remain unchanged. The result cannot publish, and the chain expires when its recorded branch head or incarnation changes.

The design reuses detached writes, native tags, existing policy actions and control gates. It avoids another branch registry, mutable tip, mutation replay log or TTL service. Those are useful reductions in mechanism. However, the implementation adds a public write mode, another commit kind, a versioned record codec, ancestry rules, recursive drop, retention rules and storage stamp 16. Ordinary publication can activate that stamp, so the deployment obligation also reaches users who never select this mode. See the writer, lifecycle owner, and format fence.

The net long-term liability increases. That can be justified only by a workload that needs this separate operation. Five similar additions would multiply commit-kind dispatch, lifecycle tests and compatibility obligations across the same public doors. Keep shared snapshot retention and native-ref selection in their existing owners. A read-specific retry, an expiry delay, or another tag registry would leave these causes intact or add avoidable state.

The draft RFC's baseline and acceptance requirements remain unmet, as the PR body states. Before merge, record the named scratch-branch baseline and the complete unattached lifecycle cost. Include chain length, retained population, storage requests, latency, and cleanup. Then accept the observable contract before enabling its format. This is a separate acceptance gate, not a claim that this path is slower. No speed advantage is established.

I surveyed the other 30 open PRs and inspected the related proposals. The main connections are:

• #785 targets ordinary published-write throughput with group commit. Detached benchmark results cannot stand in for that workload.
• #822 protects a captured merge input while releasing its source gate. It reinforces the same requirement here: releasing coordination must preserve the captured data until its consumer finishes. #825 also seeks to remove work caused by unrelated branches. Global tag decoding works against that direction.
• #899 adds storage-completion tracking for shutdown. New tag writes and deletes must participate when these changes combine. #951 would extend authorization below the branch level. Every new snapshot door will need that shared policy boundary. These are integration obligations, not defects proved against unmerged combinations.
• #677 proposes immutable snapshots and explicit retention roots. Resolve the overlap during RFC acceptance. It should not create a second lifetime model by accident.

I checked the current upstream guidance and the actual unpatched Lance 11.0.0 source at ab6b5bbe46009ed78746b444df8db59a8bc5d842. In particular, Tags::fetch_tags/list opens every tag before returning. The same implementation remains in v14.0.0-beta.13. That candidate upgrade does not supply the missing name filter. Current Blob guidance also states that an open dataset handle does not protect a snapshot from cleanup. OmniGraph must own the graph-wide retention guarantee. Its existing single-writer-process boundary remains necessary.

Local validation used an isolated detached checkout. Before: d6604a19965e5529a127be8d0b0143dddeb0c791. Reviewed head: 18b114ff6cecf114d9ffaf7f64305671ac91ddbf. All test commands used:

export RUSTUP_TOOLCHAIN=1.97.1
export CARGO_TARGET_DIR=/tmp/review-928/target
export RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable'

Command	Local result
cargo test -p omnigraph-gqt --locked --test gq_logic_tests unattached_branch	14 passed, zero failed. The cases also ran their declared DST seeds, 0 and 42.
cargo test -p omnigraph-engine --locked --features failpoints --test failpoints unattached_	Clean baseline: 4 passed, zero failed.
cargo test -p omnigraph-engine --locked --features failpoints --test failpoints review_unattached_ -- --nocapture	With temporary tests only: 2 failed for the predicted behaviors below. Production source stayed at the reviewed head.
cargo fmt --all --check and git diff --check	Passed after restoring the temporary tests.

An initial GQT command used --lib and selected zero tests. I excluded it from behavioral evidence and ran the correct target above. The initial tag probe used a wrapper that missed direct local-file reads. I replaced it with the existing ProbedStores accounting before reporting costs.

The final probes found:

1. A one-row Document snapshot was live when its blob read passed capture_read_view. The test parked at BLOB_READ_POST_CAPTURE. It then advanced main and ran cleanup with one retained version and age zero. The admitted read failed with HistoricalVersionReclaimed. The same failure occurred with an explicit snapshot drop before the main write. The existing age helper made detached manifests eligible before admission. This models an old, still-live snapshot, not a read paused for seven days.
2. With zero proposals, adding 0, 1, 2, 3 and 4 unrelated unattached roots increased proposal-list manifest read operations to 2, 3, 4, 5 and 6. The assertion required the count to remain at 2. These are local storage-operation counts, not cloud latency measurements.

Please include these regressions with the fixes and prove that the same tests pass after correction. The existing Rust failpoint owner is appropriate: these checks need an admitted blob-read pause, physical manifest age, and Lance I/O counters. GQT cannot express those checks. This PR introduces a feature, so the parent commit lacks the syntax and APIs. I did not count a parent compile/parser failure as a before/after proof. No fixed successor exists in this review, so the new findings have failing-head evidence only. All temporary edits were restored, and no test changes were pushed.

Exact-head CI provides broader evidence. CI passed workspace tests, storage compatibility, Clippy, formatting, AWS-server, RustFS and Azurite jobs. GQT CI passed, including its served and DST jobs. The overall CI gate is red because Dependency Guard reports unsafe-libyaml / RUSTSEC-2024-0449. This PR does not change Cargo dependencies or the lockfile. I did not rerun cloud tests or benchmark end-to-end latency locally.

There are no separate optional style findings.

Temporary regression probes, appended to the existing failpoints.rs owner

These reuse its existing fixtures and helpers. The second probe counts direct local reads as well as object-store reads.

// Temporary review probes: no production source changes.
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
#[serial]
async fn review_unattached_admitted_blob_read_survives_drop_and_cleanup() {
    let _scenario = FailScenario::setup();
    let mut outcomes = Vec::new();
    for drop_first in [false, true] {
        let dir = tempfile::tempdir().unwrap();
        let db = Arc::new(seed_blob_write_graph(dir.path().to_str().unwrap()).await);
        let query = r#"query set_note($title: String, $note: String) as unattached branch {
        update Document set { note: $note } where title = $title
    }"#;
        let root = db
            .mutate_with_receipt(
                "main",
                query,
                "set_note",
                &params(&[("$title", "readme"), ("$note", "root")]),
            )
            .await
            .unwrap()
            .commit
            .unwrap();
        let table_uri = node_table_uri(&db, "Document").await;
        age_detached_manifests(&table_uri);
        let rendezvous =
            helpers::failpoint::Rendezvous::park_first(&catalog::BLOB_READ_POST_CAPTURE);
        let reader = {
            let db = Arc::clone(&db);
            let id = root.graph_commit_id.clone();
            tokio::spawn(async move {
                db.read_blob_at(ReadTarget::snapshot(SnapshotId::new(id)), readme("content"))
                    .await
            })
        };
        rendezvous.wait_until_reached().await;
        if drop_first {
            db.drop_commit_as(&root.graph_commit_id, None)
                .await
                .unwrap();
        }
        set_readme_note(&db, "after").await;
        let rows = db.cleanup(keep_one_at_any_age()).await.unwrap();
        assert!(rows.iter().all(|row| row.error.is_none()), "{rows:?}");
        rendezvous.release();
        let result = reader.await.unwrap();
        eprintln!("drop_first={drop_first}: {result:?}");
        outcomes.push(result.is_ok());
    }
    assert_eq!(
        outcomes,
        vec![true, true],
        "admitted reads survive expiry and drop plus cleanup"
    );
}

#[tokio::test]
#[serial]
async fn review_unattached_population_does_not_add_proposal_tag_reads() {
    let wrapper = lance_io::utils::tracking_store::IOTracker::default();
    let probes = QueryIoProbes {
        manifest_wrapper: Some(Arc::new(wrapper.clone())),
        ..Default::default()
    };
    let stores = probes.manifest_stores.clone();
    with_query_io_probes(probes, async {
        let dir = tempfile::tempdir().unwrap();
        let db = init_and_load(&dir).await;
        let mut observed = Vec::new();
        for n in 0..=4 {
            if n > 0 {
                db.mutate_with_receipt(
                    "main",
                    UNATTACHED_ROOT_QUERY,
                    "insert_root",
                    &person_params(&format!("root-{n}"), n, None),
                )
                .await
                .unwrap()
                .commit
                .unwrap();
            }
            helpers::cost::drain_probed_io(&wrapper, &stores);
            assert!(db.list_unpublished_commits(None).await.unwrap().is_empty());
            let io = helpers::cost::drain_probed_io(&wrapper, &stores);
            eprintln!(
                "population={n}, manifest_read_iops={}, bytes={}",
                io.read_iops, io.read_bytes
            );
            observed.push(io.read_iops);
        }
        assert_eq!(
            observed,
            vec![observed[0]; 5],
            "proposal listing must not read unattached tag contents"
        );
    })
    .await;
}

[ragnorc]

I still recommend request changes at abea5634fc47fca2f142e5e33de311055c8c0071. The YAML migration fixes the dependency gate. The new tag-selection change fixes the earlier P2 finding. The admitted-read retention defect remains. The latest commit also repairs the native-access guard counts. I link the existing inline finding below to avoid a duplicate thread.

This PR lets a benchmark write, inspect, extend and discard temporary graph results. Each write creates an immutable snapshot through detached table versions, a detached graph manifest and a native tag. A child includes its parent's changes. These snapshots never publish and expire when their recorded branch authority changes. This provides a separate operation from named scratch branches, with new lifetime and format obligations.

Commit e196eeb6 replaces serde_yaml and unsafe-libyaml with yaml_serde 0.10.7 and libyaml-rs 0.3.0. It changes consumers across configuration, policy, CLI and GQT, and extends the existing YAML tests. I checked the full delta and the exact dependency source. The scanner retains its stable allocation, input lifetime, token checks and cleanup ownership. The upstream yaml_serde migration guidance describes compatible APIs. libyaml-rs remains an unsafe translation of libyaml. This removes reliance on abandoned packages without adding a format, adapter or parallel parser.

It retains the existing unsafe-code maintenance obligation. I found no new blocking defect in this migration.

The remaining runtime blocker is P1: admitted reads lose their table versions.

The read boundary still releases its permit after capture. The collector still excludes expired snapshots without protecting active consumers. #958 correctly tracks the wider cause. A separate prerequisite fix is reasonable, but an issue does not satisfy invariant 3 or the unchanged admitted-read promise.

Protect the captured table pins until each consumer finishes, including consumption of returned Blob readers. Apply that rule at the shared read/collector boundary, covering expiry, explicit drop, ordinary queries, blobs and historical/proposal reads. A delay or read-specific retry leaves the cause intact.

The P2 tag-read finding is fixed. The new native-tag helpers enumerate names before reading selected contents. Lookup, listing and descendant traversal share that mechanism. NativeTags caches selected reads and missing tags for one operation. Its sealed mode uses only the collector's captured inventory, so ancestry checks cannot mix that inventory with newer tags.

This adds transient cache state and two helper APIs, but no durable registry. It removes unrelated content reads and their failure paths at the owning layer. Directory listing and sorting still grow with the total tag population.

I checked Lance 11.0.0's read_dir, tag paths and TagContents::from_path against these calls. Lance's full tag listing opens all tag contents. v14.0.0-beta.13 keeps that behavior. The explicit name selection therefore remains relevant to that possible upgrade. The single-writer-process boundary remains unchanged.

My unchanged engine probes confirm both outcomes at the final head. With zero proposals, adding 0–4 unrelated unattached roots now leaves proposal-list reads at 2, 2, 2, 2, 2. Before the fix they were 2, 3, 4, 5, 6. These are storage-operation counts, not cloud latency measurements.

The reader probe still fails with HistoricalVersionReclaimed for both expiry and explicit drop. It admits and pauses a one-row Blob read at BLOB_READ_POST_CAPTURE, advances main, runs cleanup, then resumes. Detached manifests are made eligible for cleanup before admission. An optional drop before the main write gives the second case. Neither case preserves the admitted read.

The final commit also aligns the native-access registry with the refactor. Its strict guard now passes locally.

The feature still adds long-term software liability. Reusing detached writes and native refs avoids a second persistent authority. However, a new commit kind, ancestry rules, recursive drop and storage stamp 16 add lasting compatibility and lifecycle work. Five similar additions would multiply dispatch and retention cases across the same doors. The draft RFC still requires acceptance and a measured named-branch baseline. Ordinary publication can activate stamp 16, so the rollout obligation also reaches users who never choose this mode. No speed advantage is established.

I refreshed the survey of all other open PRs. The relevant direction remains shared lifetime protection (#822), less unrelated metadata work (#825), and explicit snapshot roots (#677). Group commit (#785) targets ordinary published writes, whose costs this benchmark cannot replace. Shutdown tracking (#899) and actor views (#951) remain integration obligations. #957 changes query projection semantics and does not resolve the retention defect. These are code/design assessments, not tests of unmerged combinations.

Local checks used an isolated checkout, Rust 1.97.1, CARGO_TARGET_DIR=/tmp/review-928/target, and RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable'. Before the tag fix: e196eeb6aee04661dd2f5b874699212750266c2d. After: abea5634fc47fca2f142e5e33de311055c8c0071.

Command	Local result
cargo test -p omnigraph-catalog --locked --lib proposal_readers_never_open_an_unattached_tag	Before-fix clean baseline: 1 passed.
cargo test -p omnigraph-catalog --locked --lib proposal_reader_reads_are_independent_of_the_unattached_population -- --nocapture	Before: 1 test failed for the predicted extra read. Only the new test was backported.
cargo test -p omnigraph-catalog --locked --lib unattached	After: all 8 passed, including that same regression.
cargo test -p omnigraph-engine --locked --features failpoints --test failpoints unattached_	Clean final-head baseline: 4 passed.
cargo test -p omnigraph-engine --locked --features failpoints --test failpoints review_unattached_ -- --nocapture	Before: both probes failed. Final head: tag-read probe passed, admitted-read probe failed. Temporary tests only.
cargo test -p omnigraph-gqt --locked --test gq_logic_tests unattached_branch	14 passed, including the declared DST seeds 0 and 42.
cargo test -p omnigraph-engine --locked --test forbidden_apis graph_visible_write_chokepoints_are_registered -- --exact	1 passed at the final head.
cargo fmt --all --check	Passed at the final head.

The checked-in regression uses the catalog test owner and measures actual manifest-store reads, including Lance's direct local reads. GQT cannot assert that mechanism. I copied only that test into the before-fix checkout, after its existing focused baseline passed. Before the fix, one unrelated root increased listing reads from 2 to 3, so the test failed. At the reviewed head, the same regression passed as part of all eight focused catalog tests. This is a behavioral red/green result.

The temporary reader-retention probe is unchanged from the previous review. Please include it with the shared fix and demonstrate a behavioral pass. It needs an admitted blob-read pause and physical manifest aging, which GQT cannot express. It failed at 18b114ff6cecf114d9ffaf7f64305671ac91ddbf, e196eeb6aee04661dd2f5b874699212750266c2d, and the reviewed head. There is no corrected successor for retention here, so that finding still lacks a completed red/green proof.

Exact-head CI and GQT CI passed. This includes workspace tests, storage compatibility, the dependency guard, Clippy, formatting, cloud emulator jobs, served GQT and DST. Earlier failures at intermediate commits are not the final head's status.

Locally, cargo deny --locked check could not run because cargo-deny is not installed. I use CI evidence for that gate. I did not rerun cloud tests or measure end-to-end latency locally. The eight YAML/runner-configuration tests also passed locally at e196eeb6; their code is unchanged at the final head.

All temporary source/test edits were restored. The checkout is clean, and no changes were pushed.

There are no separate optional findings.