diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 8c640b0b2d..43df25a94c 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -2966,13 +2966,24 @@ def cancel_one(run_id: str) -> tuple[str, str | None]: return failures -def force_cancel_workflow_run_refs(run_refs: Sequence[tuple[str, str]]) -> None: - """Force-cancel repository-qualified runs while retaining bounded batches.""" +def force_cancel_workflow_run_refs(run_refs: Sequence[tuple[str, str]]) -> list[tuple[str, str]]: + """Force-cancel repository-qualified runs and return the ones actually cancelled. + + ``force_cancel_workflow_runs`` reports GitHub's per-run cancellation rejections + as a ``{run_id: failure_reason}`` dict rather than raising. A caller that treats + every requested ref as gone once this returns would misclassify a run GitHub + refused to cancel as cancelled -- letting a duplicate review dispatch alongside + a run that is, in fact, still active. Exclude rejected refs from the result so + every caller can tell the difference. + """ runs_by_repo: dict[str, list[str]] = {} for run_repo, run_id in run_refs: runs_by_repo.setdefault(run_repo, []).append(run_id) + cancelled: list[tuple[str, str]] = [] for run_repo, run_ids in runs_by_repo.items(): - force_cancel_workflow_runs(run_repo, run_ids) + failures = force_cancel_workflow_runs(run_repo, run_ids) + cancelled.extend((run_repo, run_id) for run_id in run_ids if run_id not in failures) + return cancelled def cancel_stale_pr_runs(repo: str, pr: dict[str, Any], *, dry_run: bool) -> list[str]: @@ -2981,8 +2992,8 @@ def cancel_stale_pr_runs(repo: str, pr: dict[str, Any], *, dry_run: bool) -> lis return [] require_github_actions_control_actor("force-cancel-stale-pr-runs") run_ids = stale_pr_run_ids(repo, pr) - force_cancel_workflow_runs(repo, run_ids) - return run_ids + failures = force_cancel_workflow_runs(repo, run_ids) + return [run_id for run_id in run_ids if run_id not in failures] def cancel_stale_opencode_runs(repo: str, workflow: str, pr: dict[str, Any], *, dry_run: bool) -> list[str]: @@ -2991,8 +3002,8 @@ def cancel_stale_opencode_runs(repo: str, workflow: str, pr: dict[str, Any], *, return [] require_github_actions_control_actor("force-cancel-stale-opencode-review") _, stale_refs = active_opencode_run_refs(repo, workflow, pr) - force_cancel_workflow_run_refs(stale_refs) - return [run_id for _, run_id in stale_refs] + cancelled_refs = force_cancel_workflow_run_refs(stale_refs) + return [run_id for _, run_id in cancelled_refs] def discover_opencode_required_run_id(repo: str, head_sha: str) -> int | None: @@ -3131,7 +3142,7 @@ def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry run_title="Strix Security Scan", workflow_aliases=frozenset({"Strix Security Scan"}), ) - force_cancel_workflow_run_refs(stale_run_refs) + cancelled_refs = force_cancel_workflow_run_refs(stale_run_refs) if current_run_refs: print( "Strix evidence dispatch skipped: active same-head workflow run(s) " @@ -3142,12 +3153,12 @@ def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry return "already_running" target_repo = validate_github_repository(repo) dispatch_repo = repository_dispatch_target(target_repo) - stale_ids = {run_id for _, run_id in stale_run_refs} + 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 and run_data.get("name") == workflow and run_data.get("event") == "repository_dispatch" and str(run_data.get("display_title") or "").startswith( diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index d859b1730d..a87ad2dafc 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1486,6 +1486,55 @@ def maybe_fail(args): } +def test_cancel_revalidated_review_run_refs_preserves_failed_cancellation(monkeypatch): + """Keep a review ref busy when GitHub rejects its destructive cancellation. + + Discovered mid-flight during PR #1669's development (the naruon headRefOid + incident fix) and intentionally scoped out of that PR; landing fresh here per + docs/doctoring/scheduler-stale-headrefoid-cancellation.md. That branch's + prototype named this cancellation path ``_cancel_revalidated_review_run_refs``; + current main's actual shared choke point for cancelling a revalidated batch of + stale/superseded review run refs -- used by both ``dispatch_opencode_review`` + and ``dispatch_strix_evidence`` -- is :func:`force_cancel_workflow_run_refs`, + so this test (kept under the established name) targets that real function. + """ + stale_refs = [("owner/repo", "101"), ("owner/repo", "202")] + + def cancel(_repo, run_ids): + run_id = str(run_ids[0]) + return {run_id: "GitHub rejected cancellation"} if run_id == "101" else {} + + monkeypatch.setattr(sched, "force_cancel_workflow_runs", cancel) + + cancelled = sched.force_cancel_workflow_run_refs(stale_refs) + + assert ("owner/repo", "101") not in cancelled + assert ("owner/repo", "202") in cancelled + + +def test_cancel_stale_opencode_runs_preserves_failed_cancellation(monkeypatch): + """Keep a stale review active when GitHub rejects its cancellation.""" + stale_refs = [("owner/repo", "101"), ("owner/repo", "202")] + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) + monkeypatch.setattr( + sched, + "active_opencode_run_refs", + lambda _repo, _workflow, _pr: ([], stale_refs), + ) + + def cancel(_repo, run_ids): + run_id = str(run_ids[0]) + return {run_id: "GitHub rejected cancellation"} if run_id == "101" else {} + + monkeypatch.setattr(sched, "force_cancel_workflow_runs", cancel) + + run_ids = sched.cancel_stale_opencode_runs( + "owner/repo", "OpenCode Review", make_pr(), dry_run=False + ) + + assert run_ids == ["202"] + + def test_cancel_stale_opencode_runs_dry_run_skips_lookup_and_mutation(monkeypatch): calls = [] monkeypatch.setattr(sched, "stale_opencode_run_ids", lambda *args: calls.append(args) or ["1"]) @@ -5387,6 +5436,22 @@ def fake_run(args, stdin=None): assert any("status=in_progress" in " ".join(call) for call in calls) +def test_cancel_stale_pr_runs_preserves_failed_cancellation(monkeypatch): + """Do not report a stale run cancelled when GitHub rejected the API call.""" + monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda _action: None) + monkeypatch.setattr(sched, "stale_pr_run_ids", lambda _repo, _pr: ["101", "202"]) + + def cancel(_repo, run_ids): + run_id = str(run_ids[0]) + return {run_id: "GitHub rejected cancellation"} if run_id == "101" else {} + + monkeypatch.setattr(sched, "force_cancel_workflow_runs", cancel) + + run_ids = sched.cancel_stale_pr_runs("owner/repo", make_pr(), dry_run=False) + + assert run_ids == ["202"] + + def test_mutations_refuse_local_credentials(monkeypatch): calls = [] monkeypatch.setattr(sched, "run", lambda args: calls.append(args) or "")