[grug] Elder deadlocks: same SHA + changed snapshot_id read as "superseded by a newer commit", cancels then re-enqueues forever

[quadseven]

Why

Elder never completed on quadseven/infra#1994 - pending for 20+ minutes on a
2-file XS diff, then still pending. It is not slow, it is in a closed loop.

Verbatim from grug-consumer, all within 2 seconds:

elder_review_cancelled_mid_flight    repo=quadseven/infra pr=1994
llm_backend_transport_failed         backend=cave kind=RequestError
code_review_llm_degraded             kind=all_failed
                                     error="cancelled: superseded by a newer commit"
code_review_stale_before_publish
    reviewed_head_sha:    51a25556
    current_head_sha:     51a25556      <-- IDENTICAL
    reviewed_snapshot_id: v1:85db95f6
    current_snapshot_id:  v1:afb18d95   <-- only this differs
elder_review_enqueued                head_sha=51a25556 snapshot_id=v1:afb18d95
elder_in_progress_check_skipped      reason=already_in_progress

The loop

1. A review is in flight for 51a25556.
2. The snapshot id changes while it runs (v1:85db95f6 -> v1:afb18d95) with
   no new commit - head_sha is byte-identical on both sides.
3. code_review_stale_before_publish treats that as staleness and discards the
   result. The user-facing error even says "superseded by a newer commit",
   which is factually wrong - there is no newer commit.
4. It re-enqueues against the SAME sha with the new snapshot id.
5. elder_in_progress_check_skipped: already_in_progress then declines to post
   the check-run, because one is still open from step 1.
6. Back to 1.

Net effect: the check-run sits pending indefinitely and no findings are ever
published. On a repo where Elder IS a required check, that bricks the merge.

Why it matters beyond one PR

repos.py:126-131 justifies Elder being advisory precisely so "a stuck/slow/
LLM-brownout review must not brick merges for an hour". That escape hatch worked
here - infra only requires Grug - Chief, so #1994 could be merged. But the
escape hatch is compensating for a bug, and any repo that promotes Elder to
required inherits an unbounded stall.

It also cost real time today: #1994 was the fix for a completely broken
Pulumi stack (every apply failing), and it sat behind a phantom review.

What

1. Stop treating a snapshot-id change as a new commit. Staleness should key
on head_sha first. If reviewed_head_sha == current_head_sha, the review is
NOT superseded, whatever the snapshot id says. At minimum, correct the message -
"superseded by a newer commit" when the SHA is unchanged sent me looking for a
push that never happened.

2. Break the deadlock explicitly. already_in_progress must not be able to
suppress the publish for a review that has just been cancelled. Either cancel the
in-flight check-run when re-enqueuing, or let the newest generation take over the
existing check-run rather than declining to post.

3. Bound it. A review that re-enqueues against an unchanged SHA more than N
times should fail the check-run with a real message instead of staying pending
forever. A stuck check that says nothing is worse than one that says "I gave up".

4. Look at llm_backend_transport_failed backend=cave. The cancel cascade
started from a cave transport failure. Whether cave contention is causing the
mid-flight cancellations, or merely co-occurring, is worth establishing - the
Archimagirus judge was already disabled over shared-vLLM contention with Grug.

Acceptance criteria

• A snapshot-id change with an unchanged head_sha does NOT cancel an
  in-flight review.
• Cancel + re-enqueue on the same SHA still publishes a check-run - no
  already_in_progress suppression deadlock.
• Repeated re-enqueues on an unchanged SHA terminate with a failed/neutral
  check carrying a real reason, never indefinite pending.
• Repro fixture: in-flight review + snapshot-id change, no new commit.

Related

• [grug] Metric for abandoned mid-flight Elder generations still competing at spark-gateway #638 (metric for abandoned mid-flight Elder generations still competing) -
  this is a live instance of exactly that, and the metric would have made it
  visible.
• [grug] [arch] Own the Elder snapshot lifecycle in one module (eligibility, lease, freshness, terminal transition) #583 (own the Elder snapshot lifecycle in one module) - the snapshot-vs-sha
  conflation here is a symptom of that lifecycle being spread out.

Size: M

[github-actions]

🚧 Reopened by the close-completeness guard — closed as completed but still has 4 unchecked item(s):

• A snapshot-id change with an unchanged head_sha does NOT cancel an
• Cancel + re-enqueue on the same SHA still publishes a check-run - no
• Repeated re-enqueues on an unchanged SHA terminate with a failed/neutral
• Repro fixture: in-flight review + snapshot-id change, no new commit.

Finish + check them, or close it as "not planned" / add a force-close label. Exempt sections (Out of scope / Blocked by / Reverse-if-wrong / Candidate / Further notes) aren't counted.