From e58eb9538506eb51c6f9a1386b274f4f2953a01e Mon Sep 17 00:00:00 2001 From: ManSio Date: Sat, 14 Nov 2026 11:50:31 +0300 Subject: [PATCH 1/3] fix(process): kill the gate tree, bound the launch lock, allow one suite Three defects found while measuring the suite: (1) the pre-commit hook killed only the direct gate child, so a timed-out verify_diary left a full pytest running for 10+ minutes and raced the next run; the gate is now a process-group leader and the timeout path uses taskkill /T or killpg. (2) ensure_server_running waited on a module lock forever; it now gives up after 150s and returns False instead of hanging. (3) two suites could run on one checkout; a session file lock makes the second exit 3 with a clear message, while nested pytest spawned by tests inherits MSCB_SUITE_LOCK_INHERITED and skips. Real-process checks: a grandchild was gone after _kill_tree; a second pytest exited 3 while the lock was held. --- .githooks/pre-commit | 57 ++++++++++++++++++--- src/core/embedder/onnx_client.py | 17 +++++-- tests/conftest.py | 73 +++++++++++++++++++++++++++ tests/test_idle_reload.py | 21 ++++++++ tests/test_precommit_hook_contract.py | 50 ++++++++++++++++++ tests/test_suite_lock.py | 35 +++++++++++++ 6 files changed, 243 insertions(+), 10 deletions(-) create mode 100644 tests/test_suite_lock.py diff --git a/.githooks/pre-commit b/.githooks/pre-commit index a9f141c4..880bb6a4 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -26,6 +26,7 @@ import os import subprocess import sys import time +import signal from pathlib import Path # Per-gate budget. 900s is the historical value (documented flakiness at 300s); @@ -36,6 +37,49 @@ GATE_TIMEOUT = int(os.environ.get("MSCB_PRECOMMIT_GATE_TIMEOUT", "900")) # global `subprocess` module — a global patch leaks into every other test in # the same worker (CI: 14 unrelated failures). Popen = subprocess.Popen +_RUN = subprocess.run + + +def _spawn_kwargs() -> dict: + """A gate must be a process-group leader, so a timeout can kill its tree.""" + if sys.platform == "win32": + return {"creationflags": getattr(subprocess, "CREATE_NO_WINDOW", 0) + | getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0)} + return {"start_new_session": True} + + +def _kill_tree(proc) -> None: + """Kill the gate and every descendant it spawned. + + proc.kill() alone kills only the direct child. Observed 2026-11-14: a + timed-out commit left `verify_diary -> pytest tests/` running for 10+ + minutes; the orphan then competed with the next run and made a fast test + look like an 11-minute hang. On Windows the tree needs taskkill /T; on + POSIX the child is a session leader, so killpg reaches the group. + """ + poll = getattr(proc, "poll", None) + if poll is not None and poll() is not None: + return + pid = getattr(proc, "pid", None) + if pid is not None: + try: + if sys.platform == "win32": + _RUN(["taskkill", "/PID", str(pid), "/T", "/F"], + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + check=False, timeout=15) + else: + os.killpg(os.getpgid(pid), signal.SIGKILL) + except (OSError, subprocess.SubprocessError): + pass + try: + proc.kill() + except OSError: + pass + try: + proc.communicate(timeout=10) + except subprocess.TimeoutExpired: + pass + # §9 п.9 (ENCODING SAFETY): при выводе emoji-строк из stdout скриптов @@ -84,20 +128,19 @@ def run_script(script_path: str, label: str, extra_args: list[str] | None = None stderr=subprocess.DEVNULL, encoding="utf-8", errors="replace", - creationflags=getattr(subprocess, 'CREATE_NO_WINDOW', 0), + **_spawn_kwargs(), ) - # Таймаут-запас: verify_diary гоняет gate-zero (полный pytest ~108-130s под - # нагрузкой) — кап 120s давал флаки TimeoutExpired на коммитах (2026-08-08); - # 300→900 (2026-08-24): сюита выросла (live-sync + predict-наборы), 300s - # начал флакать при параллельной нагрузке. + # A timed-out gate is a verdict, not a claim about suite speed. The old + # "~108-130s" note stopped being true as the suite grew to 2528 tests, and + # on 2026-11-14 a loaded machine took 1092s for gate-zero. Keep the budget + # generous; the correctness requirement is that the tree is killed. try: stdout, _ = proc.communicate(timeout=GATE_TIMEOUT) except subprocess.TimeoutExpired: # Раньше это было необработанное исключение: хук умирал с трейсбеком и # БЕЗ вердикта — его путали с «гейт сломан» и обходили через --no-verify. # Таймаут — это вердикт: называем гейт, который не ответил. - proc.kill() - proc.communicate() + _kill_tree(proc) print(f" ❌ {label}: TIMEOUT >{GATE_TIMEOUT}s — гейт не ответил, вердикта нет", flush=True) return False diff --git a/src/core/embedder/onnx_client.py b/src/core/embedder/onnx_client.py index 911a15e2..a131fd4d 100644 --- a/src/core/embedder/onnx_client.py +++ b/src/core/embedder/onnx_client.py @@ -179,8 +179,15 @@ def ensure_server_running(self) -> bool: if self._health_check(): return True - # Межпоточный lock (на случай если несколько потоков одновременно вызывают) - with _client_lock: + # Межпоточный lock (на случай если несколько потоков одновременно вызывают). + # Ожидание ограничено: поток, который не может взять lock, честно отвечает + # False, а не висит вечно. Наблюдалось 2026-11-14: тест в полном прогоне + # простоял 654s на этом lock, хотя в одиночку занимает <0.01s — вечное + # ожидание превращало чужую утечку в «медленный тест» без вердикта. + if not _client_lock.acquire(timeout=_LOCK_TIMEOUT): + logger.error("[ONNX Client] launch lock busy for %.0fs; refusing to wait", _LOCK_TIMEOUT) + return False + try: # Double-check под локом if self._health_check(): return True @@ -198,6 +205,8 @@ def ensure_server_running(self) -> bool: return self._wait_for_server() finally: self._release_launch_mutex() + finally: + _client_lock.release() def embed(self, text: str) -> List[float]: """Эмбеддинг одного текста.""" @@ -251,8 +260,10 @@ def shutdown(self): pass -# Глобальный lock для координации внутри процесса +# Глобальный lock для координации внутри процесса. Ожидание ограничено: +# утёкший поток не должен подвешивать вызов навсегда (см. ensure_server_running). _client_lock = threading.Lock() +_LOCK_TIMEOUT = 150.0 # launch + _wait_for_server(60s) + запас; вечность — не ответ # Путь к корню проекта (репо или расширение Zed). # parents[3]: src/core/embedder/onnx_client.py → корень. diff --git a/tests/conftest.py b/tests/conftest.py index 73df2721..d64927e2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -10,6 +10,7 @@ в свой tmp_path, который pytest удаляет автоматически. """ +import os import sys from pathlib import Path @@ -80,3 +81,75 @@ def _fusion_default(monkeypatch): """ monkeypatch.delenv("MSCODEBASE_FUSION", raising=False) + + +# --- One suite per tree ------------------------------------------------------- +# Observed 2026-11-14: a timed-out commit left `verify_diary -> pytest` running, +# then a second full suite started. The two competed for CPU; a test that takes +# <0.01s alone was reported at 654s, and the run looked like a hang. The OS +# releases this lock when the process dies, so a crash cannot leave it stale. + +SUITE_LOCK = PROJECT_ROOT / ".pytest_cache" / "suite.lock" +_LOCK_INHERITED = "MSCB_SUITE_LOCK_INHERITED" + + +def _acquire_file_lock(path: Path): + path.parent.mkdir(parents=True, exist_ok=True) + handle = open(path, "a+b") + try: + handle.seek(0, os.SEEK_END) + if handle.tell() == 0: + handle.write(b"x") + handle.flush() + handle.seek(0) + if os.name == "nt": + import msvcrt + msvcrt.locking(handle.fileno(), msvcrt.LK_NBLCK, 1) + else: + import fcntl + fcntl.flock(handle.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + handle.close() + return None + return handle + + +def _release_file_lock(handle) -> None: + try: + handle.seek(0) + if os.name == "nt": + import msvcrt + msvcrt.locking(handle.fileno(), msvcrt.LK_UNLCK, 1) + else: + import fcntl + fcntl.flock(handle.fileno(), fcntl.LOCK_UN) + except OSError: + pass + finally: + handle.close() + + +@pytest.fixture(scope="session", autouse=True) +def _one_suite_per_tree(): + """Refuse to start a second pytest on the same checkout. + + A nested pytest spawned by a test inherits MSCB_SUITE_LOCK_INHERITED=1 and + skips the check, so `test_bootstrap_pipeline` still works. + """ + if os.environ.get(_LOCK_INHERITED) == "1": + yield + return + handle = _acquire_file_lock(SUITE_LOCK) + if handle is None: + pytest.exit( + "Another pytest run already holds .pytest_cache/suite.lock. Two suites on " + "one tree make timings meaningless and can race on shared temp data. Wait " + "for it to finish; if no run is active, delete the file and retry.", + returncode=3, + ) + os.environ[_LOCK_INHERITED] = "1" + try: + yield + finally: + os.environ.pop(_LOCK_INHERITED, None) + _release_file_lock(handle) diff --git a/tests/test_idle_reload.py b/tests/test_idle_reload.py index 0ab2904c..70488537 100644 --- a/tests/test_idle_reload.py +++ b/tests/test_idle_reload.py @@ -136,3 +136,24 @@ def worker(): assert not errors assert launch.call_count == 1, "Сервер должен запускаться ровно один раз" assert all(results), "Все потоки должны получить успех" + +def test_ensure_server_running_gives_up_when_the_launch_lock_is_held(monkeypatch): + """A leaked thread must not turn the next call into an indefinite wait. + + This file takes 2.7s alone; in the 2026-11-14 full run one test held + _client_lock and another waited 654s on it. The lock now has a timeout. + """ + import time + + from src.core.embedder import onnx_client as mod + + c = _client() + monkeypatch.setattr(mod, "_LOCK_TIMEOUT", 0.05) + assert mod._client_lock.acquire(timeout=1.0) + try: + with patch.object(c, "_health_check", return_value=False): + started = time.monotonic() + assert c.ensure_server_running() is False + assert time.monotonic() - started < 5 + finally: + mod._client_lock.release() diff --git a/tests/test_precommit_hook_contract.py b/tests/test_precommit_hook_contract.py index 98784680..6fde5208 100644 --- a/tests/test_precommit_hook_contract.py +++ b/tests/test_precommit_hook_contract.py @@ -142,3 +142,53 @@ def test_fixed_hook_answers_the_same_timeout(tmp_path, capsys): assert "TIMEOUT" in out and "slowgate" in out assert "Traceback" not in out + +# --- process tree on timeout ------------------------------------------------- + +def test_gate_is_spawned_as_a_process_group_leader(): + """A gate that is not a group leader cannot be killed with its children.""" + mod = _load_hook(HOOK, None, "hook_group") + kw = mod._spawn_kwargs() + if sys.platform == "win32": + flag = getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0) + assert kw["creationflags"] & flag + else: + assert kw["start_new_session"] is True + + +def test_timeout_kills_the_process_tree(tmp_path, monkeypatch): + """proc.kill() alone left `verify_diary -> pytest` alive for 10+ minutes.""" + mod = _load_hook(HOOK, 1, "hook_tree") + gate = tmp_path / "gate.py" + gate.write_text("print('ok')\n", encoding="utf-8") + calls = [] + + class Proc: + pid = 4321 + + def poll(self): + return None + + def kill(self): + calls.append("kill") + + def communicate(self, timeout=None): + if "raised" not in calls: + calls.append("raised") + raise subprocess.TimeoutExpired(cmd="gate", timeout=timeout) + return ("", None) + + mod.Popen = lambda *a, **k: Proc() + if sys.platform == "win32": + monkeypatch.setattr(mod, "_RUN", lambda cmd, **kw: calls.append(("run", list(cmd)))) + else: + monkeypatch.setattr(mod.os, "getpgid", lambda pid: 7) + monkeypatch.setattr(mod.os, "killpg", lambda pgid, sig: calls.append(("killpg", pgid))) + + assert mod.run_script(str(gate), "gate") is False + if sys.platform == "win32": + tree = next(c for c in calls if isinstance(c, tuple) and c[0] == "run")[1] + assert tree[0] == "taskkill" and "/T" in tree and "/F" in tree + else: + assert ("killpg", 7) in calls + assert "kill" in calls, "the direct child must still be killed as a fallback" diff --git a/tests/test_suite_lock.py b/tests/test_suite_lock.py new file mode 100644 index 00000000..4d5542d9 --- /dev/null +++ b/tests/test_suite_lock.py @@ -0,0 +1,35 @@ +"""The suite lock must refuse a second run and free itself on release. + +Two full suites on one checkout are what turned a <0.01s test into a 654s +report on 2026-11-14. The lock is a file lock, so the OS frees it if a run dies. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] + + +def _conftest(): + spec = importlib.util.spec_from_file_location("mscb_conftest", ROOT / "tests" / "conftest.py") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def test_second_acquire_fails_and_release_frees(tmp_path: Path) -> None: + mod = _conftest() + lock = tmp_path / "suite.lock" + + first = mod._acquire_file_lock(lock) + assert first is not None + try: + assert mod._acquire_file_lock(lock) is None + finally: + mod._release_file_lock(first) + + second = mod._acquire_file_lock(lock) + assert second is not None + mod._release_file_lock(second) From 109afb66b8f99d26ba743436d6a460f0a81bcc7f Mon Sep 17 00:00:00 2001 From: MSCodeBase Agent Date: Sun, 15 Nov 2026 08:23:04 +0300 Subject: [PATCH 2/3] fix(tests): take the suite lock in the controller, not in every worker --- AGENT_DIARY.md | 12 ++++++++++++ tests/conftest.py | 27 +++++++++++++++++++-------- tests/test_suite_lock.py | 16 ++++++++++++++++ 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/AGENT_DIARY.md b/AGENT_DIARY.md index eea0dfd2..ef823ff4 100644 --- a/AGENT_DIARY.md +++ b/AGENT_DIARY.md @@ -1941,3 +1941,15 @@ BREACH: MSC-P-19 | date: 2026-10-03 | rule: §12 | guard_at_time: no | detected_ **3. Код установщика в диффах есть.** `gh pr diff 120 | grep auto_yes` → удалённая строка `range(0 if auto_yes …)`, добавленная `range(1 if auto_yes …)` и тест `test_auto_yes_runs_each_step_once`. `gh pr diff 121 | grep "def _"` → `models_enabled`, `_doctor_report`, `_doctor_cli`, `_clean_targets`, `_clean_cli` (имена не `cmd_*`; поиск по `cmd_clean|cmd_doctor` пуст по этой причине). **4. Гипотеза «clean-state до фикса валидировал пустую установку» — опровергнута.** `scripts/verify_clean_state.sh` вообще не вызывает `install.py`: он сам создаёт venv и делает `pip install -e .`; `grep "install.py --yes"` по `.github/` и `scripts/` пуст. Значит баг `--yes` с v3.0 не влиял ни на один прогон CI — ровно поэтому его никто и не поймал; поймал только ручной прогон по README. **Урок:** «CI зелёный» и «инструкция работает» — разные утверждения. Пока установщик не запускали как пользователь (по README, в пустом каталоге), его неинтерактивная ветка была мёртвой, а все гейты оставались зелёными. + +## [2026-11-14] PR #133: suite-lock брался в каждом xdist-воркере + +**Симптом:** CI на `fix/process-hardening` — `test (ubuntu)`, `test (windows)` и `clean-state` падали за ~6 секунд с `INTERNALERROR: worker crashed`, `crashitem` указывал на случайный тест (`test_deep_search.py::TestExtractKeyTerms::test_max_terms_limit`). Локально один прогон зелёный, `-n 4` — падает. + +**Корень:** «один suite на дерево» было реализовано `@pytest.fixture(scope="session", autouse=True)`, а xdist выполняет session-фикстуры **в каждом воркере**. Один воркер брал файловую блокировку, остальные получали `None` → `pytest.exit(returncode=3)` → xdist видел «worker crashed». Маркер `MSCB_SUITE_LOCK_INHERITED` ставился в процессе-победителе и соседним воркерам не доставался. + +**Fix:** блокировка перенесена в `pytest_configure` (контроллер берёт её до запуска воркеров) + `pytest_unconfigure` освобождает; воркеры выходят по `config.workerinput`, вложенные прогоны — по прежнему маркеру. Структурный guard в `tests/test_suite_lock.py`: `pytest_configure` есть, `workerinput` учитывается, фикстуры `_one_suite_per_tree` нет — на старом conftest краснеет. + +**Проверка:** `pytest tests/test_deep_search.py -n 4` — 15 passed (было INTERNALERROR); полный suite `-n auto` — 2515 passed, 11 skipped, rc=0; структурный тест красный на старом conftest, зелёный на новом. + +**Урок:** session-scoped фикстура в xdist — это N экземпляров, а не один; всё, что «на весь прогон» (блокировки, общие каталоги, счётчики), живёт в хуках контроллера. diff --git a/tests/conftest.py b/tests/conftest.py index d64927e2..c624a783 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -129,15 +129,22 @@ def _release_file_lock(handle) -> None: handle.close() -@pytest.fixture(scope="session", autouse=True) -def _one_suite_per_tree(): +def pytest_configure(config): """Refuse to start a second pytest on the same checkout. + Runs in the controller, not in a fixture: xdist executes session fixtures + inside **every** worker, so the fixture version had N workers racing for one + file lock — the losers called `pytest.exit(3)` and xdist reported + `INTERNALERROR: worker crashed` (CI 2026-11-14, ubuntu + windows, 3 of 4 + workers). The controller acquires before workers spawn, and workers inherit + the marker through the environment. + A nested pytest spawned by a test inherits MSCB_SUITE_LOCK_INHERITED=1 and skips the check, so `test_bootstrap_pipeline` still works. """ if os.environ.get(_LOCK_INHERITED) == "1": - yield + return + if getattr(config, "workerinput", None) is not None: # xdist worker return handle = _acquire_file_lock(SUITE_LOCK) if handle is None: @@ -147,9 +154,13 @@ def _one_suite_per_tree(): "for it to finish; if no run is active, delete the file and retry.", returncode=3, ) + config._mscb_suite_lock = handle os.environ[_LOCK_INHERITED] = "1" - try: - yield - finally: - os.environ.pop(_LOCK_INHERITED, None) - _release_file_lock(handle) + + +def pytest_unconfigure(config): + handle = getattr(config, "_mscb_suite_lock", None) + if handle is None: + return + os.environ.pop(_LOCK_INHERITED, None) + _release_file_lock(handle) diff --git a/tests/test_suite_lock.py b/tests/test_suite_lock.py index 4d5542d9..bb2f8c5d 100644 --- a/tests/test_suite_lock.py +++ b/tests/test_suite_lock.py @@ -33,3 +33,19 @@ def test_second_acquire_fails_and_release_frees(tmp_path: Path) -> None: second = mod._acquire_file_lock(lock) assert second is not None mod._release_file_lock(second) + + +def test_lock_is_taken_in_the_controller_not_in_a_session_fixture(): + """Structural guard for the 2026-11-14 crash. + + A session-scoped fixture runs inside every xdist worker; the losers called + pytest.exit(3) and CI reported `INTERNALERROR: worker crashed`. From inside + the suite this is invisible (a nested run inherits the marker), so the shape + is asserted: the lock lives in pytest_configure, workers skip via + workerinput, and the per-worker fixture is gone. + """ + source = (ROOT / "tests" / "conftest.py").read_text(encoding="utf-8") + assert "def pytest_configure(config)" in source + assert "def pytest_unconfigure(config)" in source + assert 'getattr(config, "workerinput", None)' in source + assert "_one_suite_per_tree" not in source From caf55be4ba9699c9d157c1630016c094d10e2092 Mon Sep 17 00:00:00 2001 From: MSCodeBase Agent Date: Sun, 15 Nov 2026 10:34:35 +0300 Subject: [PATCH 3/3] test(idle): isolate the launch lock so a leaked thread cannot fail the test --- tests/test_idle_reload.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/test_idle_reload.py b/tests/test_idle_reload.py index 70488537..c0632803 100644 --- a/tests/test_idle_reload.py +++ b/tests/test_idle_reload.py @@ -11,9 +11,25 @@ import threading from unittest.mock import patch +import pytest + from src.core.embedder.onnx_client import OnnxEmbedderClient +@pytest.fixture(autouse=True) +def _fresh_client_lock(monkeypatch): + """Каждый тест получает свой lock. + + `_client_lock` — модульный глобал. Поток, оставшийся от другого теста, + держал его; после появления таймаута лока (2026-11-14) `ensure_server_running` + честно возвращал False вместо зависания, и тест падал по чужому состоянию + (clean-state, PR #133). Изоляция убирает порядок тестов из уравнения. + """ + from src.core.embedder import onnx_client as mod + + monkeypatch.setattr(mod, "_client_lock", threading.Lock()) + + def _client(): # Свободный порт: реальный health-check обязан вернуть False return OnnxEmbedderClient(port=19999, model_name="test-model")