[ragnorc]

Fixes #1236

What changed

After a stacked branch's parent is fast-forward merged and deleted, the merge base of the child and main is a commit written by the deleted branch. branch diff, two-point diff, commit changes, query --snapshot and the change feed refused it with not_found (ensure_incarnation_live), while branch merge opened it through pinned_graph_commit_with_session.

Those readers now share one door (GraphCoordinator::settled_commit_state, and the feed's PollSnapshots::of):

• A commit of the live incarnation takes the existing path, unchanged.
• On a live miss, the commit opens only if a live branch (main first) still reaches it through parents or merged parents, and the retired native ref matches the exact commit (ensure_retired_incarnation_opens, built on pinned_graph_commit_with_session). A recreated branch name cannot satisfy the exact match.
• A commit no live branch retains is ReadTargetNotFound for the id the caller gave, not branch 'x' not found. A recreated name keeps its incarnation-witness error.
• A reclaimed manifest or table pin stays the existing loud gap. The gap message now says commit instead of merge base.
• The change feed takes the retired path only while the polled branch is itself still live, so a poll that races the deletion of its own branch still fails with the incarnation refusal. The post-open head reproof skips snapshots opened as retired incarnations, since their native ref name is unique and immutable.
• branch create --from-commit of such a commit stays branch_deleted.

docs/user/branching/changes.md and a changelog note are updated.

Not changed

The server's commit changes route still authorizes read on the branch that wrote the commit. A policy that denies the deleted name answers 404 even when a readable live branch retains the commit. Branch diff already authorizes its two live sides.

Testing

• New .gqt case issue_1236_stacked_diff_after_parent_delete (branch diff, two-point diff, read at the commit; engine, DST and served). It failed on main at the post-delete branch diff.
• New engine test changes::stacked_branch_reads_survive_its_parents_delete_issue_1236 (commit changes, feed from the beginning and after the commit, snapshot read, recreated name, unretained commit). It failed on main with BranchNotFound { branch: "a" }.
• changes::history_reads_are_the_same_after_a_merged_branch_is_deleted_and_recreated pinned the old refusal for a commit main merged; it now asserts the commit opens, and that an unmerged head is not found.
• Passing: the full GQT corpus, engine changes::, branching:: and merge_ integration tests, the engine failpoints suite (including the feed ABA cases), the catalog tests, engine lib tests, cargo dev-clippy and cargo fmt --all --check. These ran with RUST_MIN_STACK=16777216, because branching::branch_delete_retires_native_parent_and_cleanup_preserves_live_child overflows the default stack on main in this sandbox.

[ragnorc]

Revise. The reported stacked-diff case is fixed. Two readers of the same commit still use the old refusal.

Why

Issue #1236 is one broken rule. Merge could open a commit after the branch that wrote it was deleted. Diff, commit changes, a snapshot read, and the change feed could not, while a live branch still reached that commit.

What

GraphCoordinator::settled_commit_state is the right door for a settled commit. A live writer takes the existing path. A retired writer opens only when a live branch reaches the commit through parents or merged parents, and pinned_graph_commit_with_session still opens that exact commit. A recreated logical name cannot pass that match. GraphCommit equality includes graph_commit_id, and a new incarnation has a new native ref. branch create --from-commit still returns branch_deleted.

Blocking

1. The change feed of a live child with no commit of its own fails.

PollSnapshots::of treats "the head commit's writer is live" as "the polled branch is live". Those differ when the child was created from the parent and has not committed. The head row still names the parent. After the parent is fast-forward merged and deleted, changes poll --branch b from the beginning returns BranchNotFound { branch: "a" }. The child is live, and main retains the commit. A snapshot of that commit on a main handle succeeds.

This is the same check the pull request describes as the race fence. It sees a deletion when the polled branch wrote the head. A head inherited from a writer that is still live does not show that the polled branch was deleted. Compare a fresh hash of the polled branch's Lance identifier with cut.witness. That token already records the polled branch.

2. commit_state still refuses its captured head with ensure_incarnation_live.

The fast path runs before settled_commit_state. It runs only when exact_graph_head() is the requested id. That key is set only when the head row's native ref is this coordinator's native ref, so a handle synced to the child does not take it. A handle that captured the parent before the delete does. On that handle, snapshot_of and commit_changes_page of the retained commit return BranchNotFound { branch: "a" }. A new handle on main reads the same commit.

Send this path through the same decision. When the writer is retired, open the retired ref by exact commit and mark the snapshot retired. The captured head is retention only while this branch is still that incarnation.

Add both shapes to changes::stacked_branch_reads_survive_its_parents_delete_issue_1236. The .gqt case cannot express the feed or a second handle. The current engine test gives the child its own commit, so the head writer is the live child.

Optional

• On the retired feed path, a native ref is stored in retired and not in live. The guard is !self.live.contains(native), so every later commit of that native repeats the head check and ensure_retired_incarnation_opens. A poll walks 128 commits by default and 512 at the ceiling. Skip a native already in retired. The live path already caches one check per native, and it does not walk lineage.
• settled_commit_state maps Ok(()) from the second ensure_incarnation_live to ReadTargetNotFound. A retired native ref cannot become the live ref of a recreated name, because a new incarnation mints a new ULID. An Ok from that call would report a live commit as missing.
• The unretained-commit assertion accepts Manifest, BranchNotFound, or ReadTargetNotFound. The display check rejects a message that contains 'gone'. Pin ReadTargetNotFound of the caller's commit id.
• HTTP commit changes still authorizes read on the writer, as this pull request says. With a per-branch policy that does not grant the deleted name, that route stays 404 while the engine can read the commit. The new user-guide sentence says the commit is readable. Leave the authorization in this change, and keep the guide aligned with that limit.

Liability

This removes a split contract. The class is every read of a commit whose writing incarnation is gone, while a live branch's parent or merged-parent chain still reaches that commit id. The fast-forward stacked diff is one shape. A child with no local commit, and a handle still bound to the deleted writer, are the same class.

The new pieces are a boolean on Snapshot, two catalog predicates, and a lineage walk that runs only after a live miss. Five more fixes of this kind would add more writer-liveness checks beside the real question: does the caller's live branch still retain this commit, and does the retired ref still open that exact commit? One function should answer that. commit_state and PollSnapshots::of should both call it. pinned_graph_commit_with_session already owns the exact open. No new identity state is needed.

Live-commit cost, from the code and not from a benchmark: a main commit returns before any manifest read. A live named branch pays the same native-ref resolve that ensure_incarnation_live paid before. The lineage walk runs only after that resolve says the writer is gone. On a miss of an unretained commit, the walk opens every live branch.

Other open pull requests

#1235 and #1184 touch the catalog and the coordinator, and they auto-merge with this head. Recheck both if either lands first. #1184 changes lookup of a commit held on a branch. #1235 changes branch bases. #1241 shares no files with this change. #1204, #1229, and #1175 conflict with current main in files this pull request does not edit.

Validation

Merge base ca11eb7a2020a7e08124cfe6371d0607e6049067. Head 9c4c32f4efdd7fe3dc27397b937979cbdbdb9863. RUST_MIN_STACK=16777216.

On the merge base, with the new .gqt case copied in and then removed:

cargo test -p omnigraph-gqt --test gq_logic_tests issue_1236_stacked_diff -- --test-threads=1 --nocapture

Failed. Step 8 is the branch diff after branch delete a. It returned '<commit id>' names no branch and no commit on the local engine and on DST seeds 0 and 42, including each replay. The diff before the delete had passed. This command does not run the served environment. That environment needs --server.

On the merge base, with the new integration test appended and the file restored afterward:

cargo test -p omnigraph-engine --test integration stacked_branch_reads_survive_its_parents_delete_issue_1236 -- --test-threads=1 --nocapture

Failed at commit_changes_page of the parent's commit with BranchNotFound { branch: "a" }.

At this head, the same two commands passed. GQT: 1 passed, 2.28s. Integration: 1 passed, 0.24s.

A temporary probe at this head, removed afterward, created b from a with no write on b, fast-forward merged a, and deleted a.

• poll_change_feed of b from the beginning returned BranchNotFound { branch: "a" }.
• snapshot_of of that commit on the main handle returned Ok.
• snapshot_of and commit_changes_page on a handle synced to b after the delete returned Ok.
• Both calls on a handle synced to a before the delete returned BranchNotFound { branch: "a" }.

CI on this exact head is green, including GQT ordinary, DST, and served, Test Workspace, and clippy. Devin Review did not review the diff.

Limitations

I did not measure the live-path or retired-path cost. I did not run a concurrent delete during a poll. I did not run the served GQT target locally. Lance 13.0.0 Dataset::delete_branch removes the ref and the branch tree. This change depends on logical delete continuing to archive the ref and leave the tree. That retirement behavior is unchanged here.

Inline: crates/omnigraph/src/changes/feed.rs:525

ensure_incarnation_live here checks the head commit's writer. It does not check the polled branch.

I ran a child b created from a with no commit on b, then fast-forward merged a into main and deleted a. poll_change_feed of b from the beginning returned BranchNotFound { branch: "a" }. b is live. A snapshot of that same commit on a main handle succeeded.

Use cut.witness. Re-resolve the polled branch and compare a fresh identifier hash with the witness captured for this cut. That fails when this branch was deleted or recreated. It passes when the branch is the same incarnation and its head was written by another branch.

A retired native is inserted into retired only. The guard above is !self.live.contains(native), so every later commit of that native repeats the head check and ensure_retired_incarnation_opens. Skip a native already in retired. The default poll walks 128 commits, and the ceiling is 512.

Inline: crates/omnigraph/src/db/graph_coordinator.rs:764

This comment is still the old rule, and the fast path under it still applies that rule.

exact_graph_head() equals the requested id only when the head row's native ref is this coordinator's native ref. A handle synced to a child that inherited the commit does not take this path. Snapshot and commit changes succeed on that handle.

A handle that captured the parent before the delete does take it. After the fast-forward merge and the delete, snapshot_of and commit_changes_page of the retained commit on that handle returned BranchNotFound { branch: "a" }. The same snapshot on a new main handle returned Ok.

When the writer is retired, call settled_commit_state, or open the retired ref by exact commit and mark the snapshot retired. The captured head is retention only while this branch is still that incarnation.

[ragnorc]

Rebased on current main; no conflicts. Changes per point:

1. Feed of a live child with no commit of its own: PollSnapshots::of no longer checks the head writer. Retention is now the polled branch, accepted while a fresh hash of its Lance identifier equals cut.witness. A deleted or recreated polled branch still fails with the incarnation refusal. The engine test polls a child kid created from a with no commit.
2. commit_state fast path: the captured head goes through the same decision and opens the retired ref by exact commit, with the snapshot marked retired. The engine test uses a handle synced to a before the delete, for snapshot_of and commit_changes_page.

One function, admit_commit, makes the decision for both readers. It takes a Retention of either any live branch or the polled branch with its witness.

Optional points:

• The feed skips natives already in retired.
• An Ok from the second ensure_incarnation_live now means the writer is live and the commit reads normally; it is no longer mapped to not found.
• The unretained assertion pins ReadTargetNotFound of the caller's commit id.
• Read authorization is unchanged. The user guide now says the engine reads such a commit and the HTTP commit changes route still needs read on the writing branch (404 under a policy that does not grant the deleted name).

#1235 and #1184 had not landed when I rebased. Neither has been rechecked against this change.

[ragnorc]

Ready. The two blocking findings from the review of 9c4c32f4 are fixed at one door. I found no new blocking defect on 753bd9ba.

Why

Issue #1236 is one broken rule. Merge could open a commit after the branch that wrote it was deleted. Diff, commit changes, a snapshot read, and the change feed could not. A live branch still reached that commit.

What

admit_commit is that door for both readers. Ok(false) means the writing incarnation is live, and the existing path runs. Ok(true) means the writer is retired, retention holds, and pinned_graph_commit_with_session opens that exact commit. The caller marks the snapshot retired. A miss is ReadTargetNotFound of the caller's spelling, or the incarnation-witness error when the name was created again.

Retention::LiveBranches is the rule for commit_state. Retention::PolledBranch is the rule for the feed. The witness is a fresh hash of the polled branch's Lance identifier, compared with cut.witness. branch create --from-commit does not call this function. It still returns branch_deleted.

Previous findings

1. The feed of a live child with no commit of its own is fixed. PollSnapshots::of admits the head commit with the polled branch's witness. The engine test creates kid from a with no write, fast-forward merges a, and deletes a. The feed of kid from the beginning contains Una's insert.

2. The commit_state fast path is fixed. exact_graph_head() goes through admit_commit. A retired writer opens by exact commit. The engine test uses a handle synced to a before the delete. snapshot_of and commit_changes_page of that commit succeed.

The four optional points from that review are in this head. The feed skips a native already in retired. Ok from the second ensure_incarnation_live returns Ok(false). The unretained assertion requires ReadTargetNotFound of the caller's commit id. HTTP commit changes still authorizes read on the writing branch. The user guide states that limit. The skill text never stated the old refusal, so it needs no new sentence.

Where the reply is wider than the code

The reply says a deleted or recreated polled branch still fails with the incarnation refusal. The witness compare runs only after incarnation_is_live is false. A page whose commits were all written by incarnations that are still live does not read the witness. A fresh poll of a deleted branch fails at open with BranchNotFound. An in-flight poll that already holds a cut does not. When the writer is retired and the polled name is gone, the error is ReadTargetNotFound of the commit id. When the name was created again, the second ensure_incarnation_live returns the incarnation-witness error, and that error propagates. This is optional. The inline note on admit_commit has the change.

A BranchIdentifier collision is not a practical hole. Lance 13.0.0 BranchIdentifier::new appends a new UUID, and branch_witness hashes the JSON of that value. A recreated name fails the witness compare. pinned_graph_commit_with_session then requires full GraphCommit equality, including graph_commit_id.

Optional

1. The polled-branch witness compare is skipped when the writer is still live. See the inline note on admit_commit.

2. The feed marks the cut's live head snapshot retired when the head commit's writer native is retired. reprove_named_branch_heads then skips that snapshot. For a child with no commit, both sides of that head compare are absent, so this run does not show a wrong page. The flag comment says the snapshot was opened through the retired ref. The cut's head snapshot was opened on the live child. See the inline note.

3. Two comments state the old rule. The fast-path comment says the captured head is retention only while this branch is still that incarnation. The call uses Retention::LiveBranches, and the stale-handle test shows the read succeeds. diff_endpoint_snapshot still says a commit whose writing branch is gone is no longer readable. The function now reads it through admit_commit.

Liability

This removes a split contract. The class is every read of a settled commit whose writing incarnation is gone, while the caller's retention still reaches that commit id. Merge already opened that commit. Diff, commit changes, a snapshot read, and the feed now use the same decision.

The new pieces are one function, a Retention argument, a boolean on Snapshot, and a lineage walk that runs only after a live miss. Five more fixes of this kind should add a caller or a Retention arm. They should not add another liveness check beside this door.

The live-writer path returns before any manifest walk. The unretained miss opens every live branch, main first. That cost is the right spend. The miss is the refusal path, and it fails closed. A hole in lineage returns not found rather than a partial page. CommitGraph::lineage already errors when a required parent is absent.

The async function was large enough to break dependents. On adcd0590, boxing only the walk left clippy, Test Workspace, Storage Upgrade, and Azurite red. Rustc reported query depth increased by 130 for offline_fixture in omnigraph-cluster test failpoints, and for omnigraph-bench. Those crates use the default recursion limit. The engine crate already sets 256. 753bd9ba boxes admit_commit at the three call sites. Those four jobs pass on this head. The boxes are the maintenance cost of the new future. The next await inside admit_commit can push a dependent over the limit again.

This change spends efficiency on the unretained miss so the live path and the single door stay simple. That is the right spend for this contract.

Other open pull requests

#1184 merges with current main and conflicts with this head in crates/omnigraph/src/db/graph_coordinator.rs. Its fast path still calls ensure_incarnation_live. Whichever lands first must keep admit_commit as the only door, including the hinted-branch lookup #1184 adds. #1235 and #1241 merge cleanly with this head. #1204, #1175, #1229, and #1243 conflict with current main in files this change does not edit. The same files conflict against this head.

Validation

Merge base 61de2535f045dc7c8a6dcd35c83c60b016541c72. Head 753bd9ba5a2d5d239e64a5927511b4dfc3574b63. RUST_MIN_STACK=16777216. The head moved from adcd0590 during this review. That last commit only boxes the decision at its callers. The local tests ran on 753bd9ba.

At the head:

cargo test -p omnigraph-engine --test integration -- stacked_branch_reads_survive_its_parents_delete_issue_1236 history_reads_are_the_same_after_a_merged_branch --test-threads=1 --nocapture

2 passed, 0.97s.

cargo test -p omnigraph-gqt --test gq_logic_tests issue_1236_stacked_diff -- --test-threads=1 --nocapture

1 passed, 2.46s. DST seeds 0 and 42 replayed. This command does not run the served environment.

On the merge base, production sources unchanged, with the head's changes.rs test edits and the new .gqt case copied in, then the worktree removed:

The engine command above, plus a temporary probe probe_kid_and_stale_handle_issue_1236 that was not part of the pull request. 3 failed, 0.59s.

• history_reads_are_the_same_after_a_merged_branch_is_deleted_and_recreated panicked at snapshot_of of the merged head with BranchNotFound { branch: "feature" }.
• stacked_branch_reads_survive_its_parents_delete_issue_1236 panicked at the first commit_changes_page with BranchNotFound { branch: "a" }. That unwrap is before the child and the stale handle.
• The probe reached both shapes. The feed of kid from the beginning, snapshot_of of a1 on the handle synced to a before the delete, and commit_changes_page of a1 on that handle each returned BranchNotFound { branch: "a" }.

The same GQT command on that base failed in 2.23s. Step 8 is the branch diff after branch delete a. It returned '<commit id>' names no branch and no commit on the local engine and on DST seeds 0 and 42, including each replay.

CI on this exact head: clippy pass, Test Workspace pass (both engine tests pass), Storage Upgrade pass, Azurite pass, CI Gate pass (run 41739941301). GQT dst and GQT served each report ok issue_1236_stacked_diff_after_parent_delete (run 41739941289). Devin Review did not review the diff.

Limitations

I did not measure the lineage walk. I did not run a delete concurrent with a poll. Local GQT did not run the served target. CI did, on this head. Lance 13.0.0 Branches::delete removes the ref and the branch tree, and it refuses while another branch references that branch. This change depends on logical delete continuing to archive the ref and leave the tree. That retirement path is unchanged.

Inline: crates/omnigraph/src/db/graph_coordinator.rs:1229

This return runs before the polled-branch witness compare. A page whose commits were all written by incarnations that are still live never reads cut.witness. A delete or a recreate of the polled branch is invisible on that page. A fresh poll fails at open with BranchNotFound. An in-flight poll that already holds the cut does not.

Compare the fresh identifier with the witness before this return, or once per poll. A matching witness still takes this live path.

When the writer is retired and the polled name is gone, the miss below becomes ReadTargetNotFound of the commit id. That is not the incarnation-witness error. A recreated name still returns that error from the second ensure_incarnation_live, and the error propagates.

Inline: crates/omnigraph/src/changes/feed.rs:539

This flag follows the commit row's writer native, not the snapshot's own native ref. The cut's head snapshot comes from build_change_feed_cut, which uses the polled coordinator's snapshot. Its graph_branch is the polled branch. For kid with no commit, the head row's native ref is the deleted parent. That native is in retired, so the live head snapshot is marked retired.

reprove_named_branch_heads then returns before it reproves kid. The flag comment says the snapshot was opened through the retired ref. This snapshot was not. For a child with no commit, both sides of the head compare are absent, so this path does not show a wrong page.

Set the flag when snapshot.native_branch() is the retired native. A history snapshot of the deleted writer has that native. The live head snapshot does not.

Inline: crates/omnigraph/src/db/graph_coordinator.rs:738

This sentence says the captured head is retention only while this branch is still that incarnation. The call uses Retention::LiveBranches. A handle synced to a before the delete reads the commit because main retains it. The engine test pins that read. State that this fast path is the same decision as settled_commit_state.