fix(scheduler): don't report a rejected run cancellation as cancelled - #1712
Merged
Merged
Conversation
force_cancel_workflow_runs() returns a {run_id: failure_reason} dict for
cancellation calls GitHub actually rejects, but cancel_stale_pr_runs(),
cancel_stale_opencode_runs() (via force_cancel_workflow_run_refs()), and
dispatch_strix_evidence()'s busy-run exclusion set all discarded that
return value and treated every requested run_id as gone. A rejected
cancellation could therefore let a duplicate review dispatch alongside a
run that was, in fact, still active.
force_cancel_workflow_run_refs() now returns only the refs GitHub actually
cancelled; its three callers (cancel_stale_opencode_runs,
dispatch_opencode_review, dispatch_strix_evidence) and the sibling direct
caller cancel_stale_pr_runs() use that corrected result instead of
assuming success.
Discovered mid-flight during PR #1669's development (the naruon
headRefOid cancellation fix) and intentionally scoped out of that PR to
keep its diff to the bug it was opened for; landing fresh here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
📝 Info: Ignored result does not alter dispatch
dispatch_opencode_review uses force_cancel_workflow_run_refs only for side effects. Its dispatch decision depends solely on separately classified same-head runs.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+3156
to
+3161
| cancelled_ids = {run_id for _, run_id in cancelled_refs} | ||
| busy_refs = [ | ||
| (dispatch_repo, str(run_data["id"])) | ||
| for run_data in active_workflow_runs(dispatch_repo) | ||
| if run_data.get("id") | ||
| and str(run_data["id"]) not in stale_ids | ||
| and str(run_data["id"]) not in cancelled_ids |
Contributor
There was a problem hiding this comment.
📝 Info: Busy filtering preserves cancellation races
The second Strix inventory can briefly retain successfully cancelled runs. cancelled_ids excludes those entries while keeping rejected cancellations busy.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added
area: ci-cd
ChatGPT Codex Connector
seonghobae
added a commit
that referenced
this pull request
Sep 2, 2026
…l-failure fix The org's own automated PR review/fix loop had already reconciled this branch with main once (commit c946c7b), but that reconciliation silently regressed the actual headRefOid fix back to the pre-fix buggy pattern (str(pr.get("headRefOid") or "").lower()) in stale_pr_run_ids() and active_review_run_refs(), and lost the entire live-revalidation safety net (_direct_pr_run_still_superseded, _review_run_still_superseded, _cancel_revalidated_review_run_refs). Its own attempt to push a corrected commit then failed closed (correctly) when its configured push credential was unavailable, leaving the branch stuck in the regressed state. This commit resolves a fresh merge of current main (with #1707/#1702/#1704/ #1711/#1712 all applied) directly against a37a428 -- this branch's last verified-good commit (100% coverage, 2614 tests, real regression tests reproducing the naruon PR #1528 incident) -- rather than building on top of the already-regressed c946c7b. Combines both fixes at every cancellation call site (cancel_stale_pr_runs, cancel_stale_opencode_runs, _cancel_revalidated_review_run_refs): PR #1669's live revalidation immediately before each destructive cancel call (closing the TOCTOU gap a snapshot-only headRefOid check can't), and #1712's check of force_cancel_workflow_runs's actual per-run failure result (so a run proven stale but whose cancel API call GitHub itself rejected is never reported as cancelled). #1712's simpler force_cancel_workflow_run_refs wrapper is removed as dead code now that its call sites all use the more thorough per-caller revalidation; its own tests were adapted to target the functions that actually carry its safety guarantee forward, not deleted. Verified: PYTHONPATH=. coverage run -m pytest tests -- 2621 passed, 1 skipped, 21 subtests; coverage report -- 100% on scripts/ci; interrogate -- 100% docstrings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
force_cancel_workflow_runs()returns a{run_id: failure_reason}dict forcancellation calls GitHub actually rejects (it never raises for those). Three
call paths discarded that return value and treated every requested
run_idas cancelled anyway:
cancel_stale_pr_runs()— callsforce_cancel_workflow_runs()directly andreturned the full requested
run_idslist unconditionally.cancel_stale_opencode_runs()— routes throughforce_cancel_workflow_run_refs()(a thin per-repo batching wrapper around
force_cancel_workflow_runs()) andreturned the full requested ref list unconditionally.
dispatch_strix_evidence()— used the raw stale-ref id set (not the actually-cancelled set) to exclude runs from its target-repository "busy" check, so a
run GitHub failed to cancel could be wrongly excluded from that busy check.
A rejected cancellation was therefore reported to callers as if the run were
gone, which could let a duplicate review dispatch alongside a still-running
one instead of correctly detecting it as still busy.
Fix
force_cancel_workflow_run_refs()now returns the list of refs GitHubactually cancelled (excluding any present in the per-repo failure dict),
instead of
None. Its three callers (cancel_stale_opencode_runs,dispatch_opencode_review,dispatch_strix_evidence) and the sibling directcaller
cancel_stale_pr_runs()now build their results from that correcteddata instead of assuming every requested run_id succeeded — the root-cause
fix applied once at the shared choke point rather than patched separately at
each caller.
Regression tests
Three new tests, each mocking
force_cancel_workflow_runsto reject exactlyone run id among several (proving partial-failure handling, not just
all-succeed/all-fail):
test_cancel_stale_pr_runs_preserves_failed_cancellationtest_cancel_stale_opencode_runs_preserves_failed_cancellationtest_cancel_revalidated_review_run_refs_preserves_failed_cancellationEach was confirmed RED against the unfixed code (asserting the rejected
run_id is absent from the "cancelled" result, which failed before this fix)
and is GREEN after it.
On the third test's name: the discarded prototype from PR #1669's branch
(below) named this cancellation path
_cancel_revalidated_review_run_refs,which does not exist under that name on current
main. Current main's realshared choke point for cancelling a batch of stale/superseded review run
refs — used by both
dispatch_opencode_reviewanddispatch_strix_evidence— is
force_cancel_workflow_run_refs, so the test targets that functionwhile keeping the established name.
Provenance
Discovered mid-flight during #1669's development (the
naruonheadRefOidcancellation-bug fix) by the org's autonomous PR review/fix loop, but
intentionally scoped out of that PR — its RED tests and a one-shot repair
workflow were removed there as out-of-scope debris rather than landed, to
keep that PR's diff to the bug it was opened for. This PR is the fresh,
hand-written follow-up for that finding.
CI gates
Merge boundary
The organization-wide Actions capacity incident is not authorization to bypass branch protection. Merge or auto-merge only through ordinary governance after the unchanged exact head has terminal required checks, zero valid unresolved findings, and every then-live independent review/ruleset requirement. Do not use administrator bypass, self-approval, stale evidence, or check suppression to accelerate this fix; while the queue is waiting, continue independent repair lanes.
🤖 Generated with Claude Code