diff --git a/.github/workflows/ci-queue-janitor.yml b/.github/workflows/ci-queue-janitor.yml index fbc9a7639096..1f2f13d9944e 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])