Skip to content

fix: prevent Telegram media group exceptions from being silently swallowed#7537

Open
he-yufeng wants to merge 1 commit intoAstrBotDevs:masterfrom
he-yufeng:fix/telegram-media-group-exception
Open

fix: prevent Telegram media group exceptions from being silently swallowed#7537
he-yufeng wants to merge 1 commit intoAstrBotDevs:masterfrom
he-yufeng:fix/telegram-media-group-exception

Conversation

@he-yufeng
Copy link
Copy Markdown
Contributor

@he-yufeng he-yufeng commented Apr 14, 2026

Summary

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 usually not configured in AstrBot. 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 logger with full traceback.
  • Register an EVENT_JOB_ERROR listener on the scheduler as a safety net for any future scheduled job that throws.

Test plan

  • Send a media group (album) to a Telegram bot and verify it processes normally
  • Simulate a convert_message() failure (e.g. network timeout) and verify the error appears in AstrBot logs instead of being swallowed

Fixes #7512

Summary by Sourcery

Improve visibility of exceptions raised by Telegram media group processing jobs.

Bug Fixes:

  • Ensure exceptions in Telegram media group processing are logged by AstrBot instead of being silently swallowed by the scheduler.

Enhancements:

  • Add a scheduler-level EVENT_JOB_ERROR listener to log any future scheduled job failures with job ID and exception details.

…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
@auto-assign auto-assign bot requested review from Fridemn and advent259141 April 14, 2026 09:32
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. area:platform The bug / feature is about IM platform adapter, such as QQ, Lark, Telegram, WebChat and so on. labels Apr 14, 2026
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.
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.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +84 to +89
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,
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:platform The bug / feature is about IM platform adapter, such as QQ, Lark, Telegram, WebChat and so on. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Telegram media group (album) 消息在 process_media_group 中异常被 APScheduler 静默吞掉,无任何日志输出

1 participant