Skip to content

Make branch delete's dependency check manifest-only per surviving branch#369

Closed
ragnorc wants to merge 2 commits into
mainfrom
branch-op-complexity-audit
Closed

Make branch delete's dependency check manifest-only per surviving branch#369
ragnorc wants to merge 2 commits into
mainfrom
branch-op-complexity-audit

Conversation

@ragnorc

@ragnorc ragnorc commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

What & why

branch_delete proves no surviving branch still references the deleted branch's per-table forks, but its per-branch check performed a full cold target resolve — a manifest state scan plus a commit-lineage scan plus a schema-contract re-read/re-validate per foreign branch — making deletion O(branches × history) on un-compacted graphs and timing out on large ones. This switches the loop to the existing manifest-only per-branch snapshot (the same shape cleanup uses), halving the per-surviving-branch __manifest cost from 2 reads/opens to 1 and dropping the per-branch contract reads and schema recompiles entirely.

Backing issue / RFC

  • Fixes an accepted issue: Closes #
  • Implements / is an accepted RFC:
  • Trivial fast-lane — no issue/RFC required

Maintainer perf fix from a branch-operation cost audit; no pre-existing accepted issue.

Checklist

  • Change is focused (one logical change)
  • Tests added/updated for behavior changes — new branch_control_cost.rs slope gate on the shared cost harness, landed red (2 reads/opens per surviving branch) one commit before the fix turns it green (1 per branch)
  • Public docs updated if user-facing surface changed — docs/dev/testing.md row for the new cost test; no user-facing surface change
  • Reviewed against docs/dev/invariants.md — no invariant weakened; this closes a cold re-derivation cost on a control path

Notes for reviewers

The dropped per-foreign-branch schema validation was incidental coupling: it could wedge deletion of an unrelated branch behind another branch's schema drift, while the deleting operation's own schema is still validated under the schema gate. The dependency loop itself is defense-in-depth (created-from lineage is already refused by the earlier descendant check and by the ref layer), and its predicate and refusal messages are unchanged.


Note

Medium Risk
Touches branch control and removes incidental per-foreign-branch schema validation; behavior for the dependency predicate is intended to be identical, but large-graph delete paths are sensitive and the change is covered mainly by a cost regression test rather than new functional cases.

Overview
branch_delete no longer times out on graphs with many branches or deep history when proving no other branch still references the victim’s per-table forks.

The loop over surviving branches now calls fresh_snapshot_for_branch_unchecked (manifest state only, same shape as cleanup) instead of snapshot_of, which pulled commit lineage and re-read/re-validated the schema contract per foreign branch. Dependency detection still scans each snapshot’s manifest table_branch entries; conflict messages and predicates are unchanged. The deleting operation’s schema remains validated under the existing schema gate, so the dropped per-branch schema checks were redundant work—and could block deleting one branch when another branch had schema drift.

Adds branch_control_cost.rs with a slope gate on __manifest reads/opens per surviving branch (1 per branch, not 2), and documents the test in docs/dev/testing.md.

Reviewed by Cursor Bugbot for commit ad6c609. Bugbot is set up for automated code reviews on this repo. Configure here.

Greptile Summary

This PR makes branch deletion inspect only each surviving branch’s manifest. The main changes are:

  • Replaces full branch resolution with a manifest-only snapshot.
  • Adds a cost-slope test for manifest reads and opens.
  • Documents the new branch-control cost coverage.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
crates/omnigraph/src/db/omnigraph.rs Uses a fresh manifest-only snapshot for each surviving branch without changing the dependency predicate.
crates/omnigraph/tests/branch_control_cost.rs Adds a two-point cost test that bounds incremental manifest reads and internal opens.
docs/dev/testing.md Adds the branch-control cost test to the developer coverage map.

Reviews (1): Last reviewed commit: "Read manifest-only snapshots in branch d..." | Re-trigger Greptile

Context used:

  • Context used - AGENTS.md (source)
  • Context used - CLAUDE.md (source)

ragnorc added 2 commits July 18, 2026 17:27
branch_delete must prove no surviving branch still references the deleted
branch's per-table forks. That check needs one manifest-only snapshot (one
__manifest open + state scan) per surviving branch, so the new
branch_control_cost.rs gate bounds the SLOPE of the delete's __manifest
reads/opens per surviving branch on the shared helpers::cost harness.

Red at this commit by design (the rule-12 red half of the pair): the current
implementation performs a full cold target resolve per foreign branch —
manifest state scan PLUS commit-lineage scan plus a schema-contract
re-read/re-validate — measuring exactly 2 reads and 2 internal opens per
surviving branch against a budget of 1 (+2 slack). The following commit
converges the dependency check on the manifest-only snapshot and turns this
gate green.
The branch_delete dependency check visits every surviving branch to prove
none still references the deleted branch's per-table forks, but it consumes
only each branch's manifest table_branch entries. It previously performed a
full cold target resolve per foreign branch — a manifest state scan PLUS a
commit-lineage scan plus a schema-contract re-read/re-validate — making
deletion O(branches x history) on un-compacted graphs and timing out on
large ones. The per-foreign-branch schema validation could also wedge
deletion of an unrelated branch behind another branch's schema drift, even
though the deleting operation's own schema is already validated under the
schema gate.

Switch the loop to fresh_snapshot_for_branch_unchecked — the same
manifest-only per-branch snapshot cleanup already uses — halving the
per-surviving-branch __manifest cost from 2 reads/opens to 1 and dropping
the per-branch contract reads and recompiles entirely. Turns the
branch_control_cost.rs slope gate green (previous commit, red by design):
19->35 reads / 16->32 opens across 4->12 branches before, 15->23 / 13->21
after.

Copy link
Copy Markdown
Collaborator

Closing this PR as stale against the current write path.

Since this branch was cut, the B1 work changed branch-delete admission, recovery, lifecycle fencing, gate ownership, and operation-local capture. The PR's slope test now passes on current main without its production change, while a direct rebase adds a cold main manifest open/scan and fails the existing branch-control cost budget. The manifest-only loop also drops per-survivor catalog validation and still performs one history-sensitive manifest scan per surviving branch, so the stated complexity result is not established.

The underlying idea may be revisited as a current-main fast-lane optimization after measuring the remaining schema/lineage cost, preserving catalog validation, and avoiding the extra main scan.

@aaltshuler aaltshuler closed this Jul 19, 2026
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