fix(dev-lead): a bot-comment retry cancelled before it starts no longer holds the pending window or an attempt (#2262)

Merged
DO
10d ago
+1214-17

Summary

The bot-comment retry sweep now knows what happened to the run it dispatched.

  • After a successful dispatch the sweep finds the run it created (the dev-lead caller stub's repository_dispatch run titled dev-lead-reviews-retry, created at or after the dispatch) and records it on the retry marker as run=<id>. It records only when exactly one candidate exists; otherwise the marker keeps today's time-based behaviour. The marker is edited, not re-posted, because the caller stub subscribes to issue_comment created only, so the edit starts no run in the lane.
  • On later scans it reads each recorded run. A run that completed as cancelled with no job having run a step is lost: it is not pending and does not count toward BOT_COMMENT_RETRY_MAX_ATTEMPTS.
  • A recorded run that is still queued or running now holds every comment on the PR, bounded by the pending window.
  • After a lost retry, the PR's next retry waits BOT_COMMENT_RETRY_LOST_BACKOFF_SEC (default: the 15-minute MIN_AGE).
  • BOT_COMMENT_RETRY_MAX_TOTAL still counts every marker, lost ones included, so a lane that keeps cancelling is bounded.
  • bcr_retry_decisions stays pure: the run states are passed in as an optional sixth argument, and all network calls are in the sweep.

Problem

Closes #2262. A retry's dispatched run joins the PR's concurrency lane, where GitHub keeps one pending run. When a newer event cancelled it before any step ran, the sweep still treated the marker as "queued or running" for 150 minutes and counted it as one of two attempts.

Reading the real runs for the issue's evidence changed the picture: on PR #2205 the retries cancelled each other. Run 38073395880 (the "CodeAnt Nitpicks" retry) was created at 17:50:45 and cancelled at 18:00:00, three seconds after the sweep posted the retry marker for the second comment at 17:59:57. The pending check was per comment, so the next scan dispatched for comment B while comment A's retry was still queued. That is why this change adds the PR-wide hold and the backoff: releasing a cancelled retry promptly with nothing else would turn the loss into a ping-pong up to MAX_TOTAL.

Risk

Medium. It changes when the sweep dispatches fix-bot-comment retries, in scripts/dev-lead-retry.sh and scripts/lib/bot-comment-retry.sh. Every unknown falls back to today's behaviour: a marker with no run=, a run missing from the map, an unreadable Actions API, a run cancelled after it started, a truncated listing. A non-numeric run= is ignored, and run ids are taken only from markers that pass the existing trusted-login rule. Older readers are unaffected: run= is a trailing attribute, and the unchanged library decides identically on markers that carry it.

Known limits, not addressed here:

  • A dispatch that creates no run at all is not detected; an empty listing cannot be told apart from a filter mismatch, and reading it as lost would duplicate live runs. So acceptance criterion 1 is met for a cancelled run and not for "never started".
  • Markers already on open PRs carry no run= and keep the time-based behaviour, as does any dispatch whose run cannot be paired (two retries reaching one repo within a few seconds).
  • Posting a retry marker still fires issue_comment.created into the lane. That is existing behaviour.
  • The bot-thread retry (#2046) has the same blind spot for its own cancelled run; bot-thread-retry.sh is untouched.
  • The sweep needs to read Actions runs in each target repo. A classic PAT with repo can; a fine-grained token needs Actions read. If it cannot, each affected scan logs a ::warning:: and keeps today's behaviour. Private target repos are unverified.
  • Pairing can still record another PR's run when two retries reach one repo within seconds and ours is listed later than both looks. The effect is bounded by BOT_COMMENT_RETRY_MAX_TOTAL.
  • The recording path has not run against a real dispatch yet. That needs a live check once a channel carries it.

Test plan

  • 19 new bats tests: 16 in tests/dev-lead/unit/test_bot_comment_retry.bats, 2 in test_dev_lead_retry.bats, 1 in test_intent_bot_comment_retry.bats. They cover a cancelled retry being re-dispatched, cancelled retries not exhausting the attempt limit, the hard bound, and a live run not being duplicated, at the pure-decision level and through the sweep with a stubbed gh. No existing test line is changed.
  • bats on the three touched files: 86/86, 36/36, 23/23 (run twice, by the implementing agent and again independently).
  • Counterfactual: with the scripts/ hunks reverse-applied, all 19 new tests fail and the 126 existing ones pass.
  • A wider run of 18 related suites: 611 passed, 0 failed.
  • shellcheck --severity=warning -x on both scripts: clean. scripts/check-retry-payload-fields.sh and scripts/check-duplicate-decls.sh: pass.
  • Read-only against the real API: the state reader returns cancelled-and-not-started for runs 38073395880 and 38077617157 and success for 38073354743; the pairing function picks exactly 38073395880 from the real listing for the evidence window.

Review follow-up (b816e34b)

  • Backoff longer than the pending window (CodeAnt). bcr_retry_run_ids reads a retry's run inside the longer of the two windows, so a lost run past the pending window is still known lost and the backoff holds.
  • Skipped steps (CodeAnt). A job whose only steps were skipped no longer counts as started. A cancelled step, or one with no conclusion, still does: it may have begun.
  • Pairing the wrong run (CodeAnt, QA Lead). A run is recorded only when two looks in a row show the same single candidate. One look could record another PR's retry run that was listed before this dispatch's own. This narrows the race and does not close it: if our run is listed later than both looks, or never, the other run is still the only candidate. Added to the known limits below.
  • Lookup time (CodeAnt). BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC (default 1200): once the scan has run that long, a dispatch records no run instead of waiting, so the waits cannot use up the workflow's 30-minute limit. The confirming look adds one wait (5 s by default) to each dispatch.
  • Real API data (QA Lead). tests/fixtures/bot-comment-retry/ holds what the Actions API returned on 2026-10-11 for PR #2205's cancelled retry run 38073395880, its jobs, and the event=repository_dispatch listing around it, cut down to the fields read. The pairing and the run-state reader are tested against it. The server-side created= and event= filters were exercised by that read; the tests still use a fake gh that ignores them.
  • Cancelled before any job existed (QA Lead): a sweep-level test pins that it reads as lost.
  • Not done: the rollout check QA Lead suggests (compare [run] lines with dispatches for a few days after promotion and treat 0% as a failed rollout). It is a step for whoever promotes the channel.
  • 7 new tests; 5 fail on the previous scripts, and the fixture test and the "no job existed" test pass either way. test_bot_comment_retry 93/93, test_dev_lead_retry 36/36, test_intent_bot_comment_retry 23/23. shellcheck, check-retry-payload-fields.sh and check-duplicate-decls.sh: clean.

Rollback

Revert this PR. Markers that already carry run= are read by the older code exactly as before.

Monitoring

The dev-lead-retry workflow log: [run] ā€¦ retry attempt N is Actions run <id> after a dispatch, [lost] ā€¦ was cancelled before it started on a later scan, and the ::warning:: lines when a run cannot be read. Inert until the dev-lead channel tag moves onto it (next autocut to dev-lead/v139-next, then ring promotion); pr-review's own channel also runs this scan.

Interaction contract

  • N/A: no agentic role added or changed.

šŸ¤– Generated with Claude Code