diff --git a/scripts/notebook_tools/check_exec_ratchet.py b/scripts/notebook_tools/check_exec_ratchet.py index 685ac18209..47db82ecd6 100644 --- a/scripts/notebook_tools/check_exec_ratchet.py +++ b/scripts/notebook_tools/check_exec_ratchet.py @@ -21,20 +21,21 @@ Head verdicts read the working tree (in CI the checkout IS the head), base verdicts read the blob via `git show`. Exit code 1 iff at least one -CLEAN -> non-CLEAN regression exists. +CLEAN -> non-CLEAN regression exists. Exit code 2 iff git could not be +spawned at all (instrument unavailable, #16164) -- never confuse "could not +measure" with "measured 0". """ import argparse -import errno import json import subprocess import sys -import time from pathlib import Path sys.path.insert(0, str(Path(__file__).resolve().parent)) from check_exec_sequence import code_exec_counts, sequence_verdict +from fork_retry import run_with_fork_retry # Same exclusions as notebook-execution-required.yml's detect step plus the # checkpoints rule of the tier-1 scanner: archived, papermill-output and @@ -42,33 +43,35 @@ EXCLUDE_MARKERS = ("/.ipynb_checkpoints/", "/archive/", "/_output/", "/research/") -# EAGAIN au spawn (contention de processus, p.ex. pytest-xdist -n 4 sur le -# runner) est transitoire : le `except OSError: return None` historique -# transformait ce pic de charge en "changed notebooks : 0" -> faux vert CI -# (flake diagnostique sur #16125, tentative 3). On retente borne avant de -# rendre l'echec. -_EAGAIN_ERRNOS = (errno.EAGAIN, getattr(errno, "EWOULDBLOCK", errno.EAGAIN)) -_EAGAIN_ATTEMPTS = 3 -_EAGAIN_BACKOFF = (0.05, 0.15) # avant les 2e et 3e tentatives +class InstrumentUnavailable(RuntimeError): + """git n'a pas pu etre lance : le ratchet n'a rien mesure (#16164). -def _est_eagain(exc): - return isinstance(exc, OSError) and exc.errno in _EAGAIN_ERRNOS + Epuisement des retries EAGAIN, ou OSError non transitoire au spawn + (ENOENT, EACCES, ...). Distinct de ``returncode != 0`` (git A repondu : + l'instrument a tourne, sa reponse vaut None). Decision #16164 : + fail-closed sur instrument indisponible, en convergence avec le canon + (check_kernel_suffix_canon.py, dont l'OSError propage) -- un garde ne + rend jamais un verdict sur un arbre qu'il n'a pas lu. + """ + + def __init__(self, exc): + super().__init__( + f"git spawn failed ({exc.__class__.__name__}: " + f"errno={getattr(exc, 'errno', None)} {exc})") def git(*args, cwd=None): - """Run a git command, returning stdout (utf-8) or None on failure.""" - for tentative in range(_EAGAIN_ATTEMPTS): - try: - out = subprocess.run(["git", *args], cwd=cwd, capture_output=True, - encoding="utf-8", errors="replace", - check=False) - return out.stdout if out.returncode == 0 else None - except OSError as exc: - if not _est_eagain(exc) or tentative == _EAGAIN_ATTEMPTS - 1: - return None - time.sleep(_EAGAIN_BACKOFF[tentative]) - return None + """Run a git command, returning stdout (utf-8), None on git-level + failure (returncode != 0), or raising InstrumentUnavailable when the + spawn itself failed after bounded fork retries (#16217 primitive).""" + try: + out = run_with_fork_retry( + ["git", *args], cwd=cwd, capture_output=True, + encoding="utf-8", errors="replace", check=False) + except OSError as exc: + raise InstrumentUnavailable(exc) from exc + return out.stdout if out.returncode == 0 else None def resolve_base(base, cwd=None): @@ -157,8 +160,20 @@ def main(): help="Machine-readable output") args = ap.parse_args() - resolved = resolve_base(args.base) - records = ratchet(args.base) + try: + resolved = resolve_base(args.base) + records = ratchet(args.base) + except InstrumentUnavailable as exc: + # #16164 : "n'a pas pu mesurer" n'est pas "a mesure 0". Le faux vert + # historique (changed notebooks : 0 + exit 0) confondait les deux ; + # exit 2 est distinct de 1 (regression) pour que CI comme humains + # distinguent l'instrument en panne de l'arbre propre. + print(f"instrument indisponible : {exc}", file=sys.stderr) + print("le ratchet n'a rien mesure -- contention de processus " + "(EAGAIN, p.ex. pytest-xdist -n 4) ou git absent du PATH ; " + "relancer le job, ne pas lire ceci comme « 0 changements ».", + file=sys.stderr) + sys.exit(2) regressions = [r for r in records if r["regression"]] if args.as_json: diff --git a/scripts/notebook_tools/tests/test_check_exec_ratchet.py b/scripts/notebook_tools/tests/test_check_exec_ratchet.py index dc94787129..d53e661832 100644 --- a/scripts/notebook_tools/tests/test_check_exec_ratchet.py +++ b/scripts/notebook_tools/tests/test_check_exec_ratchet.py @@ -5,6 +5,7 @@ never required to improve; added notebooks are reported, not failed. No network, no kernel. """ +import errno import json import subprocess import sys @@ -248,50 +249,79 @@ class TestTransientSpawnRetry: """EAGAIN au spawn = contention de processus transitoire (pytest-xdist -n 4 sur le runner). Le `except OSError: return None` historique transformait ce pic en "changed notebooks : 0" -> faux vert CI (flake - #16125) : le wrapper doit retenter borne, pas rendre None au premier - echec de spawn.""" - - def _patch_run(self, monkeypatch, etat, resultat_ok): - import errno as _errno - - def faux_run(*args, **kwargs): - etat["appels"] += 1 - if etat["appels"] <= etat["echecs"]: - raise BlockingIOError(_errno.EAGAIN, - "Resource temporarily unavailable") - return resultat_ok - - monkeypatch.setattr(ratchet.time, "sleep", - lambda s: etat["dors"].append(s)) - monkeypatch.setattr(ratchet.subprocess, "run", faux_run) - - def test_eagain_retente_puis_passe(self, monkeypatch): - etat = {"appels": 0, "echecs": 2, "dors": []} - ok = subprocess.CompletedProcess(args=(), returncode=0, stdout="ok\n") - self._patch_run(monkeypatch, etat, ok) - assert ratchet.git("status") == "ok\n" - assert etat["appels"] == 3 - assert etat["dors"] == list(ratchet._EAGAIN_BACKOFF) - - def test_autre_oserror_rend_none_immediatement(self, monkeypatch): - etat = {"appels": 0, "echecs": 1, "dors": []} - import errno as _errno - - def faux_run(*args, **kwargs): - etat["appels"] += 1 - raise OSError(_errno.ENOENT, "git introuvable") - - monkeypatch.setattr(ratchet.time, "sleep", - lambda s: etat["dors"].append(s)) - monkeypatch.setattr(ratchet.subprocess, "run", faux_run) - assert ratchet.git("status") is None - assert etat["appels"] == 1 - assert etat["dors"] == [] - - def test_eagain_epuise_rend_none_avec_backoff_complet(self, monkeypatch): - etat = {"appels": 0, "echecs": 99, "dors": []} - ok = subprocess.CompletedProcess(args=(), returncode=0, stdout="ok\n") - self._patch_run(monkeypatch, etat, ok) - assert ratchet.git("status") is None - assert etat["appels"] == ratchet._EAGAIN_ATTEMPTS - assert etat["dors"] == list(ratchet._EAGAIN_BACKOFF) + #16125). + + Depuis #16164 + #16217 la reprise bornee est mutualisee dans + fork_retry.run_with_fork_retry (couverture propre dans + test_fork_retry.py). Ce qui reste specifique au ratchet : la traduction + de l'OSError remontee en InstrumentUnavailable, et l'absence de retry + sur une OSError non transitoire.""" + + def _fork_vers_erreur(self, monkeypatch, exc): + import fork_retry + monkeypatch.setattr(fork_retry.subprocess, "run", + lambda *a, **kw: (_ for _ in ()).throw(exc)) + + def test_eagain_epuise_leve_instrument_indisponible(self, monkeypatch): + # A l'epuisement des retries, l'echec de spawn monte au CLI (exit 2) + # au lieu du faux vert « changed notebooks : 0 ». + import fork_retry + monkeypatch.setattr(fork_retry.time, "sleep", lambda s: None) + self._fork_vers_erreur( + monkeypatch, + BlockingIOError(errno.EAGAIN, "Resource temporarily unavailable")) + with pytest.raises(ratchet.InstrumentUnavailable): + ratchet.git("status") + + def test_autre_oserror_leve_instrument_indisponible_sans_retry( + self, monkeypatch): + # git absent / EACCES n'est pas une reponse : un seul appel, pas de + # retry (le filtre etroit est couvert par test_fork_retry.py ; ici on + # epingle que le ratchet le respecte). + self._fork_vers_erreur( + monkeypatch, OSError(errno.ENOENT, "git introuvable")) + with pytest.raises(ratchet.InstrumentUnavailable): + ratchet.git("status") + + +class TestInstrumentIndisponible: + """#16164 : « n'a pas pu mesurer » n'est pas « a mesure 0 ». + + Le contrat lenient historique (git() -> None -> « changed notebooks : + 0 » -> exit 0) confondait l'echec de spawn avec une mesure nulle. La + decision arbitree ici : fail-closed sur instrument indisponible, en + convergence avec le canon (check_kernel_suffix_canon.py laisse + l'OSError propager) ; exit 2 reste distinct de 1 (regression) et de + 0 (mesure faite, rien a signaler). + """ + + def test_verdict_at_base_ne_dit_pas_absent_si_le_blob_est_illisible( + self, monkeypatch): + # git show en echec de SPAWN n'est pas un blob absent : avant #16164 + # l'OSError avaldee faisait lire ABSENT (donc « ajoute par la PR ») + # a un notebook existant a la base. + def faux_git(*args, **kwargs): + raise ratchet.InstrumentUnavailable( + OSError(errno.EAGAIN, "Resource temporarily unavailable")) + + monkeypatch.setattr(ratchet, "git", faux_git) + with pytest.raises(ratchet.InstrumentUnavailable): + ratchet.verdict_at_base("origin/main", "a.ipynb") + + def test_cli_exit_2_instrument_indisponible(self, monkeypatch, capsys): + # distinct de 0 (mesure propre) et de 1 (regression) ; le message + # nomme l'echec de spawn et ne imprime PAS « changed notebooks ». + def faux_git(*args, **kwargs): + raise ratchet.InstrumentUnavailable( + OSError(errno.EAGAIN, "Resource temporarily unavailable")) + + monkeypatch.setattr(ratchet, "git", faux_git) + monkeypatch.setattr(sys, "argv", + ["check_exec_ratchet.py", "origin/main"]) + with pytest.raises(SystemExit) as sortie: + ratchet.main() + assert sortie.value.code == 2 + capture = capsys.readouterr() + assert "instrument indisponible" in capture.err + assert "changed notebooks" not in capture.out + assert "changed notebooks" not in capture.err