From 82876277ffacd2d000a450e9a2967cc2d97989b6 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Sun, 18 Oct 2026 09:24:57 -0700 Subject: [PATCH] fix(logging): keep retired file handlers closed (#218) --- tests/test_utils_logging_config.py | 60 ++++++++++++++++++++++++++++++ utils/logging_config.py | 11 +++++- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/tests/test_utils_logging_config.py b/tests/test_utils_logging_config.py index a462c86..286c1cf 100644 --- a/tests/test_utils_logging_config.py +++ b/tests/test_utils_logging_config.py @@ -235,6 +235,66 @@ class TestFilesAndRotation(IsolatedLoggerTestCase): class TestConfiguration(IsolatedLoggerTestCase): + def test_in_flight_record_cannot_reopen_retired_file_handler(self) -> None: + handler = next( + h + for h in self.manager.logger.handlers + if isinstance(h, logging.FileHandler) + ) + stream = handler.stream + selected = threading.Event() + resume = threading.Event() + errors = [] + message = "in-flight record selected before file logging disabled" + + def pause_before_handler_lock(record: logging.LogRecord) -> bool: + if record.getMessage() == message: + selected.set() + if not resume.wait(5): + raise RuntimeError("logging thread was not released") + return True + + def log_from_background_thread() -> None: + try: + self.manager.logger.info(message) + except BaseException as error: + errors.append(error) + + # Allow this record through the fixture's normal main-thread filter. + handler.filters.clear() + handler.addFilter(pause_before_handler_lock) + worker = threading.Thread(target=log_from_background_thread, daemon=True) + worker.start() + try: + self.assertTrue(selected.wait(5), "logger did not select the file handler") + self.assertFalse(stream.closed) + self.manager.update_config({"file_logging": False}) + self.assertNotIn(handler, self.manager.logger.handlers) + self.assertTrue(stream.closed) + self.assertIsNone(handler.stream) + + resume.set() + worker.join(5) + self.assertFalse(worker.is_alive(), "logging thread did not finish") + self.assertEqual(errors, []) + self.assertIsNone(handler.stream) + self.assertTrue(stream.closed) + self.assertFalse(self.manager.config["file_logging"]) + self.assertFalse( + any( + isinstance(h, logging.FileHandler) + for h in self.manager.logger.handlers + ) + ) + self.assertNotIn( + message, + pathlib.Path(self.log_dir, "prompt_manager.log").read_text(), + ) + finally: + resume.set() + worker.join(5) + handler.close() + def test_rebuild_closes_replaced_file_stream(self) -> None: old_handler = next( h diff --git a/utils/logging_config.py b/utils/logging_config.py index a084417..d9a6286 100644 --- a/utils/logging_config.py +++ b/utils/logging_config.py @@ -19,6 +19,15 @@ from typing import Dict, Any, Optional, List from pathlib import Path +class _OwnedRotatingFileHandler(logging.handlers.RotatingFileHandler): + """Keep a retired owned sink closed when an in-flight record resumes.""" + + def emit(self, record: logging.LogRecord) -> None: + # Handler.handle and FileHandler.close hold the same handler lock. + if not self._closed: + super().emit(record) + + class PromptManagerLogger: """ Centralized logging system for PromptManager. @@ -112,7 +121,7 @@ class PromptManagerLogger: # File handler with rotation and safe encoding for Windows if self.config["file_logging"]: log_file = self.log_dir / "prompt_manager.log" - file_handler = logging.handlers.RotatingFileHandler( + file_handler = _OwnedRotatingFileHandler( log_file, maxBytes=self.config["max_file_size"], backupCount=self.config["backup_count"],