Repository navigation
fix(dev-lead): a bot-comment retry cancelled before it starts no longer holds the pending window or an attempt (#2262) - #2267
Conversation
…er holds the pending window or an attempt (#2262) A fix-bot-comment retry joins the PR's concurrency lane, where GitHub keeps one pending run, so a newer PR event cancels it before a step runs. The sweep only saw its own marker: the cancelled retry stayed "pending" for 150 minutes and used up one of the two attempts. The sweep now records the run each dispatch created on the retry marker (run=<id>, by editing the marker) and reads recorded runs back on later scans: - a run cancelled with no job step run is lost: not pending, and not counted toward BOT_COMMENT_RETRY_MAX_ATTEMPTS. It still counts toward BOT_COMMENT_RETRY_MAX_TOTAL, and the PR's next retry waits out a short backoff, so a lane that keeps cancelling stays bounded; - a run still queued or running holds every comment on the PR, so one comment's retry no longer cancels another's queued run (the PR #2205 sequence); - a run that cannot be paired with its dispatch (none, or more than one candidate) or cannot be read keeps the time-based behaviour. An API error is never read as "cancelled". bcr_retry_decisions stays pure: run states are passed in as data. Markers without run= parse as before, and an older parser reads a marker with run= unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 6 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
CodeAnt Nitpicks2 code suggestions1. When run listings lag, three serial sleeps add 15 seconds per dispatched PR; enough retries can exhaust the workflow’s 30-minute limit before later PRs are scanned.Performance · 2. A two-second lookback can match another PR’s retry before this dispatch appears; recording that run makes this PR’s later decisions depend on another PR’s state.Race condition · |
CodeAnt PR Risk: Medium Risk
Assessed commit: |
|
dev-lead is withholding action on this item. It is labeled To re-enable automated pickup: remove the |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-11T05:05:33Z. |
QA Lead — test-risk advisoryRisk tier: MEDIUM. This changes when automated retries are dispatched. If it goes wrong, the result is duplicate or cancelled agent runs, still capped by What I'd shore up (highest leverage first):
Escalate? No. The tests are thorough (19 new, with a counterfactual run and fail-safe defaults). The gaps above are worth closing but don't need to block the merge. Advisory only — I comment, I do not change code. Ground: BMAD Test Architecture (bmad-tea v1.19.0). Opt out on this item with the qa-lead:hands-off label. |
- bcr_retry_run_ids reads a retry's run inside the lost-retry backoff as well as the pending window. With a backoff longer than the pending window a lost run past the pending window was never read, so the backoff could not hold. - bcr_run_state no longer counts a job whose only steps were skipped as started. A cancelled step, or one with no conclusion, still counts as run. - record_bot_comment_retry_run records a run 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. The second look narrows that race; a run of ours listed later than both looks still leaves the other as the only candidate. - BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC (default 1200): past it a dispatch records no run instead of waiting, so lookup waits cannot use up the retry workflow's 30-minute limit. - Tests: 7 new, 5 of which fail on the previous scripts. One runs the pairing and the run-state reader against the Actions API's real answers for PR #2205's cancelled retry (tests/fixtures/bot-comment-retry/). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
2 participants
Summary
The bot-comment retry sweep now knows what happened to the run it dispatched.
repository_dispatchrun titleddev-lead-reviews-retry, created at or after the dispatch) and records it on the retry marker asrun=<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 toissue_commentcreatedonly, so the edit starts no run in the lane.BOT_COMMENT_RETRY_MAX_ATTEMPTS.BOT_COMMENT_RETRY_LOST_BACKOFF_SEC(default: the 15-minuteMIN_AGE).BOT_COMMENT_RETRY_MAX_TOTALstill counts every marker, lost ones included, so a lane that keeps cancelling is bounded.bcr_retry_decisionsstays 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-commentretries, inscripts/dev-lead-retry.shandscripts/lib/bot-comment-retry.sh. Every unknown falls back to today's behaviour: a marker with norun=, a run missing from the map, an unreadable Actions API, a run cancelled after it started, a truncated listing. A non-numericrun=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:
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).issue_comment.createdinto the lane. That is existing behaviour.bot-thread-retry.shis untouched.repocan; 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.BOT_COMMENT_RETRY_MAX_TOTAL.Test plan
tests/dev-lead/unit/test_bot_comment_retry.bats, 2 intest_dev_lead_retry.bats, 1 intest_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 stubbedgh. No existing test line is changed.batson the three touched files: 86/86, 36/36, 23/23 (run twice, by the implementing agent and again independently).scripts/hunks reverse-applied, all 19 new tests fail and the 126 existing ones pass.shellcheck --severity=warning -xon both scripts: clean.scripts/check-retry-payload-fields.shandscripts/check-duplicate-decls.sh: pass.Review follow-up (b816e34)
bcr_retry_run_idsreads 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.skippedno longer counts as started. A cancelled step, or one with no conclusion, still does: it may have begun.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.tests/fixtures/bot-comment-retry/holds what the Actions API returned on 2026-10-11 for PR feat: implement issue #1787 — calibrate solution-architect escalation so conforming changes are not escalated #2205's cancelled retry run 38073395880, its jobs, and theevent=repository_dispatchlisting around it, cut down to the fields read. The pairing and the run-state reader are tested against it. The server-sidecreated=andevent=filters were exercised by that read; the tests still use a fakeghthat ignores them.[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.test_bot_comment_retry93/93,test_dev_lead_retry36/36,test_intent_bot_comment_retry23/23.shellcheck,check-retry-payload-fields.shandcheck-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-retryworkflow log:[run] … retry attempt N is Actions run <id>after a dispatch,[lost] … was cancelled before it startedon 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 todev-lead/v139-next, then ring promotion); pr-review's own channel also runs this scan.Interaction contract
🤖 Generated with Claude Code