can you review https://github.com/entirehq/mirror-pipeline/pull/135 · Entire
PR #135 Review — worker: classify concurrent-ref pushes as raced
Verdict: 🔴 Request changes — one blocker, everything else optional polish
The worker-side design is sound — the raced disposition, the Nak→Term escalation, the nak+term-numerator carve-out, the fail-open parity, the log-level split. But the typed sentinel the entire feature hinges on never actually attaches on the real receive-pack path, so in production this PR changes nothing — every real CAS miss still classifies nak, still logs ERROR "Sync failed", still feeds the alert. CI is green only because every test fabricates the error.
🔴 Blocker — the feature is dead code in production
errors.As pointer-vs-value mismatch in vendored git-sync — vendor/entire.io/entire/git-sync/internal/gitproto/push.go:238 (asRefRejectedError), consumed at push.go:302.
asRefRejectedError (and the pre-existing annotateLeaseFailure at push.go:133) do:
var cs *packp.CommandStatusErr // target type: *CommandStatusErr
if !errors.As(err, &cs) { return err } // never matches → returns early
But go-git v6 returns CommandStatusErr by value — value receiver Error() (report_status.go:35), constructed by value at report_status.go:185. errors.As against a **CommandStatusErr target can't match a value-typed CommandStatusErr in the chain, so it returns early, *RefRejectedError is never built, moved is never set, and errors.Is(err, ErrTargetRefMoved) is always false. isConcurrentRefError → always false in prod.
I verified this empirically — a throwaway test driving the exact error go-git hands git-sync:
dynamic type of report.Error() = packp.CommandStatusErr (value)
errors.As(err, **CommandStatusErr) = false ← git-sync's actual target
errors.As(err, *CommandStatusErr) = true ← the fix
errors.Is(wrapped, gitsync.ErrTargetRefMoved) = false ← real path, sentinel lost
And confirmed the worker uses neither BestEffort nor OnRejection nor force flags, so asRefRejectedError at push.go:302 is the sole attachment path for it — and it's the broken one. (The only other wrap, syncer.go:475, needs BestEffort + ForceWithLease, which the worker never sets.)
Why it's a true blocker: the PR's stated goal is killing ~304 spurious ERROR/nak ticks per 7d. With the sentinel never attaching, that number doesn't move. The new outcome:raced metric, entire.sync.raced tag, and shouldMarkSpanError carve-out are all inert. The same bug also means the lease-hint annotation has silently never fired.
Fix (in git-sync, then re-vendor — you already control the pin):
asRefRejectedErrorannotateLeaseFailure:var cs packp.CommandStatusErr(value target).- Add a git-sync test that drives a real
report.Error()throughasRefRejectedErrorand assertserrors.Is(ErrTargetRefMoved)— not a pre-wrapped sentinel. - In the worker, add one test feeding a value
packp.CommandStatusErr{Status:"remote ref has changed"}through the chain, so this exact regression is caught instead of papered over byfmt.Errorf("...%w", ErrTargetRefMoved).