From dcdcfb99776f5e419563ae94a52e45101c296218 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Thu, 16 Jul 2026 19:14:53 +0000 Subject: [PATCH] fix(streaming): degrade self-heal SessionDB to None on failed rebuild Codex gate on #6143 found a SILENT regression the reuse-path fix introduced: _replace_session_db_in_kwargs early-returned the OLD handle when the rebuild failed, but master assigned None. If the old handle was already CLOSED, both credential self-heal callers (streaming.py:9032/10253) then built a fresh agent around a closed SessionDB -> every persist/search fails with 'NoneType has no attribute execute' while the chat keeps going. Guard on _session_db_is_open: keep a live handle (subagents hold it by ref), else degrade to None (clean lazy reinit, master behaviour). +2 regression tests. Co-authored-by: carlotestor --- api/streaming.py | 11 +++++- tests/test_v050259_sessiondb_fd_leak.py | 52 +++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/api/streaming.py b/api/streaming.py index 55c51d0e6..5e222a5f1 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -6350,7 +6350,16 @@ def _replace_session_db_in_kwargs(agent_kwargs, state_db_path): _old_session_db = agent_kwargs.get("session_db") _next_session_db = _build_session_db_for_stream(state_db_path) if _next_session_db is None: - return _old_session_db + # Replacement construction failed. Keep the prior handle only if it is + # still open (live subagents may hold it by reference); otherwise + # degrade cleanly to None — as master did — so the rebuilt agent lazily + # reinitialises its SessionDB instead of reusing a closed handle and + # failing every persist/search with + # "'NoneType' object has no attribute 'execute'". + if _session_db_is_open(_old_session_db): + return _old_session_db + agent_kwargs["session_db"] = None + return None if _session_db_is_open(_old_session_db): # Keep the live handle; discard the unused new one. try: diff --git a/tests/test_v050259_sessiondb_fd_leak.py b/tests/test_v050259_sessiondb_fd_leak.py index 6ad48124c..12bfd90e4 100644 --- a/tests/test_v050259_sessiondb_fd_leak.py +++ b/tests/test_v050259_sessiondb_fd_leak.py @@ -280,3 +280,55 @@ def test_lru_eviction_closes_evicted_session_db(): assert db1.close_calls == 1, "evicted agent's SessionDB must be closed exactly once" assert db2.close_calls == 0, "remaining agents' SessionDBs must not be touched" assert db3.close_calls == 0 + + +# ── 6: self-heal path must not reuse a CLOSED handle when the rebuild fails ── + + +def _import_replace_helper(): + """Import the real credential-self-heal SessionDB replacer.""" + try: + from api.streaming import _replace_session_db_in_kwargs # type: ignore + except Exception as exc: + pytest.skip(f"api.streaming not importable: {exc}") + return _replace_session_db_in_kwargs + + +def test_replace_degrades_to_none_when_rebuild_fails_and_old_is_closed(monkeypatch): + """Credential self-heal regression (Codex gate finding on PR #6143). + + When ``_build_session_db_for_stream`` returns None (rebuild failed) AND the + prior handle is already CLOSED, ``_replace_session_db_in_kwargs`` must leave + ``agent_kwargs['session_db'] = None`` — as master did — so the rebuilt agent + lazily reinitialises. Retaining the closed handle (the pre-fix behaviour) + makes every persist/search fail with + ``'NoneType' object has no attribute 'execute'`` while the chat continues. + """ + import api.streaming as streaming + + _replace = _import_replace_helper() + monkeypatch.setattr(streaming, "_build_session_db_for_stream", lambda _p: None) + + old_db = _MockSessionDB("old", open_=False) # already closed + kwargs = {"session_db": old_db} + result = _replace(kwargs, "/tmp/does-not-matter.db") + + assert result is None, "must not hand back a closed handle when rebuild fails" + assert kwargs["session_db"] is None, "kwargs must degrade to None (clean lazy reinit)" + + +def test_replace_keeps_open_handle_when_rebuild_fails(monkeypatch): + """Inverse: a still-OPEN prior handle (held by live subagents) is retained + when the rebuild fails — do not orphan a live shared connection.""" + import api.streaming as streaming + + _replace = _import_replace_helper() + monkeypatch.setattr(streaming, "_build_session_db_for_stream", lambda _p: None) + + old_db = _MockSessionDB("old", open_=True) # still live (subagents hold it) + kwargs = {"session_db": old_db} + result = _replace(kwargs, "/tmp/does-not-matter.db") + + assert result is old_db, "a live handle must be kept when the rebuild fails" + assert kwargs["session_db"] is old_db + assert old_db.close_calls == 0, "must not close a live shared handle"