Skip to content

Commit 23044b9

Browse files
manavgupclaude
andcommitted
fix: Resolve global state management and memory leak in enhanced logging
Addresses critical issues identified in PR #463 review: 1. **Global State Management Risk** (Lines 30-32, 72-99) - Removed global handler caching (_file_handler, _text_handler, _storage_handler) - Renamed _get_file_handler() → _create_file_handler() to reflect fresh creation - Renamed _get_text_handler() → _create_text_handler() - Each initialize() call now creates new handlers with current config - Fixes test isolation and allows config changes to take effect 2. **Memory Leak in StorageHandler** (Lines 118-235) - Removed self.loop instance variable that held permanent event loop reference - emit() now gets fresh event loop reference on each invocation - Added proper fallback: get_running_loop() → asyncio.get_event_loop() - Added closed-loop checking before use - Prevents memory leak from stale loop references 3. **Linting Fixes** - Removed unused AbstractEventLoop import - Added explicit type hint for handler variable (mypy compliance) - All linting checks now pass (ruff + mypy) **Testing**: - ✅ All 28 unit tests passing - ✅ Ruff linting: 0 errors - ✅ MyPy type checking: 0 errors Fixes issues raised in: #463 (comment) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 15d8036 commit 23044b9

File tree

1 file changed

+64
-68
lines changed

1 file changed

+64
-68
lines changed

backend/core/enhanced_logging.py

Lines changed: 64 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -20,17 +20,12 @@
2020
import logging
2121
import logging.handlers
2222
import os
23-
from asyncio import AbstractEventLoop, get_running_loop
23+
from asyncio import get_running_loop
2424

2525
from pythonjsonlogger import jsonlogger
2626

2727
from core.log_storage_service import LogLevel, LogStorageService
2828

29-
# Global handlers will be created lazily
30-
_file_handler: logging.Handler | None = None
31-
_text_handler: logging.StreamHandler | None = None
32-
_storage_handler: logging.Handler | None = None
33-
3429
# Text formatter
3530
_text_formatter = logging.Formatter(
3631
"[%(asctime)s] %(levelname)-8s %(name)s: %(message)s",
@@ -43,16 +38,16 @@
4338
)
4439

4540

