diff --git a/scripts/ci/current_head_run_coalescer.py b/scripts/ci/current_head_run_coalescer.py index 778ba4d8c3..ae40b85ac4 100644 --- a/scripts/ci/current_head_run_coalescer.py +++ b/scripts/ci/current_head_run_coalescer.py @@ -15,6 +15,7 @@ import os import re import subprocess +import time from typing import Any, Iterable, Mapping, Sequence from urllib.parse import urlsplit @@ -26,6 +27,8 @@ PR_EVENTS = frozenset({"pull_request", "pull_request_target"}) ACTIVE_STATUSES = ("queued", "in_progress") API_TIMEOUT_SECONDS = 30 +CANCELLATION_POLL_ATTEMPTS = 6 +CANCELLATION_POLL_INTERVAL_SECONDS = 1.0 class CoalescingRefused(RuntimeError): @@ -369,8 +372,15 @@ def _fetch_run(repo: str, run_id: int) -> dict[str, Any]: def _cancel_run(repo: str, run_id: int) -> None: - """Request ordinary cancellation using the same explicit token/timeout contract.""" + """Cancel one run and prove GitHub reached its terminal cancelled state.""" _run_json(["gh", "api", "-X", "POST", f"repos/{repo}/actions/runs/{run_id}/cancel"]) + for attempt in range(CANCELLATION_POLL_ATTEMPTS): + run_data = _fetch_run(repo, run_id) + if run_data.get("status") == "completed" and run_data.get("conclusion") == "cancelled": + return + if attempt + 1 < CANCELLATION_POLL_ATTEMPTS: + time.sleep(CANCELLATION_POLL_INTERVAL_SECONDS) + raise RuntimeError(f"workflow run {run_id} did not reach completed/cancelled") def _associated_prs( diff --git a/tests/test_current_head_run_coalescer.py b/tests/test_current_head_run_coalescer.py index eaece3f4f3..571368677f 100644 --- a/tests/test_current_head_run_coalescer.py +++ b/tests/test_current_head_run_coalescer.py @@ -454,13 +454,38 @@ def pages(args): def test_cancel_run_uses_explicit_transport_and_ordinary_endpoint(monkeypatch) -> None: - """Cancellation shares the token/timeout transport and never uses force-cancel.""" + """Cancellation uses the ordinary endpoint and proves terminal state.""" module = load_module() calls: list[list[str]] = [] monkeypatch.setattr(module, "_run_json", lambda args: calls.append(list(args))) + states = iter( + [ + {"status": "in_progress", "conclusion": None}, + {"status": "completed", "conclusion": "cancelled"}, + ] + ) + monkeypatch.setattr(module, "_fetch_run", lambda _repo, _run_id: next(states)) + sleeps: list[float] = [] + monkeypatch.setattr(module.time, "sleep", sleeps.append) module._cancel_run("o/r", 123) assert calls == [["gh", "api", "-X", "POST", "repos/o/r/actions/runs/123/cancel"]] assert "force-cancel" not in " ".join(calls[0]) + assert sleeps == [module.CANCELLATION_POLL_INTERVAL_SECONDS] + + +def test_cancel_run_fails_when_terminal_cancellation_is_unproven(monkeypatch) -> None: + """An accepted cancellation is not reported complete while GitHub stays active.""" + module = load_module() + monkeypatch.setattr(module, "_run_json", lambda _args: None) + monkeypatch.setattr( + module, + "_fetch_run", + lambda _repo, _run_id: {"status": "in_progress", "conclusion": None}, + ) + monkeypatch.setattr(module.time, "sleep", lambda _seconds: None) + + with pytest.raises(RuntimeError, match="did not reach completed/cancelled"): + module._cancel_run("o/r", 123) def test_associated_pr_fetches_only_same_head_noncurrent_numbers(monkeypatch) -> None: @@ -568,7 +593,11 @@ def test_coalesce_cancels_only_revalidated_redundant_candidates(monkeypatch, cap sibling = run_record(101, 10) monkeypatch.setattr(module, "_fetch_pr", lambda *_args: live_pr()) monkeypatch.setattr(module, "_active_runs", lambda *_args: [candidate, sibling]) - monkeypatch.setattr(module, "_fetch_run", lambda _repo, run_id: sibling if run_id == 101 else candidate) + monkeypatch.setattr( + module, + "_fetch_run", + lambda _repo, run_id: sibling if run_id == 101 else candidate, + ) cancelled: list[int] = [] monkeypatch.setattr(module, "_cancel_run", lambda _repo, run_id: cancelled.append(run_id)) assert module.coalesce("ContextualWisdomLab/.github", 1, "ContextualWisdomLab/.github", "feature/current", "a" * 40) == [100] @@ -576,6 +605,33 @@ def test_coalesce_cancels_only_revalidated_redundant_candidates(monkeypatch, cap assert "Cancelled redundant queued current-head run 100" in capsys.readouterr().out +def test_coalesce_fails_before_reporting_unproven_cancellation(monkeypatch, capsys) -> None: + """A cancellation that never reaches terminal state must not be reported.""" + module = load_module() + candidate = run_record(100, 10) + sibling = run_record(101, 10) + monkeypatch.setattr(module, "_fetch_pr", lambda *_args: live_pr()) + monkeypatch.setattr(module, "_active_runs", lambda *_args: [candidate, sibling]) + monkeypatch.setattr(module, "_fetch_run", lambda _repo, run_id: sibling if run_id == 101 else candidate) + monkeypatch.setattr( + module, + "_cancel_run", + lambda _repo, _run_id: (_ for _ in ()).throw( + RuntimeError("terminal cancellation unproven") + ), + ) + + with pytest.raises(RuntimeError, match="terminal cancellation unproven"): + module.coalesce( + "ContextualWisdomLab/.github", + 1, + "ContextualWisdomLab/.github", + "feature/current", + "a" * 40, + ) + assert "Cancelled redundant" not in capsys.readouterr().out + + def test_parse_args_main_and_script_help(monkeypatch) -> None: """CLI parsing forwards exact identity and the executable entrypoint is reachable.""" module = load_module() diff --git a/tests/test_current_head_run_coalescer_review_regressions.py b/tests/test_current_head_run_coalescer_review_regressions.py index 477dbabba0..3f10ab3165 100644 --- a/tests/test_current_head_run_coalescer_review_regressions.py +++ b/tests/test_current_head_run_coalescer_review_regressions.py @@ -289,13 +289,19 @@ def test_transport_is_token_bound_and_individually_timeout_bounded(monkeypatch) def success(args, **kwargs): calls.append((list(args), dict(kwargs))) - stdout = "{}" if "/cancel" not in " ".join(args) else "" + command = " ".join(args) + if "/cancel" in command: + stdout = "" + elif command.endswith("actions/runs/123"): + stdout = '{"status":"completed","conclusion":"cancelled"}' + else: + stdout = "{}" return SimpleNamespace(returncode=0, stdout=stdout, stderr="") monkeypatch.setattr(module.subprocess, "run", success) assert module._run_json(["gh", "api", "repos/owner/repo"]) == {} module._cancel_run("owner/repo", 123) - assert len(calls) == 2 + assert len(calls) == 3 assert all(call_kwargs.get("timeout") == module.API_TIMEOUT_SECONDS for _, call_kwargs in calls)