diff --git a/.github/scripts/check_coverage_map_health.py b/.github/scripts/check_coverage_map_health.py index 739ddc6bae..f4dddc9838 100644 --- a/.github/scripts/check_coverage_map_health.py +++ b/.github/scripts/check_coverage_map_health.py @@ -11,54 +11,93 @@ MAX_AGE_DAYS = 10 MIN_FRACTION = 0.80 +# coverage-refresh.yml moves this ref to the commit it just rebuilt the map from, on EVERY +# successful refresh -- including the ones whose entries come out identical and therefore +# push no commit at all. +# +# The map's own _meta.git_sha cannot answer the freshness question. It only advances when a +# commit lands, and the refresh deliberately skips the commit when the coverage entries are +# unchanged (a no-op commit on every push is what #1683 removed). So a coverage-relevant +# commit whose coverage happens to be identical pinned git_sha in the past and made this +# check fail every day thereafter, with no way to recover: a refresh that WAS working +# looked broken. The question here is "has a refresh run since the last relevant commit", +# and only the refresh itself can answer it. +# +# It lives outside refs/heads/ and refs/tags/ so that updating it fires no `push` workflow +# trigger. actions/checkout does not fetch it; coverage-health.yml fetches it explicitly. +VERIFIED_REF = "refs/coverage-map/verified" + # Must mirror the `paths:` trigger of coverage-refresh.yml. A superset would raise false # alarms (a change that never triggers a refresh would look like a missed refresh). COVERAGE_RELEVANT_PATHS = [":(glob)src/**/*.fpp", "toolchain/mfc/test/cases.py"] -def built_after_last_change(git_sha): - """Was the map built at or after the last coverage-relevant commit? None if unknowable. +def verified_sha(cwd=None): + """Commit the last successful refresh verified the map against, or None if unknown. + + None covers both "the ref was never pushed" (a fork, or the window before the first + refresh after VERIFIED_REF was introduced) and "the fetch did not bring it down". The + caller must read that as undeterminable and fall back to the wall-clock age rule, not + as a failure -- an absent ref is not evidence of a broken refresh. + """ + rev = subprocess.run(["git", "rev-parse", "--verify", "--quiet", f"{VERIFIED_REF}^{{commit}}"], capture_output=True, text=True, check=False, cwd=cwd) + return rev.stdout.strip() or None + + +def verified_after_last_change(git_sha, cwd=None): + """Did that refresh run at or after the last coverage-relevant commit? None if unknowable. This is the direct question the wall-clock age check only approximated: it stays quiet - over a genuinely quiet repo (no refresh needed, so no heartbeat commit needed either) - and fires on the next relevant push after a refresh breaks, rather than 10 days later. + over a genuinely quiet repo (no refresh needed, so no heartbeat needed either) and + fires on the next relevant push after a refresh breaks, rather than 10 days later. """ if not git_sha: return None - last = subprocess.run(["git", "log", "-1", "--format=%H", "--", *COVERAGE_RELEVANT_PATHS], capture_output=True, text=True, check=False) + last = subprocess.run(["git", "log", "-1", "--format=%H", "--", *COVERAGE_RELEVANT_PATHS], capture_output=True, text=True, check=False, cwd=cwd) if last.returncode != 0 or not last.stdout.strip(): return None # shallow clone or no such commit -> fall back to the age rule - ancestor = subprocess.run(["git", "merge-base", "--is-ancestor", last.stdout.strip(), git_sha], capture_output=True, check=False) + ancestor = subprocess.run(["git", "merge-base", "--is-ancestor", last.stdout.strip(), git_sha], capture_output=True, check=False, cwd=cwd) return {0: True, 1: False}.get(ancestor.returncode) # anything else -> None (unknown sha, shallow history) -entries, meta = load_map(COVERAGE_MAP_PATH) -if entries is None: - sys.exit("Coverage map missing or corrupt.") -# Compute each current test's coverage key. Loading a case executes its case -# file; some (e.g. chemistry examples) import optional deps like cantera that are -# not installed in this lightweight job. Skip any case that fails to load instead -# of crashing — map_health measures the fraction of *loadable* current tests that -# are mapped, so a smaller current_keys cannot produce a false "stale" result. -current_keys = set() -unloadable = [] -for b in list_cases(): - try: - current_keys.add(b.to_case().coverage_key()) - except Exception as exc: # noqa: BLE001 — a case file that won't import must not crash the health check - last_line = (str(exc).strip().splitlines() or ["(no message)"])[-1][:140] - unloadable.append((getattr(b, "trace", repr(b)), last_line)) -if unloadable: - print(f"Note: {len(unloadable)} case(s) could not be loaded in this lightweight job (excluded from the check):") - for trace, err in unloadable[:15]: - print(f" - {trace}: {err}") -ok, msg = map_health( - meta=meta, - current_keys=current_keys, - mapped_keys=set(entries), - now=datetime.datetime.now(datetime.timezone.utc).isoformat(), - max_age_days=MAX_AGE_DAYS, - min_fraction=MIN_FRACTION, - built_after_last_change=built_after_last_change(meta.get("git_sha")), -) -print(msg) -sys.exit(0 if ok else 1) + +def main(): + entries, meta = load_map(COVERAGE_MAP_PATH) + if entries is None: + sys.exit("Coverage map missing or corrupt.") + # Compute each current test's coverage key. Loading a case runs its case file as a + # subprocess, so anything that file imports must resolve. Keep skipping the ones that + # do not rather than crashing -- map_health measures the fraction of *loadable* + # current tests that are mapped, so a smaller current_keys cannot produce a false + # "stale" result -- but the skip is a safety net, not the expected path: every case + # here is meant to load. The 16 chemistry cases that used to land in this list did so + # because get_py_program_output ran them under PATH's python3 while this job invokes + # build/venv/bin/python3 directly, leaving the venv's cantera out of reach. + current_keys = set() + unloadable = [] + for b in list_cases(): + try: + current_keys.add(b.to_case().coverage_key()) + except Exception as exc: # noqa: BLE001 -- a case file that won't import must not crash the health check + last_line = (str(exc).strip().splitlines() or ["(no message)"])[-1][:140] + unloadable.append((getattr(b, "trace", repr(b)), last_line)) + if unloadable: + print(f"Note: {len(unloadable)} case(s) could not be loaded in this lightweight job (excluded from the check):") + for trace, err in unloadable[:15]: + print(f" - {trace}: {err}") + ok, msg = map_health( + meta=meta, + current_keys=current_keys, + mapped_keys=set(entries), + now=datetime.datetime.now(datetime.timezone.utc).isoformat(), + max_age_days=MAX_AGE_DAYS, + min_fraction=MIN_FRACTION, + verified_after_last_change=verified_after_last_change(verified_sha()), + ) + print(msg) + return 0 if ok else 1 + + +# Guarded so the two git predicates above can be imported and unit-tested against a +# throwaway repository; running the module still performs the full check. +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/coverage-health.yml b/.github/workflows/coverage-health.yml index cd66d57fd3..e50d300272 100644 --- a/.github/workflows/coverage-health.yml +++ b/.github/workflows/coverage-health.yml @@ -9,10 +9,20 @@ jobs: if: github.repository == 'MFlowCode/MFC' runs-on: ubuntu-latest steps: - # Full history: the freshness check asks whether the map's git_sha contains the last - # commit touching src/**/*.fpp or cases.py, which a shallow clone cannot answer. + # Full history: the freshness check asks whether refs/coverage-map/verified -- the + # commit the last successful refresh rebuilt from -- contains the last commit + # touching src/**/*.fpp or cases.py, which a shallow clone cannot answer. - uses: actions/checkout@v5 with: { fetch-depth: 0 } + - name: Fetch the coverage-map verification ref + # actions/checkout fetches only refs/heads and refs/tags, and refs/coverage-map/ + # verified deliberately lives outside both so that updating it fires no `push` + # workflow trigger. Absent on a fork and in the window before the first refresh + # after it was introduced, so this must not fail the job: the check treats a + # missing ref as undeterminable and falls back to the wall-clock age rule. + run: | + git fetch --no-tags origin '+refs/coverage-map/verified:refs/coverage-map/verified' \ + || echo "::notice::No coverage-map verification ref yet; using the wall-clock age rule." - uses: actions/setup-python@v6 with: { python-version: '3.12' } - name: Initialize MFC diff --git a/.github/workflows/coverage-refresh.yml b/.github/workflows/coverage-refresh.yml index 1f9b3747fd..626faf5e3b 100644 --- a/.github/workflows/coverage-refresh.yml +++ b/.github/workflows/coverage-refresh.yml @@ -17,7 +17,11 @@ concurrency: jobs: refresh: if: github.repository == 'MFlowCode/MFC' - timeout-minutes: 240 + # Wall clock here is dominated by the SLURM queue, not by the build: observed waits on + # the phoenix `embers` QOS have reached 6h, and a 240-minute cap silently killed the + # refresh of #1717 at 4h05m. Cap generously -- a refresh that never finishes leaves the + # map behind, which is exactly what coverage-health.yml then reports. + timeout-minutes: 480 runs-on: group: phoenix labels: gt @@ -71,3 +75,26 @@ jobs: # Discard the rebuilt file so the runner's working tree matches master. git checkout -- tests/coverage_map.json.gz fi + # Record that a refresh verified the map against THIS commit, in BOTH branches + # above. coverage-health.yml cannot use the map's _meta.git_sha for this: git_sha + # advances only when a commit lands, and the guard above deliberately skips the + # commit when the entries are unchanged, so a coverage-relevant commit whose + # coverage is identical left the map looking permanently stale to the health + # check while this workflow was working fine. + # + # refs/coverage-map/ is outside refs/heads/ and refs/tags/ on purpose: a branch or + # tag would fire the `push` trigger of any workflow that filters on paths alone + # (homebrew.yml does), starting unrelated jobs on every refresh. + # + # Non-fatal: the map itself is already committed and pushed by this point, so a + # failed bookkeeping push must not red a refresh that succeeded. It is not silent + # either -- the warning lands in the run summary, and a ref that stops advancing + # drops coverage-health.yml back to its wall-clock rule, which goes red within + # MAX_AGE_DAYS. + # $GITHUB_SHA, not HEAD: when the entries changed, HEAD is the bot's new map + # commit, one past the commit this refresh actually rebuilt from. Either answers + # the health check's ancestry question, but only $GITHUB_SHA names the same thing + # in both branches -- the source state the map was built against. + git push "https://x-access-token:${CACHE_PUSH_TOKEN}@github.com/MFlowCode/MFC.git" \ + --force "$GITHUB_SHA:refs/coverage-map/verified" \ + || echo "::warning::Could not update refs/coverage-map/verified; coverage-health.yml falls back to the age rule." diff --git a/toolchain/mfc/common.py b/toolchain/mfc/common.py index ff066c8672..c3985dc966 100644 --- a/toolchain/mfc/common.py +++ b/toolchain/mfc/common.py @@ -2,6 +2,7 @@ import os import shutil import subprocess +import sys import typing from os.path import abspath, dirname, join, normpath, realpath @@ -135,10 +136,21 @@ def get_program_output(arguments: typing.List[str] = None, cwd=None): def get_py_program_output(filepath: str, arguments: typing.List[str] = None): + """Run a case file and capture its stdout. + + sys.executable, not a bare "python3": a case file imports the same optional deps the + toolchain venv provides (cantera, pyrometheus, scipy, ...), so it must run under the + interpreter the toolchain itself is running under. Nearly every entry point activates + the venv first, which makes PATH's python3 the venv's -- but a caller that invokes + build/venv/bin/python3 DIRECTLY does not, and then case files silently ran under the + system interpreter instead. That is what made coverage-health.yml report 16 chemistry + cases as unloadable with ModuleNotFoundError: No module named 'cantera', in a job whose + venv had cantera installed. + """ dirpath = os.path.abspath(os.path.dirname(filepath)) filename = os.path.basename(filepath) - return get_program_output(["python3", filename] + arguments, cwd=dirpath) + return get_program_output([sys.executable, filename] + arguments, cwd=dirpath) def isspace(s: str) -> bool: diff --git a/toolchain/mfc/test/coverage.py b/toolchain/mfc/test/coverage.py index 1fca077311..26d664affb 100644 --- a/toolchain/mfc/test/coverage.py +++ b/toolchain/mfc/test/coverage.py @@ -269,22 +269,27 @@ def format_summary(*, ran, total, reason, meta, now) -> str: return f"Coverage selection: ran {ran}/{total} tests · {age} · {reason}" -def map_health(*, meta, current_keys, mapped_keys, now, max_age_days, min_fraction, built_after_last_change=None): +def map_health(*, meta, current_keys, mapped_keys, now, max_age_days, min_fraction, verified_after_last_change=None): """Return (ok, message). Loud anti-rot check used by the health workflow. - `built_after_last_change` is the caller's git verdict on whether the map was built at - or after the most recent coverage-relevant commit: True (provably current), False + `verified_after_last_change` is the caller's git verdict on whether a refresh ran at or + after the most recent coverage-relevant commit: True (provably current), False (provably behind), or None (undeterminable -> fall back to the wall-clock rule). A map only decays when the sources or test list it was built from move, so wall-clock age is a poor proxy: it cries STALE over a quiet weekend and stays silent for 10 days after a refresh actually breaks. + + It asks about the refresh RUN, not about the map's own _meta.git_sha, which advances + only when the rebuilt entries differ from the committed ones. A relevant commit whose + coverage is unchanged leaves a correct map with an old git_sha; reading that as + "stale" made this check fail permanently while the refresh was working fine. """ if not meta or not meta.get("built_at"): return False, "Coverage map has no build metadata." age = (datetime.datetime.fromisoformat(now) - datetime.datetime.fromisoformat(meta["built_at"])).days - if built_after_last_change is False: - return False, "Coverage map is STALE: built before the most recent coverage-relevant commit. Refresh workflow may be broken." - if built_after_last_change is None and age > max_age_days: + if verified_after_last_change is False: + return False, "Coverage map is STALE: no successful refresh since the most recent coverage-relevant commit. Refresh workflow may be broken." + if verified_after_last_change is None and age > max_age_days: return False, f"Coverage map is STALE: {age}d old (max {max_age_days}d). Refresh workflow may be broken." if current_keys: frac = len(current_keys & mapped_keys) / len(current_keys) diff --git a/toolchain/mfc/test/test_coverage_unit.py b/toolchain/mfc/test/test_coverage_unit.py index c4a0f558ab..24f52ec1aa 100644 --- a/toolchain/mfc/test/test_coverage_unit.py +++ b/toolchain/mfc/test/test_coverage_unit.py @@ -438,12 +438,12 @@ def test_health_quiet_repo_is_not_stale_when_map_is_current(): now="2026-05-29T00:00:00+00:00", max_age_days=10, min_fraction=0.8, - built_after_last_change=True, + verified_after_last_change=True, ) assert ok, msg -def test_health_fails_immediately_when_map_predates_last_source_change(): +def test_health_fails_immediately_when_no_refresh_ran_since_last_source_change(): """Detects a dead refresh on the next relevant push, not 10 days later.""" ok, msg = map_health( meta={"built_at": "2026-05-28T00:00:00+00:00", "n_tests": 600}, @@ -452,7 +452,7 @@ def test_health_fails_immediately_when_map_predates_last_source_change(): now="2026-05-29T00:00:00+00:00", max_age_days=10, min_fraction=0.8, - built_after_last_change=False, + verified_after_last_change=False, ) assert not ok and "stale" in msg.lower() @@ -535,3 +535,97 @@ def test_guard_errors_when_the_rebuilt_map_is_missing(): rc = _run_guard(repo) assert rc == 2 assert rc != 10 + + +# --- check_coverage_map_health.py: the freshness signal the health workflow branches on --- + +HEALTH_SCRIPT = Path(__file__).resolve().parents[3] / ".github" / "scripts" / "check_coverage_map_health.py" + + +def _health_module(): + """Import the health script by path; it is a script, not an installed module.""" + import importlib.util + + spec = importlib.util.spec_from_file_location("check_coverage_map_health", HEALTH_SCRIPT) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +def _repo_with_history(d): + """A throwaway repo: one commit touching src/**/*.fpp, then one that does not.""" + repo = Path(d) + env = _env_without_git() + git = ["git", "-c", "user.name=t", "-c", "user.email=t@t", "-C", str(repo)] + subprocess.run([*git, "init", "-q", "-b", "master"], check=True, env=env) + (repo / "src" / "simulation").mkdir(parents=True) + (repo / "src" / "simulation" / "m_rhs.fpp").write_text("! v1\n") + subprocess.run([*git, "add", "-A"], check=True, env=env) + subprocess.run([*git, "commit", "-q", "--no-verify", "-m", "relevant"], check=True, env=env) + relevant = subprocess.run([*git, "rev-parse", "HEAD"], capture_output=True, text=True, check=True, env=env).stdout.strip() + (repo / "README.md").write_text("docs\n") + subprocess.run([*git, "add", "-A"], check=True, env=env) + subprocess.run([*git, "commit", "-q", "--no-verify", "-m", "irrelevant"], check=True, env=env) + later = subprocess.run([*git, "rev-parse", "HEAD"], capture_output=True, text=True, check=True, env=env).stdout.strip() + return repo, relevant, later + + +def _set_verified(repo, sha): + subprocess.run(["git", "-C", str(repo), "update-ref", "refs/coverage-map/verified", sha], check=True, env=_env_without_git()) + + +def test_verified_sha_is_none_when_the_ref_was_never_pushed(): + """A fork, or the window before the first refresh: undeterminable, not broken.""" + health = _health_module() + with tempfile.TemporaryDirectory() as d: + repo, _, _ = _repo_with_history(d) + assert health.verified_sha(cwd=repo) is None + # None must reach map_health as None, which falls back to the wall-clock rule. + assert health.verified_after_last_change(health.verified_sha(cwd=repo), cwd=repo) is None + + +def test_verified_after_last_change_true_when_a_refresh_ran_since_the_change(): + health = _health_module() + with tempfile.TemporaryDirectory() as d: + repo, relevant, later = _repo_with_history(d) + _set_verified(repo, later) + assert health.verified_sha(cwd=repo) == later + assert health.verified_after_last_change(later, cwd=repo) is True + # The relevant commit itself counts: a refresh AT the change is current. + assert health.verified_after_last_change(relevant, cwd=repo) is True + + +def test_verified_after_last_change_false_when_the_refresh_predates_the_change(): + """The genuine broken-refresh case this check exists to catch.""" + health = _health_module() + with tempfile.TemporaryDirectory() as d: + repo, relevant, _ = _repo_with_history(d) + env = _env_without_git() + git = ["git", "-c", "user.name=t", "-c", "user.email=t@t", "-C", str(repo)] + before = subprocess.run([*git, "rev-parse", "HEAD~1"], capture_output=True, text=True, check=True, env=env).stdout.strip() + assert before == relevant + (repo / "src" / "simulation" / "m_rhs.fpp").write_text("! v2\n") + subprocess.run([*git, "add", "-A"], check=True, env=env) + subprocess.run([*git, "commit", "-q", "--no-verify", "-m", "relevant again"], check=True, env=env) + _set_verified(repo, relevant) + assert health.verified_after_last_change(relevant, cwd=repo) is False + + +def test_a_no_op_refresh_still_keeps_the_map_healthy(): + """The regression #1683 introduced: unchanged entries push no commit, so the map's + _meta.git_sha stays behind while the refresh is working. The ref, not git_sha, is what + the health check reads -- an old git_sha must not read as STALE.""" + health = _health_module() + with tempfile.TemporaryDirectory() as d: + repo, relevant, later = _repo_with_history(d) + _set_verified(repo, later) + ok, msg = map_health( + meta={"built_at": "2026-05-28T00:00:00+00:00", "git_sha": "ancient", "n_tests": 1}, + current_keys={"a"}, + mapped_keys={"a"}, + now="2026-05-29T00:00:00+00:00", + max_age_days=10, + min_fraction=0.8, + verified_after_last_change=health.verified_after_last_change(health.verified_sha(cwd=repo), cwd=repo), + ) + assert ok, msg diff --git a/toolchain/mfc/test_common.py b/toolchain/mfc/test_common.py new file mode 100644 index 0000000000..a5d9e952d4 --- /dev/null +++ b/toolchain/mfc/test_common.py @@ -0,0 +1,45 @@ +import os +import stat +import sys +import tempfile +from pathlib import Path +from unittest.mock import patch + +from mfc.common import get_py_program_output + + +def test_case_files_run_under_the_toolchain_interpreter(): + """A case file must run under sys.executable, not under whatever python3 PATH finds. + + coverage-health.yml invokes build/venv/bin/python3 directly instead of activating the + venv, so PATH's python3 was the system interpreter and every chemistry case failed with + ModuleNotFoundError: No module named 'cantera' -- in a job whose venv had cantera. The + case file reports the interpreter that ran it; it must be this one. + """ + with tempfile.TemporaryDirectory() as d: + case = Path(d) / "case.py" + case.write_text("import sys\nprint(sys.executable)\n") + out, err = get_py_program_output(str(case), []) + assert err == 0 + assert out.strip() == sys.executable + + +def test_a_hostile_python3_on_path_is_not_consulted(): + """Put a python3 on PATH that refuses to run anything; the case file must still load. + + This is the failure the health job hit, made deterministic: PATH's python3 is a + different, wrong interpreter. Asserting on sys.executable alone would still pass if + someone reintroduced a PATH lookup on a machine where the two happen to coincide. + """ + with tempfile.TemporaryDirectory() as d: + bindir = Path(d) / "bin" + bindir.mkdir() + fake = bindir / "python3" + fake.write_text("#!/bin/sh\necho 'wrong interpreter' >&2\nexit 3\n") + fake.chmod(fake.stat().st_mode | stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH) + case = Path(d) / "case.py" + case.write_text("print('loaded')\n") + with patch.dict(os.environ, {"PATH": f"{bindir}{os.pathsep}{os.environ.get('PATH', '')}"}): + out, err = get_py_program_output(str(case), []) + assert err == 0, "case file ran under PATH's python3 instead of sys.executable" + assert out.strip() == "loaded"