diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cc409e3..30a610ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Hermes Web UI -- Changelog +## [Unreleased] + +### Fixed (1 PR) + +- **`state.db` connection FD leak in sidebar polling** (#1494, fix shape provided by @insecurejezza, closes #1494, addresses Bug #2 of #1458) — production WebUI on macOS launchd reproduced this after #1483 fixed the bootstrap supervisor double-fork: process alive, port listening, every HTTP request reset before a response. Investigation traced it to FD exhaustion from `~/.hermes/state.db` handles (366 total FDs, 238 of them `state.db*` on a wedged process). Root cause: four sqlite callsites used `with sqlite3.connect(...) as conn:`, but Python's `sqlite3.Connection` context manager only commits or rolls back on exit — it does **not** close the connection. `/api/sessions` polling calls these on every sidebar refresh, so each poll leaked one or more open `state.db` / `state.db-wal` / `state.db-shm` FDs until the process hit the macOS soft limit (256) and new connections RST'd before any handler bytes were written. **Fix:** wrap each `sqlite3.connect(...)` call in `contextlib.closing(...)` at: `api/agent_sessions.py:read_importable_agent_session_rows`, `api/agent_sessions.py:read_session_lineage_metadata`, `api/models.py:get_cli_session_messages`, `api/models.py:delete_cli_session`. The reporter verified the fix in production (FD count flat at 92 across a 100-request stress loop against `/api/sessions` and `/api/projects`, vs. monotonic growth before). 4 regression tests in `tests/test_issue1494_state_db_fd_leak.py` monkeypatch `sqlite3.connect` with a tracking wrapper that records `.close()` calls and assert every connection opened by each function is explicitly closed — verified to fail (catching the original bug) when the `closing()` wrap is reverted. Bug #3 from #1458 (HTTP-unhealthy wedge in absence of FD exhaustion) remains open pending separate diagnostic data. (`api/agent_sessions.py`, `api/models.py`, `tests/test_issue1494_state_db_fd_leak.py`) + ## [v0.50.271] — 2026-05-02 ### Changed (1 self-built PR) diff --git a/api/agent_sessions.py b/api/agent_sessions.py index 3bf8b722..7b024f8b 100644 --- a/api/agent_sessions.py +++ b/api/agent_sessions.py @@ -1,6 +1,7 @@ """Shared helpers for reading Hermes Agent sessions from state.db.""" import logging import sqlite3 +from contextlib import closing from pathlib import Path logger = logging.getLogger(__name__) @@ -234,7 +235,7 @@ def read_importable_agent_session_rows( return [] log = log or logger - with sqlite3.connect(str(db_path)) as conn: + with closing(sqlite3.connect(str(db_path))) as conn: conn.row_factory = sqlite3.Row cur = conn.cursor() @@ -306,7 +307,7 @@ def read_session_lineage_metadata(db_path: Path, session_ids: list[str] | set[st return {} try: - with sqlite3.connect(str(db_path)) as conn: + with closing(sqlite3.connect(str(db_path))) as conn: conn.row_factory = sqlite3.Row cur = conn.cursor() cur.execute("PRAGMA table_info(sessions)") diff --git a/api/models.py b/api/models.py index 62afc9ed..b5b33c8f 100644 --- a/api/models.py +++ b/api/models.py @@ -6,6 +6,7 @@ import os import threading import time import uuid +from contextlib import closing from pathlib import Path import api.config as _cfg @@ -1101,7 +1102,7 @@ def get_cli_session_messages(sid) -> list: return [] try: - with sqlite3.connect(str(db_path)) as conn: + with closing(sqlite3.connect(str(db_path))) as conn: conn.row_factory = sqlite3.Row cur = conn.cursor() cur.execute(""" @@ -1142,7 +1143,7 @@ def delete_cli_session(sid) -> bool: return False try: - with sqlite3.connect(str(db_path)) as conn: + with closing(sqlite3.connect(str(db_path))) as conn: cur = conn.cursor() cur.execute("DELETE FROM messages WHERE session_id = ?", (sid,)) cur.execute("DELETE FROM sessions WHERE id = ?", (sid,)) diff --git a/tests/test_issue1494_state_db_fd_leak.py b/tests/test_issue1494_state_db_fd_leak.py new file mode 100644 index 00000000..701852b3 --- /dev/null +++ b/tests/test_issue1494_state_db_fd_leak.py @@ -0,0 +1,234 @@ +"""Regression test for #1494: state.db connection FD leak via context-manager use. + +The bug: Python's sqlite3 connection context manager (`with sqlite3.connect(...) as +conn:`) commits or rolls back on exit. It does NOT close the connection. In a +long-running server with sidebar polling (`/api/sessions` calls +`read_importable_agent_session_rows` and `read_session_lineage_metadata` on every +poll), every poll leaked one or more open file descriptors against `~/.hermes/state.db`. + +In production this drove the WebUI process past macOS's 256-FD soft limit, after +which new requests reset before producing a response (see #1458, #1494) — the +process stayed alive, the port stayed listening, but every connection RST'd because +sqlite3.connect() in a freshly accepted handler raised on FD exhaustion before any +bytes were written. + +The fix wraps each `sqlite3.connect(...)` in `contextlib.closing(...)` so the +connection is explicitly closed on scope exit (in addition to the auto-commit / +rollback semantics). + +This file pins all four production callsites the issue reporter (insecurejezza) +audited as still leaking on master @ 7fddc33: + + * api/agent_sessions.py:read_importable_agent_session_rows + * api/agent_sessions.py:read_session_lineage_metadata + * api/models.py:get_cli_session_messages + * api/models.py:delete_cli_session + +Each test monkeypatches sqlite3.connect to track every connection the function +opens, then asserts every connection is .close()'d after the call returns. +""" +import sqlite3 + +import pytest + + +def _make_state_db(path): + """Minimal state.db schema sufficient for the four functions under test. + + Bypasses sqlite3.connect (uses sqlite3.Connection directly) so the + seed-data setup is not counted by the _TrackingConn monkeypatch — only + connections opened by the function under test should appear in + `_TrackingConn.instances`. + """ + conn = sqlite3.Connection(str(path)) + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + title TEXT, + model TEXT, + message_count INTEGER DEFAULT 0, + started_at TEXT, + source TEXT, + parent_session_id TEXT, + ended_at TEXT, + end_reason TEXT + ); + CREATE INDEX idx_sessions_parent ON sessions(parent_session_id); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, + role TEXT, + content TEXT, + timestamp TEXT + ); + INSERT INTO sessions (id, title, model, message_count, started_at, source) + VALUES ('s1', 'cli session', 'gpt-x', 2, '2026-01-01T00:00:00Z', 'cli'); + INSERT INTO messages (session_id, role, content, timestamp) + VALUES ('s1', 'user', 'hi', '2026-01-01T00:00:01Z'), + ('s1', 'assistant', 'hello', '2026-01-01T00:00:02Z'); + """ + ) + conn.commit() + conn.close() + + +class _TrackingConn: + """Wraps a real sqlite3.Connection to record open/close lifecycle. + + Mirrors the lightweight wrapper pattern already in + test_pr1370_lineage_metadata_perf_and_orphan.py — keeping it inline here so + this regression test stays self-contained and survives refactors there. + """ + + instances: list = [] + + def __init__(self, *args, **kwargs): + self._real = sqlite3.Connection(*args, **kwargs) + self.closed = False + _TrackingConn.instances.append(self) + + # Connection-shaped delegation + def cursor(self): + return self._real.cursor() + + def execute(self, *a, **kw): + return self._real.execute(*a, **kw) + + def executescript(self, *a, **kw): + return self._real.executescript(*a, **kw) + + def commit(self): + return self._real.commit() + + def rollback(self): + return self._real.rollback() + + def close(self): + self.closed = True + return self._real.close() + + # row_factory needs to round-trip onto the real connection + @property + def row_factory(self): + return self._real.row_factory + + @row_factory.setter + def row_factory(self, value): + self._real.row_factory = value + + # Context-manager protocol — the bug only triggers if a caller relies on + # __exit__ to close. Defer to the real Connection's CM (commit/rollback) + # so we faithfully reproduce the leak shape that prompted the fix. + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return self._real.__exit__(exc_type, exc, tb) + + +@pytest.fixture +def tracking_sqlite(monkeypatch): + """Monkeypatch sqlite3.connect to use _TrackingConn and reset the instance log.""" + _TrackingConn.instances = [] + + def _connect(*args, **kwargs): + return _TrackingConn(*args, **kwargs) + + monkeypatch.setattr(sqlite3, "connect", _connect) + return _TrackingConn + + +def _assert_all_closed(tracking, fn_name): + assert tracking.instances, ( + f"{fn_name}: no sqlite connections were opened — test setup is wrong" + ) + leaked = [c for c in tracking.instances if not c.closed] + assert not leaked, ( + f"{fn_name} leaked {len(leaked)} of {len(tracking.instances)} sqlite " + f"connection(s) — context-manager-only `with sqlite3.connect()` does " + f"not close. Wrap in contextlib.closing(). See #1494." + ) + + +def test_read_importable_agent_session_rows_closes_connection(tmp_path, tracking_sqlite): + """`read_importable_agent_session_rows` must close every sqlite connection.""" + db = tmp_path / "state.db" + _make_state_db(db) + + from api.agent_sessions import read_importable_agent_session_rows + + # Call repeatedly — under the bug each call leaked a connection. + for _ in range(5): + read_importable_agent_session_rows(db) + + _assert_all_closed(tracking_sqlite, "read_importable_agent_session_rows") + assert len(tracking_sqlite.instances) == 5 + + +def test_read_session_lineage_metadata_closes_connection(tmp_path, tracking_sqlite): + """`read_session_lineage_metadata` must close every sqlite connection.""" + db = tmp_path / "state.db" + _make_state_db(db) + + from api.agent_sessions import read_session_lineage_metadata + + for _ in range(5): + read_session_lineage_metadata(db, ["s1"]) + + _assert_all_closed(tracking_sqlite, "read_session_lineage_metadata") + assert len(tracking_sqlite.instances) == 5 + + +def test_get_cli_session_messages_closes_connection(tmp_path, tracking_sqlite, monkeypatch): + """`get_cli_session_messages` must close every sqlite connection.""" + db = tmp_path / "state.db" + _make_state_db(db) + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + # Stub get_active_hermes_home so the tmp_path is used regardless of profile state. + import api.profiles + monkeypatch.setattr(api.profiles, "get_active_hermes_home", lambda: str(tmp_path)) + + from api.models import get_cli_session_messages + + for _ in range(5): + rows = get_cli_session_messages("s1") + # Sanity: the seeded messages are returned (proves we hit the real query path). + assert len(rows) == 2 + + _assert_all_closed(tracking_sqlite, "get_cli_session_messages") + assert len(tracking_sqlite.instances) == 5 + + +def test_delete_cli_session_closes_connection(tmp_path, tracking_sqlite, monkeypatch): + """`delete_cli_session` must close its sqlite connection (also keeps explicit commit working).""" + db = tmp_path / "state.db" + _make_state_db(db) + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + import api.profiles + monkeypatch.setattr(api.profiles, "get_active_hermes_home", lambda: str(tmp_path)) + + from api.models import delete_cli_session + + deleted = delete_cli_session("s1") + assert deleted is True, "delete_cli_session should report the row was removed" + + # Second call: row gone, nothing to delete — connection must still close cleanly. + deleted_again = delete_cli_session("s1") + assert deleted_again is False + + _assert_all_closed(tracking_sqlite, "delete_cli_session") + # First call commits; second call short-circuits but still opens+closes a connection. + assert len(tracking_sqlite.instances) == 2 + + # Verify the commit semantics survived the closing() change — row really is gone. + real = sqlite3.Connection(str(db)) + try: + cur = real.execute("SELECT COUNT(*) FROM sessions WHERE id = ?", ("s1",)) + assert cur.fetchone()[0] == 0 + cur = real.execute("SELECT COUNT(*) FROM messages WHERE session_id = ?", ("s1",)) + assert cur.fetchone()[0] == 0 + finally: + real.close()