From e75fef0c0858e6aa1288e9f2d8b998d0c63df718 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Wed, 17 Jun 2026 09:35:25 +0200 Subject: [PATCH 1/3] Don't misreport fast-forwards against a have-pruned store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Entire-Checkpoint: 60c242ac6016 --- internal/planner/planner.go | 83 +++++++++++++++++++++++---- internal/planner/planner_test.go | 98 ++++++++++++++++++++++++++++++++ 2 files changed, 170 insertions(+), 11 deletions(-) diff --git a/internal/planner/planner.go b/internal/planner/planner.go index cfffc34a..33b06543 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -334,7 +334,7 @@ func PlanRef(store storer.EncodedObjectStorer, want DesiredRef, targetHash plumb return plan, nil } - isFF, err := ReachesCommit(store, want.SourceHash, targetHash) + ancestry, err := CheckAncestry(store, want.SourceHash, targetHash) if err != nil { if errors.Is(err, ErrAncestryDepthExceeded) { // Can't prove fast-forward within depth limit — block with explanation. @@ -344,7 +344,7 @@ func PlanRef(store storer.EncodedObjectStorer, want DesiredRef, targetHash plumb } return plan, fmt.Errorf("check fast-forward for %s: %w", want.TargetRef, err) } - if isFF { + if ancestry == AncestryReachable { plan.Action = ActionUpdate plan.Reason = ShortHash(targetHash) + " -> " + ShortHash(want.SourceHash) return plan, nil @@ -357,6 +357,15 @@ func PlanRef(store storer.EncodedObjectStorer, want DesiredRef, targetHash plumb } plan.Action = ActionBlock + if ancestry == AncestryIndeterminate { + // The target already has the commits where the two histories meet, so + // the fetch pruned them from the planning store and a fast-forward + // can't be confirmed locally. Don't fail the sync or claim divergence + // we haven't proven — block with an actionable message. + plan.Reason = "cannot verify fast-forward for " + want.TargetRef.String() + + " locally (the target already has the relevant commits); use --force-with-lease if this is a valid fast-forward" + return plan, nil + } plan.Reason = ShortHash(targetHash) + " is not an ancestor of " + ShortHash(want.SourceHash) return plan, nil } @@ -400,27 +409,61 @@ func PlanReplicationRef(want DesiredRef, targetHash plumbing.Hash, existsOnTarge // on real repos — even the Linux kernel has ~1.3M commits. const MaxAncestryDepth = 2_000_000 -// ErrAncestryDepthExceeded is returned when ReachesCommit exceeds MaxAncestryDepth. +// ErrAncestryDepthExceeded is returned when the ancestry walk exceeds MaxAncestryDepth. var ErrAncestryDepthExceeded = errors.New("ancestry check exceeded depth limit") -// ReachesCommit checks whether the commit at startHash has targetHash as an +// AncestryResult is the outcome of a fast-forward ancestry check against a +// store that was populated by a fetch advertising the target's refs as haves. +type AncestryResult int + +const ( + // AncestryReachable means targetHash is provably an ancestor of startHash: + // the update is a fast-forward. + AncestryReachable AncestryResult = iota + // AncestryUnreachable means startHash's entire ancestry was walked without + // reaching targetHash and without hitting any object the target already + // has — a genuine non-fast-forward (the two histories have diverged). + AncestryUnreachable + // AncestryIndeterminate means the walk reached the frontier of objects the + // target already has (absent from this store because the fetch pruned + // everything reachable from a target ref) before the question was settled. + // The deciding commits live beyond that frontier, so a fast-forward can be + // neither confirmed nor ruled out from this store alone. + AncestryIndeterminate +) + +// CheckAncestry reports whether the commit at startHash has targetHash as an // ancestor, bounded to MaxAncestryDepth commits to prevent degenerate walks. -func ReachesCommit(store storer.EncodedObjectStorer, startHash, targetHash plumbing.Hash) (bool, error) { +// +// The store is the one BuildPlans runs against: a fetch populated it using the +// target's refs as haves, so every object reachable from any target ref is +// pruned. startHash and the commits between it and the target are normally +// present (they are the new work being synced), but when another target ref +// already points into that range the frontier commits are missing. A missing +// object therefore means "the target already has this" rather than an error — +// the walk records that it could not cross the frontier and returns +// AncestryIndeterminate instead of misreporting a non-fast-forward or failing +// the whole sync on a missing start commit. +func CheckAncestry(store storer.EncodedObjectStorer, startHash, targetHash plumbing.Hash) (AncestryResult, error) { if startHash == targetHash { - return true, nil + return AncestryReachable, nil } start, err := object.GetCommit(store, startHash) if err != nil { - return false, fmt.Errorf("load source commit %s: %w", startHash, err) + if errors.Is(err, plumbing.ErrObjectNotFound) { + return AncestryIndeterminate, nil + } + return 0, fmt.Errorf("load source commit %s: %w", startHash, err) } seen := map[plumbing.Hash]struct{}{} stack := []*object.Commit{start} + hitFrontier := false for len(stack) > 0 { if len(seen) >= MaxAncestryDepth { - return false, ErrAncestryDepthExceeded + return 0, ErrAncestryDepthExceeded } current := stack[len(stack)-1] stack = stack[:len(stack)-1] @@ -431,7 +474,7 @@ func ReachesCommit(store storer.EncodedObjectStorer, startHash, targetHash plumb for _, parentHash := range current.ParentHashes { if parentHash == targetHash { - return true, nil + return AncestryReachable, nil } if _, ok := seen[parentHash]; ok { continue @@ -439,14 +482,32 @@ func ReachesCommit(store storer.EncodedObjectStorer, startHash, targetHash plumb parent, err := object.GetCommit(store, parentHash) if err != nil { if errors.Is(err, plumbing.ErrObjectNotFound) { + // Pruned ancestor: the target already has it, so the + // walk cannot cross this point. The answer may lie beyond. + hitFrontier = true continue } - return false, fmt.Errorf("load parent commit %s: %w", parentHash, err) + return 0, fmt.Errorf("load parent commit %s: %w", parentHash, err) } stack = append(stack, parent) } } - return false, nil + if hitFrontier { + return AncestryIndeterminate, nil + } + return AncestryUnreachable, nil +} + +// ReachesCommit reports whether targetHash is provably an ancestor of +// startHash. It is a convenience wrapper over CheckAncestry for callers that +// only care about a definite fast-forward; AncestryIndeterminate is reported +// as false (not provably reachable). +func ReachesCommit(store storer.EncodedObjectStorer, startHash, targetHash plumbing.Hash) (bool, error) { + result, err := CheckAncestry(store, startHash, targetHash) + if err != nil { + return false, err + } + return result == AncestryReachable, nil } // ObjectsToPush computes the set of objects that need to be sent to the target. diff --git a/internal/planner/planner_test.go b/internal/planner/planner_test.go index 284e2462..15eab819 100644 --- a/internal/planner/planner_test.go +++ b/internal/planner/planner_test.go @@ -3,6 +3,7 @@ package planner import ( "fmt" "slices" + "strings" "testing" "time" @@ -76,6 +77,103 @@ func TestPlanRefFastForwardAndBlock(t *testing.T) { } } +// A fully-populated store must still distinguish a real non-fast-forward +// (divergence) from the pruned-frontier case below. +func TestCheckAncestryUnreachableOnDivergence(t *testing.T) { + repo, err := git.Init(memory.NewStorage(), nil) + if err != nil { + t.Fatalf("init repo: %v", err) + } + root := seedCommit(t, repo, nil) + next := seedCommit(t, repo, []plumbing.Hash{root}) + side := seedCommit(t, repo, []plumbing.Hash{root}) + + result, err := CheckAncestry(repo.Storer, side, next) + if err != nil { + t.Fatalf("CheckAncestry: %v", err) + } + if result != AncestryUnreachable { + t.Fatalf("expected AncestryUnreachable for divergent history, got %v", result) + } +} + +// When the source tip itself is absent — it lives behind the have frontier +// because the target already has it under another ref — the check must report +// indeterminate, not fail (which previously aborted the whole sync with a +// "load source commit" error). +func TestCheckAncestryIndeterminateOnMissingStart(t *testing.T) { + repo, err := git.Init(memory.NewStorage(), nil) + if err != nil { + t.Fatalf("init repo: %v", err) + } + missingStart := plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + target := plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") + + result, err := CheckAncestry(repo.Storer, missingStart, target) + if err != nil { + t.Fatalf("CheckAncestry should not error on a pruned start commit: %v", err) + } + if result != AncestryIndeterminate { + t.Fatalf("expected AncestryIndeterminate for missing start, got %v", result) + } +} + +// When the walk reaches a parent that was pruned (the target already has it), +// the answer lies beyond the frontier: indeterminate, not a false "diverged". +func TestCheckAncestryIndeterminateOnPrunedAncestor(t *testing.T) { + repo, err := git.Init(memory.NewStorage(), nil) + if err != nil { + t.Fatalf("init repo: %v", err) + } + prunedParent := plumbing.NewHash("cccccccccccccccccccccccccccccccccccccccc") + tip := seedCommit(t, repo, []plumbing.Hash{prunedParent}) + target := plumbing.NewHash("dddddddddddddddddddddddddddddddddddddddd") + + result, err := CheckAncestry(repo.Storer, tip, target) + if err != nil { + t.Fatalf("CheckAncestry: %v", err) + } + if result != AncestryIndeterminate { + t.Fatalf("expected AncestryIndeterminate when an ancestor is pruned, got %v", result) + } +} + +// PlanRef must turn the indeterminate case into a clean, actionable block +// rather than a hard error, and --force must still let it through. +func TestPlanRefIndeterminateBlocksAndForceOverrides(t *testing.T) { + repo, err := git.Init(memory.NewStorage(), nil) + if err != nil { + t.Fatalf("init repo: %v", err) + } + missingSource := plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + target := plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") + want := DesiredRef{ + Kind: RefKindBranch, Label: "main", + SourceRef: plumbing.NewBranchReferenceName("main"), + TargetRef: plumbing.NewBranchReferenceName("main"), + SourceHash: missingSource, + } + + plan, err := PlanRef(repo.Storer, want, target, false) + if err != nil { + t.Fatalf("PlanRef should not error on indeterminate ancestry: %v", err) + } + if plan.Action != ActionBlock { + t.Fatalf("expected ActionBlock, got %s", plan.Action) + } + if !strings.Contains(plan.Reason, "cannot verify fast-forward") { + t.Fatalf("expected actionable indeterminate reason, got %q", plan.Reason) + } + + forced, err := PlanRef(repo.Storer, want, target, true) + if err != nil { + t.Fatalf("PlanRef (force): %v", err) + } + if forced.Action != ActionUpdate { + t.Fatalf("expected ActionUpdate under force, got %s", forced.Action) + } +} + func TestPlanReplicationRefOverwritesDivergence(t *testing.T) { target := plumbing.NewHash("1111111111111111111111111111111111111111") source := plumbing.NewHash("2222222222222222222222222222222222222222") From 97b33b2f2a4ea9abba7c8921097b770b4a79d9d7 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Wed, 17 Jun 2026 09:44:30 +0200 Subject: [PATCH 2/3] Drop now-unused ReachesCommit wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Entire-Checkpoint: cf5b16a42703 --- internal/planner/benchmark_test.go | 6 +++--- internal/planner/planner.go | 12 ------------ internal/syncer/integration_test.go | 2 +- 3 files changed, 4 insertions(+), 16 deletions(-) diff --git a/internal/planner/benchmark_test.go b/internal/planner/benchmark_test.go index ab4d7d2d..a2e230af 100644 --- a/internal/planner/benchmark_test.go +++ b/internal/planner/benchmark_test.go @@ -105,7 +105,7 @@ func BenchmarkSampledCheckpointCandidates(b *testing.B) { } } -func BenchmarkReachesCommit(b *testing.B) { +func BenchmarkCheckAncestry(b *testing.B) { repo, err := git.Init(memory.NewStorage(), nil) if err != nil { b.Fatalf("init repo: %v", err) @@ -123,11 +123,11 @@ func BenchmarkReachesCommit(b *testing.B) { b.ResetTimer() for range b.N { - ok, err := ReachesCommit(repo.Storer, tip, root) + result, err := CheckAncestry(repo.Storer, tip, root) if err != nil { b.Fatal(err) } - if !ok { + if result != AncestryReachable { b.Fatal("expected tip to reach root") } } diff --git a/internal/planner/planner.go b/internal/planner/planner.go index 33b06543..be747750 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -498,18 +498,6 @@ func CheckAncestry(store storer.EncodedObjectStorer, startHash, targetHash plumb return AncestryUnreachable, nil } -// ReachesCommit reports whether targetHash is provably an ancestor of -// startHash. It is a convenience wrapper over CheckAncestry for callers that -// only care about a definite fast-forward; AncestryIndeterminate is reported -// as false (not provably reachable). -func ReachesCommit(store storer.EncodedObjectStorer, startHash, targetHash plumbing.Hash) (bool, error) { - result, err := CheckAncestry(store, startHash, targetHash) - if err != nil { - return false, err - } - return result == AncestryReachable, nil -} - // ObjectsToPush computes the set of objects that need to be sent to the target. // // A fetch with target refs as haves prunes the source pack server-side, so diff --git a/internal/syncer/integration_test.go b/internal/syncer/integration_test.go index 35487c8e..f44f7e84 100644 --- a/internal/syncer/integration_test.go +++ b/internal/syncer/integration_test.go @@ -294,7 +294,7 @@ func TestRun_IntegrationSkipsLocalFetchOnRelayOnlySync(t *testing.T) { // TestRun_IntegrationKeepsLocalFetchWhenAncestryNeeded ensures the fetch is // still performed for a fast-forward update where BuildPlans calls -// ReachesCommit on the local store. Skipping it would crash the planner. +// CheckAncestry on the local store. Skipping it would crash the planner. func TestRun_IntegrationKeepsLocalFetchWhenAncestryNeeded(t *testing.T) { sourceRepo, sourceFS := newSourceRepo(t) makeCommits(t, sourceRepo, sourceFS, 2) From 594c86a837476ae56805eb794ef9a6bef7b5ac7e Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Wed, 17 Jun 2026 15:46:30 +0200 Subject: [PATCH 3/3] Add AncestryInvalid zero value for ancestry errors 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) Entire-Checkpoint: 6229e797c9d4 --- internal/planner/planner.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/internal/planner/planner.go b/internal/planner/planner.go index be747750..be0c3fb4 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -417,9 +417,14 @@ var ErrAncestryDepthExceeded = errors.New("ancestry check exceeded depth limit") type AncestryResult int const ( + // AncestryInvalid is the zero value and is returned alongside every error. + // Keeping it distinct from AncestryReachable means a caller that ignores + // the error (result, _ := CheckAncestry(...)) reads a failure as invalid + // rather than mistaking it for a provable fast-forward. + AncestryInvalid AncestryResult = iota // AncestryReachable means targetHash is provably an ancestor of startHash: // the update is a fast-forward. - AncestryReachable AncestryResult = iota + AncestryReachable // AncestryUnreachable means startHash's entire ancestry was walked without // reaching targetHash and without hitting any object the target already // has — a genuine non-fast-forward (the two histories have diverged). @@ -454,7 +459,7 @@ func CheckAncestry(store storer.EncodedObjectStorer, startHash, targetHash plumb if errors.Is(err, plumbing.ErrObjectNotFound) { return AncestryIndeterminate, nil } - return 0, fmt.Errorf("load source commit %s: %w", startHash, err) + return AncestryInvalid, fmt.Errorf("load source commit %s: %w", startHash, err) } seen := map[plumbing.Hash]struct{}{} @@ -463,7 +468,7 @@ func CheckAncestry(store storer.EncodedObjectStorer, startHash, targetHash plumb for len(stack) > 0 { if len(seen) >= MaxAncestryDepth { - return 0, ErrAncestryDepthExceeded + return AncestryInvalid, ErrAncestryDepthExceeded } current := stack[len(stack)-1] stack = stack[:len(stack)-1] @@ -487,7 +492,7 @@ func CheckAncestry(store storer.EncodedObjectStorer, startHash, targetHash plumb hitFrontier = true continue } - return 0, fmt.Errorf("load parent commit %s: %w", parentHash, err) + return AncestryInvalid, fmt.Errorf("load parent commit %s: %w", parentHash, err) } stack = append(stack, parent) }