Merge pull request #86 from entireio/fix/already-exists-create-race

4991b7b→main·

nodo·1mo ago·3 files·+27 added/-3 removed

gitproto: classify create-side CAS races (already exists) as ErrTargetRefMoved

Changes

3

• MCHANGELOG.md+6

• internal/gitproto

• Mpush.go+17/-3

• Mpush_test.go+4

4 unmodified lines

5
6
7
8
9
10
11
12
13
14
15
16

4 unmodified lines

The format is based on [Keep a Changelog](https://keepachangelog.com/),
and this project adheres to [Semantic Versioning](https://semver.org/).

## [Unreleased]

### Fixed

- Concurrent **create** races on the target are now classified as `ErrTargetRefMoved`, matching the existing concurrent-update handling. entire-server rejects a create command (old = zero hash) for a ref that already exists with `already exists`; git-sync only plans a create for a ref it found absent at plan time, so that rejection is an unambiguous benign race — a second sync of the same repo created the ref first — exactly like the update-side `remote ref has changed`. Previously only the update reason was in `concurrentMoveMarkers`, so a create race fell through as a generic push failure and `errors.Is(err, ErrTargetRefMoved)` returned false; embedders that key redelivery/alerting off the sentinel (e.g. mirror-pipeline's worker) misclassified it as a hard sync failure. Both the create and update CAS rejections now satisfy `errors.Is(err, ErrTargetRefMoved)`.

## [0.7.0] - 2026-06-16

### Added

MCHANGELOG.md+6

197 unmodified lines

198
199
200
201
201
202
203
204
205
206
207
208
209
210
208
209
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
5 unmodified lines

231
232
233
234
235
236
237

197 unmodified lines

// concurrentMoveMarkers are receive-pack ng reasons that UNAMBIGUOUSLY mean the
// target ref changed under us between plan and push — a clean compare-and-swap /
// lease miss that a plain retry resolves. Deliberately NARROWER than
// lease miss that a plain retry resolves. "Changed" covers both an existing ref
// moving to a new tip AND a planned-absent ref appearing: each is a concurrent
// push of the same repo winning the race, and each self-heals on the next run
// (re-plan against the new state usually no-ops). Deliberately NARROWER than
// leaseFailureMarkers: "non-fast-forward" / "fetch first" are excluded because an
// update that is legitimately non-fast-forward and wasn't force-pushed looks
// identical to a race, and treating it as a benign move would mask a real
// "needs --force" failure.
//
// This set is server-specific by design. "remote ref has changed" is
// entire-server's compare-and-swap rejection (storage.ErrReferenceHasChanged) —
// the one git-sync's own targets emit, and the case that matters in practice.
// entire-server's update-side compare-and-swap rejection
// (storage.ErrReferenceHasChanged): the planned old hash no longer matches the
// target tip. "already exists" is the same CAS on the create side — git-sync
// only emits a create command (old = zero hash) for a ref it found ABSENT at
// plan time, so the server reporting it present at push time can only mean a
// concurrent sync created it first. That makes "already exists" exactly as
// unambiguous a race as "remote ref has changed", just for the create rather
// than the update path; both are the rejections git-sync's own targets emit, and
// the cases that matter in practice. (entire-server's bootstrap-mode planner has
// a separate "target ref ... already exists" *planning* error, but that never
// reaches receive-pack report-status, so it can't surface here.)
//
// "stale info" is git's force-with-lease lease-miss phrasing, kept for
// consistency with leaseFailureMarkers and defence-in-depth; note it is
// primarily a client-side status, so it may not arrive as a server ng reason on
5 unmodified lines

// RefRejectedError).
var concurrentMoveMarkers = []string{
    "remote ref has changed",
    "already exists",
    "stale info",
}

Minternal/gitproto/push.go+17/-3

591 unmodified lines

592
593
594
595
596
597
598
599
600
20 unmodified lines

621
622
623
624
625
626
627

591 unmodified lines

}{
        {"entire-server CAS rejection", "remote ref has changed", true},
        {"CAS rejection with surrounding detail", "command error on refs/heads/main: remote ref has changed", true},
        {"entire-server create-side CAS rejection", "already exists", true},
        {"create-side CAS with surrounding detail", "command error on refs/heads/PIE-11736: already exists", true},
        {"already exists case-insensitive", "Already Exists", true},
        {"force-with-lease stale info", "stale info", true},
        {"stale info case-insensitive", "Stale Info", true},
        // Deliberately NOT moves: a plain non-fast-forward that wasn't force-pushed
20 unmodified lines

Moved  bool
    }{
        {"concurrent move (remote ref has changed)", "remote ref has changed", true},
        {"concurrent create (already exists)", "already exists", true},
        {"lease miss (stale info)", "stale info", true},
        {"ambiguous non-fast-forward is not a move", "non-fast-forward", false},
        {"policy rejection is not a move", "deny updating a hidden ref", false},