From a818f22caf01a105b87eca1f0bc3f9059c8d6e04 Mon Sep 17 00:00:00 2001 From: jsboige Date: Mon, 14 Sep 2026 09:49:45 +0200 Subject: [PATCH] =?UTF-8?q?fix(nb-tools,#16111):=20check=5Ftwin=5Fparity?= =?UTF-8?q?=20=E2=80=94=20=5Frepo=5Froot=20sans=20fork=20+=20repli=20borne?= =?UTF-8?q?=20sur=20EAGAIN?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_repo_root()` mourait sur `BlockingIOError: [Errno 11]` (EAGAIN au fork) en CI, avant d'avoir lu une seule paire. Deux gestes independants, tous deux bornes a ce que l'issue etablit : le numero de ligne designait OU l'echec a atterri, pas le coupable — la pression de fork est exterieure a la boucle du script. 1. `_repo_root()` ne forke plus. `git rev-parse --show-toplevel` se reduit a une remontee de parents jusqu'a un `.git`, faite en pur Python, zero processus. Repli sur git si la remontee ne trouve rien (depot nu, GIT_DIR explicite) — soit les cas ou l'ancien code forkait de toute facon, donc jamais moins bien. 2. Les 7 sites `subprocess.run` passent par un helper `_run_git` qui retente `EAGAIN`/`EWOULDBLOCK` de facon BORNEE (3 tentatives, backoff 0.05/0.15 s). Toute autre `OSError` remonte inchangee : une panne reelle ne doit pas etre diluee en trois essais silencieux. Chaque site conserve ses options d'origine, y compris le mode binaire de `_git_show_file` (`text=False`). Ce qui n'est PAS fait, et pourquoi : reduire le fan-out de forks (157 paires x N forks, un `git cat-file --batch` en flux) est le troisieme axe de l'issue, qu'elle laisse explicitement hors scope tant qu'aucune mesure n'a montre que ce script est un contributeur majeur de la pression — ce que son ordre d'appel rend douteux. Le comptage de processus sur le runner reste a faire ; l'issue reste donc ouverte. Tests : 13 nouveaux (`test_check_twin_parity_fork_pressure.py`), dont le controle POSITIF qui manquerait sinon — une `OSError` non-EAGAIN n'est PAS retentee, sans quoi un `except OSError` trop large avalerait une panne reelle — et l'equivalence `_repo_root()` == `git rev-parse --show-toplevel`. Le test du repli asserte sa premisse (`tmp_path` hors de tout depot) pour ne pas passer a vide. 127 passed sur la suite twin_parity : aucune regression du refactor des 7 sites. Organe bout en bout : 157 paires, OK=157 DRIFT=0, rc=0. See #16111 Co-Authored-By: Claude Sonnet 5 --- scripts/notebook_tools/check_twin_parity.py | 93 +++++-- .../test_check_twin_parity_fork_pressure.py | 258 ++++++++++++++++++ 2 files changed, 324 insertions(+), 27 deletions(-) create mode 100644 scripts/notebook_tools/tests/test_check_twin_parity_fork_pressure.py diff --git a/scripts/notebook_tools/check_twin_parity.py b/scripts/notebook_tools/check_twin_parity.py index 0cc414e3b5..d01556b4b9 100644 --- a/scripts/notebook_tools/check_twin_parity.py +++ b/scripts/notebook_tools/check_twin_parity.py @@ -140,11 +140,13 @@ import argparse import datetime as _dt +import errno import hashlib import json import re import subprocess import sys +import time from pathlib import Path try: @@ -152,6 +154,44 @@ except ImportError: # pragma: no cover yaml = None +# --- pression de fork (#16111) ---------------------------------------------- +# Un `BlockingIOError` (EAGAIN, errno 11) au fork/clone n'est pas une erreur de +# git : c'est le noyau qui refuse un processus de plus, table de processus +# pleine. Sur la CI, les organes co-tenants tournent dans UN seul job +# (`Always-on guards -- N organes, 1 checkout`), donc dans une seule table : le +# premier fork d'un organe tardif tombe quand elle est deja saturee, et l'organe +# meurt avant d'avoir lu quoi que ce soit. `EAGAIN` est *temporaire* par +# definition -- echouer au premier refus transforme une contention transitoire +# en rouge de base. On retente donc un nombre borne de fois, sans jamais +# masquer une erreur d'une autre nature. +_EAGAIN_ATTEMPTS = 3 +_EAGAIN_BACKOFF = (0.05, 0.15) # avant les 2e et 3e tentatives + + +def _is_fork_pressure(exc: OSError) -> bool: + """Vrai si l'OS a refuse de creer un processus (EAGAIN au fork/clone).""" + return exc.errno in (errno.EAGAIN, errno.EWOULDBLOCK) + + +def _run_git(args: list[str], *, cwd=None, text: bool = True) -> subprocess.CompletedProcess: + """`subprocess.run` de git, avec repli borne sur la pression de fork. + + Seul `EAGAIN` est retente ; toute autre `OSError` remonte inchangee, pour + qu'une panne reelle reste visible au lieu d'etre diluee en trois essais. + """ + last_exc = None + for attempt in range(_EAGAIN_ATTEMPTS): + if attempt: + time.sleep(_EAGAIN_BACKOFF[attempt - 1]) + options = {"encoding": "utf-8", "errors": "replace"} if text else {} + try: + return subprocess.run(args, cwd=cwd, capture_output=True, text=text, **options) + except OSError as exc: + if not _is_fork_pressure(exc): + raise + last_exc = exc + raise last_exc + # Le registre vit desormais en un fichier par paire sous `twin_pairs.d/` # (#8542 Option C). Un fichier = une entree = plus rien a fusionner en serie # (la classe de conflit recurrente #8415/#8476/#8499/#8505/#8526/#8492 est @@ -383,10 +423,7 @@ def scan_coverage(repo_root: Path, pairs: list) -> dict: Les `*_output.ipynb` (artefacts d'execution) sont exclus des deux cotes. """ - r = subprocess.run( - ["git", "ls-files", "--", "*.ipynb"], - cwd=repo_root, capture_output=True, text=True, encoding="utf-8", errors="replace", - ) + r = _run_git(["git", "ls-files", "--", "*.ipynb"], cwd=repo_root) if r.returncode != 0: raise SystemExit("Erreur : `git ls-files` a echoue (depot inaccessible ?).") @@ -425,10 +462,7 @@ def _git_blob_sha(repo_root: Path, rel_path: str, git_ref: str = "HEAD") -> str Accepte un ref arbitraire (HEAD, origin/main, HEAD~1, , ...) -- permet de lire l'etat du depot a un instant donne sans modifier le working tree. """ - r = subprocess.run( - ["git", "ls-tree", git_ref, "--", rel_path], - capture_output=True, text=True, encoding="utf-8", errors="replace", cwd=str(repo_root), - ) + r = _run_git(["git", "ls-tree", git_ref, "--", rel_path], cwd=str(repo_root)) if r.returncode != 0 or not r.stdout.strip(): return None # format : " \t" @@ -465,10 +499,7 @@ def _blob_ancestor_in(repo_root: Path, blob_sha: str, ref: str = "HEAD") -> bool """ if not blob_sha or len(blob_sha) != 40: return False - r = subprocess.run( - ["git", "rev-list", "--objects", ref], - capture_output=True, text=True, encoding="utf-8", errors="replace", cwd=str(repo_root), - ) + r = _run_git(["git", "rev-list", "--objects", ref], cwd=str(repo_root)) if r.returncode != 0: return False # Chaque ligne de `rev-list --objects` est soit "" soit @@ -509,10 +540,7 @@ def _git_show_file(repo_root: Path, git_ref: str, rel_path: str) -> str | None: Utilise `git show :` (mode stream), evite de checkout le working tree. Necessaire pour lire le registre YAML au base-ref sans polluer le workspace CI. """ - r = subprocess.run( - ["git", "show", f"{git_ref}:{rel_path}"], - capture_output=True, cwd=str(repo_root), - ) + r = _run_git(["git", "show", f"{git_ref}:{rel_path}"], cwd=str(repo_root), text=False) if r.returncode != 0: return None return r.stdout.decode("utf-8", errors="replace") @@ -537,9 +565,9 @@ def _load_registry_at_ref(repo_root: Path, git_ref: str, reg_path: Path) -> list reg_rel = Path(reg_path.name).as_posix() # (1) Le ref porte-t-il le REPERTOIRE file-per-entry ? - r_ls = subprocess.run( + r_ls = _run_git( ["git", "ls-tree", "-r", "--name-only", git_ref, "--", f"{reg_rel}/"], - capture_output=True, text=True, encoding="utf-8", errors="replace", cwd=str(repo_root), + cwd=str(repo_root), ) entries: list = [] if r_ls.returncode == 0 and r_ls.stdout.strip(): @@ -624,10 +652,20 @@ def _load_registry_at_ref(repo_root: Path, git_ref: str, reg_path: Path) -> list def _repo_root() -> Path: - r = subprocess.run( - ["git", "rev-parse", "--show-toplevel"], - capture_output=True, text=True, encoding="utf-8", errors="replace", - ) + """Racine du depot contenant le cwd -- **sans fork**. + + `git rev-parse --show-toplevel` se reduit a une remontee de parents jusqu'a un + `.git` : faite en pur Python, elle ne coute aucun processus. C'est exactement + le fork qui mourait sous pression (#16111) : l'organe echouait ici, avant + d'avoir lu une seule paire, et un organe qui ne demarre pas ne rapporte rien. + Repli sur git si la remontee ne trouve rien (depot nu, `GIT_DIR` explicite) -- + soit les cas ou l'ancien code forkait de toute facon. + """ + cwd = Path.cwd() + for parent in (cwd, *cwd.parents): + if (parent / ".git").exists(): + return parent + r = _run_git(["git", "rev-parse", "--show-toplevel"]) if r.returncode != 0: raise SystemExit("Erreur : pas un depot git (impossible de trouver la racine).") return Path(r.stdout.strip()) @@ -1523,9 +1561,10 @@ def main(argv=None) -> int: p.add_argument("--registry", default=str(DEFAULT_REGISTRY), help=f"Chemin du registre YAML (defaut: {DEFAULT_REGISTRY.name})") p.add_argument("--repo-root", default=None, - help="Racine du depot git (defaut: detectee via `git rev-parse " - "--show-toplevel`). Utile pour les tests (mini-depot tmp_path) " - "et le cron CI qui pointe sur le checkout explicite.") + help="Racine du depot git (defaut: detectee en remontant les parents " + "jusqu'a un `.git`, sans fork -- cf #16111). Utile pour les tests " + "(mini-depot tmp_path) et le cron CI qui pointe sur le checkout " + "explicite.") p.add_argument("--family", default=None, help="Restreindre a une famille (ex. SMT/Z3-API)") p.add_argument("--check", action="store_true", @@ -1656,9 +1695,9 @@ def main(argv=None) -> int: # deja connue (mais apres args.repo_root parse, qui peut etre override # dans les tests --repo-root sur tmp_path). for state_file, label in (("MERGE_HEAD", "merge"), ("REBASE_HEAD", "rebase")): - r = subprocess.run( + r = _run_git( ["git", "rev-parse", "-q", "--verify", state_file], - capture_output=True, text=True, encoding="utf-8", errors="replace", cwd=str(repo_root_for_state), + cwd=str(repo_root_for_state), ) if r.returncode == 0: p.error(f"--update pendant un {label} non committe : HEAD ne contient pas " diff --git a/scripts/notebook_tools/tests/test_check_twin_parity_fork_pressure.py b/scripts/notebook_tools/tests/test_check_twin_parity_fork_pressure.py new file mode 100644 index 0000000000..5a74bd8642 --- /dev/null +++ b/scripts/notebook_tools/tests/test_check_twin_parity_fork_pressure.py @@ -0,0 +1,258 @@ +"""Tests for check_twin_parity -- pression de fork (`_repo_root` sans fork, repli EAGAIN). + +Why this exists +--------------- +`check_twin_parity.py` est mort en CI a `_repo_root()`, ligne 627, sur un +`BlockingIOError: [Errno 11]` -- `EAGAIN` au `fork`. Le numero de ligne designe +OU l'echec a atterri, pas le coupable : `_repo_root()` est appele depuis `main` +AVANT toute boucle par paire, donc le processus n'avait quasiment rien forke +lui-meme. La pression qui epuise la table de processus lui est EXTERIEURE (les +organes co-tenants d'un job `Always-on guards -- N organes, 1 checkout`), et +aucun comptage de processus n'a ete fait sur le runner : c'est une lecture de +l'ordre d'appel, pas une mesure d'attribution. + +Les deux garde-fous ci-dessous sont donc bornes a ce qui est etabli : + +1. `_repo_root()` ne forke plus. `git rev-parse --show-toplevel` n'est qu'une + remontee de parents jusqu'a un `.git` -- le test le prouve en faisant ECHOUER + `subprocess.run` : si la fonction forke encore, elle echoue. C'est le seul + point ou l'on sait que le fork mourait, et un organe qui demarre est un + organe qui peut rapporter. +2. `_run_git` retente `EAGAIN` de facon BORNEE. La contrepartie est aussi + testee (controle positif) : une `OSError` d'une autre nature ne doit PAS etre + retentee. Sans ce controle, un `except OSError` trop large avalerait une + panne reelle en trois essais silencieux -- le remede deviendrait pire que le + mal. + +Ce qui n'est PAS teste, volontairement : que `_run_git` reduise la pression de +fork. Il ne la reduit pas sur les 6 autres sites -- il rend seulement l'echec +transitoire surmontable. Reduire le fan-out (157 paires x N forks, un +`git cat-file --batch` en flux) est un troisieme axe, explicitement laisse hors +de ce correctif. +""" +from __future__ import annotations + +import errno +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +import check_twin_parity as ctp # noqa: E402 + +REPO = Path(__file__).resolve().parents[3] + + +def _meme_chemin(a: Path, b: Path) -> bool: + """Egalite de chemins robuste (separateurs et casse Windows).""" + return os.path.normcase(str(a)) == os.path.normcase(str(b)) + + +# -------------------------------------------------------------------------- +# 1. `_repo_root` ne forke plus +# -------------------------------------------------------------------------- + +def test_repo_root_ne_fork_pas(monkeypatch): + """Le fork doit avoir disparu : `subprocess.run` qui leve = echec du test. + + Le repertoire est choisi IMBRIQUE dans le depot : une remontee de parents + doit le trouver, ce qu'un simple `Path.cwd()` ne ferait pas. + """ + def interdit(*_args, **_kwargs): + raise AssertionError("_repo_root() a forke (subprocess.run appele)") + + monkeypatch.setattr(subprocess, "run", interdit) + monkeypatch.chdir(REPO / "scripts" / "notebook_tools" / "tests") + + assert _meme_chemin(ctp._repo_root(), REPO) + + +def test_repo_root_sans_fork_depuis_un_sous_dossier_profond(monkeypatch): + """Meme garantie depuis un dossier profond, et sur un `.git` FILE (worktree).""" + def interdit(*_args, **_kwargs): + raise AssertionError("_repo_root() a forke (subprocess.run appele)") + + monkeypatch.setattr(subprocess, "run", interdit) + monkeypatch.chdir(REPO / "MyIA.AI.Notebooks") + + racine = ctp._repo_root() + assert _meme_chemin(racine, REPO) + # Le `.git` detecte est bien celui du depot (dossier ou fichier de worktree). + assert (racine / ".git").exists() + + +def test_repo_root_equivaut_a_git_rev_parse(monkeypatch): + """Equivalence avec le `git rev-parse --show-toplevel` qu'elle remplace. + + C'est la preuve que la remontee est FIDELE, pas seulement rapide : si les + deux divergent un jour, ce test tombe avant que le reste du registre ne + derive sur une racine fausse. + """ + monkeypatch.chdir(REPO) + attendu = subprocess.run( + ["git", "rev-parse", "--show-toplevel"], + capture_output=True, text=True, encoding="utf-8", errors="replace", + ) + assert attendu.returncode == 0, "premisse : le depot de test est bien un depot git" + + assert _meme_chemin(ctp._repo_root(), Path(attendu.stdout.strip())) + + +def test_repo_root_repli_sur_git_quand_aucun_git(monkeypatch, tmp_path): + """Hors de tout depot, l'ancien comportement (fork unique) est preserve. + + La premisse est asserte : si un `.git` trainait dans un parent de `tmp_path`, + le repli ne serait jamais atteint et le test passerait pour la mauvaise + raison. + """ + assert not any((p / ".git").exists() for p in (tmp_path, *tmp_path.parents)), \ + "premisse : tmp_path ne doit etre sous aucun depot" + + appels: list[list[str]] = [] + racine_factice = "C:/faux/depot" + + def faux_run(args, **_kwargs): + appels.append(list(args)) + return subprocess.CompletedProcess(args, 0, stdout=racine_factice + "\n", stderr="") + + monkeypatch.setattr(subprocess, "run", faux_run) + monkeypatch.chdir(tmp_path) + + assert _meme_chemin(ctp._repo_root(), Path(racine_factice)) + assert len(appels) == 1 + assert appels[0] == ["git", "rev-parse", "--show-toplevel"] + + +def test_repo_root_repli_signale_l_absence_de_depot(monkeypatch, tmp_path): + """Hors depot, un git en echec donne toujours le meme SystemExit.""" + def faux_run(args, **_kwargs): + return subprocess.CompletedProcess(args, 128, stdout="", stderr="fatal: not a git repository") + + monkeypatch.setattr(subprocess, "run", faux_run) + monkeypatch.chdir(tmp_path) + + with pytest.raises(SystemExit, match="pas un depot git"): + ctp._repo_root() + + +# -------------------------------------------------------------------------- +# 2. `_run_git` : repli borne sur EAGAIN +# -------------------------------------------------------------------------- + +def _eagain() -> BlockingIOError: + return BlockingIOError(errno.EAGAIN, "Resource temporarily unavailable") + + +def test_run_git_retente_sur_eagain(monkeypatch): + """EAGAIN est temporaire : les 2 premiers forks echouent, le 3e passe.""" + appels = {"n": 0} + sentinelle = subprocess.CompletedProcess(["git"], 0, stdout="ok\n", stderr="") + + def faux_run(args, **_kwargs): + appels["n"] += 1 + if appels["n"] < 3: + raise _eagain() + return sentinelle + + monkeypatch.setattr(subprocess, "run", faux_run) + monkeypatch.setattr(ctp.time, "sleep", lambda _s: None) + + assert ctp._run_git(["git", "status"]) is sentinelle + assert appels["n"] == 3 + + +def test_run_git_ne_retente_pas_une_autre_erreur(monkeypatch): + """CONTROLE POSITIF -- une panne reelle ne doit pas etre avalee. + + Sans ce test, un `except OSError` trop large transformerait une erreur + permanente (git absent, `ENOENT`) en trois essais silencieux : le remede + serait pire que le mal. Ici l'exception doit remonter AU PREMIER appel. + """ + appels = {"n": 0} + + def faux_run(_args, **_kwargs): + appels["n"] += 1 + raise FileNotFoundError(errno.ENOENT, "No such file or directory: 'git'") + + monkeypatch.setattr(subprocess, "run", faux_run) + monkeypatch.setattr(ctp.time, "sleep", lambda _s: None) + + with pytest.raises(FileNotFoundError): + ctp._run_git(["git", "status"]) + assert appels["n"] == 1, "une erreur non-EAGAIN ne doit pas etre retentee" + + +@pytest.mark.parametrize("errno_erreur", [errno.EACCES, errno.ENOMEM, errno.EINVAL]) +def test_run_git_ne_retente_pas_les_autres_errno(monkeypatch, errno_erreur): + """Le repli est etroit : seuls EAGAIN/EWOULDBLOCK declenchent une tentative.""" + appels = {"n": 0} + + def faux_run(_args, **_kwargs): + appels["n"] += 1 + raise OSError(errno_erreur, os.strerror(errno_erreur)) + + monkeypatch.setattr(subprocess, "run", faux_run) + monkeypatch.setattr(ctp.time, "sleep", lambda _s: None) + + with pytest.raises(OSError): + ctp._run_git(["git", "status"]) + assert appels["n"] == 1 + + +def test_run_git_epuise_les_tentatives_puis_remonte(monkeypatch): + """La borne est reelle : apres `_EAGAIN_ATTEMPTS` essais, l'erreur remonte.""" + appels = {"n": 0} + + def faux_run(_args, **_kwargs): + appels["n"] += 1 + raise _eagain() + + monkeypatch.setattr(subprocess, "run", faux_run) + monkeypatch.setattr(ctp.time, "sleep", lambda _s: None) + + with pytest.raises(BlockingIOError): + ctp._run_git(["git", "status"]) + assert appels["n"] == ctp._EAGAIN_ATTEMPTS + + +def test_run_git_backoff_borne(monkeypatch): + """Le repli ne dort pas indefiniment : attentes courtes, nombre connu.""" + dors = [] + appels = {"n": 0} + + def faux_run(args, **_kwargs): + appels["n"] += 1 + if appels["n"] < ctp._EAGAIN_ATTEMPTS: + raise _eagain() + return subprocess.CompletedProcess(args, 0, stdout="", stderr="") + + monkeypatch.setattr(subprocess, "run", faux_run) + monkeypatch.setattr(ctp.time, "sleep", lambda s: dors.append(s)) + + ctp._run_git(["git", "status"]) + + assert dors == list(ctp._EAGAIN_BACKOFF) + assert sum(dors) <= 1.0, "le repli doit rester borne en temps" + + +def test_run_git_transmet_le_mode_texte(monkeypatch): + """`text=False` (site `git show`, binaire) ne doit pas recevoir d'encodage.""" + vus = [] + + def faux_run(args, **kwargs): + vus.append(kwargs) + return subprocess.CompletedProcess(args, 0, stdout=b"", stderr=b"") + + monkeypatch.setattr(subprocess, "run", faux_run) + + ctp._run_git(["git", "show", "HEAD:x"], cwd=".", text=False) + ctp._run_git(["git", "status"], cwd=".") + + assert "encoding" not in vus[0] and "errors" not in vus[0] + assert vus[0]["text"] is False + assert vus[1]["encoding"] == "utf-8" and vus[1]["errors"] == "replace" + assert vus[1]["text"] is True