Review and Fix Git Sync Bootstrap Bug

Reviewed PR #116 — read the full diff, ran the suite locally, and probed a few hypotheses with throwaway tests.

Verdict

The core fix is right and well-targeted. Routing on the marker's presence rather than letting it fall out as an undesired ref is the correct read: the namespace has one writer, so a survivor is a positive signal. Three things I checked and confirmed hold up:

• isLiveBootstrapMarker sits at the right seam. Putting the exception in addPruneCandidates (candidacy) rather than in each builder's delete emission means there is exactly one statement of it, and I verified there is no third delete-emission path that bypasses it (ActionDelete is only produced at planner.go:167 and :237, both downstream of addPruneCandidates).
• The removed NoErrAlreadyUpToDate branch really was dead. gitproto emits that sentinel only when len(wants) == 0 (fetch.go:184, :227, :431), collectWants doesn't filter against haves, and all three Execute callers gate on len(desiredRefs) == 0 first.
• Target refs are never prefix-narrowed (they come from a full receive-pack advertisement at syncer.go:858), so the marker is always visible to the route. And resolveEmptyDesiredSet returns ErrNoRefsSelected for a scaffolding-only source rather than falling through to convergence — it cannot delete the target's refs.

TestRun_IntegrationReplicateResumesInterruptedBatchedBootstrap asserting cmd.Old == marker.Hash() is the right assertion; it pins the actual resume rather than just the routing.

Finding 1 — one-shot marker cleanup destroys resume state under --best-effort (new in this PR)

bootstrap.go:200-214. The justifying comment says "every ref this bootstrap set out to create has landed" — that is false whenever OnRejection is installed. gitproto/push.go:453-470 swallows per-ref ng statuses and returns nil when best-effort is on, so pushErr == nil does not mean the create landed. leftoverTempRefDeletes then deletes the marker for a branch that does not exist.

Confirmed with a probe (target rejects only the branch create, accepts the pack):

The run reports success, the branch is absent, and the resume position is gone — the next run re-transfers from zero.

Reachable through the CLI where best-effort is on by default: --all-refs implies BestEffort for both bootstrap (cmd/git-sync/bootstrap.go:68) and sync (cmd/git-sync/syncplan.go:121), and library callers pass Policy.BestEffort straight through (client.go:133) regardless of mode. Impact is bounded — the one-shot path is by definition the small-repo path — but it's the exact hazard the replicate-side carve-out was added to prevent, left open on the path this PR introduced.

Cheapest fix: drop the cleanup entirely and let prune handle it. isLiveBootstrapMarker already classifies correctly (branch present → stale → pruned; branch absent → live → kept), which is the fallback the comment names anyway. Costs one extra run to converge, and can't be wrong. Otherwise, filter delCmds by the rejection set.

Related, pre-existing: the batched cutover at bootstrap.go:687-691 deletes the temp ref unconditionally too, and the create at :683 rides the same best-effort push. I ran the same probe against origin/main and it fails identically there, so it's not a regression — but given this PR's whole subject is preserving resume state, it belongs in ENT-2060.

Finding 2 — prune is suspended for the entire run whenever the marker route fires

