fix: prevent Telegram media group exceptions from being silently swallowed#7537
fix: prevent Telegram media group exceptions from being silently swallowed#7537he-yufeng wants to merge 1 commit intoAstrBotDevs:masterfrom
Conversation
…lowed process_media_group() is invoked by APScheduler via add_job(). If convert_message() or handle_msg() raises (e.g. get_file() network timeout, file download failure), APScheduler catches the exception internally and only logs it through its own logger, which is often not configured in AstrBot. The result is that the media group silently disappears with no trace in the application logs. Two changes: - Wrap the body of process_media_group() in try/except so failures are logged through AstrBot's own logger with full traceback. - Register an EVENT_JOB_ERROR listener on the scheduler as a safety net, so any future scheduled job that throws will also surface in the logs. Fixes AstrBotDevs#7512
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The scheduler error listener is defined inline as a lambda, which makes it harder to reuse or unit test; consider extracting it into a small named method so the logging logic is easier to find and maintain.
- In
process_media_group, the broadexcept Exceptioncurrently swallows the error after logging; if callers should be aware of failures (e.g., for retries or higher-level error handling), consider re-raising after logging or at least making the intentional swallowing explicit in a comment. - When logging
EVENT_JOB_ERROR,ev.exceptionmay beNonefor some error types; defensively guard or format the log message to handle that case gracefully instead of assuming a non-null exception.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The scheduler error listener is defined inline as a lambda, which makes it harder to reuse or unit test; consider extracting it into a small named method so the logging logic is easier to find and maintain.
- In `process_media_group`, the broad `except Exception` currently swallows the error after logging; if callers should be aware of failures (e.g., for retries or higher-level error handling), consider re-raising after logging or at least making the intentional swallowing explicit in a comment.
- When logging `EVENT_JOB_ERROR`, `ev.exception` may be `None` for some error types; defensively guard or format the log message to handle that case gracefully instead of assuming a non-null exception.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request improves error handling in the Telegram adapter by adding a listener for scheduled job errors and wrapping media group processing in a try-except block. Feedback suggests refactoring the job error listener from a lambda to a named method to improve readability and more accurately capture stack traces using the event's traceback attribute.
| self.scheduler.add_listener( | ||
| lambda ev: logger.error( | ||
| "Scheduled job %s raised: %s", ev.job_id, ev.exception, exc_info=ev.exception | ||
| ), | ||
| EVENT_JOB_ERROR, | ||
| ) |
There was a problem hiding this comment.
While using a lambda for the error listener is functional, it's generally better practice to use a named method for event listeners in classes. This improves readability and makes it easier to manage the listener if needed in the future. Additionally, consider explicitly passing the traceback to exc_info using a tuple (type(ev.exception), ev.exception, ev.traceback) to ensure the stack trace is correctly captured, as APScheduler provides the traceback object specifically for this purpose.
| self.scheduler.add_listener( | |
| lambda ev: logger.error( | |
| "Scheduled job %s raised: %s", ev.job_id, ev.exception, exc_info=ev.exception | |
| ), | |
| EVENT_JOB_ERROR, | |
| ) | |
| self.scheduler.add_listener(self._on_job_error, EVENT_JOB_ERROR) | |
| self._terminating = False | |
| self._loop: asyncio.AbstractEventLoop | None = None | |
| self._polling_recovery_requested = asyncio.Event() | |
| def _on_job_error(self, event): | |
| logger.error( | |
| "Scheduled job %s raised: %s", | |
| event.job_id, | |
| event.exception, | |
| exc_info=(type(event.exception), event.exception, event.traceback) | |
| ) |
Summary
process_media_group()is invoked by APScheduler viaadd_job(). Ifconvert_message()orhandle_msg()raises (e.g.get_file()network timeout, file download failure), APScheduler catches the exception internally and only logs it through its own logger -- which is usually not configured in AstrBot. The media group silently disappears with no trace in the application logs.Two changes:
process_media_group()in try/except so failures are logged through AstrBot's logger with full traceback.EVENT_JOB_ERRORlistener on the scheduler as a safety net for any future scheduled job that throws.Test plan
convert_message()failure (e.g. network timeout) and verify the error appears in AstrBot logs instead of being swallowedFixes #7512
Summary by Sourcery
Improve visibility of exceptions raised by Telegram media group processing jobs.
Bug Fixes:
Enhancements: