From ded60a833048c6bb7ddbe54dd1fe019cf1479b8e Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Fri, 25 Sep 2026 20:46:59 -0700 Subject: [PATCH 1/3] fix(ci): measured per-file timeout budgets + 16 queue slices (t_bf25c6b2) 13 of 14 failed merge_group runs (2026-09-25 15:00-20:30 PT) were two files crossing the FIXED 300 s per-file wall (0 assertion failures): test_kanban_home_session.py and test_execution_flag_detection.py. Each kill ejected the queue entry and cancelled the builds behind it. - run_tests_parallel.py: per-file budget = clamp(3 x p90, floor, cap), floor = --file-timeout (300), cap = max(floor, 900). Basis = p90 of the file's recent samples (new test_durations_history.json) plus its last cached duration. --generate-slices stamps `file_timeouts` (path=secs, only files above the floor) into each slice row; the test job passes it via the new --file-timeouts flag. - tests.yml: generate restores the history cache (own key/path, LPT cache untouched); save-durations appends each main run's durations, keeping the newest 20 per file. - ci.yaml: merge_group runs 16 slices like pull_request (was 8). Verified: tests/test_run_tests_parallel_file_budget.py 19 passed (+ arm and runner routing suites, 86 total); mutants (runner ignores budgets / generate skips stamping / basis ignores history) turn 4 and 2 tests RED. Replayed over 399 real slice-duration artifacts: 3x last-sample would still have killed 2 runs; 3x p90(last 20) killed 0. --- .github/workflows/ci.yaml | 10 +- .github/workflows/tests.yml | 46 +++- scripts/run_tests_parallel.py | 232 ++++++++++++++++++- tests/test_run_tests_parallel_file_budget.py | 201 ++++++++++++++++ 4 files changed, 484 insertions(+), 5 deletions(-) create mode 100644 tests/test_run_tests_parallel_file_budget.py diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a6d3d01fd92db..e99bb4751772e 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -156,7 +156,15 @@ jobs: # and all CI jobs share the 60-concurrent hosted cap: 13 in-flight # runs at ~20 jobs held the queue at 0 merges in 40 min. Halving the # gate's matrix halves its share of that cap. - slice_count: ${{ github.event_name == 'merge_group' && 8 || 16 }} + # + # 2026-09-25 (t_bf25c6b2): merge_group back to 16. At 8 each queue slice + # carried ~2x the files and ran 10-12 min, and heavy files sharing a + # loaded runner crossed the fixed 300 s per-file wall: 13 of 14 failed + # queue runs 15:00-20:30 PT were a per-file timeout (0 assertion + # failures), each ejecting its entry and cancelling the builds behind + # it. The hosted cap is 120 since 09-25, so the cap-share argument + # above no longer binds. + slice_count: 16 test_scope: ${{ needs.detect.outputs.test_scope }} # macOS + Windows lanes. The main `tests` lane above is Linux-only, and diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 727ddf6365c7b..c9a490fb74e3a 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -60,6 +60,18 @@ jobs: restore-keys: | test-durations- + - name: Restore duration history (per-file timeout budgets) + # Separate entry from the LPT cache above (t_bf25c6b2): the newest + # samples per file, whose p90 sets each file's measured timeout + # budget. Absent -> budgets fall back to the single cached value, then + # to the 300 s floor (today's behaviour). + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: test_durations_history.json + key: test-durations-history-restore-${{ github.run_id }} + restore-keys: | + test-durations-history- + - name: Generate test slices id: matrix env: @@ -291,7 +303,10 @@ jobs: run: | source .venv/bin/activate # Verbose pytest output lets timeout verdicts name the last test reached. - scripts/run_tests.sh --files '${{ matrix.slice.files }}' -- -v + # file_timeouts: measured per-file budgets stamped by the generate job + # (3x p90 duration, floor 300 s, cap 900 s; t_bf25c6b2). Empty for + # a slice with no heavy file, and for a matrix that lacks the field. + scripts/run_tests.sh --files '${{ matrix.slice.files }}' --file-timeouts '${{ matrix.slice.file_timeouts }}' -- -v env: # Ensure tests don't accidentally call real APIs OPENROUTER_API_KEY: "" @@ -330,6 +345,14 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 10 steps: + # First, so checkout's clean cannot touch the downloaded durations. + - name: Checkout runner script + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + sparse-checkout: scripts/run_tests_parallel.py + sparse-checkout-cone-mode: false + - name: Download all slice durations uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: @@ -374,6 +397,27 @@ jobs: path: test_durations.json key: test-durations-${{ github.run_id }} + # Rolling per-file history for the measured timeout budgets (see the + # generate job): restore the newest entry, append this run, save anew. + # Same cache path as the generate job's restore (actions/cache versions + # entries by path). + - name: Restore previous duration history + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: test_durations_history.json + key: test-durations-history-restore-${{ github.run_id }} + restore-keys: | + test-durations-history- + + - name: Append this run to the duration history + run: python3 scripts/run_tests_parallel.py --merge-duration-history test_durations.json + + - name: Save duration history + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: test_durations_history.json + key: test-durations-history-${{ github.run_id }} + e2e: needs: [generate, placement] if: always() && !cancelled() && needs.generate.result == 'success' diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index 6d90348ca8c7e..c6166ab1a8d94 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -195,6 +195,30 @@ def format_worker_sizing_log( # time while keeping a genuinely hung file bounded. _DEFAULT_FILE_TIMEOUT_SECONDS = 300.0 +# Per-file MEASURED budget (t_bf25c6b2). A fixed 300 s ceiling is a coin flip +# for a file whose cached wall time is 150-280 s: it passes on a fast runner +# and is SIGKILL'd on a loaded one. Measured 2026-09-25: 13 of 14 failed +# merge_group runs were two such files, each kill ejecting its queue entry and +# cancelling every build behind it. The budget is therefore +# clamp(_FILE_TIMEOUT_MULTIPLIER x cached_duration, floor, cap) +# where floor is --file-timeout (default 300 s) and cap is +# max(floor, _FILE_TIMEOUT_CAP_SECONDS). A genuinely hung file stays bounded +# by the cap; files with no cached duration keep the floor. +_FILE_TIMEOUT_MULTIPLIER = 3.0 +_FILE_TIMEOUT_CAP_SECONDS = 900.0 + +# The duration cache above holds ONE last-observed wall per file, which is a +# bad budget basis: measured over 399 slice artifacts (2026-09-26 02:24-03:24Z) +# test_kanban_home_session.py ranged 75-300 s on single attempts and +# test_execution_flag_detection.py 13-413 s. A budget of 3x a fast sample +# (75 s -> 300 s floor) kills the next slow run. So the save-durations job also +# keeps the last _DURATION_HISTORY_KEEP samples per file (main-branch runs) in +# a separate cache entry, and the budget basis is the p90 of those samples plus +# the last-observed value. Separate file + cache key so the existing LPT cache +# entry (and its actions/cache version hash) is untouched. +_DURATION_HISTORY_FILE = "test_durations_history.json" +_DURATION_HISTORY_KEEP = 20 + # One-shot retry of failing test FILES. A file that exits non-zero is re-run # once in a fresh subprocess; if the re-run passes, the file counts as passed # but is loudly reported as FLAKY so it gets fixed rather than hidden. @@ -1282,6 +1306,129 @@ def _load_durations(repo_root: Path) -> dict[str, float]: return {} +def _load_duration_history(repo_root: Path) -> dict[str, list[float]]: + """Read ``test_durations_history.json`` (path -> recent wall samples). + + Missing, corrupt, or wrongly-shaped data -> empty (budgets then fall back + to the single-value duration cache, then to the floor).""" + path = repo_root / _DURATION_HISTORY_FILE + if not path.is_file(): + return {} + try: + raw = json.loads(path.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError): + return {} + if not isinstance(raw, dict): + return {} + out: dict[str, list[float]] = {} + for rel, samples in raw.items(): + if isinstance(samples, list): + vals = [float(v) for v in samples if isinstance(v, (int, float)) and v > 0] + if vals: + out[str(rel)] = vals + return out + + +def _merge_duration_history( + history: dict[str, list[float]], + new: dict[str, float], + keep: int = _DURATION_HISTORY_KEEP, +) -> dict[str, list[float]]: + """Append one run's durations to *history*, keeping the newest *keep*.""" + merged = {rel: list(samples) for rel, samples in history.items()} + for rel, value in new.items(): + if isinstance(value, (int, float)) and value > 0: + merged.setdefault(rel, []).append(round(float(value), 3)) + return {rel: samples[-keep:] for rel, samples in merged.items() if samples} + + +def _p90(samples: List[float]) -> float: + ordered = sorted(samples) + return ordered[min(len(ordered) - 1, int(round(0.9 * (len(ordered) - 1))))] + + +def _budget_basis( + rel: str, + durations: dict[str, float], + history: dict[str, list[float]] | None = None, +) -> float | None: + """p90 of the file's history samples plus its last-observed duration.""" + samples = list((history or {}).get(rel, [])) + last = durations.get(rel) + if isinstance(last, (int, float)) and last > 0: + samples.append(float(last)) + return _p90(samples) if samples else None + + +def _measured_file_timeout(duration: float | None, floor: float) -> float: + """Return the per-file budget for a file whose basis wall is *duration*. + + ``clamp(3 x duration, floor, max(floor, 900))``. Unknown or non-positive + durations get the floor, so an empty cache reproduces the old fixed cap. + """ + cap = max(floor, _FILE_TIMEOUT_CAP_SECONDS) + if not isinstance(duration, (int, float)) or duration <= 0: + return floor + return min(cap, max(floor, _FILE_TIMEOUT_MULTIPLIER * float(duration))) + + +def _file_timeouts_spec( + files: List[Path], + durations: dict[str, float], + repo_root: Path, + floor: float = _DEFAULT_FILE_TIMEOUT_SECONDS, + history: dict[str, list[float]] | None = None, +) -> str: + """Encode the budgets that exceed *floor* as ``path=secs:path=secs``. + + Only raised budgets are listed, so the string stays tiny (a handful of + heavy files) inside the ~350 KB slice matrix. Same separator as + ``--files`` so ``_split_pathspec`` parses it. + """ + parts = [] + for f in files: + rel = _format_file(f, repo_root) + budget = _measured_file_timeout( + _budget_basis(rel, durations, history), floor + ) + if budget > floor: + parts.append(f"{rel}={int(round(budget))}") + return ":".join(parts) + + +def _parse_file_timeouts(spec: str) -> dict[str, float]: + """Parse a ``--file-timeouts`` spec. Malformed entries are ignored + (they fall back to the floor, i.e. today's behaviour).""" + out: dict[str, float] = {} + for item in _split_pathspec(spec or ""): + rel, sep, secs = item.rpartition("=") + if not sep or not rel: + continue + try: + value = float(secs) + except ValueError: + continue + if value > 0: + out[rel] = value + return out + + +def _stamp_file_timeouts( + matrix: dict, durations: dict[str, float], repo_root: Path +) -> None: + """Stamp each slice row with ``file_timeouts`` (see _file_timeouts_spec). + + The test jobs do not restore the duration cache, so the generate job, + which already has it for LPT, carries the budgets to them in the matrix. + """ + history = _load_duration_history(repo_root) + for row in matrix.get("slice", []): + files = [repo_root / f for f in _split_pathspec(str(row.get("files", "")))] + row["file_timeouts"] = _file_timeouts_spec( + files, durations, repo_root, history=history + ) + + def _save_durations( file_times: List[Tuple[Path, float]], repo_root: Path, @@ -1454,7 +1601,31 @@ def main() -> int: help=( "Per-file wall-clock cap in seconds. On timeout, the pytest " "subprocess and its full process tree are SIGKILL'd. " - f"Default: {_DEFAULT_FILE_TIMEOUT_SECONDS}s ({round(_DEFAULT_FILE_TIMEOUT_SECONDS/60)} min), env: HERMES_TEST_FILE_TIMEOUT." + f"Default: {_DEFAULT_FILE_TIMEOUT_SECONDS}s ({round(_DEFAULT_FILE_TIMEOUT_SECONDS/60)} min), env: HERMES_TEST_FILE_TIMEOUT. " + "This is the FLOOR: a file with a cached duration gets " + f"clamp({_FILE_TIMEOUT_MULTIPLIER:g} x duration, floor, " + f"max(floor, {_FILE_TIMEOUT_CAP_SECONDS:g}))." + ), + ) + parser.add_argument( + "--merge-duration-history", + metavar="NEW_DURATIONS_JSON", + default=None, + help=( + "CI plumbing (save-durations job): append the per-file durations " + f"in NEW_DURATIONS_JSON to {_DURATION_HISTORY_FILE} at the repo " + f"root, keep the newest {_DURATION_HISTORY_KEEP} samples per file, " + "and exit." + ), + ) + parser.add_argument( + "--file-timeouts", + default="", + help=( + "Measured per-file budgets as 'path=secs:path=secs' (stamped into " + "the slice matrix by --generate-slices). Files not listed use the " + "budget computed from the local test_durations.json, else the " + "--file-timeout floor. Never lowers a budget below the floor." ), ) parser.add_argument( @@ -1642,7 +1813,9 @@ def main() -> int: # (``-k=expr``, ``--tb=long``) are self-contained and need no lookahead. OUR_FLAGS = { "-j", "--jobs", "--paths", "--include-integration", - "--file-timeout", "--file-retries", "--slice", "--generate-slices", "--files", + "--file-timeout", "--file-timeouts", "--merge-duration-history", + "--file-retries", "--slice", + "--generate-slices", "--files", "--changed-files-scope", "--test-scope", "--self-hosted-slots", "--self-hosted-labels", "--arm-hosted-slices", "--x64-hosted-min", "--blacksmith-slices", "--event", "--same-repo", @@ -1787,6 +1960,24 @@ def _is_our_flag(tok: str) -> bool: repo_root = Path(__file__).resolve().parent.parent + if args.merge_duration_history: + try: + new = json.loads( + Path(args.merge_duration_history).read_text(encoding="utf-8") + ) + except (OSError, json.JSONDecodeError) as e: + print(f"error: cannot read {args.merge_duration_history}: {e}", file=sys.stderr) + return 1 + if not isinstance(new, dict): + print("error: durations JSON must be an object", file=sys.stderr) + return 1 + merged = _merge_duration_history(_load_duration_history(repo_root), new) + (repo_root / _DURATION_HISTORY_FILE).write_text( + json.dumps(merged, indent=1, sort_keys=True) + "\n", encoding="utf-8" + ) + print(f"duration history: {len(merged)} files", file=sys.stderr) + return 0 + if args.changed_files_scope: print(_plugin_scope_from_changes(sys.stdin.read().splitlines())) return 0 @@ -1805,6 +1996,9 @@ def _is_our_flag(tok: str) -> bool: _route_blacksmith_slices( scoped_matrix, args.blacksmith_slices, args.event, args.same_repo ) + _stamp_file_timeouts( + scoped_matrix, _load_durations(repo_root), repo_root + ) print( f"Test scope: {args.test_scope} + core smoke" f" ({len(scoped_matrix['slice'])} slices)", @@ -1889,6 +2083,7 @@ def _is_our_flag(tok: str) -> bool: _route_blacksmith_slices( matrix, args.blacksmith_slices, args.event, args.same_repo ) + _stamp_file_timeouts(matrix, durations, repo_root) # Print to stdout so the CI step can capture it with $(). print(json.dumps(matrix)) return 0 @@ -1992,13 +2187,44 @@ def _on_done(file: Path, started_at: float, fut: "Future[Tuple[Path, int, str, D if rc != 0: _print_inline_failure(fpath, output, repo_root, pytest_passthrough) + # Measured per-file budgets: the matrix-stamped spec wins, then the local + # duration cache; never below the --file-timeout floor. + stamped = _parse_file_timeouts(args.file_timeouts) + local_durations = _load_durations(repo_root) if not stamped else {} + local_history = _load_duration_history(repo_root) if not stamped else {} + file_budgets: dict[Path, float] = {} + for file in files: + rel = _format_file(file, repo_root) + if rel in stamped: + budget = min( + max(args.file_timeout, _FILE_TIMEOUT_CAP_SECONDS), + max(args.file_timeout, stamped[rel]), + ) + else: + budget = _measured_file_timeout( + _budget_basis(rel, local_durations, local_history), + args.file_timeout, + ) + file_budgets[file] = budget + raised = sorted( + (b, _format_file(f, repo_root)) + for f, b in file_budgets.items() + if b > args.file_timeout + ) + if raised: + print( + f"Measured per-file budgets above the {args.file_timeout:.0f}s floor: " + + ", ".join(f"{rel}={b:.0f}s" for b, rel in reversed(raised)), + flush=True, + ) + with ThreadPoolExecutor(max_workers=args.jobs) as pool: futures: List[Future] = [] for file in files: t0 = time.monotonic() fut = pool.submit( _run_one_file, file, pytest_passthrough, repo_root, - args.file_timeout, args.file_retries, + file_budgets[file], args.file_retries, ) fut.add_done_callback(lambda f, file=file, t0=t0: _on_done(file, t0, f)) futures.append(fut) diff --git a/tests/test_run_tests_parallel_file_budget.py b/tests/test_run_tests_parallel_file_budget.py new file mode 100644 index 0000000000000..0463719eaef08 --- /dev/null +++ b/tests/test_run_tests_parallel_file_budget.py @@ -0,0 +1,201 @@ +"""Measured per-file timeout budgets (t_bf25c6b2). + +A fixed 300 s per-file wall killed files whose cached wall time sat near it +(13 of 14 failed merge_group runs on 2026-09-25, 0 assertion failures). The +budget is now clamp(3 x cached duration, floor, max(floor, 900)), stamped into +the slice matrix by --generate-slices and applied by the runner. +""" +import importlib.util +import json +import sys +from pathlib import Path + +import pytest +import yaml + +ROOT = Path(__file__).resolve().parents[1] +SCRIPT = ROOT / "scripts/run_tests_parallel.py" +# Real files so --files resolves; their content never runs (the runner seam +# is replaced below). +HEAVY = "tests/test_run_tests_parallel_timeout_verdict.py" +LIGHT = "tests/test_run_tests_parallel_stdio.py" + + +def _load(): + spec = importlib.util.spec_from_file_location("budget_runner", SCRIPT) + assert spec and spec.loader + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +@pytest.mark.parametrize( + "duration,floor,expected", + [ + (None, 300.0, 300.0), # no cache entry -> today's fixed cap + (0.0, 300.0, 300.0), + (-5.0, 300.0, 300.0), + (50.0, 300.0, 300.0), # 3x below floor -> floor + (150.0, 300.0, 450.0), # the edge case that used to be a coin flip + (240.0, 300.0, 720.0), + (400.0, 300.0, 900.0), # capped: a real hang stays bounded + (400.0, 1200.0, 1200.0), # an explicit floor above the cap wins + ], +) +def test_measured_budget(duration, floor, expected): + mod = _load() + assert mod._measured_file_timeout(duration, floor) == expected + + +def test_spec_round_trip_lists_only_raised_budgets(): + mod = _load() + durations = {HEAVY: 240.0, LIGHT: 12.0} + spec = mod._file_timeouts_spec([ROOT / HEAVY, ROOT / LIGHT], durations, ROOT) + assert spec == f"{HEAVY}=720" + assert mod._parse_file_timeouts(spec) == {HEAVY: 720.0} + # Malformed entries fall back to the floor instead of crashing the slice. + assert mod._parse_file_timeouts(f"garbage:{LIGHT}=x:=5:{HEAVY}=0") == {} + assert mod._parse_file_timeouts("") == {} + + +def test_generate_slices_stamps_file_timeouts(monkeypatch, capsys): + mod = _load() + files = [ROOT / HEAVY, ROOT / LIGHT] + monkeypatch.setattr(mod, "_discover_files", lambda roots: files) + monkeypatch.setattr(mod, "_load_durations", lambda root: {HEAVY: 240.0, LIGHT: 12.0}) + monkeypatch.setattr(sys, "argv", [str(SCRIPT), "--generate-slices", "2"]) + assert mod.main() == 0 + rows = json.loads(capsys.readouterr().out)["slice"] + by_files = {r["files"]: r["file_timeouts"] for r in rows} + assert by_files == {HEAVY: f"{HEAVY}=720", LIGHT: ""} + + +def _run_main(mod, monkeypatch, capsys, extra_args, durations=None): + seen: dict[str, float] = {} + + def fake_run(file, pytest_args, repo_root, file_timeout, retries=0): + seen[mod._format_file(file, repo_root)] = file_timeout + return file, 0, "1 passed in 0.01s", {"passed": 1}, 0.01 + + monkeypatch.setattr(mod, "_run_one_file", fake_run) + monkeypatch.setattr(mod, "_load_durations", lambda root: dict(durations or {})) + monkeypatch.setattr(mod, "_save_durations", lambda *a, **k: None) + monkeypatch.setattr( + sys, "argv", + [str(SCRIPT), "--files", f"{HEAVY}:{LIGHT}", "--no-strict-noop", *extra_args], + ) + mod.main() + out = capsys.readouterr().out + return seen, out + + +def test_runner_applies_stamped_budget_per_file(monkeypatch, capsys): + mod = _load() + seen, out = _run_main(mod, monkeypatch, capsys, ["--file-timeouts", f"{HEAVY}=720"]) + assert seen == {HEAVY: 720.0, LIGHT: 300.0} + assert f"{HEAVY}=720s" in out + + +def test_runner_never_goes_below_floor_or_above_cap(monkeypatch, capsys): + mod = _load() + seen, _ = _run_main( + mod, monkeypatch, capsys, + ["--file-timeout", "400", "--file-timeouts", f"{HEAVY}=350:{LIGHT}=5000"], + ) + assert seen == {HEAVY: 400.0, LIGHT: 900.0} + + +def test_runner_uses_local_cache_when_no_spec(monkeypatch, capsys): + mod = _load() + seen, _ = _run_main(mod, monkeypatch, capsys, [], durations={HEAVY: 200.0}) + assert seen == {HEAVY: 600.0, LIGHT: 300.0} + + +def test_budget_basis_is_p90_of_history_not_the_last_sample(): + """The measured failure: one fast sample (75 s) must not set a 300 s + budget for a file whose slow runs take 250-300 s.""" + mod = _load() + history = {HEAVY: [196, 192, 259, 106, 164, 128, 273, 154, 300, 270]} + durations = {HEAVY: 75.0} + basis = mod._budget_basis(HEAVY, durations, history) + assert basis == 273 + assert mod._measured_file_timeout(basis, 300.0) == 819.0 + # Last sample alone (no history) is still used; nothing at all -> None. + assert mod._budget_basis(HEAVY, durations, {}) == 75.0 + assert mod._budget_basis(LIGHT, {}, {}) is None + + +def test_generate_stamps_from_history(monkeypatch, capsys): + mod = _load() + files = [ROOT / HEAVY, ROOT / LIGHT] + monkeypatch.setattr(mod, "_discover_files", lambda roots: files) + monkeypatch.setattr(mod, "_load_durations", lambda root: {HEAVY: 75.0, LIGHT: 12.0}) + monkeypatch.setattr( + mod, "_load_duration_history", lambda root: {HEAVY: [250.0, 280.0, 75.0]} + ) + monkeypatch.setattr(sys, "argv", [str(SCRIPT), "--generate-slices", "2"]) + assert mod.main() == 0 + rows = json.loads(capsys.readouterr().out)["slice"] + assert {r["files"]: r["file_timeouts"] for r in rows}[HEAVY] == f"{HEAVY}=840" + + +def test_merge_duration_history_cli_appends_and_trims(tmp_path, monkeypatch): + mod = _load() + monkeypatch.setattr(mod, "__file__", str(tmp_path / "scripts" / "run_tests_parallel.py")) + hist_path = tmp_path / mod._DURATION_HISTORY_FILE + hist_path.write_text( + json.dumps({HEAVY: list(range(1, mod._DURATION_HISTORY_KEEP + 1)), "gone.py": [5]}), + encoding="utf-8", + ) + new = tmp_path / "test_durations.json" + new.write_text(json.dumps({HEAVY: 99.5, LIGHT: 3.0, "bad.py": -1}), encoding="utf-8") + monkeypatch.setattr(sys, "argv", [str(SCRIPT), "--merge-duration-history", str(new)]) + assert mod.main() == 0 + merged = json.loads(hist_path.read_text(encoding="utf-8")) + assert merged[HEAVY][-1] == 99.5 + assert len(merged[HEAVY]) == mod._DURATION_HISTORY_KEEP + assert merged[HEAVY][0] == 2 # oldest sample dropped + assert merged[LIGHT] == [3.0] + assert merged["gone.py"] == [5] # files not in this run keep their history + assert "bad.py" not in merged + # A corrupt history file must not break the merge (starts fresh). + hist_path.write_text("{not json", encoding="utf-8") + assert mod.main() == 0 + assert json.loads(hist_path.read_text(encoding="utf-8"))[HEAVY] == [99.5] + + +def test_history_cache_path_matches_between_restore_and_save(): + """actions/cache versions entries by path: generate's restore must use + the exact path save-durations saves, or it can never hit.""" + wf = yaml.safe_load((ROOT / ".github/workflows/tests.yml").read_text(encoding="utf-8")) + + def history_paths(job): + return { + s["with"]["path"] for s in wf["jobs"][job]["steps"] + if "actions/cache" in str(s.get("uses", "")) + and "history" in str(s.get("with", {}).get("key", "")) + } + + assert history_paths("generate") == {"test_durations_history.json"} + assert history_paths("save-durations") == {"test_durations_history.json"} + steps = [s.get("name", "") for s in wf["jobs"]["save-durations"]["steps"]] + assert steps[0] == "Checkout runner script" + assert steps.index("Append this run to the duration history") > steps.index( + "Merge into single durations file" + ) + + +def test_workflow_passes_the_stamped_budgets_to_the_runner(): + wf = yaml.safe_load((ROOT / ".github/workflows/tests.yml").read_text(encoding="utf-8")) + run = next( + s["run"] for s in wf["jobs"]["test"]["steps"] + if "--files" in str(s.get("run", "")) + ) + assert "--file-timeouts '${{ matrix.slice.file_timeouts }}'" in run + + +def test_merge_group_runs_the_same_slice_count_as_pull_requests(): + ci = yaml.safe_load((ROOT / ".github/workflows/ci.yaml").read_text(encoding="utf-8")) + count = ci["jobs"]["tests"]["with"]["slice_count"] + assert "merge_group" not in str(count) + assert int(count) == 16 From 5c755baed803f440b10a678f6491bc614ca8aac1 Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Fri, 25 Sep 2026 21:15:01 -0700 Subject: [PATCH 2/3] fix(ci): unmeasured test files get the 900 s cap, not the 300 s floor (t_bf25c6b2) A file with no duration sample is not known to fit in 300 s; a false kill ejects a merge-queue entry and cancels every build behind it. The budget for an unmeasured file is now max(floor, 900). - generate stamps unmeasured files at the cap; with no duration data at all it stamps a single '*=' default entry, not one per file. - --file-timeouts default is None. When the flag is passed (CI always passes it, possibly ''), the stamp is authoritative: unlisted file -> '*' entry, else floor. The test jobs have no local cache, so falling through to it would have made every file unmeasured. Verified: tests/test_run_tests_parallel_file_budget.py 23 passed. Mutants: unmeasured->floor 5 RED; passed empty stamp not authoritative 1 RED. ruff clean. --- scripts/run_tests_parallel.py | 39 +++++++++++++------- tests/test_run_tests_parallel_file_budget.py | 34 +++++++++++++++-- 2 files changed, 55 insertions(+), 18 deletions(-) diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index c6166ab1a8d94..1464d6db20df3 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -203,7 +203,8 @@ def format_worker_sizing_log( # clamp(_FILE_TIMEOUT_MULTIPLIER x cached_duration, floor, cap) # where floor is --file-timeout (default 300 s) and cap is # max(floor, _FILE_TIMEOUT_CAP_SECONDS). A genuinely hung file stays bounded -# by the cap; files with no cached duration keep the floor. +# by the cap. A file with NO measurement (new file, cold cache) gets the cap: +# nothing proves it fits in the floor, and a false kill ejects a queue entry. _FILE_TIMEOUT_MULTIPLIER = 3.0 _FILE_TIMEOUT_CAP_SECONDS = 900.0 @@ -1364,11 +1365,11 @@ def _measured_file_timeout(duration: float | None, floor: float) -> float: """Return the per-file budget for a file whose basis wall is *duration*. ``clamp(3 x duration, floor, max(floor, 900))``. Unknown or non-positive - durations get the floor, so an empty cache reproduces the old fixed cap. + durations get the cap: an unmeasured file is not known to fit the floor. """ cap = max(floor, _FILE_TIMEOUT_CAP_SECONDS) if not isinstance(duration, (int, float)) or duration <= 0: - return floor + return cap return min(cap, max(floor, _FILE_TIMEOUT_MULTIPLIER * float(duration))) @@ -1381,10 +1382,14 @@ def _file_timeouts_spec( ) -> str: """Encode the budgets that exceed *floor* as ``path=secs:path=secs``. - Only raised budgets are listed, so the string stays tiny (a handful of - heavy files) inside the ~350 KB slice matrix. Same separator as + Only raised budgets are listed (heavy files + unmeasured files), so the + string stays small inside the ~350 KB slice matrix. With no duration data + at all (cold cache) every file is unmeasured, so the spec is the single + default entry ``*=`` instead of one entry per file. Same separator as ``--files`` so ``_split_pathspec`` parses it. """ + if not durations and not history: + return f"*={int(round(max(floor, _FILE_TIMEOUT_CAP_SECONDS)))}" parts = [] for f in files: rel = _format_file(f, repo_root) @@ -1620,12 +1625,13 @@ def main() -> int: ) parser.add_argument( "--file-timeouts", - default="", + default=None, help=( "Measured per-file budgets as 'path=secs:path=secs' (stamped into " - "the slice matrix by --generate-slices). Files not listed use the " - "budget computed from the local test_durations.json, else the " - "--file-timeout floor. Never lowers a budget below the floor." + "the slice matrix by --generate-slices). When passed (even empty) " + "it is authoritative: unlisted files use the '*' entry, else the " + "--file-timeout floor. When omitted, budgets come from the local " + "test_durations.json. Never lowers a budget below the floor." ), ) parser.add_argument( @@ -2189,16 +2195,21 @@ def _on_done(file: Path, started_at: float, fut: "Future[Tuple[Path, int, str, D # Measured per-file budgets: the matrix-stamped spec wins, then the local # duration cache; never below the --file-timeout floor. - stamped = _parse_file_timeouts(args.file_timeouts) - local_durations = _load_durations(repo_root) if not stamped else {} - local_history = _load_duration_history(repo_root) if not stamped else {} + # A passed spec (even '') is the generate job's verdict: it lists every + # file whose budget is above the floor, so an unlisted file is a measured + # light file. The CI test jobs have no local cache, so falling through to + # it would make every file "unmeasured". + use_stamp = args.file_timeouts is not None + stamped = _parse_file_timeouts(args.file_timeouts or "") + local_durations = _load_durations(repo_root) if not use_stamp else {} + local_history = _load_duration_history(repo_root) if not use_stamp else {} file_budgets: dict[Path, float] = {} for file in files: rel = _format_file(file, repo_root) - if rel in stamped: + if use_stamp: budget = min( max(args.file_timeout, _FILE_TIMEOUT_CAP_SECONDS), - max(args.file_timeout, stamped[rel]), + max(args.file_timeout, stamped.get(rel, stamped.get("*", 0.0))), ) else: budget = _measured_file_timeout( diff --git a/tests/test_run_tests_parallel_file_budget.py b/tests/test_run_tests_parallel_file_budget.py index 0463719eaef08..7efa99e9a0b4d 100644 --- a/tests/test_run_tests_parallel_file_budget.py +++ b/tests/test_run_tests_parallel_file_budget.py @@ -32,9 +32,9 @@ def _load(): @pytest.mark.parametrize( "duration,floor,expected", [ - (None, 300.0, 300.0), # no cache entry -> today's fixed cap - (0.0, 300.0, 300.0), - (-5.0, 300.0, 300.0), + (None, 300.0, 900.0), # unmeasured file -> cap (not proven to fit 300) + (0.0, 300.0, 900.0), + (-5.0, 300.0, 900.0), (50.0, 300.0, 300.0), # 3x below floor -> floor (150.0, 300.0, 450.0), # the edge case that used to be a coin flip (240.0, 300.0, 720.0), @@ -70,6 +70,18 @@ def test_generate_slices_stamps_file_timeouts(monkeypatch, capsys): assert by_files == {HEAVY: f"{HEAVY}=720", LIGHT: ""} +def test_unmeasured_file_is_stamped_at_the_cap(): + mod = _load() + spec = mod._file_timeouts_spec([ROOT / HEAVY, ROOT / LIGHT], {LIGHT: 12.0}, ROOT) + assert spec == f"{HEAVY}=900" + + +def test_cold_cache_stamps_a_single_default_entry(): + mod = _load() + spec = mod._file_timeouts_spec([ROOT / HEAVY, ROOT / LIGHT], {}, ROOT) + assert spec == "*=900" + + def _run_main(mod, monkeypatch, capsys, extra_args, durations=None): seen: dict[str, float] = {} @@ -105,10 +117,24 @@ def test_runner_never_goes_below_floor_or_above_cap(monkeypatch, capsys): assert seen == {HEAVY: 400.0, LIGHT: 900.0} +def test_passed_empty_spec_is_authoritative_floor(monkeypatch, capsys): + """CI always passes --file-timeouts; '' means 'all measured, all light'. + It must NOT fall through to the (absent) local cache -> 900 for all.""" + mod = _load() + seen, _ = _run_main(mod, monkeypatch, capsys, ["--file-timeouts", ""]) + assert seen == {HEAVY: 300.0, LIGHT: 300.0} + + +def test_runner_applies_default_entry(monkeypatch, capsys): + mod = _load() + seen, _ = _run_main(mod, monkeypatch, capsys, ["--file-timeouts", f"*=900:{LIGHT}=400"]) + assert seen == {HEAVY: 900.0, LIGHT: 400.0} + + def test_runner_uses_local_cache_when_no_spec(monkeypatch, capsys): mod = _load() seen, _ = _run_main(mod, monkeypatch, capsys, [], durations={HEAVY: 200.0}) - assert seen == {HEAVY: 600.0, LIGHT: 300.0} + assert seen == {HEAVY: 600.0, LIGHT: 900.0} # LIGHT unmeasured -> cap def test_budget_basis_is_p90_of_history_not_the_last_sample(): From 4940fdfe76f02deac7d2ea1932f3584f53f07675 Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Fri, 25 Sep 2026 21:31:34 -0700 Subject: [PATCH 3/3] fix(ci): local no-stamp path keeps the explicit floor for unmeasured files (t_bf25c6b2) 5c755bae gave every unmeasured file the 900 s cap on BOTH paths. The runner's own kill self-tests (test_run_tests_parallel_timeout_verdict.py) run it with --file-timeout 8 on an uncached hanging probe, so the probe got 900 s, and the outer file hit its own 300 s budget: CI slice 4/16 TIMED OUT on run 36217271474. The cap for unmeasured files now applies only on the stamped path, which is what CI always runs. That is where the queue fix is needed. The local no-stamp path keeps the caller's floor. Verified: file_budget + timeout_verdict 35 passed; noop_guard + run_tests_parallel 19 passed / 1 skipped; ruff clean. --- scripts/run_tests_parallel.py | 12 +++++++++--- tests/test_run_tests_parallel_file_budget.py | 12 +++++++++++- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index 1464d6db20df3..66887e86672a9 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -2212,9 +2212,15 @@ def _on_done(file: Path, started_at: float, fut: "Future[Tuple[Path, int, str, D max(args.file_timeout, stamped.get(rel, stamped.get("*", 0.0))), ) else: - budget = _measured_file_timeout( - _budget_basis(rel, local_durations, local_history), - args.file_timeout, + # Local/no-stamp path: an unmeasured file keeps the explicit + # --file-timeout floor (a caller passing a small floor, e.g. the + # runner's own kill self-tests, must get that kill). The CI queue + # always runs the stamped path, where unmeasured files get the cap. + basis = _budget_basis(rel, local_durations, local_history) + budget = ( + args.file_timeout + if basis is None + else _measured_file_timeout(basis, args.file_timeout) ) file_budgets[file] = budget raised = sorted( diff --git a/tests/test_run_tests_parallel_file_budget.py b/tests/test_run_tests_parallel_file_budget.py index 7efa99e9a0b4d..0ea8a0780b8a2 100644 --- a/tests/test_run_tests_parallel_file_budget.py +++ b/tests/test_run_tests_parallel_file_budget.py @@ -134,7 +134,17 @@ def test_runner_applies_default_entry(monkeypatch, capsys): def test_runner_uses_local_cache_when_no_spec(monkeypatch, capsys): mod = _load() seen, _ = _run_main(mod, monkeypatch, capsys, [], durations={HEAVY: 200.0}) - assert seen == {HEAVY: 600.0, LIGHT: 900.0} # LIGHT unmeasured -> cap + # Local path: LIGHT unmeasured keeps the explicit floor (the stamped CI + # path is where unmeasured -> cap); a caller's small floor stays binding. + assert seen == {HEAVY: 600.0, LIGHT: 300.0} + + +def test_local_small_floor_still_kills_an_unmeasured_file(monkeypatch, capsys): + """Regression: the timeout-verdict self-tests pass --file-timeout 8 on an + uncached hanging probe; granting it the 900 s cap hung CI slice 4.""" + mod = _load() + seen, _ = _run_main(mod, monkeypatch, capsys, ["--file-timeout", "8"]) + assert seen == {HEAVY: 8.0, LIGHT: 8.0} def test_budget_basis_is_p90_of_history_not_the_last_sample():