The bootstrap route emits no deletes, so stray target refs go unpruned and Deleted reports 0 (bootstrap's Result carries no delete count — syncer.go:1398-1405). One run of delay is fine and documented. What isn't stated: if the bootstrap keeps failing (target keeps rejecting, pack still too large), the marker survives, the route fires again, and prune is starved indefinitely. Pre-PR those runs at least pruned. Worth a sentence in the routing comment.

Finding 3 — PR body has drifted from the code

The body describes dropLiveBootstrapMarkerDeletes, which no longer exists (it became isLiveBootstrapMarker at candidacy in c9423f7), and never mentions the source-side refs/gitsync/* discovery exclusion added in 89b8c69. That one is a user-visible behavior change beyond ENT-2054: a source ref under refs/gitsync/ silently stops being mirrored, and a source holding only that scaffolding now hard-errors instead of syncing. Since this repo's changelog is written from PR bodies, that should land in the description before merge.

Minor

• Sync mode still labels a resume empty-target-managed-refs (syncer.go:953), so the relay_reason signal the PR adds for operators doesn't reach sync. Already in ENT-2060.
• desiredTargetRefsAbsent runs twice on the bootstrap route (directly, then inside replicateCanBootstrap). Trivial.

Full suite green locally; CI green. I left the worktree back on its original branch and removed the probe files.

Want me to post this as a review on the PR?

Posted: https://github.com/entireio/git-sync/pull/116#pullrequestreview

Condensed to the verdict, the one real bug (best-effort marker deletion on the new one-shot path, with the probe output and a fix), the prune-starvation note, the stale PR body, and two minor items.

Posted. Summary of the re-review:

Finding 1 is genuinely fixed. I re-ran my original probe against 653e9c5 without relying on their test: the marker now survives the best-effort rejected create, still at its original hash. Dropping the cleanup entirely (rather than filtering it by rejection) was the right choice — prune's live/stale classification already answers the question, and it can't be wrong. Their new test is meaningful; I verified the test-server short-circuit mechanic they flagged actually matters. Findings 2 and 3 addressed, PR body now matches the code, full suite green locally, CI green.

One substantive point I raised on the deferred batched cutover. My probe still fails there (it fails on origin/main too, so not a regression), but two things argue against filing it as a simple follow-up:

• It's the higher-consequence half — batched is the large-repo path that ENT-1948 is about, so a lost position costs a full re-transfer of exactly the repos this PR exists for. And bootstrap --all-refs implies BestEffort, so bootstrap --all-refs --target-max-pack-bytes=N against a target that refuses the create silently loses the whole import and reports success.
• The fix they used here won't port: dropping the delete would leave a permanent marker, because Bootstrap() rejects --prune and no cleaner would ever run. It needs the rejection threaded into the strategy — which is cheap, since the syncer already records it.

Plus one nit: replicateCanBootstrap now has no production caller, only a test.

Saved. From now on I'll draft the review in the terminal and wait for your go-ahead before every post — including re-reviews and short follow-ups, since a yes on one post doesn't carry to the next.

Both reviews are already up on #116. If you'd rather one of them weren't, say the word and I'll delete it.

Yes — it's a real bug, and it's pre-existing on main rather than something #116 introduced. Here's the mechanism.

What the probe does

Batched bootstrap imports a branch in checkpoints, pushing each one to a temp ref refs/gitsync/bootstrap/heads/<branch>. That temp ref is the resume marker — it's the only record of how far the import got. On the final checkpoint the push carries two commands in one request: advance the temp ref, and create the real branch at that hash.

My probe stands up a target that refuses only the branch create and accepts everything else — a protected-branch or pre-receive policy, which is exactly what --best-effort exists to tolerate. Output on main:

Eight checkpoint packs of real work, and the target ends up with neither the branch nor the marker.

Why it happens

gitproto/push.go:453-470: when OnRejection is installed (best-effort), per-ref ng statuses go to the callback and PushPack returns nil. So a nil push error does not mean the create landed — that's the entire point of the flag.

bootstrap.go:654 then deletes the temp ref unconditionally:

Old: current matches, so the delete succeeds. The guard just above at :647 can't catch it either — it consults p.TargetRefs, the ref map captured at session start, not re-read after the push.

This is the same faulty inference #116 introduced on the one-shot path and then fixed in 653e9c5. The batched copy predates both.

How bad

The rejection itself is surfaced honestly — Pushed: 0, Warned: 1, plan action=warn. What's not surfaced anywhere is that the run also threw away the checkpoint, and the process exits 0, so an automated caller sees success.

The follow-on cost is what makes it worse than the one already fixed. With the marker gone and the branch never created, those objects are unreferenced on the target: no ref points at them, so the next run has no have to negotiate against and re-fetches the entire history — and the target is free to GC them meanwhile. And batched is by definition the large-repo path; it's the ENT-1948 (>10 GiB) case. The one-shot bug #116 fixed cost a small repo's re-transfer. This one costs the big one's.

Reachable through the CLI wherever best-effort is implied: --all-refs turns it on for bootstrap (cmd/git-sync/bootstrap.go:68) and sync (syncplan.go:121). Not for replicate via CLI — that's deliberate and documented — but client.go:133 passes Policy.BestEffort through for any mode, so a library or worker caller can have it on there too.

Why I flagged it rather than blocked on it

I ran the identical probe against origin/main and it fails the same way, so #116 doesn't regress it — and the PR does fix its own instance of the mistake. It's disclosed in the PR body's known-limitations and tracked in ENT-2060. Landing #116 is a strict improvement either way.

But "pre-existing" isn't "fine". My read is this shouldn't sit in a limitations bullet — it's silent loss of the most expensive work the tool does.

The fix isn't the one used for one-shot

You can't just drop the delete here the way 653e9c5 did. Bootstrap() rejects --prune (syncer.go:1287), so on that path no cleaner would ever exist and every batched bootstrap would leave permanent scaffolding.

It needs the rejection actually reaching the strategy. The syncer already records it — s.rejections, populated at syncer.go:708 — so it's a Params field (Rejected func(plumbing.ReferenceName) bool) plus a check at the cutover: if the branch create was rejected, skip the temp-ref delete and leave the marker. The next run then resumes from it instead of starting over.

Want me to write that up as a Linear issue, or draft the patch? (I'll show you either before anything goes to GitHub.)

Committed locally on soph/batched-cutover-drops-resume-marker — one file, +111, test only, no fix.

What it does: drives a batched bootstrap (8 checkpoints) against a target that refuses only the branch create and applies everything else, then asserts the resume marker survives. On main it fails with:

Verification I did before committing: prototyped the fix I described (a Rejected predicate on bstrap.Params, fed from s.rejections, skipping the temp-ref delete when the create was refused) — the test passes and the full suite stays green. Then I reverted the prototype, so the branch is test-only. That confirms the test pins the real defect rather than being red for an incidental reason. Nothing else in the suite fails.

Two decisions I made that you may want to change:

1. Test-only, no fix — read your ask literally. The consequence is CI goes red, so I'd open it as a draft with the body saying so explicitly.
2. Based on main, not on #116 — makes it independent, mergeable in any order, and it demonstrates a bug #116 doesn't introduce.

Before I push anything: want me to

• push the branch and open the draft PR as-is,
• add the fix to the same branch so it lands green and mergeable, or
• leave it local while you look at the diff first?