Fix Batched Resume Marker on Branch Create Refusal

Fixed and pushed to PR #117 (still a draft — flip it to ready when you want review).

The bug (ENT-2060 item 3, pre-existing, not from #116): the batched cutover deleted refs/gitsync/bootstrap/heads/<branch> on the strength of a nil push error. Under BestEffort a nil error covers the request, not each command — gitproto hands a per-ref ng to OnRejection and returns nil — so a target refusing the branch create ended the run with neither branch nor marker, exit 0, and the next run re-transferred the whole history.

The fix (30c56dbb): bootstrap.Params.Rejected func(plumbing.ReferenceName) bool, answered by the syncer from the s.rejections map it already fills from the pusher's callback. The cutover consults it and skips the temp-ref delete for a create it can't confirm; the kept marker is recorded as a completed ref (it genuinely holds that hash, so it still serves as a have) and the retention is logged and printed. Conditional delete rather than dropping cleanup the way #116 did on the one-shot path — Bootstrap() rejects --prune, so prune can't be the cleaner there and the marker would become permanent. All three bootstrap entry points go through bootstrapWithInputs, so all are covered.

Tests: Sophie's pin now passes, plus …ResumesAfterRejectedCreate, which asserts the marker is worth keeping — the retry routes on bootstrap-resume-marker and finishes in 0 checkpoint packs / 334 bytes to the target vs. the first run's 427 KiB. Both share a refCreateDenier hook extracted from the pin. Verified both fail without the fix; full suite and golangci-lint green.

Two notes: I rebased the branch onto main (force-pushed with a lease, per your choice) since the resume test needs #116's routing, and I rewrote the PR title/body, which still claimed the PR was intentionally red. Remaining ENT-2060 items — subset resume, sync-mode route, --map/branch-scoped stale markers, dry-run disclosure — are untouched; say the word if you want ENT-2060 updated to reflect item 3 landing.

Yes — the existing one, updated in place: https://github.com/entireio/git-sync/pull/117

Still a draft, now with two commits:

• f03f215f Pin the batched cutover dropping its resume marker on a rejected create (Sophie's, rebased)
• 30c56dbb Keep the batched resume marker when the target refuses the branch create (the fix)

I didn't open a new PR — you asked me to add commits to that one.

All items addressed in 1aa4f3d5, pushed to PR #117.

Blocking

1. Fixture — confirmed exactly as reported: the hook's report returns before transport.ReceivePack, so the cutover pack was never unpacked. The server now unpacks pushed objects before a hook's report wins (opt-in field, so no other hook's target changes), and both tests assert the target resolves what the refs name — the marker's commit, plus every commit and tree reachable from the tip. The resume test's 334 bytes now mean what they claimed.
2. Doubled ng — fixed; bare reason like the DenyRefsReport call sites, with a test on the reason reaching plan.Reason.

Should fix

1. Temp-ref advance — guarded: a refused temp ref stops the run naming the ref and the server's reason, before current advances or the delete carries an unaccepted hash.
2. report-status — TargetReportsRefStatus threaded from s.target.features.ReportStatus. Silence is "unknown, keep the marker", never "landed" and never "refused" — the latter would fail every batched bootstrap against such a target. Integration test added; note go-git's server-side ReceivePack returns before applying ref commands when the client can't be told, so the fixture supplies that.
3. "already exists" — gitproto.IsConcurrentMove exported for that one question; the branch is present, so the marker is deleted rather than stranded on the no-prune route.
4. Subsumed path — same guard; a refused lone create no longer records a have or increments BatchCount.

Worth considering — took the per-push shape you suggested: gitproto.Pusher records its most recent Push* call's rejections and Params.RefRefused reads that window, not s.rejections. Recording rides the existing OnRejection, so a caller with none still gets a fatal per-ref ng. Plus Warned/ActionWarn assertions, five bootstrap-package unit tests over a two-branch graph (including the multi-branch have preservation the integration tests can't reach), cmd.Old CAS enforcement, and a per-branch log field. The DenyRefsReport overlap stays — it builds statuses but applies nothing, which is the one thing this hook exists to do.

Each new guard was verified to fail its test when removed. Full suite and golangci-lint green.

The problem. Copying a huge repo happens in chunks. git-sync keeps a bookmark ref on the target saying "I got this far", so an interrupted copy can resume instead of starting over. At the end it creates the real branch and deletes the bookmark.

The last step sent both commands — move the bookmark to the end and create the branch — in one request. If the target accepted the first but refused the second (protected branch, push policy), git-sync saw "no error" and deleted the bookmark anyway. Result: no branch, no bookmark, exit code 0. Everything uploaded so far was now unreferenced garbage on the target, so the next run re-uploaded the entire history — on exactly the multi-gigabyte repos this chunking exists for.

The fix. Don't delete the bookmark unless the branch actually exists. "No error" isn't proof, so git-sync now asks the target what it did with each ref and only cleans up on a yes. On a no, it keeps the bookmark and says so, and the next run finishes from where it stopped.

Review round. Three follow-ups, same root cause — assuming success from a silent server:

• A target that refuses everything refused the bookmark move too, and the run kept going as if it had worked. Now it stops.
• Some targets never report per-ref results at all. There, silence means "don't know", which now keeps the bookmark rather than deleting it — while still not failing the run.
• One rejection ("branch already exists") actually means the branch is there, so the bookmark is genuinely finished and gets cleaned up as normal.

Plus a real fixture bug: the test's fake server was skipping the step that stores uploaded objects, so the bookmark pointed at a commit the target didn't have. The tests were passing on matching hashes over missing data — they now check the objects are really there.

It already does that — the delete is a separate request sent after the create (bootstrap.go: the create rides the final pack push, then a follow-up PushCommands deletes the temp ref). Ordering was never the gap.

The gap is that the delete had no way to know the create worked. Under --best-effort, a per-ref ng goes to a callback and the push still returns nil — that's true whether the request carried one command or five. So even with the create alone in its own request, you'd get "no error" back from a target that refused it, and delete the bookmark anyway. Sequencing tells you when things happened, not whether they were accepted; only the per-ref status does that.

Two ordering-flavoured variants that would work, and why not:

• Make the create fatal (push it non-best-effort so a rejection becomes an error). But --best-effort is the user's choice, and --all-refs implies it precisely so one protected branch among hundreds doesn't fail the run. Turning that specific ref back into a hard error takes the choice away.
• Re-list the target's refs after the cutover and check the branch is there. This genuinely works — it's just a second round trip to learn what the push report already said, and it can't tell a refusal apart from a target that says nothing.

There's also the option of never deleting the bookmark and letting prune do it — which is exactly how #116 fixed the one-shot path. It doesn't port here: Bootstrap() rejects --prune, so on that route no cleaner would ever exist and the bookmark would be permanent.

The fix is: ask the target what it did with the branch, and only delete the bookmark if the answer is yes.

Concretely, three commits on soph/batched-cutover-drops-resume-marker:

• gitproto.Pusher now remembers the per-ref ng statuses from its most recent push, so a caller can ask "did the ref I just sent land?" instead of trusting the nil error.
• bootstrap.Params gains RefRefused + TargetReportsRefStatus, answered by the syncer. The batched cutover consults them before deleting the temp ref: refused → keep it and log why; target can't report → keep it (unknown ≠ success); refused with "already exists" → the branch is there, delete as normal. The same check guards the temp-ref advance (a refusal there stops the run) and the subsumed-branch create (no phantom have or batch count).
• Tests: Sophie's pin now passes, plus a resume test proving the kept bookmark finishes the import in 334 bytes rather than 427 KiB; five bootstrap unit tests; two gitproto tests; one for a target without report-status. The fake server was also fixed to actually store pushed objects, which is what made the original tests pass on missing data.

Adversarial review running in the background at xhigh against PR #117 (all three commits). I'll report the findings when it lands.

The adversarial pass found 15 issues; 14 are fixed in eeb61697, one is deliberately left open. The important one was structural:

My previous fix guessed at target state. It read the rejection's text — gitproto's concurrent-move markers, a substring heuristic built for CAS misses — as proof the branch existed. A pre-receive message like refusing to create refs/heads/main: a tag with that name already exists matched it, deleted the marker with the branch still absent, and reintroduced the exact re-transfer this PR prevents. Same flaw the other way for targets without report-status: markers kept forever on runs that fully succeeded.

Replaced with evidence: an unconfirmed create is held back, and one ref listing after the last branch settles every doubtful case — present → stale scaffolding, delete; absent → resume position, keep; listing unavailable → keep everything, run still succeeds. The confirmed path pays no extra round trip (pinned by a test), and IsConcurrentMove went back to unexported.

Also fixed, all verified in code first:

• Terminal-injection hole: the ng reason reached the notice unsanitized (gitproto stored the raw status; only OnRejection filtered), so a target could clear the warning's line and redraw it as a success. Filtered now, with a test that fails without it.
• A refused delete of a stale marker restarted from zero and wedged every subsequent run; now stops with both reasons named. A refused delete of a finished marker was silent; now surfaced non-fatally.
• A report omitting a command left that ref looking unrejected — gitproto now treats unauthored silence as a rejection.
• The misleading notice ("did not land" for creates that had landed, promising a resume that couldn't happen).
• Fixture: unsynchronized state shared with the httptest goroutine (latent under CI's -race), the no-report-status flag ignored on ref-only pushes, three drifted copies of the unpack/apply blocks now one helper each, and assertCommitHistoryPresent checking only trees while claiming blobs — it now walks trees and blobs and compares commit counts against the source.

Left open: the one-shot path still justifies never deleting its marker by deferring to prune, which Bootstrap() forbids — the same permanent-marker gap one function up, inherited from #116. Out of scope here, and now cheap to close with the same listing. Full suite green under -race, lint clean.

Three commits on top of Sophie's failing-test pin, all on soph/batched-cutover-drops-resume-marker (PR #117).

Round 1 — the bug itself (30c56dbb)

A batched bootstrap's cutover deleted its resume marker on the strength of a nil push error. Under --best-effort that error covers the request, not each command, so a target refusing the branch create ended the run with neither the branch nor the marker, reported as success — and the next run re-uploaded the whole history. Fixed by threading the target's per-ref answer into the strategy and only deleting the marker once the branch is confirmed. Conditional delete rather than dropping cleanup the way #116 did on the one-shot path, since Bootstrap() refuses --prune and would leave the marker permanent.

Round 2 — first review (1aa4f3d5)

The fixture wasn't proving anything: a hook returning its own report short-circuits the test server before it stores pushed objects, so the marker pointed at a commit the target didn't have and the tests passed on matching hashes over missing data. Also three more paths inferred "landed" from a nil error — the temp-ref advance, the subsumed-branch create, and the case where the target never advertises report-status. Plus: the predicate read a session-wide rejection map instead of the push just made, and the fixture doubled the ng prefix and ignored cmd.Old.

Round 3 — adversarial review (eeb61697)

My round-2 fix guessed at target state: it read the rejection text as proof the branch existed, so a pre-receive message containing "already exists" deleted the marker with the branch still absent — the exact data loss the PR prevents. Replaced with evidence: unconfirmed creates are held back, one ref listing after the last branch settles them all, and the confirmed path still costs no extra round trip. Also a terminal-injection hole (the ng reason reached the notice unsanitized, so a target could redraw a warning as a success), a refused stale-marker delete that wedged every later run, a silent refused cleanup, reports that omit a command, a misleading operator notice, an unsynchronized fixture field latent under CI's -race, and a history assertion that claimed to check blobs but didn't.

Left open: the one-shot path still defers marker cleanup to prune, which Bootstrap() forbids — the same gap one function up, inherited from #116. Flagged in the PR, not fixed here.

Every guard was verified to fail its test when removed. Full suite green under -race, golangci-lint clean.

The bug: copying a huge repo leaves a bookmark saying "got this far". At the end git-sync created the branch and deleted the bookmark — but it deleted it even when the target had refused the branch, because a refusal still looks like success in best-effort mode. Result: no branch, no bookmark, exit 0, and the next run re-uploaded everything.

What I fixed, in three passes:

1. Only delete the bookmark once the branch is actually confirmed to exist.
2. Two more places made the same assumption (the bookmark's own updates, and targets that report nothing). Also: the test's fake server wasn't storing uploaded data, so the tests were passing on missing content.
3. My round-2 check guessed from the wording of the refusal — "already exists" could appear in a message where the branch was still missing, which would delete the bookmark and cost a full re-upload. Now git-sync just asks the target which branches it has. Plus a security fix: the target's error text was printed to the terminal unfiltered, so it could disguise a warning as a success.

Still open: the same cleanup gap exists in the simpler non-chunked path, inherited from the earlier PR. Flagged, not fixed here.

Race-focused adversarial review running in the background against PR #117. I'll report what it finds.

No data race exists — the reviewer verified that every push, callback, and marker decision runs on the single goroutine driving Execute, and last round's fixture mutex holds under -race. What it found was ordering, plus one rule I had backwards. Fixed in ec4a45fc.

My "silence = refusal" rule was wrong. Last round I made gitproto synthesize a rejection for a command the target's report omitted. That contradicted the guard two lines above it: an omitted status on a temp ref would have aborted a multi-GB import over something nothing refused, and it printed a client-invented reason as though the target had said it. Silence is now a third answer — Pusher.LastOutcome reports applied / refused / unknown, and unknown means "go look", never "failed" and never "fine". That also deleted Params.TargetReportsRefStatus: the Pusher holds the advertisement, so the capability question is its own, and it answers by value so no caller can mutate its state.

The cleanup deleted the marker whenever the branch name existed, at any hash. If a concurrent sync had created the branch at an older commit, our marker held the only reference to everything above it — and we deleted it, causing exactly the re-transfer this PR prevents, in the one case where the import really hadn't landed. Cleanup now requires the branch to be at the hash this run pushed. The test that asserted the old behaviour had seeded an unrelated hash, so it was asserting the bug; it now covers both sides.

Ordering fixes: the settlement ran after the tail phase, leaving the listing minutes stale while tags pushed — long enough for a concurrent run to adopt the marker before we deleted it (now runs right after the last create); the deferred delete was the only new push not checked for refusal; createConfirmed could read the previous branch's answer on the one path where a create plan exists but no push carries it; and the tail phase re-derived haves from the plans, re-claiming the creates the cutover withheld.

Smaller: nil-target guard, a documented receive.hideRefs caveat (safe direction — a hidden branch reads as absent and keeps a marker that wasn't needed), a redundant wrapper removed, and three pre-existing fixtures that emitted the doubled ng prefix this PR itself warns about.

Full suite green under -race, lint clean, each new guard verified to fail its test when removed.

One finding left open, and it's your call: the one-shot path still defers marker cleanup to a prune that Bootstrap() forbids — the same permanent-marker gap, inherited from #116, where you deliberately chose "defer to prune, cost one run". It's now ~25 lines with the same listing, but it changes semantics your merged PR tested, so I left it. Say the word and I'll close it.

Done in fb287a6c. One correction on the first item:

RefRefused and TargetReportsRefStatus no longer exist — ec4a45fc replaced them with a single Pusher-side tri-state, so the doc-vs-code disagreement is already gone and your probe no longer reproduces: a target that can't report per-ref status is now settled by the ref listing, and the marker is deleted once the branch is confirmed at the pushed hash. (I did find and fix a stale see Params.RefRefused pointer in the cutover comment.)

What survives is your permanence remark, and it deserved stating outright. keepMarker now documents it: on replicate/sync a marker kept unnecessarily is reaped by the next prune; on the bootstrap route nothing reaps it, and against a target that neither reports per-ref status nor answers a listing it's permanent — accepted in that order of harm, since a marker is inert (the resume route needs the branch absent, so it can't misroute) while a marker deleted in error costs a full re-import.

Changelog — added an ## [Unreleased] section with the fix and, separately, the exit-code change you flagged: a refused temp-ref update now fails the run even under --best-effort, so a target blocking refs/gitsync/* moves from 0-with-warnings to non-zero, called out for anyone alerting on exit codes.

The "PACK" scan — fixed properly rather than noted: it now walks the pkt-line framing to the flush that ends the command list. The request router's own hasPack test used the same scan, so both share the helper and the footgun is gone from both, with a test that pushes a ref literally named refs/heads/PACKAGING.

Full suite green under -race, lint clean. The one-shot marker cleanup on the no-prune route is still the only open item, still your call on #116's tradeoff.

Done and green — it's waiting on you, not on me.

PR #117: open, no longer a draft, mergeable, awaiting review. 6 commits — Sophie's failing-test pin plus four from me.

• The bug is fixed and the pin passes.
• Four review rounds, all findings resolved (last one, fb287a6c, was docs + changelog + a fixture nit).
• Full suite green under -race, golangci-lint clean, every guard verified to fail its test when removed.
• Changelog has an Unreleased entry, including the one behavior change worth flagging: a target blocking refs/gitsync/* now exits non-zero instead of 0-with-warnings.

Nothing is blocking. The only open item is a judgment call I deliberately left to you: the one-shot path still defers marker cleanup to a prune that Bootstrap() forbids — the same gap, inherited from #116, where you chose "defer to prune, cost one run". ~25 lines to close with the machinery now in place. Reasonable to merge as-is and file it as a follow-up.

I'd stop reviewing here — the returns have flattened out (the last round produced one stale doc pointer and a test-fixture nit).

https://github.com/entireio/git-sync/pull/117

When a target refuses the branch create at the end of a chunked bootstrap, git-sync deleted the bookmark recording how far the import got — silently throwing away the whole transfer and forcing the next run to re-upload the entire history, on exactly the huge repos chunking exists for.

Only delete the bookmark once the branch is actually on the target at the hash this run pushed — and when the push can't establish that, ask the target for its refs rather than guessing.

Concretely:

• The push answers per ref, not per request: Pusher.LastOutcome reports applied, refused, or nothing said for each ref, because in best-effort mode a refusal still returns no error.
• Applied → delete the bookmark immediately (the common path, no extra round trip).
• Refused or unknown → don't guess. One ref listing after the last branch settles it: branch there at our hash → the bookmark is spent, delete it; anything else → it's the resume position, keep it and say why.
• Never infer from wording. An earlier version read "already exists" as proof the branch existed; a pre-receive message containing that phrase would have deleted the bookmark with the branch still missing.
• Fail loudly where there's no partial progress to keep: a target that refuses the bookmark's own updates stops the run instead of advancing state it never accepted.

Yes — and the hash check is what makes the failure direction safe. Four cases:

Someone advances the branch between our create and our listing. We see a hash that isn't ours, so we keep the marker even though we didn't need to. Their tip is a descendant of ours, so nothing is at risk — the cost is leftover scaffolding (reaped by prune on the replicate/sync routes, permanent on the bootstrap one). This is the conservative direction, and it's where the check lands whenever anything unexpected happened.

Someone creates or force-pushes the branch to an older commit. Also a mismatch, also kept — and here keeping is required: the span between their tip and ours is reachable from the marker and nothing else. This is the case the previous version got wrong by treating "branch name exists" as good enough.

Someone moves the branch after our listing but before our delete. Real TOCTOU, and not fixable without a lock. But it's indistinguishable from a force-push one second after any clean run finishes and legitimately deletes its marker — objects becoming unreferenced when someone rewrites a branch is ordinary git, not a git-sync fault. I narrowed the window from "after the tail phase" (minutes of tag pushing) to "immediately after the last create" in the last round precisely because of this.

Someone moves or deletes the marker itself — another git-sync run resuming. Our delete carries Old: resumeHash, so a compare-and-swap failure means we can't clobber their advanced resume position, and that refusal is now logged and surfaced rather than silently reported as cleanup.

The one residual harm is a concurrent run that adopted the marker and is mid-resume at the same hash when we delete it: its next push CAS-fails and it starts over. That requires it to have routed before our create landed (routing needs the branch absent, and our listing just saw it present), and two concurrent bootstraps of the same target branch aren't a supported shape — the marker namespace is designed to have one writer per branch.

Now the subsumed path — make it consistent with the cutover: