fix: close plugin handlers after draining QueueListener in LoggerManager.stop()#4137
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughLoggerManager.stop() now stops the QueueListener and then explicitly flushes (exceptions ignored) and closes each plugin LogHandlerBase in self.plugin_handlers. A new test verifies plugin handlers are flushed/closed and that queued records are delivered before shutdown. ChangesLogging stop / plugin handler shutdown
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant LM as LoggerManager
participant Q as Queue
participant QL as QueueListener
participant PH as PluginHandler
Test->>LM: create LoggerManager (skip_plugin_handlers=True)
Test->>QL: start QueueListener (listening Q)
Test->>Q: put log record (via QueueHandler)
Test->>LM: call stop()
LM->>QL: stop() / drain queue
QL->>Q: read record
QL->>PH: emit(record)
PH->>PH: flush()
PH->>PH: close()
LM->>Test: return (shutdown complete)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
…ger.stop() Plugin handlers are attached to the QueueListener, not the logger itself, so they were never explicitly closed during shutdown. This could leak file handles and lose buffered data in logger plugins that write to files. The fix adds an explicit close loop for plugin_handlers after the queue is drained but before the logger's own handlers are cleaned up. Closes snakemake#4136
f89fad9 to
640c58f
Compare
cademirch
approved these changes
Mar 28, 2026
7 tasks
Contributor
|
Beat me to it. I think |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/snakemake/logging.py`:
- Around line 746-753: The loop closing plugin handlers can stop early if
handler.close() raises and currently only flush() is wrapped; update the cleanup
in the class that iterates self.plugin_handlers to wrap both handler.flush() and
handler.close() in try/except blocks (optionally logging exceptions at DEBUG) so
every handler is attempted even on errors, mirroring logging.shutdown()
behavior—locate the code that calls handler.flush() / handler.close() on items
in self.plugin_handlers and add separate try/except around close() (and improve
the existing bare except to log.debug(...) instead of silent pass if you want
diagnostics).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b01cecea-0f97-48ee-a6ca-7027093f943f
📒 Files selected for processing (2)
src/snakemake/logging.pytests/test_logging.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_logging.py
Comment on lines
+746
to
+753
| # Flush and close plugin handlers managed by the QueueListener | ||
| # (not attached to the logger). | ||
| for handler in self.plugin_handlers: | ||
| try: | ||
| handler.flush() | ||
| except Exception: | ||
| pass | ||
| handler.close() |
Contributor
There was a problem hiding this comment.
Wrap close() in try-except to ensure all handlers are cleaned up.
If handler.close() raises an exception, remaining handlers in self.plugin_handlers won't be closed, potentially leaving resource leaks. This mirrors how logging.shutdown() wraps both flush and close in exception handlers.
Additionally, the static analysis flags the bare except Exception: pass — consider logging at DEBUG level for diagnostic visibility during shutdown issues, though silently ignoring is acceptable in cleanup code.
🛡️ Proposed fix to wrap close() and optionally log flush errors
# Flush and close plugin handlers managed by the QueueListener
# (not attached to the logger).
for handler in self.plugin_handlers:
try:
handler.flush()
- except Exception:
- pass
- handler.close()
+ except Exception:
+ pass # Ignore flush errors during shutdown
+ try:
+ handler.close()
+ except Exception:
+ pass # Ensure remaining handlers still get closed🧰 Tools
🪛 Ruff (0.15.9)
[error] 751-752: try-except-pass detected, consider logging the exception
(S110)
[warning] 751-751: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/snakemake/logging.py` around lines 746 - 753, The loop closing plugin
handlers can stop early if handler.close() raises and currently only flush() is
wrapped; update the cleanup in the class that iterates self.plugin_handlers to
wrap both handler.flush() and handler.close() in try/except blocks (optionally
logging exceptions at DEBUG) so every handler is attempted even on errors,
mirroring logging.shutdown() behavior—locate the code that calls handler.flush()
/ handler.close() on items in self.plugin_handlers and add separate try/except
around close() (and improve the existing bare except to log.debug(...) instead
of silent pass if you want diagnostics).
johanneskoester
pushed a commit
that referenced
this pull request
May 14, 2026
🤖 I have created a release *beep* *boop* --- ## [9.21.0](v9.20.0...v9.21.0) (2026-05-13) ### Features * add a function to help with prepending arguments to filenames; close [#672](#672) ([#4090](#4090)) ([14ccd1d](14ccd1d)) ### Bug Fixes * close plugin handlers after draining QueueListener in LoggerManager.stop() ([#4137](#4137)) ([b2a9e69](b2a9e69)) ### Performance Improvements * adjust default sqlite PRAGMAs, auto detect network fstype ([#4152](#4152)) ([3df2d35](3df2d35)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
7 tasks
cademirch
added a commit
that referenced
this pull request
Jun 11, 2026
<!--Add a description of your PR here--> Minor addendum to #4137: - Also flush all loggers attached to `manger.logger`, extend tests to cover this case. - Wrap all calls to `Handler.close()` in `try/except`. - For non-plugin loggers (attached to `manager.logger` directly), acquire lock before flushing/closing. Not necessary for plugin loggers because they are all feed events through a `QueueListener` and its thread has already been shut down. ### QC <!-- Make sure that you can tick the boxes below. --> * [x] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [x] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). * [x] I, as a human being, have checked each line of code in this pull request ### AI-assistance disclosure <!-- If AI tools were involved in creating this PR, please check all boxes that apply below and make sure that you adhere to our AI-assisted contributions policy: https://github.com/snakemake/snakemake/blob/main/docs/project_info/contributing.rst --> I used AI assistance for: * [ ] Code generation (e.g., when writing an implementation or fixing a bug) * [ ] Test/benchmark generation * [ ] Documentation (including examples) * [x] Research and understanding <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved logging shutdown so all log handlers are flushed and closed safely; failures in one handler no longer interrupt complete shutdown and teardown is more thread-safe. * **Tests** * Expanded tests to verify that all log handlers are flushed, closed, and receive expected records during shutdown. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Jared Lumpe <jared.lumpe@cepheid.com> Co-authored-by: Cade Mirchandani <cmirchan@ucsc.edu> Co-authored-by: Johannes Köster <johannes.koester@tu-dortmund.de>
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
3 participants
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
LoggerManager.stop(), leaking file handles and potentially losing buffered data in logger plugins that write to filesstop()Closes #4136
Test plan
test_stop_closes_plugin_handlersfails without the fix (RED) and passes with it (GREEN)test_logfile,test_issue4063,test_log_events_dryrun,test_log_events,test_rule_failure)Summary by CodeRabbit
Bug Fixes
Tests