Keep the batched cutover's resume marker when the target refuses the branch create

[Soph]

Fixes item 3 of ENT-2060 — the batched instance of the mistake #116 fixed one-shot (653e9c5). Pre-existing, byte-identical on origin/main before that PR and untouched by its diff, so nobody should bisect it there.

Rebased onto main so it carries #116; the pinning commit is unchanged in content.

The bug

A batched bootstrap's temp ref refs/gitsync/bootstrap/heads/<branch> is the only record of how far an import got. The cutover push carries two commands in one request — advance the temp ref to the final checkpoint, and create the real branch there — and the temp ref was deleted immediately afterwards on the strength of that push returning no error.

Under --best-effort that inference is invalid. gitproto/push.go hands a per-ref ng to OnRejection and returns nil, so a target that refuses the branch create (protected branch, pre-receive policy) ended the run with neither the branch nor the marker, reporting success:

[server] request with 2 command(s), rejecting only refs/heads/master:
  refs/gitsync/bootstrap/heads/master c416d48f -> cbfbb940 : ok
  refs/heads/master                   <zero>   -> cbfbb940 : ng rejected by policy

Run() error : <nil>          ← exit 0
Pushed/Warned/BatchCount : 0 / 1 / 8
target after run: branch present=false, marker present=false

The nearby guard couldn't catch it either: it consults p.TargetRefs, the ref map captured at session start, not re-read after the push.

The rejection itself was surfaced honestly (Pushed: 0, Warned: 1, plan action=warn). What was surfaced nowhere is that the run also destroyed the checkpoint, and the process exits 0. With the marker gone and the branch never created, every object pushed so far is unreferenced on the target: nothing points at it, so the next run has no fetch have to negotiate against and re-transfers the entire history — and the target is free to GC it meanwhile. Batching is by definition the large-repo path (ENT-1948, >10 GiB), so this cost exactly the imports that can least afford it.

Reachable wherever best-effort is implied: --all-refs turns it on for bootstrap (cmd/git-sync/bootstrap.go) and sync (syncplan.go). Not for replicate via CLI — deliberate and documented — but client.go passes Policy.BestEffort through for any mode.

The fix

The rejection now reaches the strategy. bootstrap.Params grows

Rejected func(plumbing.ReferenceName) bool

which the syncer answers from the map it already fills from the pusher's OnRejection callback (s.rejections, one method, refRejected). The cutover consults it before deleting: a create this run cannot confirm leaves the marker at the final checkpoint, and the next run resumes from it.

Conditional delete rather than no delete, which is how #116 fixed the one-shot path: Bootstrap() rejects --prune (syncer.go), so on that route no cleaner would ever exist and every batched bootstrap would leave permanent scaffolding. Prune can own the marker only where prune can run.

Two smaller consequences:

• The kept marker is recorded as a completed ref, since it genuinely holds that hash — the objects behind it stay usable as fetch haves for the branches planned after this one.
• Retaining it logs and prints a line naming the marker and the hash the next run resumes from, so the one thing that was silent no longer is.

