fix(logging): prevent infinite recursion in InterceptHandler fixes:#708#792
fix(logging): prevent infinite recursion in InterceptHandler fixes:#708#792VasuS609 wants to merge 8 commits intoAOSSIE-Org:mainfrom
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
📝 WalkthroughWalkthroughUpdated InterceptHandler.emit in backend and sync-microservice to stop mutating incoming LogRecord by constructing a new LogRecord and forwarding it directly to root logger handlers (skipping InterceptHandler/self) with defensive error handling; tightened OpenAPI: wrapped Changes
Sequence Diagram(s)sequenceDiagram
participant AppLogger as Logger
participant Intercept as InterceptHandler
participant Root as RootLogger
participant H1 as RootHandler1
participant Hn as RootHandlerN
AppLogger->>Intercept: emit(original_record)
rect rgba(173,216,230,0.5)
Intercept->>Intercept: build new_record (prefix module, copy timing)
end
Intercept->>Root: forward new_record
Root->>H1: handler.handle(new_record)
Root->>Hn: handler.handle(new_record)
note right of Hn: handler exceptions are caught and ignored
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
sync-microservice/app/logging/setup_logging.py (2)
253-263: Removeexc_infoandstack_infofrom the new LogRecord.Based on learnings, stack traces should not be shown in logs when rerouting through
InterceptHandler. The current implementation preservesexc_infoandstack_info, which contradicts this requirement.Apply this diff:
new_record = logging.LogRecord( name=module_name, level=record.levelno, pathname=record.pathname, lineno=record.lineno, msg=f"[uvicorn] {msg}", args=(), - exc_info=record.exc_info, + exc_info=None, func=record.funcName, - sinfo=record.stack_info, + sinfo=None, )
258-258: Consider using a dynamic prefix based on the source logger.The
[uvicorn]prefix is hardcoded, but this handler also interceptsasynciologs (see line 316-319). Consider using the original logger name ormodule_namefor the prefix to avoid misleading log output.- msg=f"[uvicorn] {msg}", + msg=f"[{module_name}] {msg}",backend/app/logging/setup_logging.py (2)
248-258: Removeexc_infoandstack_infofrom the new LogRecord.Same issue as in
sync-microservice/app/logging/setup_logging.py. Based on learnings, stack traces should not be shown in logs when rerouting throughInterceptHandler.Apply this diff:
new_record = logging.LogRecord( name=module_name, level=record.levelno, pathname=record.pathname, lineno=record.lineno, msg=f"[uvicorn] {msg}", args=(), - exc_info=record.exc_info, + exc_info=None, func=record.funcName, - sinfo=record.stack_info, + sinfo=None, )
212-274: Consider extracting sharedInterceptHandlerto reduce duplication.The
InterceptHandlerclass is nearly identical betweenbackend/app/logging/setup_logging.pyandsync-microservice/app/logging/setup_logging.py. This duplication increases maintenance burden and risks the implementations drifting apart.Consider moving
InterceptHandlerto a shared module underutils/logging/that both services can import.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/logging/setup_logging.py(1 hunks)docs/backend/backend_python/openapi.json(1 hunks)sync-microservice/app/logging/setup_logging.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T16:40:22.622Z
Learnt from: Hemil36
Repo: AOSSIE-Org/PictoPy PR: 548
File: backend/app/logging/setup_logging.py:224-243
Timestamp: 2025-10-13T16:40:22.622Z
Learning: In the PictoPy project's centralized logging system (backend/app/logging/setup_logging.py and sync-microservice/app/logging/setup_logging.py), stack traces should not be shown in logs. When rerouting logs through InterceptHandler, exc_info and stack_info should not be preserved.
Applied to files:
backend/app/logging/setup_logging.pysync-microservice/app/logging/setup_logging.py
🧬 Code graph analysis (2)
backend/app/logging/setup_logging.py (1)
sync-microservice/app/logging/setup_logging.py (1)
InterceptHandler(220-277)
sync-microservice/app/logging/setup_logging.py (1)
backend/app/logging/setup_logging.py (1)
InterceptHandler(212-274)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (3)
sync-microservice/app/logging/setup_logging.py (1)
270-277: Recursion prevention approach looks correct.The strategy of dispatching directly to root handlers while skipping
InterceptHandlerinstances effectively breaks the recursion loop. The identity check (handler is not self) combined with the type check provides robust protection.backend/app/logging/setup_logging.py (1)
265-274: Recursion prevention implementation is sound.The approach of routing directly to root handlers while skipping
InterceptHandlerinstances correctly eliminates the infinite recursion. The silent exception handling prevents handler failures from crashing the application.docs/backend/backend_python/openapi.json (1)
1120-1127: Schema wrapping change looks correct.The
allOfwrapper around the$refis a standard OpenAPI pattern for adding metadata (liketitleanddefault) to a referenced schema. This is typically auto-generated by FastAPI/Pydantic and doesn't change the API behavior.Confirm this file is auto-generated (e.g., via
openapi.jsonexport from FastAPI) and not manually edited, to ensure consistency with the actual API schema.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/app/logging/setup_logging.py (1)
300-302: Consider removing unused formatter configuration.The formatter configured on the InterceptHandler (lines 300-302) is never used because
emit()bypasses the handler's formatting pipeline by sending records directly to root logger handlers (lines 266-271). This code can be safely removed to improve clarity.Apply this diff to remove the unused formatter setup:
# Create an intercept handler with our component name intercept_handler = InterceptHandler(component_name) - # Make sure the handler uses our ColorFormatter - config = load_config() - component_config = config.get("components", {}).get( - component_name, {"prefix": component_name.upper(), "color": "white"} - ) - level_colors = config.get("colors", {}) - env_settings = config.get("environments", {}).get( - os.environ.get("ENV", config.get("default_environment", "development")), {} - ) - use_colors = env_settings.get("colored_output", True) - - fmt = "[%(component)s] | %(module)s | %(levelname)s | %(message)s" - formatter = ColorFormatter(fmt, component_config, level_colors, use_colors) - intercept_handler.setFormatter(formatter) - # Configure Uvicorn loggers to use our handler for logger_name in ["uvicorn", "uvicorn.error", "uvicorn.access"]:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/logging/setup_logging.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T16:40:22.622Z
Learnt from: Hemil36
Repo: AOSSIE-Org/PictoPy PR: 548
File: backend/app/logging/setup_logging.py:224-243
Timestamp: 2025-10-13T16:40:22.622Z
Learning: In the PictoPy project's centralized logging system (backend/app/logging/setup_logging.py and sync-microservice/app/logging/setup_logging.py), stack traces should not be shown in logs. When rerouting logs through InterceptHandler, exc_info and stack_info should not be preserved.
Applied to files:
backend/app/logging/setup_logging.py
🧬 Code graph analysis (1)
backend/app/logging/setup_logging.py (1)
sync-microservice/app/logging/setup_logging.py (1)
InterceptHandler(220-277)
…o drop exc_info/sinfo
2 participants
This PR fixes a
RecursionErrorthat occurs when Uvicorn (or other stdlib loggers) emit logs, and theInterceptHandlertries to re-log them.What was happening?
The handler called
logger.log(...)insideemit(), which triggered the same handler again → infinite loop → crash.How is it fixed?
[uvicorn].InterceptHandlerinstances to avoid re-entrancy.This approach:
Tested locally — the backend now starts without crashing, and Uvicorn logs appear as expected.
Fixes the issue described in #708
Summary by CodeRabbit
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.