Stage 272: PR #1495 — state.db FD leak fix (#1494, Bug #2 of #1458)

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)

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)")

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,))

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()