All three bootstrap entry points (the replicate resume route, sync's bootstrap route, and Bootstrap()) funnel through bootstrapWithInputs, so all three are covered. Non-best-effort callers pass no predicate and behave exactly as before — there a per-ref ng fails the push outright.

Tests

• TestRun_IntegrationBatchedCutoverKeepsResumeMarkerOnRejectedCreate — the pin from the first commit, now passing.
• TestRun_IntegrationBatchedCutoverResumesAfterRejectedCreate — the half that makes the marker worth keeping. Once the target accepts the create, the retry routes on bootstrap-resume-marker, finishes the import from the marker (0 checkpoint packs, 334 bytes to the target against the first run's 427 KiB) and only then deletes it.

Both share a refCreateDenier receive-pack hook, extracted from the first test and carrying the reason it cannot use syncertest.DenyRefsReport: a non-nil report short-circuits the test server before it applies anything, which would leave the temp ref at the previous checkpoint and make the delete that follows carry a stale Old — a shape a real receive-pack would reject, hiding the bug behind a second failure.

Verified the pair fails without the fix (marker gone in both). Full suite and golangci-lint green.

Still open on ENT-2060

Untouched here: partial-progress subset resume, a sync-mode resume route, stale markers under --map or branch-scoped prune, and dry-run's silence on marker lifecycle.

🤖 Generated with Claude Code

https://claude.ai/code/session_018BKBGBh3rJ6WGnZKDbBRy7

---

Note

Medium Risk
Changes core batched-bootstrap state machine and exit codes when refs/gitsync/* writes are blocked; behavior is heavily tested but affects large-repo imports and best-effort alerting.

Overview
Batched bootstrap no longer treats a successful push as proof the branch was created. Under --best-effort, a per-ref ng still yields a nil push error, so a cutover that advanced the temp ref but had the real branch refused used to delete refs/gitsync/bootstrap/heads/<branch> and exit 0 — leaving no resume anchor and forcing a full re-transfer on the next run.

The pusher now records per-push ref outcomes (RefOutcome / LastOutcome) without changing strict vs best-effort semantics. Bootstrap consults that signal (and, when status is unknown, a mid-run target ref listing via TargetRefsNow) before deleting markers: unconfirmed creates defer cleanup, keep the temp ref as a fetch have for later branches, and settle in resolvePendingMarkers by checking whether the branch exists at the pushed hash. Refusal to update the bootstrap temp ref is fatal even under best-effort, since checkpoint state would otherwise diverge from the target.

The syncer wires refOutcome and targetRefsNow into bootstrapWithInputs; rejection reasons are sanitized for notices. Integration tests cover refused creates, resume-after-retry, targets without report-status, and related edge cases.

^{Reviewed by Cursor Bugbot for commit 864584b. Configure here.}

[nodo]

Added the fix as a second commit (30c56dbb) — the PR is no longer intentionally red, so the title and description now describe the change rather than the pin. @nodo force-pushed the branch onto main first, because the resume test needs #116's routing; the pinning commit is unchanged in content, only rebased.

The fix is the shape ENT-2060 predicted: bootstrap.Params.Rejected, answered from the s.rejections map the syncer already fills, consulted at the cutover so a create the run cannot confirm keeps the marker. Conditional delete rather than dropping cleanup the way #116 did on the one-shot path, since Bootstrap() refuses --prune and prune therefore cannot be the cleaner on that route.

Your receivePackHook note was the load-bearing part of the second test — it's now a refCreateDenier type shared by both, with your DenyRefsReport rationale on the type. The new test asserts the marker earns its keep: the retry finishes from it in 334 bytes instead of re-pushing 427 KiB.

🤖 Generated with Claude Code

[nodo]

Review addressed in 1aa4f3d5. Every item, in your numbering:

1 — fixture applied refs whose objects never arrived. Confirmed exactly as described: the hook's report returns before transport.ReceivePack, so the cutover pack was never unpacked. The server now unpacks the pushed objects before a hook's report wins, behind a receivePackUnpackForHook opt-in so no other hook's target silently gains objects. Both tests now assert the target resolves what the refs name — the marker's commit, and every commit and tree reachable from the tip — since assertHeadsMatch compares hashes only. The resume test's "334 bytes" now means what it claimed.

2 — doubled ng. Fixed; the denier passes the bare reason like the DenyRefsReport call sites, and the pinned test now asserts the reason reaching plan.Reason is the target's, not the fixture's.

3 — temp-ref advance assumed landed. Guarded in the checkpoint loop: a refused temp ref stops the run, naming the ref and the server's reason, before current advances or the delete can carry a hash the target never accepted. Only a refusal stops it — see 4.

4 — blind without report-status. TargetReportsRefStatus is threaded from s.target.features.ReportStatus. Silence from a target that cannot report is now "unknown, so keep the marker", never "landed" and never "refused" — treating it as a refusal would fail every batched bootstrap against such a target. Pinned by an integration test; note go-git's server-side ReceivePack returns before applying any ref command when the client can't be told the outcome, so the fixture supplies that itself.

5 — benign "already exists" leaked the marker. gitproto.IsConcurrentMove is exported for that one question: the branch is on the target, so the scaffolding is stale and gets deleted rather than stranded on the route that refuses --prune.

6 — subsumed path. Same guard: a refused lone create no longer records a have or increments BatchCount.

Session-wide predicate. Taken, in the shape you suggested: gitproto.Pusher now records the rejections of its most recent Push* call and Params.RefRefused answers from that window, not from s.rejections. The recording rides the existing OnRejection callback, so a caller that installed none still gets a fatal per-ref ng rather than a quietly best-effort push — with a test for each half.

Nits. cmd.Old is now enforced, so the stale-Old CAS the fixture's comment rests on is real. The log field is per-branch (pushed_checkpoints). The DenyRefsReport overlap stays: it builds statuses but applies nothing, which is the one thing this hook exists to do — the type now says so.

Also added. The rejection-surfaced assertions (Warned/ActionWarn), and the bootstrap-package unit tests: a two-branch graph covering refused-create-keeps-marker-as-have (the multi-branch have preservation the integration tests can't reach), concurrent move, refused temp ref, unconfirmable create, refused subsumed create. Each new guard was verified to fail its test when removed. Full suite and golangci-lint green.

🤖 Generated with Claude Code

[nodo]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 864584b. Configure here.}