fix(tests): close leaked SessionDB handles suite-wide and cap pytest memory
Root cause of the 2026-08-16 OOM incidents (three runs of `python -m pytest -o addopts= -q tests/hermes_cli/` ballooning to 16-25 GB RSS and getting killed): ~40 files under tests/hermes_cli/ construct SessionDB() directly and never close it. Each instance keeps the writer connection (state.db + -wal fds), up to _READ_POOL_MAX pooled readers with their SQLite page caches, and — once token accounting has run — an atexit registration that pins the instance alive until interpreter exit. In one process over 637 files those accumulate without bound; the sanctioned per-file runner masks it, so CI never saw it. Fix the class, not the sites: * hermes_state: register every successfully constructed SessionDB in a test-only WeakSet (populated only when HERMES_TEST_ISOLATION is set, i.e. under this test suite; production never touches it). * tests/conftest.py: autouse _close_leaked_session_dbs teardown closes everything left in the registry after each test. close() is idempotent and unregisters the pinning atexit hook, so instances become collectable. * tests/conftest.py: session-scoped _pytest_memory_cap applies a defensive RLIMIT_AS of 12 GiB (Linux only) so any future in-process leak fails fast with MemoryError instead of eating the box. Overridable/disable-able via HERMES_PYTEST_MEM_CAP (documented in scripts/run_tests_parallel.py). * tests/hermes_state/test_session_db_leak_sweep.py: behavior contract for registration, idempotent close, and the cross-test sweep. Measured (capped single-process `pytest -o addopts= -q tests/hermes_cli/`): peak RSS 4.16 GiB before -> 1.67 GiB after; per-test open .db fd count previously climbed monotonically (0 -> 12 -> 17 -> 104 within the SessionDB-heavy files), now stays bounded (<= 5, transient). Sanctioned runner over the affected 35 files: 495 passed, 0 failed, no FLAKY. Incident evidence: ~/.hermes/logs/oom-incidents/20260816-202114 (fd dumps show 100+ open state.db/state.db-wal handles across pytest tmpdirs; 3rd recurrence that day).
This commit is contained in:
@@ -37,7 +37,7 @@ from hermes_state_errors import (
|
|||||||
)
|
)
|
||||||
from hermes_state_guard import (
|
from hermes_state_guard import (
|
||||||
_STATE_DB_GUARD_BYPASS_ENV, _in_test_context, _is_production_state_db, _real_platform_state_root,
|
_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_readpool import _READ_POOL_MAX, _proc_fd_targets, _read_budget_for
|
||||||
from hermes_state_sessions import SessionSessionsMixin
|
from hermes_state_sessions import SessionSessionsMixin
|
||||||
@@ -562,6 +562,10 @@ class SessionDB(
|
|||||||
if not initialization_complete:
|
if not initialization_complete:
|
||||||
conn, self._conn = self._conn, None
|
conn, self._conn = self._conn, None
|
||||||
self._close_connection_quietly(conn)
|
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:
|
def _open_writer(self) -> None:
|
||||||
"""Writable open: preflight, zero-byte quarantine, connect + schema (one in-place repair of a
|
"""Writable open: preflight, zero-byte quarantine, connect + schema (one in-place repair of a
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ state.db; env-based so subprocess children are protected too."""
|
|||||||
import os
|
import os
|
||||||
import sys
|
import sys
|
||||||
import threading
|
import threading
|
||||||
|
import weakref
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Any, Optional
|
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"
|
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
|
# Last SessionDB() init error, per-process; surfaced by /resume-style slash
|
||||||
# commands so users know WHY. Only SessionDB.__init__ writes it.
|
# commands so users know WHY. Only SessionDB.__init__ writes it.
|
||||||
_last_init_error: Optional[str] = None
|
_last_init_error: Optional[str] = None
|
||||||
|
|||||||
@@ -35,6 +35,12 @@ Environment:
|
|||||||
HERMES_TEST_PATHS Override discovery roots (colon-sep; on Windows
|
HERMES_TEST_PATHS Override discovery roots (colon-sep; on Windows
|
||||||
';' also works and drive letters are handled;
|
';' also works and drive letters are handled;
|
||||||
default: 'tests')
|
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.
|
Exit code: 0 if every file's pytest exited 0; 1 otherwise.
|
||||||
"""
|
"""
|
||||||
|
|||||||
@@ -613,6 +613,108 @@ def _neutralize_git_safe_directory_read(request, monkeypatch):
|
|||||||
monkeypatch.setattr(_subprocess_compat, "_user_safe_directories", lambda base_env: [], raising=False)
|
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)
|
@pytest.fixture(autouse=True)
|
||||||
def _neutralize_webbrowser(monkeypatch):
|
def _neutralize_webbrowser(monkeypatch):
|
||||||
"""Record browser-open attempts instead of opening real browser windows."""
|
"""Record browser-open attempts instead of opening real browser windows."""
|
||||||
|
|||||||
60
tests/hermes_state/test_session_db_leak_sweep.py
Normal file
60
tests/hermes_state/test_session_db_leak_sweep.py
Normal file
@@ -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
|
||||||
Reference in New Issue
Block a user