From c4a438d75dfa9239b1f435a1d507dd6dc07e4d7c Mon Sep 17 00:00:00 2001 From: Byrd Date: Sat, 21 Mar 2026 10:03:02 -0400 Subject: [PATCH 1/3] fix: handle closed logging streams in dispose_engine shutdown During interpreter shutdown, atexit/weakref finalizer callbacks can fire after logging handler streams are already closed, raising ValueError. Wrap the logger.info call in dispose_engine with a try/except so cleanup is silent when streams are unavailable. Closes #1520 Made-with: Cursor --- pyrit/memory/sqlite_memory.py | 5 ++++- tests/unit/memory/test_sqlite_memory.py | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/pyrit/memory/sqlite_memory.py b/pyrit/memory/sqlite_memory.py index d59a6571d2..7035380250 100644 --- a/pyrit/memory/sqlite_memory.py +++ b/pyrit/memory/sqlite_memory.py @@ -346,7 +346,10 @@ def dispose_engine(self) -> None: """ if self.engine: self.engine.dispose() - logger.info("Engine disposed and all connections closed.") + try: + logger.info("Engine disposed and all connections closed.") + except (ValueError, OSError): + pass def export_conversations( self, diff --git a/tests/unit/memory/test_sqlite_memory.py b/tests/unit/memory/test_sqlite_memory.py index 5e6d3b168e..512020b9f4 100644 --- a/tests/unit/memory/test_sqlite_memory.py +++ b/tests/unit/memory/test_sqlite_memory.py @@ -668,3 +668,23 @@ def test_get_conversation_stats_batches_multiple_conversations(sqlite_instance): assert result[conv_ids[0]].message_count == 1 assert result[conv_ids[1]].message_count == 2 assert result[conv_ids[2]].message_count == 3 + + +def test_dispose_engine_tolerates_closed_log_stream(): + """Verify dispose_engine does not raise when logging streams are already closed (GH-1520).""" + import io + import logging + + from pyrit.memory.sqlite_memory import SQLiteMemory + + stream = io.StringIO() + handler = logging.StreamHandler(stream) + root = logging.getLogger() + root.addHandler(handler) + + try: + mem = SQLiteMemory(db_path=":memory:") + stream.close() + mem.dispose_engine() + finally: + root.removeHandler(handler) From 2076853277bff47cc6aec0766dca261ec0c6067e Mon Sep 17 00:00:00 2001 From: Byrd Date: Sat, 21 Mar 2026 16:20:12 -0400 Subject: [PATCH 2/3] fix: use logging.raiseExceptions instead of try/except for shutdown safety Address Copilot review feedback: - StreamHandler.emit() catches ValueError internally and calls handleError() which prints to stderr without re-raising, so a try/except around logger.info() won't suppress the shutdown noise. Use logging.raiseExceptions = False instead, which handleError() checks before printing. - Test now uses sqlite_instance fixture instead of direct construction, sets logger level to INFO so the log line actually emits, and asserts no "Logging error" appears in stderr via capsys. Made-with: Cursor --- pyrit/memory/sqlite_memory.py | 6 ++++-- tests/unit/memory/test_sqlite_memory.py | 15 ++++++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/pyrit/memory/sqlite_memory.py b/pyrit/memory/sqlite_memory.py index 7035380250..f58e880328 100644 --- a/pyrit/memory/sqlite_memory.py +++ b/pyrit/memory/sqlite_memory.py @@ -346,10 +346,12 @@ def dispose_engine(self) -> None: """ if self.engine: self.engine.dispose() + previous_raise = logging.raiseExceptions + logging.raiseExceptions = False try: logger.info("Engine disposed and all connections closed.") - except (ValueError, OSError): - pass + finally: + logging.raiseExceptions = previous_raise def export_conversations( self, diff --git a/tests/unit/memory/test_sqlite_memory.py b/tests/unit/memory/test_sqlite_memory.py index 512020b9f4..1b118527f7 100644 --- a/tests/unit/memory/test_sqlite_memory.py +++ b/tests/unit/memory/test_sqlite_memory.py @@ -670,12 +670,14 @@ def test_get_conversation_stats_batches_multiple_conversations(sqlite_instance): assert result[conv_ids[2]].message_count == 3 -def test_dispose_engine_tolerates_closed_log_stream(): - """Verify dispose_engine does not raise when logging streams are already closed (GH-1520).""" +def test_dispose_engine_tolerates_closed_log_stream(sqlite_instance, capsys): + """Verify dispose_engine does not emit 'Logging error' when streams are closed (GH-1520).""" import io import logging - from pyrit.memory.sqlite_memory import SQLiteMemory + pyrit_logger = logging.getLogger("pyrit") + prev_level = pyrit_logger.level + pyrit_logger.setLevel(logging.INFO) stream = io.StringIO() handler = logging.StreamHandler(stream) @@ -683,8 +685,11 @@ def test_dispose_engine_tolerates_closed_log_stream(): root.addHandler(handler) try: - mem = SQLiteMemory(db_path=":memory:") stream.close() - mem.dispose_engine() + sqlite_instance.dispose_engine() finally: root.removeHandler(handler) + pyrit_logger.setLevel(prev_level) + + captured = capsys.readouterr() + assert "Logging error" not in captured.err From 1689bb81e307551c9e43aa592722d1da9e4b4d9c Mon Sep 17 00:00:00 2001 From: Richard Lundeen Date: Sat, 21 Mar 2026 13:54:25 -0700 Subject: [PATCH 3/3] Simplify shutdown logging fix and extend to AzureSQLMemory - Use logging.raiseExceptions pattern in both SQLiteMemory and AzureSQLMemory dispose_engine() to suppress logging errors when handler streams are already closed during interpreter shutdown - Move inline imports to module level in the test file - Add comment explaining why raiseExceptions is needed (logging framework catches ValueError internally, so try/except at the call site does not help) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pyrit/memory/azure_sql_memory.py | 10 +++++++++- pyrit/memory/sqlite_memory.py | 3 +++ tests/unit/memory/test_sqlite_memory.py | 7 +++---- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/pyrit/memory/azure_sql_memory.py b/pyrit/memory/azure_sql_memory.py index c9f349c0d9..6782cddc4c 100644 --- a/pyrit/memory/azure_sql_memory.py +++ b/pyrit/memory/azure_sql_memory.py @@ -640,7 +640,15 @@ def dispose_engine(self) -> None: """ if self.engine: self.engine.dispose() - logger.info("Engine disposed successfully.") + # During interpreter shutdown, logging handler streams may already be closed, + # causing the framework to print "Logging error" to stderr (GH-1520). + # Temporarily suppress logging errors for this teardown message. + previous_raise = logging.raiseExceptions + logging.raiseExceptions = False + try: + logger.info("Engine disposed successfully.") + finally: + logging.raiseExceptions = previous_raise def get_all_embeddings(self) -> Sequence[EmbeddingDataEntry]: """ diff --git a/pyrit/memory/sqlite_memory.py b/pyrit/memory/sqlite_memory.py index f58e880328..d5a521700d 100644 --- a/pyrit/memory/sqlite_memory.py +++ b/pyrit/memory/sqlite_memory.py @@ -346,6 +346,9 @@ def dispose_engine(self) -> None: """ if self.engine: self.engine.dispose() + # During interpreter shutdown, logging handler streams may already be closed, + # causing the framework to print "Logging error" to stderr (GH-1520). + # Temporarily suppress logging errors for this teardown message. previous_raise = logging.raiseExceptions logging.raiseExceptions = False try: diff --git a/tests/unit/memory/test_sqlite_memory.py b/tests/unit/memory/test_sqlite_memory.py index 1b118527f7..d174d58a19 100644 --- a/tests/unit/memory/test_sqlite_memory.py +++ b/tests/unit/memory/test_sqlite_memory.py @@ -1,6 +1,8 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT license. +import io +import logging import os import uuid from collections.abc import Sequence @@ -671,10 +673,7 @@ def test_get_conversation_stats_batches_multiple_conversations(sqlite_instance): def test_dispose_engine_tolerates_closed_log_stream(sqlite_instance, capsys): - """Verify dispose_engine does not emit 'Logging error' when streams are closed (GH-1520).""" - import io - import logging - + """Verify dispose_engine does not raise or emit 'Logging error' when streams are closed (GH-1520).""" pyrit_logger = logging.getLogger("pyrit") prev_level = pyrit_logger.level pyrit_logger.setLevel(logging.INFO)