46-
def _get_file_handler(
47-
log_file: str = "rag_modulo.log",
48-
log_folder: str | None = "logs",
49-
log_rotation_enabled: bool = True,
50-
log_max_size_mb: int = 10,
51-
log_backup_count: int = 5,
52-
log_filemode: str = "a",
53-
log_format: str = "json",
41+
def _create_file_handler(
42+
log_file: str,
43+
log_folder: str | None,
44+
log_rotation_enabled: bool,
45+
log_max_size_mb: int,
46+
log_backup_count: int,
47+
log_filemode: str,
48+
log_format: str,
5449
) -> logging.Handler:
55-
"""Get or create the file handler.
50+
"""Create a new file handler with the specified configuration.
5651
5752
Args:
5853
log_file: Log filename
@@ -67,49 +62,46 @@ def _get_file_handler(
6762
logging.Handler: Either a RotatingFileHandler or regular FileHandler
6863
6964
Raises:
70-
ValueError: If file logging is disabled or no log file specified
65+
ValueError: If no log file specified
7166
"""
72-
global _file_handler
73-
if _file_handler is None:
74-
if not log_file:
75-
raise ValueError("No log file specified")
76-
77-
# Ensure log folder exists
78-
if log_folder:
79-
os.makedirs(log_folder, exist_ok=True)
80-
log_path = os.path.join(log_folder, log_file)
81-
else:
82-
log_path = log_file
83-
84-
# Create appropriate handler based on rotation settings
85-
if log_rotation_enabled:
86-
max_bytes = log_max_size_mb * 1024 * 1024 # Convert MB to bytes
87-
_file_handler = logging.handlers.RotatingFileHandler(
88-
log_path, maxBytes=max_bytes, backupCount=log_backup_count, mode=log_filemode
89-
)
90-
else:
91-
_file_handler = logging.FileHandler(log_path, mode=log_filemode)
92-
93-
# Set formatter based on format type
94-
if log_format == "json":
95-
_file_handler.setFormatter(_json_formatter)
96-
else:
97-
_file_handler.setFormatter(_text_formatter)
98-
99-
return _file_handler
100-
101-
102-
def _get_text_handler() -> logging.StreamHandler:
103-
"""Get or create the text handler for console output.
67+
if not log_file:
68+
raise ValueError("No log file specified")
69+
70+
# Ensure log folder exists
71+
if log_folder:
72+
os.makedirs(log_folder, exist_ok=True)
73+
log_path = os.path.join(log_folder, log_file)
74+
else:
75+
log_path = log_file
76+
77+
# Create appropriate handler based on rotation settings
78+
handler: logging.Handler
79+
if log_rotation_enabled:
80+
max_bytes = log_max_size_mb * 1024 * 1024 # Convert MB to bytes
81+
handler = logging.handlers.RotatingFileHandler(
82+
log_path, maxBytes=max_bytes, backupCount=log_backup_count, mode=log_filemode
83+
)
84+
else:
85+
handler = logging.FileHandler(log_path, mode=log_filemode)
86+
87+
# Set formatter based on format type
88+
if log_format == "json":
89+
handler.setFormatter(_json_formatter)
90+
else:
91+
handler.setFormatter(_text_formatter)
92+
93+
return handler
94+
95+
96+
def _create_text_handler() -> logging.StreamHandler:
97+
"""Create a new text handler for console output.
10498
10599
Returns:
106100
logging.StreamHandler: The stream handler for console logging
107101
"""
108-
global _text_handler
109-
if _text_handler is None:
110-
_text_handler = logging.StreamHandler()
111-
_text_handler.setFormatter(_text_formatter)
112-
return _text_handler
102+
handler = logging.StreamHandler()
103+
handler.setFormatter(_text_formatter)
104+
return handler
113105

114106

115107
class StorageHandler(logging.Handler):
@@ -123,7 +115,6 @@ def __init__(self, storage_service: LogStorageService) -> None:
123115
"""
124116
super().__init__()
125117
self.storage = storage_service
126-
self.loop: AbstractEventLoop | None = None
127118

128119
def emit(self, record: logging.LogRecord) -> None:
129120
"""Emit a log record to storage.
@@ -207,17 +198,23 @@ def emit(self, record: logging.LogRecord) -> None:
207198

208199
# Store the log asynchronously
209200
try:
210-
# Get or create event loop
211-
if not self.loop:
201+
# Get fresh event loop reference each time (avoid memory leak)
202+
import asyncio
203+
204+
try:
205+
loop = get_running_loop()
206+
except RuntimeError:
207+
# No running loop in this thread - try to get the default event loop
212208
try:
213-
self.loop = get_running_loop()
209+
loop = asyncio.get_event_loop()
210+
if loop.is_closed():
211+
# Loop is closed, can't store
212+
return
214213
except RuntimeError:
215-
# No running loop, can't store
214+
# No event loop available
216215
return
217216

218217
# Schedule the coroutine
219-
import asyncio
220-
221218
asyncio.run_coroutine_threadsafe(
222219
self.storage.add_log(
223220
level=log_level,
@@ -232,7 +229,7 @@ def emit(self, record: logging.LogRecord) -> None:
232229
execution_time_ms=execution_time_ms,
233230
data=data,
234231
),
235-
self.loop,
232+
loop,
236233
)
237234
except Exception:
238235
# Silently fail to avoid logging recursion
@@ -300,14 +297,14 @@ async def initialize(
300297
root_logger.setLevel(log_level_value)
301298

302299
# Always add console/text handler for stdout/stderr
303-
text_handler = _get_text_handler()
300+
text_handler = _create_text_handler()
304301
text_handler.setLevel(log_level_value)
305302
root_logger.addHandler(text_handler)
306303

307304
# Add file handler if enabled
308305
if log_to_file and log_file:
309306
try:
310-
file_handler = _get_file_handler(
307+
file_handler = _create_file_handler(
311308
log_file=log_file,
312309
log_folder=log_folder,
313310
log_rotation_enabled=log_rotation_enabled,
@@ -336,11 +333,10 @@ async def initialize(
336333
self._storage = LogStorageService(max_size_mb=log_buffer_size_mb)
337334

338335
# Add storage handler to capture all logs
339-
global _storage_handler
340-
_storage_handler = StorageHandler(self._storage)
341-
_storage_handler.setFormatter(_text_formatter)
342-
_storage_handler.setLevel(log_level_value)
343-
root_logger.addHandler(_storage_handler)
336+
storage_handler = StorageHandler(self._storage)
337+
storage_handler.setFormatter(_text_formatter)
338+
storage_handler.setLevel(log_level_value)
339+
root_logger.addHandler(storage_handler)
344340

345341
logging.info(f"Log storage initialized with {log_buffer_size_mb}MB buffer")
346342

0 commit comments

Comments
 (0)