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/AGENT_DIARY.md b/AGENT_DIARY.md index 94721004..0e7ba4aa 100644 --- a/AGENT_DIARY.md +++ b/AGENT_DIARY.md @@ -1961,6 +1961,17 @@ BREACH: MSC-P-19 | date: 2026-10-03 | rule: §12 | guard_at_time: no | detected_ **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 экземпляров, а не один; всё, что «на весь прогон» (блокировки, общие каталоги, счётчики), живёт в хуках контроллера. ## [2026-11-14] Установщик падал на шаге 7: `httpx`, `wmic` и «тихий» `--with-models` **Симптом:** `python install.py` (интерпретатор `C:\Python314\python.exe`, без зависимостей проекта) — `⚠ Step 7/12: llama.cpp engine .. No module named 'httpx'`, дальше `[r]etry / [s]kip / [c]ancel`; итог прогона `11 ok, 1 skipped, 1351 s`. 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..c624a783 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,86 @@ 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() + + +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": + return + if getattr(config, "workerinput", None) is not None: # xdist worker + 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, + ) + config._mscb_suite_lock = handle + os.environ[_LOCK_INHERITED] = "1" + + +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_idle_reload.py b/tests/test_idle_reload.py index 0ab2904c..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") @@ -136,3 +152,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..bb2f8c5d --- /dev/null +++ b/tests/test_suite_lock.py @@ -0,0 +1,51 @@ +"""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) + + +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