-
Notifications
You must be signed in to change notification settings - Fork 516
Fix infinite recursion in InterceptHandler logging #940
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix infinite recursion in InterceptHandler logging #940
Conversation
📝 WalkthroughWalkthroughThe InterceptHandler.emit() was reworked to stop re-logging through module-specific loggers; it now prefixes the incoming LogRecord message with the module name, clears Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-10-13T16:40:22.622ZApplied to files:
📚 Learning: 2025-10-01T20:44:19.146ZApplied to files:
🔇 Additional comments (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/app/logging/setup_logging.py (1)
246-246: Consider using component_name instead of hardcoded "[uvicorn]".The handler stores
component_namein__init__but uses a hardcoded"[uvicorn]"prefix. For consistency and flexibility, consider usingself.component_nameinstead. This would make the handler more generic and reusable across different components.🔎 Proposed refactor
- record.msg = f"[uvicorn] {msg}" + record.msg = f"[{self.component_name}] {msg}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/logging/setup_logging.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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
🔇 Additional comments (1)
backend/app/logging/setup_logging.py (1)
249-252: Good fix for the recursion issue!The approach of directly calling
handler.handle(record)on root handlers while skippingselfcorrectly prevents the infinite recursion that was causing the RecursionError. This follows Python logging best practices by avoiding re-entry into the same handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/logging/setup_logging.py (1)
221-229: Consider removing or utilizing the unusedcomponent_nameparameter.The
component_nameparameter is stored inself.component_namebut never referenced in theemit()method. You could either:
- Remove the parameter if it's not needed, or
- Use it to create dynamic prefixes (as suggested in the previous comment)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/logging/setup_logging.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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
📚 Learning: 2025-10-15T07:13:34.502Z
Learnt from: Hemil36
Repo: AOSSIE-Org/PictoPy PR: 548
File: backend/app/database/images.py:115-115
Timestamp: 2025-10-15T07:13:34.502Z
Learning: In the PictoPy backend and sync-microservice, exception logging using `logger.error(f"...")` without stack traces (i.e., without `exc_info=True` or `logger.exception()`) is acceptable for console logging, as this is a deliberate design decision made with the maintainer for console-only logs.
Applied to files:
backend/app/logging/setup_logging.py
rahulharpal1603
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply the same change in sync-microservice/app/logging/setup_logging.py
|
@rahulharpal1603 I’ve applied the same changes to |
Fixes #918
What this PR does
: Fixes infinite recursion in InterceptHandler.emit
: Prevents re-entering the same logging handler
: Safely forwards intercepted Uvicorn log records to root handlers
: Ensures each log record is processed exactly once
Context
When running PictoPy (especially on Python 3.12 and Windows), backend startup could crash with a RecursionError.
This happened because InterceptHandler.emit() was re-logging intercepted records using logger.log(...), where the logger was configured with the same InterceptHandler.
As a result, the handler kept calling itself indefinitely until Python hit the maximum recursion depth.
Solution
This change avoids re-logging entirely.
Instead of emitting a new log via logger.log(), the handler:
This follows Python logging best practices and guarantees there is no recursion.
Tested
: Backend starts successfully using python main.py
: Backend runs correctly with fastapi dev
: Sync microservice starts without errors
: Verified on Windows + Python 3.12
: Confirmed working in Tauri dev flow
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.