fix: handle closed logging streams in dispose_engine shutdown#1526
Open
2byrds wants to merge 5 commits intomicrosoft:mainfrom
Open
fix: handle closed logging streams in dispose_engine shutdown#15262byrds wants to merge 5 commits intomicrosoft:mainfrom
2byrds wants to merge 5 commits intomicrosoft:mainfrom
Conversation
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 microsoft#1520 Made-with: Cursor
Author
|
@microsoft-github-policy-service agree company="ClearEdge IT Solutions, LLC" |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make SQLiteMemory.dispose_engine() safe to run during interpreter shutdown by preventing shutdown-time logging failures when logging streams/handlers have already been closed (issue #1520).
Changes:
- Wraps
SQLiteMemory.dispose_engine()’s informational log line in atry/except (ValueError, OSError)block. - Adds a unit regression test intended to reproduce the closed-log-stream shutdown scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pyrit/memory/sqlite_memory.py |
Attempts to guard shutdown-time logger.info(...) against failures when streams are closed. |
tests/unit/memory/test_sqlite_memory.py |
Adds a regression test for disposing with a closed logging stream. |
…afety 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
- 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>
rlundeen2
approved these changes
Mar 21, 2026
Contributor
|
Thx @2byrds! I made some small changes but will merge once gates finish |
Author
Thank you @rlundeen2 we appreciate this project! |
Author
i saw the upstream branch had received changes so i updated the pr @rlundeen2 |
Author
|
i continue to keep this updated with upstream @rlundeen2 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
logger.infocall inSQLiteMemory.dispose_engine()withtry/except (ValueError, OSError)to handle closed logging streams during interpreter shutdowndispose_engine()Closes #1520
Root cause
MemoryInterface.cleanup()registersdispose_enginevia bothatexit.registerandweakref.finalize. During interpreter shutdown, these callbacks can fire after other code (test runners, application cleanup) has already closed logging handler streams, causingValueError: I/O operation on closed file.Fix
The standard CPython pattern for atexit/finalizer logging: guard the log call with
try/exceptsince module globals and I/O streams may be unavailable during teardown. The fix is intentionally minimal — it does not restructure the cleanup registration, only makes the existing logging call shutdown-safe.Test plan
pytest tests/unit/memory/test_sqlite_memory.py::test_dispose_engine_tolerates_closed_log_streampassestest_sqlite_memory.pytests still passMade with Cursor