From 6ed4e29c0a782dae421e4ebe11d666faa8b3f4ae Mon Sep 17 00:00:00 2001 From: Leo Date: Tue, 22 Sep 2026 10:25:46 -0700 Subject: [PATCH] ci: reclaim macOS slots held by runs whose ci-status is already decided `ci-status` accepts only `success` or `skipped` from each of its `needs`, so an `app-host unit tests` shard concluding `failure` fails the `macos` reusable-workflow call and the required check by construction; no later job takes it back. Across the 299 CI runs created between 2026-09-22T06:05Z and 17:00Z, 21 runs had such a shard failure, `ci-status` concluded `failure` in all 21, and their sibling macOS jobs burned 1,522 macOS runner-minutes after the verdict was already fixed. This lands as a fourth category in the queue janitor rather than a second janitor. Reclaiming macOS pool capacity is that module's charter, and putting it there means one queue threshold, one priority order, one per-sweep cancel cap and one concurrency group instead of two workflows with `actions: write` and no shared bound. The threshold gate is also the right policy on its own terms: cancelling a doomed run when the pool is idle frees nothing anybody is waiting for and still destroys the remaining shard output. The category is ordered last. Categories (a) to (c) cancel runs nobody will read -- an experiment push, a closed or superseded PR, a replaced full-suite run. A doomed run is still current and its remaining shards are still readable, so it is the most debatable of the four and is spent only after the others. That same difference is why this category needs a fix-branch exclusion the others do not. A run whose diff touches the shards' own inputs is the run whose remaining shards someone is waiting on, and is never cancelled; `no-janitor` covers a fix the path list cannot recognise, and an unreadable diff preserves the run. Replayed over the 21 real runs, this preserves every app-host repair among them (#13643, #13579, #13574, #13427, #13414, #13615, #13263, #13271, and #13408 which was cancelled by hand and had to be restarted) and leaves two unrelated runs eligible. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci-queue-janitor.yml | 9 +- scripts/ci/queue_janitor.py | 105 ++++++++++++++- tests/test_ci_queue_janitor.py | 170 ++++++++++++++++++++++++- 3 files changed, 273 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci-queue-janitor.yml b/.github/workflows/ci-queue-janitor.yml index beb61e4f2a9b..a64f4db6f91b 100644 --- a/.github/workflows/ci-queue-janitor.yml +++ b/.github/workflows/ci-queue-janitor.yml @@ -3,9 +3,12 @@ name: CI queue janitor # Pull request macOS jobs share one small runner pool. When more than # CI_JANITOR_QUEUE_THRESHOLD macOS jobs are queued, cancel the runs whose # results nobody will use: exp/* experiment pushes, runs for closed, merged or -# superseded pull request heads, and full-suite runs whose PR dropped -# `full-ci` once a replacement run is waiting, in that order. Draft pull -# requests are never cancelled for being drafts. +# superseded pull request heads, full-suite runs whose PR dropped `full-ci` +# once a replacement run is waiting, and last, CI runs whose required +# ci-status is already decided against them by a failed `app-host unit tests` +# shard, in that order. Draft pull requests are never cancelled for being +# drafts, and a run whose diff changes what the failing shard does is never +# cancelled for being doomed. # Main pushes, merge groups, main schedules/dispatches, release/tag, nightly # and TestFlight/App Store runs are never candidates. scripts/ci/queue_janitor.py # holds the policy; every decision is written to the step summary. diff --git a/scripts/ci/queue_janitor.py b/scripts/ci/queue_janitor.py index 0527bc985eb2..36309406a476 100644 --- a/scripts/ci/queue_janitor.py +++ b/scripts/ci/queue_janitor.py @@ -14,6 +14,13 @@ a newer CI run for that PR is already waiting to replace them and the old run's compile admission is not mid-flight (ci.yml deliberately lets that compile finish so the queued run can reuse its product). + d. CI runs whose required ``ci-status`` is already decided against them: an + ``app-host unit tests`` shard has concluded ``failure``, so the ``macos`` + reusable-workflow call cannot report ``success`` or ``skipped`` and no + later job can take that back, while sibling macOS jobs still hold the + pool. Unlike (a)-(c) the run is current and its remaining output is still + readable, so this category is ordered last and never touches a run whose + diff changes what the failing shard does. Draft pull requests are deliberately not a category: a draft can be an active integration branch other work depends on, and ci.yml has no ready_for_review @@ -68,7 +75,29 @@ COMPILE_ONLY_POLICY = "compile-only" COMPILE_ADMISSION_JOB = re.compile(r"(^|/ )macOS compile admission$") -CATEGORY_ORDER = ("experiment", "stale-pr", "label-dropped") +CATEGORY_ORDER = ("experiment", "stale-pr", "label-dropped", "doomed") + +# The shards whose failure decides ci-status. ci-macos.yml shards this six ways +# and the reusable-call prefix makes the API name "macos / app-host unit tests +# (3/6)", so match on the substring. +DOOMED_JOB_NAME = "app-host unit tests" + +# How long a shard failure must have stood before the run is a candidate, so a +# run that just turned red keeps its siblings while someone looks at it. +DOOMED_GRACE = dt.timedelta(minutes=10) + +# The shards' own inputs, read off the app-host-unit-tests job block in +# ci-macos.yml: the XCTest sources it runs, the scripts that shard, compile, +# isolate and grade them, the known-failure quarantine list and the workflow +# that defines the job. A run that changes any of these is an attempt to change +# what the shard does, and its remaining shards are the result someone is +# waiting for. Sources/ is deliberately absent: the suite exercises it, but +# nearly every pull request changes it, and a set matching every pull request +# is not a rule. JANITOR_OPT_OUT_LABEL covers a fix that lives only there. +DOOMED_INPUT_PREFIXES = ("cmuxTests/", "scripts/ci/workloads/") +DOOMED_INPUT_MARKERS = ("app-host", "app_host") +DOOMED_INPUT_FILES = (".github/workflows/ci-macos.yml", "scripts/ci/cmux_unit_test_shard.py") +JANITOR_OPT_OUT_LABEL = "no-janitor" PROTECTED_EVENTS = frozenset({"merge_group", "release", "create", "delete", "deployment", "deployment_status"}) MAIN_ONLY_EVENTS = frozenset({"schedule", "workflow_dispatch", "repository_dispatch"}) @@ -169,6 +198,10 @@ class MacosUsage: running: int = 0 oldest_queued_at: dt.datetime | None = None compile_admission_running: bool = False + # The app-host shard whose failure decided ci-status, and when it landed. + # Read from the same pass over the run's jobs, at no extra API cost. + decided_by: str | None = None + decided_at: dt.datetime | None = None @property def held(self) -> int: @@ -179,10 +212,14 @@ def macos_usage(jobs: Iterable[Mapping[str, Any]]) -> MacosUsage: queued = running = 0 oldest: dt.datetime | None = None compiling = False + decided_by: str | None = None + decided_at: dt.datetime | None = None + undated_failure = False for job in jobs: if not is_macos_job(job): continue status = job.get("status") + name = job.get("name") or "" if status in QUEUED_JOB_STATUSES: queued += 1 created = parse_time(job.get("created_at")) @@ -190,9 +227,43 @@ def macos_usage(jobs: Iterable[Mapping[str, Any]]) -> MacosUsage: oldest = created elif status in RUNNING_JOB_STATUSES: running += 1 - if COMPILE_ADMISSION_JOB.search(job.get("name") or ""): + if COMPILE_ADMISSION_JOB.search(name): compiling = True - return MacosUsage(queued, running, oldest, compiling) + elif status == "completed" and DOOMED_JOB_NAME in name: + # Job conclusion, never step conclusion: a job whose only failed + # steps carry continue-on-error concludes `success`, so reading the + # job already excludes tolerated failures. + if job.get("conclusion") != "failure": + continue + finished = parse_time(job.get("completed_at")) + if finished is None: + undated_failure = True + elif decided_at is None or finished < decided_at: + decided_by, decided_at = name, finished + if undated_failure: + # A failure we cannot time cannot clear the grace window; fail closed. + decided_by = decided_at = None + return MacosUsage(queued, running, oldest, compiling, decided_by, decided_at) + + +def touches_doomed_job_inputs(paths: Iterable[str]) -> bool: + for path in paths: + if path in DOOMED_INPUT_FILES or path.startswith(DOOMED_INPUT_PREFIXES): + return True + if any(marker in path for marker in DOOMED_INPUT_MARKERS): + return True + return False + + +def pr_changed_paths(pr: Mapping[str, Any]) -> list[str] | None: + """Changed paths, or None when the diff cannot be read in full.""" + files = pr.get("files") or {} + if (files.get("pageInfo") or {}).get("hasNextPage"): + return None + nodes = files.get("nodes") + if nodes is None: + return None + return [str(node.get("path")) for node in nodes] def run_head_owner(run: Mapping[str, Any]) -> str: @@ -270,6 +341,7 @@ def classify( *, newer_ci_run_waiting: bool, pull_request_policy: str, + now: dt.datetime, ) -> tuple[str, str] | None: """Return (category, reason) for a wasteful run, or None to leave it alone.""" if usage.held == 0 or protected_reason(run): @@ -306,6 +378,29 @@ def classify( created = parse_time(run.get("created_at")) if created and had_label_at(pr, FULL_SUITE_LABEL, created) is True: return "label-dropped", f"full suite, but PR #{number} no longer has `{FULL_SUITE_LABEL}`" + + # Doomed: ci-status is already decided against this run. Reaching here means + # the PR is open and this run is still its current head, so unlike the + # categories above the run is live and its remaining shards are readable + # output. Everything unknown therefore preserves the run. + if ( + run.get("path") == CI_WORKFLOW_PATH + and usage.decided_by + and usage.decided_at is not None + # A re-run replays a subset of jobs, so an older attempt's failure is + # not evidence about this one. + and run.get("run_attempt") == 1 + and now - usage.decided_at >= DOOMED_GRACE + and JANITOR_OPT_OUT_LABEL not in pr_labels(pr) + ): + changed = pr_changed_paths(pr) + # An unreadable diff is not evidence that this run is not the fix. + if changed is not None and not touches_doomed_job_inputs(changed): + return "doomed", ( + f"ci-status already decided for PR #{number} ({branch}): `{usage.decided_by}` " + f"failed {format_age(now - usage.decided_at)} ago, " + f"{usage.held} macOS job(s) still held" + ) return None @@ -361,6 +456,7 @@ def build_plan( threshold: int, max_cancels: int, pull_request_policy: str, + now: dt.datetime, ) -> Plan: usages = {run["id"]: macos_usage(jobs_by_run.get(run["id"], ())) for run in runs} queued = sum(u.queued for u in usages.values()) @@ -376,6 +472,7 @@ def build_plan( run, usage, pr, newer_ci_run_waiting=newer_ci_run_waiting(run, runs), pull_request_policy=pull_request_policy, + now=now, ) if verdict: candidates.append(Candidate(run, verdict[0], verdict[1], usage)) @@ -420,6 +517,7 @@ def branches_to_resolve( number state headRefOid url headRepositoryOwner { login } labels(first: 50) { nodes { name } } + files(first: 100) { pageInfo { hasNextPage } nodes { path } } timelineItems(last: 50, itemTypes: [LABELED_EVENT, UNLABELED_EVENT]) { nodes { __typename @@ -615,6 +713,7 @@ def main(argv: Sequence[str] | None = None) -> int: plan = build_plan( runs, jobs_by_run, prs_by_branch, threshold=threshold, max_cancels=max_cancels, pull_request_policy=args.pull_request_policy, + now=now, ) results: dict[int, str] = {} diff --git a/tests/test_ci_queue_janitor.py b/tests/test_ci_queue_janitor.py index 3d973c2fe375..0571a07e278b 100644 --- a/tests/test_ci_queue_janitor.py +++ b/tests/test_ci_queue_janitor.py @@ -31,11 +31,11 @@ def iso(minutes_ago: float) -> str: def make_run(*, event="pull_request", branch="feature", path=".github/workflows/ci.yml", name="CI", - status="in_progress", sha="aaa", age=30, owner="manaflow-ai", prs=()): + status="in_progress", sha="aaa", age=30, owner="manaflow-ai", prs=(), attempt=1): run_id = next(_ids) return { "id": run_id, "event": event, "head_branch": branch, "path": path, "name": name, - "status": status, "head_sha": sha, "created_at": iso(age), + "status": status, "head_sha": sha, "created_at": iso(age), "run_attempt": attempt, "html_url": f"https://github.com/manaflow-ai/cmux/actions/runs/{run_id}", "head_repository": {"full_name": f"{owner}/cmux", "owner": {"login": owner}}, "pull_requests": [{"number": n} for n in prs], @@ -51,10 +51,28 @@ def mac_jobs(queued=0, running=0, age=20, running_name="macos / app-host shard") return jobs -def make_pr(number=1, *, state="OPEN", draft=False, head="aaa", labels=(), owner="manaflow-ai", timeline=()): +def doomed_jobs(*, queued=2, running=1, conclusion="failure", failed_age=30, name=None, + completed_at=True): + """macOS jobs for a run whose app-host shard has already concluded.""" + jobs = mac_jobs(queued=queued, running=running, running_name="macos / app-host unit tests (4/6)") + jobs.append({ + "status": "completed", "labels": [MAC], "created_at": iso(60), + "name": name or "macos / app-host unit tests (3/6)", + "conclusion": conclusion, + "completed_at": iso(failed_age) if completed_at else None, + }) + return jobs + + +def make_pr(number=1, *, state="OPEN", draft=False, head="aaa", labels=(), owner="manaflow-ai", timeline=(), + files=("web/app/page.tsx",), files_truncated=False): return { "number": number, "state": state, "isDraft": draft, "headRefOid": head, "headRepositoryOwner": {"login": owner}, + "files": None if files is None else { + "pageInfo": {"hasNextPage": files_truncated}, + "nodes": [{"path": path} for path in files], + }, "labels": {"nodes": [{"name": name} for name in labels]}, "timelineItems": {"nodes": [ {"__typename": kind, "createdAt": iso(age), "label": {"name": label}} @@ -66,6 +84,7 @@ def make_pr(number=1, *, state="OPEN", draft=False, head="aaa", labels=(), owner def plan(runs, jobs, prs=None, *, threshold=6, max_cancels=10, policy="compile-only"): return janitor.build_plan( runs, jobs, prs or {}, threshold=threshold, max_cancels=max_cancels, pull_request_policy=policy, + now=NOW, ) @@ -97,7 +116,7 @@ def test_never_touched_runs(self): pr = make_pr(state="CLOSED", draft=True, head="zzz") self.assertIsNone(janitor.classify( run, janitor.macos_usage(mac_jobs(queued=3)), pr, - newer_ci_run_waiting=True, pull_request_policy="compile-only", + newer_ci_run_waiting=True, pull_request_policy="compile-only", now=NOW, )) def test_protected_runs_count_toward_the_queue_but_are_never_planned(self): @@ -117,7 +136,7 @@ def test_ordinary_pr_and_exp_push_are_not_protected(self): class CategoryTests(unittest.TestCase): def classify(self, run, pr=None, *, jobs=None, newer=False, policy="compile-only"): usage = janitor.macos_usage(jobs if jobs is not None else mac_jobs(queued=2)) - return janitor.classify(run, usage, pr, newer_ci_run_waiting=newer, pull_request_policy=policy) + return janitor.classify(run, usage, pr, newer_ci_run_waiting=newer, pull_request_policy=policy, now=NOW) def test_experiment_push(self): run = make_run(event="push", branch="exp/incremental-foo", name="Xcode incremental generation canary") @@ -188,6 +207,147 @@ def test_stale_draft_is_still_stale(self): self.assertEqual(verdict[0], "stale-pr") +class DoomedCategoryTests(unittest.TestCase): + """An `app-host unit tests` shard failure decides ci-status by construction. + + ci-status accepts only `success` or `skipped` from the `macos` + reusable-workflow call, so one failed shard fails the required check and no + later job takes it back. Across the 299 CI runs created between + 2026-09-22T06:05Z and 17:00Z, 21 runs had such a failure and ci-status + concluded `failure` in all 21. + """ + + def classify(self, *, jobs=None, pr=None, run=None): + return janitor.classify( + run or make_run(), janitor.macos_usage(jobs if jobs is not None else doomed_jobs()), + pr if pr is not None else make_pr(), + newer_ci_run_waiting=False, pull_request_policy="compile-only", now=NOW, + ) + + def test_failed_shard_with_macos_jobs_still_held_is_doomed(self): + verdict = self.classify() + self.assertEqual(verdict[0], "doomed") + self.assertIn("app-host unit tests (3/6)", verdict[1]) + self.assertIn("3 macOS job(s) still held", verdict[1]) + self.assertIn("PR #1 (feature)", verdict[1]) + + def test_earliest_failed_shard_is_the_one_reported(self): + jobs = doomed_jobs(failed_age=30) + jobs.append({"status": "completed", "labels": [MAC], "created_at": iso(60), + "name": "macos / app-host unit tests (5/6)", "conclusion": "failure", + "completed_at": iso(40)}) + self.assertIn("(5/6)", self.classify(jobs=jobs)[1]) + + def test_no_macos_job_left_to_reclaim_is_kept(self): + # usage.held == 0 short-circuits every category: cancelling would free + # nothing and only destroy the Linux results. + self.assertIsNone(self.classify(jobs=doomed_jobs(queued=0, running=0))) + + def test_macos_jobs_held_without_a_shard_failure_are_kept(self): + for conclusion in ("success", "skipped", "cancelled"): + with self.subTest(conclusion=conclusion): + self.assertIsNone(self.classify(jobs=doomed_jobs(conclusion=conclusion))) + + def test_continue_on_error_step_failure_is_kept(self): + # The jobs API does not report continue-on-error and does not need to: + # a job whose only failed steps tolerate failure concludes `success`, + # and ci-status reads the job, not the step. + jobs = doomed_jobs(conclusion="success") + jobs[-1]["steps"] = [{"name": "Upload xcresults", "conclusion": "failure"}] + self.assertIsNone(self.classify(jobs=jobs)) + + def test_app_host_job_has_no_job_level_continue_on_error(self): + # Job-level continue-on-error is not absorbed by the job conclusion the + # test above relies on, so reading a `failure` conclusion as decisive + # would stop being sound. + macos = (ROOT / ".github/workflows/ci-macos.yml").read_text(encoding="utf-8") + block = macos.split("\n app-host-unit-tests:\n", 1)[1].split("\n ", 1)[0] + self.assertNotIn("\n continue-on-error", "\n" + block) + + def test_a_run_fixing_the_failing_job_is_kept(self): + """The run repairing app-host is the one that most needs its shards. + + Each path is a real diff from a run this rule flagged in the census + window: PR #13643 (fix/app-host-green and siblings), #13579, #13427, + #13414 and #13615 were all repairing the app-host lane when a shard of + their own run failed. Run 35738641571 on `ci-6134-isolate-ssh-fish-hang` + was cancelled by hand for exactly this and had to be restarted. + """ + repairs = { + "PR #13643 app-host test sources": "cmuxTests/AgentSessionAutoResumeSettingsTests.swift", + "PR #13408 the failing regression itself": "cmuxTests/WorkspaceSSHFishShellTests.swift", + "PR #13579 shard splitter": "scripts/ci/cmux_unit_test_shard.py", + "PR #13579 shard workload": "scripts/ci/workloads/macos-app-host-test-shard.sh", + "PR #13427 job definition": ".github/workflows/ci-macos.yml", + "PR #13414 test product build": "scripts/ci/compile-app-host-test-product.sh", + "quarantine list": "scripts/ci/app-host-known-failures.json", + } + for name, path in repairs.items(): + with self.subTest(name=name): + pr = make_pr(files=("Sources/AppDelegate.swift", path)) + self.assertIsNone(self.classify(pr=pr)) + + def test_an_unrelated_diff_is_still_doomed(self): + # PR #13218 changed nothing the shard consumes, so its remaining macOS + # jobs are only holding pool capacity. + pr = make_pr(files=("Sources/JSONC.swift", "web/app/page.tsx", "tests/test_jsonc.py")) + self.assertEqual(self.classify(pr=pr)[0], "doomed") + + def test_unreadable_diff_is_kept(self): + # A diff past the page size, or one GraphQL did not return, cannot show + # that a path is absent. + self.assertIsNone(self.classify(pr=make_pr(files_truncated=True))) + self.assertIsNone(self.classify(pr=make_pr(files=None))) + + def test_opt_out_label_is_honoured(self): + # The escape hatch for a fix that lives entirely in product code, which + # no path list can distinguish from an ordinary change. + self.assertIsNone(self.classify(pr=make_pr(labels=["full-ci", "no-janitor"]))) + + def test_recent_failure_waits_out_the_grace_window(self): + self.assertIsNone(self.classify(jobs=doomed_jobs(failed_age=5))) + + def test_undated_failure_fails_closed(self): + self.assertIsNone(self.classify(jobs=doomed_jobs(completed_at=False))) + + def test_rerun_is_kept(self): + self.assertIsNone(self.classify(run=make_run(attempt=2))) + + def test_missing_attempt_fails_closed(self): + run = make_run() + del run["run_attempt"] + self.assertIsNone(self.classify(run=run)) + + def test_only_the_ci_workflow(self): + run = make_run(path=".github/workflows/ci-macos.yml", name="CI macOS") + self.assertIsNone(self.classify(run=run)) + + def test_closed_or_superseded_pr_stays_stale_pr(self): + # The earlier categories win: they need no diff read and no grace wait. + self.assertEqual(self.classify(pr=make_pr(state="MERGED"))[0], "stale-pr") + self.assertEqual(self.classify(pr=make_pr(head="bbb"))[0], "stale-pr") + + def test_doomed_is_spent_last_and_under_the_shared_cap(self): + main_run, main_jobs = busy_main_push(queued=9) + exp = make_run(event="push", branch="exp/a", name="canary") + doomed = make_run(branch="feature", sha="aaa") + runs = [main_run, exp, doomed] + jobs = {main_run["id"]: main_jobs, exp["id"]: mac_jobs(queued=2), doomed["id"]: doomed_jobs()} + result = plan(runs, jobs, {"feature": [make_pr()]}, max_cancels=1) + self.assertEqual([c.category for c in result.to_cancel()], ["experiment"]) + skipped = [d for d in result.decisions if d.action == "skip"] + self.assertEqual([d.candidate.category for d in skipped], ["doomed"]) + self.assertIn("per-sweep cap", skipped[0].note) + + def test_doomed_does_not_fire_below_the_queue_threshold(self): + # A doomed run is only worth cancelling when its slots are contended. + doomed = make_run(branch="feature", sha="aaa") + result = plan([doomed], {doomed["id"]: doomed_jobs()}, {"feature": [make_pr()]}) + self.assertEqual(result.to_cancel(), []) + self.assertTrue(all(d.action == "skip" for d in result.decisions)) + self.assertIn("not over", result.decisions[0].note) + + class ResolvePullRequestTests(unittest.TestCase): def test_prefers_run_pull_request_number(self): run = make_run(prs=[2])