Skip to content

Don't misreport fast-forwards against a have-pruned store#79

Open
Soph wants to merge 3 commits into
mainfrom
fix/ff-ancestry-pruned-store
Open

Don't misreport fast-forwards against a have-pruned store#79
Soph wants to merge 3 commits into
mainfrom
fix/ff-ancestry-pruned-store

Conversation

@Soph

@Soph Soph commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Problem

BuildPlans runs the fast-forward ancestry check against a store populated by a fetch that advertises every target ref as a have, so the server omits all objects reachable from any target ref. ReachesCommit walked that pruned store anyway, with two failure modes:

  • Hard crash: if the new source tip already exists on the target under another ref, the start commit is pruned, object.GetCommit fails, and the whole sync aborts with load source commit.
  • False block: if an intermediate commit is pruned because some other target ref points at it, the walk dead-ends on the missing parent and a genuine fast-forward is reported as a non-ancestor → ActionBlock, pushing the user toward --force-with-lease.

ObjectsToPush already documents and tolerates this pruning; the ancestry check never did.

Fix

Make the ancestry walk frontier-aware. A commit missing from this store means the target already has it, not an error. CheckAncestry now returns a three-valued result:

  • AncestryReachable — provable fast-forward.
  • AncestryUnreachable — full ancestry walked, no target hash, no pruned objects hit → genuine divergence.
  • AncestryIndeterminate — the walk reached the frontier of objects the target already has before the question was settled.

Confirming or ruling out a fast-forward across that frontier is impossible from this store alone (a divergent branch's merge base is pruned too), so PlanRef turns AncestryIndeterminate into a clean, actionable block — not a crash and not a false "diverged" — and --force still overrides. The common cases (new commits present, true divergence with a visible merge base) are unchanged.

Why not fix it at the fetch layer

Fetching each ref's planning closure with only its own targetHash as a have would make the result always determinate, but it shatters the lazy single-closure fetch that the materialized fallback shares — N fetches instead of one, extra bandwidth for create-only refs. The frontier is the price of the bandwidth optimization; handling it gracefully is the right altitude.

Tests

  • CheckAncestry: indeterminate on a missing start, indeterminate on a pruned ancestor, unreachable on real divergence.
  • PlanRef: indeterminate → ActionBlock with an actionable reason (no error); --forceActionUpdate.

Full go test ./... passes.

🤖 Generated with Claude Code


Note

Medium Risk
Changes core sync planning and fast-forward detection; behavior shifts for pruned-store edge cases, though reachable/unreachable paths on complete stores are preserved and well tested.

Overview
Replaces ReachesCommit with CheckAncestry, which returns AncestryReachable, AncestryUnreachable, or AncestryIndeterminate when walking a planning store shaped by fetch-with-all-target-haves pruning.

Missing commits are treated as “already on the target” instead of hard errors: a pruned source tip or ancestor yields indeterminate, not a sync abort or a false “not an ancestor” block. PlanRef maps indeterminate to ActionBlock with a “cannot verify fast-forward locally” reason; --force still allows update. True divergence on a fully visible graph stays unreachable → block as before.

Adds unit tests for the three ancestry outcomes and indeterminate PlanRef behavior; renames the ancestry benchmark to BenchmarkCheckAncestry.

Reviewed by Cursor Bugbot for commit 97b33b2. Configure here.

Soph and others added 2 commits June 17, 2026 09:35
BuildPlans runs against a store populated by a fetch that advertises every
target ref as a have, so the server omits all objects reachable from any
target ref. The fast-forward ancestry check walked that pruned store anyway:

  - If the new source tip already exists on the target under another ref, the
    start commit is pruned and GetCommit failed, aborting the whole sync with
    "load source commit".
  - If an intermediate commit is pruned because some other target ref points
    at it, the walk dead-ended on the missing parent and a genuine
    fast-forward was reported as a non-ancestor (ActionBlock).

Make the check frontier-aware: a commit missing from this store means the
target already has it, not an error. CheckAncestry now returns a three-valued
result — reachable / unreachable / indeterminate — where indeterminate means
the walk reached the boundary of objects the target already has before the
question could be settled. Confirming or ruling out a fast-forward across that
boundary is impossible from this store alone (the merge base of a divergent
branch is pruned too), so PlanRef turns indeterminate into a clean, actionable
block instead of a crash or a false "diverged" — and --force still overrides.

ReachesCommit is kept as a bool wrapper over CheckAncestry for existing callers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 60c242ac6016
PlanRef now calls CheckAncestry directly, leaving ReachesCommit with no
production caller — only a benchmark used it. Point the benchmark at
CheckAncestry and remove the wrapper rather than leave dead exported surface.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: cf5b16a42703
Comment thread internal/planner/planner.go Outdated
const (
// AncestryReachable means targetHash is provably an ancestor of startHash:
// the update is a fast-forward.
AncestryReachable AncestryResult = iota

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that we return this by default (e.g. return 0, ErrAncestryDepthExceeded), ideally we would:

Suggested change
AncestryReachable AncestryResult = iota
AncestryInvalid AncestryResult = iota
AncestryReachable

That way, callers ignoring error (e.g. v, _ := f()), get AncestryInvalid instead of AncestryReachable.

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.

can you take a look?

CheckAncestry returned the literal 0 alongside every error, which equals
AncestryReachable. A caller that ignored the error would read a failure
as a provable fast-forward. Make the zero value a distinct AncestryInvalid
sentinel and return it explicitly on every error path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 6229e797c9d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants