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):

  1. asRefRejectedError
  2. annotateLeaseFailure: var cs packp.CommandStatusErr (value target).
  3. Add a git-sync test that drives a real report.Error() through asRefRejectedError and asserts errors.Is(ErrTargetRefMoved) — not a pre-wrapped sentinel.
  4. 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 by fmt.Errorf("...%w", ErrTargetRefMoved).