diff --git a/.github/scripts/test_traffic_control.py b/.github/scripts/test_traffic_control.py index ebadfe1..60715d9 100644 --- a/.github/scripts/test_traffic_control.py +++ b/.github/scripts/test_traffic_control.py @@ -3,10 +3,11 @@ """Unit tests for the pure decision core of traffic_control.py (no network). Covers: priority resolution (P-label / broken / draft / default P5), PASS 1 -preemption (only P0 cancels strictly-lower; P1-P9 never bump; self / main / -equal-or-higher never cancelled), PASS 2 hold-back (yield to strictly-higher with -an active run; same-level running-first then oldest-first), and a few end-to-end -decision scenarios.""" +preemption (P0 reclaims all strictly-lower; ANY higher PR reclaims a broken/draft +lower run; P1-P9 never bump a *normal* lower run; self / main / equal-or-higher +never cancelled), PASS 2 hold-back (yield to strictly-higher with an active run; +same-level running-first then oldest-first), and a few end-to-end decision +scenarios.""" from __future__ import annotations @@ -63,17 +64,40 @@ class EffectivePriorityTests(unittest.TestCase): class RunsToCancelTests(unittest.TestCase): - def test_non_p0_self_cancels_nothing(self): + def test_non_p0_self_does_not_bump_normal_lower_run(self): + # P1-P9 never preempt a *normal* strictly-lower run — they yield instead. me = pr(1, ["P1"]) others = [pr(2, ["P5"], status=tc.RUNNING, run_ids=[200])] self.assertEqual(tc.runs_to_cancel(me, [me, *others]), []) - def test_non_p0_self_never_cancels_even_broken(self): - # Behaviour change vs the old bash (see module/PR notes): P1-P9 no longer - # reclaim a broken target's runner — only P0 preempts. + def test_non_p0_self_reclaims_broken_lower_run(self): + # Any higher-priority PR (not just P0) may reclaim a broken target's runner. me = pr(1, ["P3"]) broken = pr(2, ["broken"], status=tc.RUNNING, run_ids=[200]) - self.assertEqual(tc.runs_to_cancel(me, [me, broken]), []) + self.assertEqual(tc.runs_to_cancel(me, [me, broken]), [200]) + + def test_non_p0_self_reclaims_draft_lower_run(self): + # A draft is not merge-ready — its run is likewise reclaimable by any higher PR. + me = pr(1, ["P3"]) + draft = pr(2, [], draft=True, status=tc.QUEUED, run_ids=[200]) + self.assertEqual(tc.runs_to_cancel(me, [me, draft]), [200]) + + def test_broken_self_does_not_cancel_equal_broken(self): + # Both effective P10 — the equal-or-higher invariant still forbids cancelling. + me = pr(1, ["broken"]) + peer = pr(2, ["broken"], status=tc.RUNNING, run_ids=[200]) + self.assertEqual(tc.runs_to_cancel(me, [me, peer]), []) + + def test_bottom_self_preempts_nothing(self): + # A broken/draft PR (P10) is the bottom: nothing is strictly-lower, so it + # cancels neither a higher (P5) nor an equal (P10) run. + me = pr(1, [], draft=True) # P10 + prs = [ + me, + pr(2, ["P5"], status=tc.RUNNING, run_ids=[200]), # higher + pr(3, ["broken"], status=tc.RUNNING, run_ids=[300]), # equal P10 + ] + self.assertEqual(tc.runs_to_cancel(me, prs), []) def test_p0_cancels_strictly_lower_active_runs(self): me = pr(1, ["P0"]) @@ -203,6 +227,20 @@ class EndToEndDecisionTests(unittest.TestCase): self.assertEqual(dec.cancel_run_ids, ()) self.assertTrue(dec.proceed) + def test_p5_reclaims_draft_then_waits_behind_higher(self): + # A non-P0 PR can BOTH reclaim a broken/draft lower run (PASS 1) AND still + # yield to a strictly-higher PR (PASS 2) in the same evaluation. + me = pr(40, ["P5"], status=tc.RUNNING, run_ids=[4000]) + prs = [ + me, + pr(41, ["P2"], status=tc.RUNNING, run_ids=[4100]), # higher — blocks + pr(42, [], draft=True, status=tc.RUNNING, run_ids=[4200]), # draft — reclaimed + ] + dec = tc.decide(me, prs) + self.assertEqual(list(dec.cancel_run_ids), [4200]) + self.assertFalse(dec.proceed) + self.assertEqual([b.number for b in dec.blockers], [41]) + class SnapshotParsingTests(unittest.TestCase): def test_from_json_label_objects_and_fields(self): diff --git a/.github/scripts/traffic_control.py b/.github/scripts/traffic_control.py index b78b5d9..fe4c7f2 100644 --- a/.github/scripts/traffic_control.py +++ b/.github/scripts/traffic_control.py @@ -19,9 +19,10 @@ ORDER OF OPERATIONS (issue #342) 1. Effective priority orders everything: the lowest-numbered `P0`-`P9` label present (P0 = highest), default `P5` if none. A `broken` OR `draft` PR is effectively P10 (bottom, below P9), overriding any P0-P9 label. -2. PASS 1 - preemption: **only a P0 (emergency) preempts.** A P0 cancels the - in-progress / queued CI runs of ALL strictly-lower OTHER open PRs to reclaim their - runners. P1-P9 never bump a lower run mid-flight. +2. PASS 1 - preemption: a strictly-lower OTHER PR's in-progress / queued run is + cancelled iff THIS PR is P0 (an emergency reclaims ALL lower runners) OR the target + is broken/draft (a wasted run any higher-priority PR may reclaim). P1-P9 never bump + a *normal* lower run mid-flight — only a P0 does that. 3. PASS 2 - bounded hold-back: a non-P0 PR yields (cancels nothing) to any strictly- higher-priority OTHER PR that has an active/queued run, and — among its OWN priority level — to any PR ordered ahead of it (running-first, then oldest by @@ -169,20 +170,29 @@ def runs_to_cancel( *, self_run_id: int | None = None, ) -> list[int]: - """PASS 1. Run ids to cancel — **empty unless THIS PR is P0**. A P0 preempts the - active (running/queued) runs of every strictly-lower OTHER PR. Invariants: never - cancel self (by number or run id), never cancel an equal-or-higher-priority PR.""" - if effective_priority(this_pr) != TOP_PRIORITY: - return [] # only P0 preempts; P1-P9 never bump + """PASS 1. Run ids to cancel. A strictly-lower OTHER PR's active (running/queued) + run is cancelled iff keeping it running is wasteful, i.e. EITHER: + * THIS PR is P0 — an emergency reclaims every strictly-lower runner now; OR + * the target is broken/draft (effective priority 10) — its run can't merge / + isn't merge-ready, so ANY higher-priority PR may reclaim its runner. + P1-P9 never cancel a *normal* strictly-lower run — they yield in PASS 2 instead. + Invariants: never cancel self (by number or run id), never cancel an + equal-or-higher-priority PR (only strictly-lower, prio > self).""" + self_prio = effective_priority(this_pr) to_cancel: list[int] = [] seen: set[int] = set() for pr in all_prs: if pr.number == this_pr.number: continue # never cancel self - if effective_priority(pr) <= TOP_PRIORITY: - continue # only strictly-lower (skip other P0s) + target_prio = effective_priority(pr) + if target_prio <= self_prio: + continue # only strictly-lower (skip equal-or-higher) if pr.run_status not in ACTIVE: continue # nothing running/queued to cancel + # Strictly lower: preemptible iff we're P0 OR the target is broken/draft + # (a bottom, priority-10, wasted run that any higher PR may reclaim). + if self_prio != TOP_PRIORITY and target_prio < BOTTOM_PRIORITY: + continue # P1-P9 don't bump a *normal* lower run for rid in pr.run_ids: if self_run_id is not None and rid == self_run_id: continue # never cancel our own run @@ -377,13 +387,14 @@ def run_live() -> int: _log(f"This PR #{self_number} effective priority: {priority_label(self_prio)} " "(P0 = highest/emergency, P9 = lowest, broken/draft = bottom).") - # ── PASS 1: PREEMPTION (only a P0 self) ────────────────────────────────── + # ── PASS 1: PREEMPTION (P0 reclaims all lower; anyone reclaims broken/draft) ── to_cancel = runs_to_cancel(this_pr, all_prs, self_run_id=self_run_id) if not to_cancel: if self_prio == TOP_PRIORITY: _log("P0 emergency — no strictly-lower active runs to cancel.") else: - _log("Not P0 — no preemption (P1-P9 never cancel a lower run mid-flight).") + _log("No preemptible runs (P1-P9 only reclaim broken/draft lower runs; " + "none active).") else: cancelled = 0 for rid in to_cancel: @@ -448,11 +459,14 @@ def run_dry(text: str) -> int: dec = decide(this_pr, all_prs, self_run_id=self_run_id) _log(f"This PR #{dec.self_number} effective priority: " f"{priority_label(dec.self_priority)}") - if dec.self_priority == TOP_PRIORITY: - _log(f"PASS 1 (preemption): P0 — cancel run ids: " - f"{list(dec.cancel_run_ids) or '(none active)'}") + if dec.cancel_run_ids: + why = ("P0 emergency (reclaims all strictly-lower)" + if dec.self_priority == TOP_PRIORITY + else "reclaiming broken/draft lower runs") + _log(f"PASS 1 (preemption): {why} — cancel run ids: " + f"{list(dec.cancel_run_ids)}") else: - _log("PASS 1 (preemption): not P0 — no cancellations.") + _log("PASS 1 (preemption): nothing to cancel.") if dec.proceed: _log("PASS 2 (hold-back): PROCEED — no blockers.") else: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dafb771..053d703 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,29 +34,34 @@ jobs: # P0 PREEMPTS: it cancels the in-progress / queued CI runs of ALL strictly- # LOWER-priority OTHER open PRs to grab their runners immediately. A preempted # PR simply re-runs on its next push / autoupdate rebase. P0 is the ONLY - # priority that preempts — P1–P9 never cancel a lower run mid-flight. + # priority that preempts a *normal* lower run — P1–P9 never bump those (a + # higher PR may still reclaim a broken/draft lower run — see below). # - # • P1–P9 = YIELD WITHOUT BUMPING. They NEVER cancel a lower-priority run that is - # already going — a higher-priority PR does not evict it, it just takes the next - # free slot. Mechanism: a bounded hold-back. This job defers (up to + # • P1–P9 = YIELD WITHOUT BUMPING a *normal* lower run. They do NOT cancel a + # normal lower-priority run already going — a higher-priority PR does not evict + # it, it just takes the next free slot (it MAY still reclaim a broken/draft + # lower run — see below). Mechanism: a bounded hold-back. This job defers (up to # HOLD_BACK_BUDGET_SECONDS, kept well under timeout-minutes) while any strictly- # higher-priority OTHER open PR still has an active/queued CI run, and — within # its OWN priority level — while any peer is ordered ahead of it (an in-flight # run keeps its place; then oldest createdAt first). It proceeds the moment it # is at the front, or when the budget elapses (a PR never blocks itself). # - # • `broken` / `draft` = BOTTOM (effective P10). Always yields, never preempts. - # A maintainer applies `broken` to a stuck/failing PR to deprioritise it below - # everything so others aren't blocked behind it; a draft isn't merge-ready, so - # it likewise waits behind every ready PR. Only a P0 may cancel a bottom PR's - # run (the same strictly-lower rule as any other target). + # • `broken` / `draft` = BOTTOM (effective P10). Always yields, never preempts — + # and because its run is wasted (a broken PR can't merge; a draft isn't merge- + # ready), ANY higher-priority PR (not just P0) MAY cancel that run to reclaim + # its runner (still the strictly-lower rule: broken/draft is the bottom, so any + # ready PR outranks it). A maintainer marks a stuck/failing PR `broken` to drop + # it below everything so others aren't blocked behind it AND may reclaim its + # runner; a draft behaves the same until it is marked ready for review. # # Hard safety invariants, enforced in the script: # • never cancels a run on main / a push event (the gh query filters # --event pull_request and drops headBranch == main); # • never cancels THIS PR's own run (skips self by PR number + run id); # • never cancels an equal-or-higher-priority PR (only strictly-lower, prio > self); - # • P1–P9 cancel NOTHING — they only wait (bounded), then proceed. + # • P1–P9 never bump a *normal* lower run (they only reclaim broken/draft) — + # otherwise they just wait (bounded), then proceed. # # Honest limitation: GitHub Actions has no native priority queue and assigns # runners roughly FIFO, so the hold-back is a BEST-EFFORT head-start, not a hard @@ -76,7 +81,7 @@ jobs: timeout-minutes: 6 # hard backstop; the P1–P9 hold-back budget stays well under this permissions: contents: read # check out .github/scripts/traffic_control.py - actions: write # cancel lower-priority runs (P0 emergencies) + actions: write # cancel lower-priority runs (P0 emergencies + broken/draft reclaim) pull-requests: read # read PR P0–P9 labels + draft state env: GH_TOKEN: ${{ github.token }} @@ -94,13 +99,13 @@ jobs: # gh is auto-configured from GH_TOKEN / GH_REPO; python3 is preinstalled on the # runner. The script guards every API call and always exits 0 (belt-and-braces # with continue-on-error), so it can never fail or block CI. - - name: Apply runner priority (P0 preempts; P1–P9 hold back) + - name: Apply runner priority (P0/broken/draft preempt; P1–P9 hold back) continue-on-error: true run: python3 .github/scripts/traffic_control.py debug-build: name: Debug build - needs: traffic-control # order after runner-priority orchestration (P0 preempts; P1–P9 hold-back) + needs: traffic-control # order after runner-priority orchestration (P0/broken/draft preempt; P1–P9 hold-back) # x86_64: Linux-arm64 runners can't set up this SDK — android-actions/setup-android's sdkmanager # fails (exit 1) on the android-37.0 preview platform, and the emulator package has no arm64-Linux # build. Build/unit-test results are host-arch-independent anyway (R8/AGP/JVM); real arm64 @@ -137,7 +142,7 @@ jobs: unit-tests: name: Unit tests - needs: traffic-control # order after runner-priority orchestration (P0 preempts; P1–P9 hold-back) + needs: traffic-control # order after runner-priority orchestration (P0/broken/draft preempt; P1–P9 hold-back) runs-on: ubuntu-latest steps: - name: Check out source @@ -194,7 +199,7 @@ jobs: static-analysis: name: Static analysis - needs: traffic-control # order after runner-priority orchestration (P0 preempts; P1–P9 hold-back) + needs: traffic-control # order after runner-priority orchestration (P0/broken/draft preempt; P1–P9 hold-back) runs-on: ubuntu-latest steps: - name: Check out source @@ -232,7 +237,7 @@ jobs: e2e: name: E2E - needs: traffic-control # order after runner-priority orchestration (P0 preempts; P1–P9 hold-back) + needs: traffic-control # order after runner-priority orchestration (P0/broken/draft preempt; P1–P9 hold-back) runs-on: ubuntu-latest strategy: fail-fast: false @@ -343,7 +348,7 @@ jobs: # 37 into the main `e2e` matrix and delete this job. e2e-preview: name: E2E (API 37 preview) - needs: traffic-control # order after runner-priority orchestration (P0 preempts; P1–P9 hold-back) + needs: traffic-control # order after runner-priority orchestration (P0/broken/draft preempt; P1–P9 hold-back) runs-on: ubuntu-latest timeout-minutes: 35 env: