Reconcile CAS races against concurrent writers#8
Closed
nodo wants to merge 5 commits into
Closed
Conversation
When two writers push the same source state concurrently (common with multiple mirror-worker pods handling bursty webhook fan-out), receive-pack returns per-ref "remote ref has changed" from the loser's CAS. Today this bubbles up as a sync error even though the target is already where we wanted it. Surface report-status per-ref failures as a structured gitproto.PushReportError, and add an internal/strategy/pushreconcile helper that, on such an error, re-advertises the target and checks whether every failed ref already matches the plan's intended outcome. Wired into the replicate, incremental, and materialized strategies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address adversarial review findings on the previous commit:
1. pushreconcile.Check now gates reconciliation on an allowlist of CAS-race
statuses ("remote ref has changed", "already exists"). Previously any
per-ref rejection — ACL denials, hook/policy failures, etc. — was
silently swallowed if a concurrent writer happened to converge the ref
to the planned hash, hiding real misconfiguration behind "reconciled"
successes.
2. buildReportError now treats any unpack status other than exact "ok" as
fatal, including empty string. Previously a malformed/degraded
receive-pack response with a blank unpack line could fall through as
success if no per-ref failures were reported, weakening detection
relative to go-git's ReportStatus.Error semantics.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: bd56885bd5b7
Collaborator
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ 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 4fc3761. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two concurrent push using mirror-worker now could cause a race, e.g.
This change changes git-sync to check if the new ref is actually what we need if that's the case just succeed.
Note
Medium Risk
Changes push error semantics and makes strategies treat some receive-pack failures as success after a target ref refresh; incorrect reconciliation logic could mask real push failures or misreport relay outcomes.
Overview
Adds push reconciliation to treat benign CAS races as success.
gitprotonow surfaces structuredreceive-packreport-statusfailures viaPushReportError(unpack vs per-ref failures) and exposesPusher.ListRefs()to re-advertise target refs.A new
strategy/pushreconcilehelper checks per-ref failures by re-listing target refs and verifying the target already matches each plan’s intended outcome (including deletes), logging warnings for unexpected status strings. Theincremental,replicate, andmaterializedstrategies (andsyncer) are updated to pass a lister and to swallow reconciled push errors while setting relay reason toreconciled; extensive tests cover reconciliation success/failure paths and the new error shaping.Reviewed by Cursor Bugbot for commit 4fc3761. Configure here.