From 7d2ca8cabb7209267c7cdd1e081b710fe74c47dc Mon Sep 17 00:00:00 2001 From: carlotestor Date: Thu, 16 Jul 2026 16:53:42 +0200 Subject: [PATCH] fix: address greptile review on SessionDB adopt helper - Log failed closes in _adopt_session_db_for_cached_agent with logger.debug(..., exc_info=True) instead of bare pass, matching _replace_session_db_in_kwargs so a failed close (EMFILE risk) is diagnosable. - Drop the fragile exec/source-slice fallback in the test helper import; real import or pytest.skip. Source-level pins still cover reverts without importing streaming. --- api/streaming.py | 12 +++++++-- tests/test_v050259_sessiondb_fd_leak.py | 35 +++++++++++-------------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/api/streaming.py b/api/streaming.py index 192c0d4e..55c51d0e 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -6305,13 +6305,21 @@ def _adopt_session_db_for_cached_agent(agent, new_session_db): try: new_session_db.close() except Exception: - pass + # Same observability as _replace_session_db_in_kwargs: a failed + # close here reintroduces the EMFILE pressure PR #1421 fixed. + logger.debug( + "Failed to close unused session_db handle in adopt helper", + exc_info=True, + ) return existing if existing is not None: try: existing.close() except Exception: - pass + logger.debug( + "Failed to close previous session_db handle in adopt helper", + exc_info=True, + ) agent._session_db = new_session_db return new_session_db diff --git a/tests/test_v050259_sessiondb_fd_leak.py b/tests/test_v050259_sessiondb_fd_leak.py index 7b17212d..6ad48124 100644 --- a/tests/test_v050259_sessiondb_fd_leak.py +++ b/tests/test_v050259_sessiondb_fd_leak.py @@ -72,6 +72,13 @@ def test_adopt_and_is_open_helpers_exist(): "_replace_session_db_in_kwargs must guard on _session_db_is_open so " "credential self-heal cannot close a handle live subagents share" ) + # adopt helper must log failed closes (not bare `pass`) so EMFILE pressure + # from a failed close is diagnosable — matches _replace_session_db_in_kwargs. + adopt_idx = src.find("def _adopt_session_db_for_cached_agent") + assert adopt_idx != -1 + adopt_block = src[adopt_idx : adopt_idx + 1800] + assert 'Failed to close unused session_db handle in adopt helper' in adopt_block + assert "logger.debug" in adopt_block # ── 2: source-level pin: LRU eviction path still closes _session_db ──────── @@ -161,32 +168,22 @@ class _MockAgent: def _import_adopt_helpers(): - """Import the helpers without pulling in the whole streaming module graph. + """Import the production helpers. - streaming.py has heavy import-time side effects; exec the helper bodies - from source so unit tests stay hermetic. + No source-slicing / exec fallback: that path breaks as soon as the helpers + reference module-level names (e.g. ``logger.debug``) or another def is + inserted between the markers. Prefer a real import; skip the behavioral + suite when the package cannot be imported (CI without full deps). + Source-level pins above still catch reverts without importing streaming. """ - import importlib.util - import types - - # Prefer real import when the package is importable. try: from api.streaming import ( # type: ignore _adopt_session_db_for_cached_agent, _session_db_is_open, ) - return _session_db_is_open, _adopt_session_db_for_cached_agent - except Exception: - pass - - src = (REPO / "api" / "streaming.py").read_text(encoding="utf-8") - # Extract just the two helper defs by line slicing between markers. - start = src.index("def _session_db_is_open") - end = src.index("def _build_session_db_for_stream") - blob = src[start:end] - ns: dict = {} - exec(blob, ns) # noqa: S102 — test-only extraction of pure helpers - return ns["_session_db_is_open"], ns["_adopt_session_db_for_cached_agent"] + except Exception as exc: + pytest.skip(f"api.streaming helpers not importable: {exc}") + return _session_db_is_open, _adopt_session_db_for_cached_agent def test_adopt_reuses_open_session_db_and_closes_new():