From d2280138320459e340ffba7587979a5d55ede5f7 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 14 Sep 2026 18:35:56 -0700 Subject: [PATCH] fix(ci): feed the timeout scaler only healthy durations, and wire the cache in CI Greptile's two findings on the original PR were both right. 1. The scaler read test_durations.json from the checkout, but CI ran on a fresh runner where that file never exists (it is gitignored and the slicing-era artifact/merge job that produced it is gone). The feature was inert exactly where the false FLAKY kills happen. tests.yml now restores the most recent main-saved cache before the run (PRs read only) and saves it after a green push to main, mirroring the ci-timings-baseline restore/save pattern already in ci.yaml. 2. _save_durations persisted every file's total subprocess wall, including the ~cap of a timed-out attempt and the retry-summed wall of a FLAKY file. With the scaler that compounds: a hang cached at ~300s earns 900s next run, then ~900s cached earns 2700s, until the job timeout is the only bound. _clean_pass_durations drops failed and FLAKY files from the write so a file's cached duration is always a first-attempt-clean measurement; those files keep their previous known-good entry. Tests trimmed to the salvage bar (<=2 invariants for the scaler plus one for the cache filter) and moved next to the other runner tests under tests/scripts/. --- .github/workflows/tests.yml | 25 ++++++++ scripts/run_tests_parallel.py | 36 +++++++++-- ...test_run_tests_parallel_timeout_scaling.py | 64 +++++++++++++++++++ ...test_run_tests_parallel_timeout_scaling.py | 54 ---------------- 4 files changed, 118 insertions(+), 61 deletions(-) create mode 100644 tests/scripts/test_run_tests_parallel_timeout_scaling.py delete mode 100644 tests/test_run_tests_parallel_timeout_scaling.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a0dca54ad0..cfe819a802 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -87,6 +87,22 @@ jobs: # re-download, keeping the persisted cache small and fast to restore. run: uv cache prune --ci + - name: Restore per-file duration cache + # scripts/run_tests_parallel.py raises a file's timeout to + # 3x its last healthy duration (_effective_file_timeout) so a + # known-slow file dilated by load is not SIGKILL'd at the flat cap + # and laundered into a FLAKY retry. The scaler reads + # test_durations.json from the checkout; without this restore the + # file is absent on a fresh runner and the scaler is inert. + # Exact key never matches (run_id differs); restore-keys picks the + # most recent cache saved by a main push. PRs read, only main writes. + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: test_durations.json + key: test-durations-never-exact + restore-keys: | + test-durations- + - name: Run tests # Per-file isolation via scripts/run_tests.sh: each test file runs # in its own freshly-spawned `python -m pytest ` subprocess @@ -125,6 +141,15 @@ jobs: OPENAI_API_KEY: "" NOUS_API_KEY: "" + - name: Save per-file duration cache (main only) + # Only green first-attempt durations are written by the runner, so + # a hang on main cannot ratchet its own bound upward. + if: github.event_name == 'push' && github.ref == 'refs/heads/main' && hashFiles('test_durations.json') != '' + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: test_durations.json + key: test-durations-${{ github.run_id }} + e2e: runs-on: ubuntu-latest timeout-minutes: 15 diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index 9f87f6b75c..4d5e0712f8 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -333,6 +333,26 @@ def _effective_file_timeout( return max(file_timeout, float(cached) * 3.0) +def _clean_pass_durations( + file_times: List[Tuple[Path, float]], + failures: List[Tuple[Path, str, Dict[str, int]]], + flaky: List[Tuple[Path, str]], +) -> List[Tuple[Path, float]]: + """Keep only durations from files that passed on their first attempt. + + ``file_times`` records every file's total subprocess wall, including a + timed-out attempt (~the cap) and retry-summed walls for FLAKY files. + Feeding those into the cache would let the timeout scaler compound: a + file that hung once is cached at ~300s, gets a 900s bound next run, + hangs again and is cached at ~900s, and so on until the job timeout + is the only bound left. A duration is a measurement of a healthy run + or it is not a measurement; failed and retried files keep their last + known-good entry instead. + """ + excluded = {f for f, _o, _s in failures} | {f for f, _o in flaky} + return [(f, t) for f, t in file_times if f not in excluded] + + def _run_one_file( file: Path, pytest_args: List[str], @@ -1225,13 +1245,15 @@ def main() -> int: print(f" {_format_file(f, repo_root)}") print(output.rstrip()) - # Save durations for future --slice runs. Each slice writes its own - # partial test_durations.json; a CI merge step joins them later. - # Locally, _save_durations merges with any existing cache so entries - # from previous runs aren't lost. - if file_times: - _save_durations(file_times, repo_root) - print(f" Durations cached to {_DURATIONS_FILE} ({len(file_times)} files)") + # Save durations for future runs (LPT slicing and the per-file timeout + # scaler, see _effective_file_timeout). _save_durations merges with any + # existing cache so entries from previous runs aren't lost. + clean_times = _clean_pass_durations( + file_times, failures, _FLAKY_RESULTS, + ) + if clean_times: + _save_durations(clean_times, repo_root) + print(f" Durations cached to {_DURATIONS_FILE} ({len(clean_times)} files)") # Per-file time distribution (throwaway diagnostic — shows how # subprocess time is distributed so we can see if startup dominates). diff --git a/tests/scripts/test_run_tests_parallel_timeout_scaling.py b/tests/scripts/test_run_tests_parallel_timeout_scaling.py new file mode 100644 index 0000000000..b56a335e71 --- /dev/null +++ b/tests/scripts/test_run_tests_parallel_timeout_scaling.py @@ -0,0 +1,64 @@ +"""Duration-aware per-file timeout scaling in scripts/run_tests_parallel.py. + +The flat --file-timeout cap (default 300s) falsely SIGKILL'd +known-slow large-collection files under CI load, then the automatic +retry passed — manufacturing FLAKY reports for healthy files +(tests/test_hermes_state.py, 2026-08-18 on main). The scaler gives a +file max(flat_cap, 3 × last observed duration) and never lowers the cap. +Only first-attempt-clean durations feed the cache, so a hang can never +compound its own bound. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +_RUNNER_PATH = REPO_ROOT / "scripts" / "run_tests_parallel.py" + + +def _load_runner(): + spec = importlib.util.spec_from_file_location("run_tests_parallel", _RUNNER_PATH) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +def test_scaler_never_lowers_the_flat_cap() -> None: + mod = _load_runner() + uncached = REPO_ROOT / "tests" / "test_example.py" + fast = REPO_ROOT / "tests" / "test_fast.py" + zero = REPO_ROOT / "tests" / "test_zero.py" + durations = { + mod._format_file(fast, REPO_ROOT): 4.2, + mod._format_file(zero, REPO_ROOT): 0.0, + } + assert mod._effective_file_timeout(uncached, REPO_ROOT, 300.0, None) == 300.0 + assert mod._effective_file_timeout(uncached, REPO_ROOT, 300.0, durations) == 300.0 + assert mod._effective_file_timeout(fast, REPO_ROOT, 300.0, durations) == 300.0 + assert mod._effective_file_timeout(zero, REPO_ROOT, 300.0, durations) == 300.0 + + +def test_slow_file_gets_proportional_headroom() -> None: + mod = _load_runner() + f = REPO_ROOT / "tests" / "test_hermes_state.py" + durations = {mod._format_file(f, REPO_ROOT): 205.0} + # 205s last run → 615s bound: a load-dilated healthy run survives, + # a genuine hang is still killed. + assert mod._effective_file_timeout(f, REPO_ROOT, 300.0, durations) == 615.0 + + +def test_only_first_attempt_clean_durations_feed_the_cache() -> None: + """A timed-out or retried file must not raise its own future bound.""" + mod = _load_runner() + clean = REPO_ROOT / "tests" / "test_clean.py" + hung = REPO_ROOT / "tests" / "test_hung.py" + flaky = REPO_ROOT / "tests" / "test_flaky.py" + file_times = [(clean, 12.0), (hung, 300.4), (flaky, 250.0)] + failures = [(hung, "(300s exceeded; process tree SIGKILL'd)", {})] + flaky_results = [(flaky, "⚠ FLAKY: failed on attempt 1, passed on retry")] + + kept = mod._clean_pass_durations(file_times, failures, flaky_results) + + assert kept == [(clean, 12.0)] diff --git a/tests/test_run_tests_parallel_timeout_scaling.py b/tests/test_run_tests_parallel_timeout_scaling.py deleted file mode 100644 index 8d05295743..0000000000 --- a/tests/test_run_tests_parallel_timeout_scaling.py +++ /dev/null @@ -1,54 +0,0 @@ -"""Duration-aware per-file timeout scaling in scripts/run_tests_parallel.py. - -The flat --file-timeout cap (default 300s) falsely SIGKILL'd -known-slow large-collection files under CI load, then the automatic -retry passed — manufacturing FLAKY reports for healthy files -(tests/test_hermes_state.py, 2026-08-18 on main). The scaler gives a -file max(flat_cap, 3 × last observed duration) and never lowers the cap. -""" - -from __future__ import annotations - -import importlib.util -from pathlib import Path - -REPO_ROOT = Path(__file__).resolve().parents[1] -_RUNNER_PATH = REPO_ROOT / "scripts" / "run_tests_parallel.py" - - -def _load_runner(): - spec = importlib.util.spec_from_file_location("run_tests_parallel", _RUNNER_PATH) - mod = importlib.util.module_from_spec(spec) - spec.loader.exec_module(mod) - return mod - - -def test_uncached_file_keeps_flat_cap() -> None: - mod = _load_runner() - f = REPO_ROOT / "tests" / "test_example.py" - assert mod._effective_file_timeout(f, REPO_ROOT, 300.0, {}) == 300.0 - assert mod._effective_file_timeout(f, REPO_ROOT, 300.0, None) == 300.0 - - -def test_fast_file_keeps_flat_cap() -> None: - mod = _load_runner() - f = REPO_ROOT / "tests" / "test_fast.py" - durations = {mod._format_file(f, REPO_ROOT): 4.2} - # 3 × 4.2 « 300 — the flat cap stays; the scaler never lowers a bound. - assert mod._effective_file_timeout(f, REPO_ROOT, 300.0, durations) == 300.0 - - -def test_slow_file_gets_proportional_headroom() -> None: - mod = _load_runner() - f = REPO_ROOT / "tests" / "test_hermes_state.py" - durations = {mod._format_file(f, REPO_ROOT): 205.0} - # 205s last run → 615s bound: a load-dilated healthy run survives, - # a genuine hang is still killed. - assert mod._effective_file_timeout(f, REPO_ROOT, 300.0, durations) == 615.0 - - -def test_zero_or_missing_duration_is_ignored() -> None: - mod = _load_runner() - f = REPO_ROOT / "tests" / "test_zero.py" - durations = {mod._format_file(f, REPO_ROOT): 0.0} - assert mod._effective_file_timeout(f, REPO_ROOT, 300.0, durations) == 300.0