diff --git a/hermes_state.py b/hermes_state.py index 86d865cc65..96400097ab 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -37,7 +37,7 @@ from hermes_state_errors import ( ) from hermes_state_guard import ( _STATE_DB_GUARD_BYPASS_ENV, _in_test_context, _is_production_state_db, _real_platform_state_root, - _set_last_init_error, get_last_init_error, + _register_test_instance, _set_last_init_error, _test_instance_registry, get_last_init_error, ) from hermes_state_readpool import _READ_POOL_MAX, _proc_fd_targets, _read_budget_for from hermes_state_sessions import SessionSessionsMixin @@ -562,6 +562,10 @@ class SessionDB( if not initialization_complete: conn, self._conn = self._conn, None self._close_connection_quietly(conn) + else: + # Test-isolation runs only (gated inside the helper): register + # for the suite-level leak sweep in tests/conftest.py. + _register_test_instance(self) def _open_writer(self) -> None: """Writable open: preflight, zero-byte quarantine, connect + schema (one in-place repair of a diff --git a/hermes_state_guard.py b/hermes_state_guard.py index ec380a580b..2f0444df57 100644 --- a/hermes_state_guard.py +++ b/hermes_state_guard.py @@ -6,6 +6,7 @@ state.db; env-based so subprocess children are protected too.""" import os import sys import threading +import weakref from pathlib import Path from typing import Any, Optional @@ -125,6 +126,30 @@ def _is_production_state_db(resolved: Path, root: Path) -> bool: return len(parts) == 3 and parts[0] == "profiles" +# Test-only SessionDB instance registry. Under the hermetic suite every +# successfully constructed SessionDB is added to this WeakSet so the autouse +# teardown in tests/conftest.py (_close_leaked_session_dbs) can close whatever +# a test forgot to close. Dozens of tests build SessionDB() directly and never +# close it; each instance holds a writer connection plus pooled readers, and a +# single-process run over tests/hermes_cli/ accumulated 16-25 GB RSS (OOM +# incident 20260816). The per-file runner masks this in CI; the registry fixes +# the class at the source instead of patching ~40 test files. +# +# Population is gated on the isolation marker (exported by tests/conftest.py +# before any test module imports). Production processes never populate it; +# do not "simplify" the gate away. WeakSet membership never pins an instance. +_test_instance_registry: "weakref.WeakSet[Any]" = weakref.WeakSet() + + +def _register_test_instance(db: Any) -> None: + """Track *db* for suite-level teardown closing (test-isolation runs only).""" + if os.environ.get(_TEST_ISOLATION_MARKER_ENV): + try: + _test_instance_registry.add(db) + except Exception: # pragma: no cover — registry must never break init + pass + + # Last SessionDB() init error, per-process; surfaced by /resume-style slash # commands so users know WHY. Only SessionDB.__init__ writes it. _last_init_error: Optional[str] = None diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index 5e20caf3cb..4dbd4aef97 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -35,6 +35,12 @@ Environment: HERMES_TEST_PATHS Override discovery roots (colon-sep; on Windows ';' also works and drive letters are handled; default: 'tests') + HERMES_PYTEST_MEM_CAP + Per-pytest-process RLIMIT_AS cap in GiB applied by + tests/conftest.py (Linux only; default 12). Any + runaway in-process accumulation dies with + MemoryError instead of eating the box. Set to 0 / + 'off' to disable, or another integer to change it. Exit code: 0 if every file's pytest exited 0; 1 otherwise. """ diff --git a/tests/conftest.py b/tests/conftest.py index 45d67cf7a9..d92255898d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -613,6 +613,108 @@ def _neutralize_git_safe_directory_read(request, monkeypatch): monkeypatch.setattr(_subprocess_compat, "_user_safe_directories", lambda base_env: [], raising=False) +@pytest.fixture(autouse=True) +def _close_leaked_session_dbs(): + """Close every SessionDB a test constructed but forgot to close. + + Root cause of OOM incident 20260816: ~40 files under tests/hermes_cli/ + build ``SessionDB(...)`` directly and never call ``close()``. Each open + instance holds the writer connection (state.db + -wal fds), up to + ``_READ_POOL_MAX`` pooled read connections, per-connection SQLite page + caches, and — once token accounting has run — an ``atexit`` registration + that pins the instance alive until interpreter exit. Under the sanctioned + per-file-process runner this is invisible, but a raw single-process + ``pytest tests/hermes_cli/`` accumulated 16-25 GB RSS and had to be + OOM-killed three times in one day. + + Rather than editing every test file, ``SessionDB.__init__`` registers each + instance in ``hermes_state_guard._test_instance_registry`` (a WeakSet, + populated only when the ``HERMES_TEST_ISOLATION`` marker is set — i.e. + only under this suite). This teardown closes whatever the test left open. + ``close()`` is idempotent (``self._conn`` is None afterwards) and also + unregisters the pinning atexit hook, so instances become collectable. + + Snapshotting the registry BEFORE the test and closing only NEW instances + is deliberately avoided: closing pre-existing instances is harmless (they + were leaked by an earlier test in the same process) and the simpler + close-everything sweep is what actually bounds the process. + + Instances opened through ``hermes_state_registry.acquire()`` are skipped: + on those ``close()`` releases a refcount rather than closing, so a sweep + would silently retire a shared generation that a wider-scoped fixture + still holds. The registry owns that lifecycle (``close_all()``). + """ + yield + try: + from hermes_state_guard import _test_instance_registry as registry + except Exception: + return + if not registry: + return + for db in list(registry): + if getattr(db, "_shared_registry_owned", False): + continue + try: + db.close() + except Exception: + # Teardown must never fail a passing test; a close that raises + # (cross-thread ProgrammingError, already-closed) leaves at most + # the one connection for the next sweep / process exit. + pass + + +@pytest.fixture(scope="session", autouse=True) +def _pytest_memory_cap(): + """Fail fast with MemoryError instead of eating the box (Linux only). + + Applies ``RLIMIT_AS`` for the pytest process so any future in-process + accumulation (like the SessionDB leak this suite once had) dies with a + loud ``MemoryError`` at the cap instead of ballooning to 25 GB and + getting OOM-killed by the machine's sentinel. + + Default cap: 12 GiB — generous headroom over the observed healthy peak + (< 1 GiB for the largest per-file runs, a few GiB for a full healthy + single-process run). Override with the ``HERMES_PYTEST_MEM_CAP`` env var: + + * ``HERMES_PYTEST_MEM_CAP=0`` (or ``off``/``none``) disables the cap; + * any other integer is the cap in GiB (e.g. ``HERMES_PYTEST_MEM_CAP=4``). + + This is a test-harness knob, not user-facing product config, hence an + env var rather than config.yaml. Skipped on non-Linux (RLIMIT_AS + semantics differ on macOS and don't exist on Windows) and when the + existing limit is already tighter. + """ + if sys.platform != "linux": + yield + return + raw = os.environ.get("HERMES_PYTEST_MEM_CAP", "").strip().lower() + if raw in {"0", "off", "none", "disable", "disabled"}: + yield + return + cap_gib = 12 + if raw: + try: + cap_gib = int(raw) + except ValueError: + cap_gib = 12 + if cap_gib <= 0: + yield + return + try: + import resource + + cap_bytes = cap_gib * 1024**3 + soft, hard = resource.getrlimit(resource.RLIMIT_AS) + new_soft = cap_bytes if soft in (resource.RLIM_INFINITY,) or soft > cap_bytes else soft + new_hard = hard if hard != resource.RLIM_INFINITY and hard < cap_bytes else cap_bytes + resource.setrlimit(resource.RLIMIT_AS, (new_soft, new_hard)) + except Exception: + # Sandboxes/containers may refuse setrlimit; the cap is defensive, + # never a reason to fail the run. + pass + yield + + @pytest.fixture(autouse=True) def _neutralize_webbrowser(monkeypatch): """Record browser-open attempts instead of opening real browser windows.""" diff --git a/tests/hermes_state/test_session_db_leak_sweep.py b/tests/hermes_state/test_session_db_leak_sweep.py new file mode 100644 index 0000000000..c6b2024183 --- /dev/null +++ b/tests/hermes_state/test_session_db_leak_sweep.py @@ -0,0 +1,60 @@ +"""Suite-wide SessionDB leak-closing contract (OOM incident 20260816). + +A raw single-process ``pytest tests/hermes_cli/`` used to accumulate every +SessionDB a test constructed and forgot to close — writer connection, +pooled read connections, and (once token accounting ran) an ``atexit`` +registration pinning the instance alive — ballooning to 16-25 GB RSS. + +The fix is two-sided: + +* ``hermes_state_guard._register_test_instance`` adds every successfully + constructed SessionDB to a WeakSet registry when the + ``HERMES_TEST_ISOLATION`` marker is set (test-isolation runs only). +* the autouse ``_close_leaked_session_dbs`` fixture in ``tests/conftest.py`` + closes everything in the registry at each test's teardown. + +These tests pin the *behavior contract*: instances register under pytest, +close() empties them idempotently, and a leaked instance from an earlier +test is actually closed by the suite-level sweep. +""" + +from __future__ import annotations + +import hermes_state_guard +from hermes_state import SessionDB + +# Deliberate cross-test handoff: test_leaked_instance_* leaks an instance; +# the later test (pytest runs file order deterministically without a +# randomizer plugin, and the sanctioned runner executes whole files in one +# process) asserts the autouse teardown sweep closed it. +_leaked: list[SessionDB] = [] + + +def test_constructed_sessiondb_is_registered(tmp_path): + db = SessionDB(db_path=tmp_path / "state.db") + try: + assert db in hermes_state_guard._test_instance_registry + finally: + db.close() + # close() must fully release the writer connection… + assert db._conn is None + # …and be idempotent: a second close (the suite sweep will call it + # again at teardown) must not raise. + db.close() + + +def test_leaked_instance_stays_open_within_the_test(tmp_path): + db = SessionDB(db_path=tmp_path / "state.db") + db.create_session(session_id="leak-probe", source="cli", model="m") + # Intentionally NOT closed — the suite-level sweep owns cleanup. + assert db._conn is not None + _leaked.append(db) + + +def test_previously_leaked_instance_was_closed_by_the_sweep(): + assert _leaked, "expected the previous test to have leaked an instance" + db = _leaked.pop() + # The autouse _close_leaked_session_dbs teardown between the two tests + # must have closed the leaked instance (writer conn released), which is + # what bounds fd/RSS growth in single-process runs. + assert db._conn is None