fix(session_waiter): track handler task to prevent execution after timeout

[Qixuan112]

问题描述

在 第 167 行有一个 TODO 注释，指出需要使用 来跟踪任务，防止超时后 handler 仍然在执行。

解决方案

1. 添加任务跟踪: 在 类中添加 属性来跟踪正在运行的 handler 任务
2. 使用 create_task: 使用 包装 handler 执行，以便进行任务管理
3. 清理时取消任务: 在 方法中取消任何正在运行的 handler 任务
4. 防止任务堆积: 如果新的触发器到达时上一个任务还未完成，则取消上一个任务
5. 正确处理取消: 显式处理 以避免错误传播

改进点

• 防止会话超时后 handler 仍继续执行，造成资源泄漏
• 确保在会话结束时正确清理所有相关任务
• 避免并发触发导致的多个 handler 同时执行

测试

• ✅ Python 语法检查通过
• ✅ 代码与现有使用场景兼容
• ✅ 保持了原有的异常处理逻辑

相关 TODO

解决了 的 TODO 注释。

Summary by Sourcery

Track and manage session handler tasks to prevent handlers from running after session timeout.

Bug Fixes:

• Ensure session handlers do not continue executing after a session timeout, avoiding resource leaks and unintended processing.

Enhancements:

• Add per-session handler task tracking and cancellation to avoid overlapping handler executions and improve cleanup behavior on session end.

[dosubot]

📄 Knowledge review

Dosu skipped reviewing this PR because your organization has used its 200 included credits for the month. Your usage will reset on 2026-08-01. To have Dosu review this PR before then, ask your organization admin to upgrade to a pro account.

---

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• Consider clearing self._handler_task (e.g., setting it to None once the task is done) to avoid holding stale task references and make the task lifecycle easier to reason about.
• In _run_handler, exceptions are caught and session.session_controller.stop(e) is called, but since await session._handler_task also propagates exceptions to the outer except Exception, stop may be invoked twice; you may want to centralize the error handling to a single place to avoid duplicate stop calls.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Consider clearing `self._handler_task` (e.g., setting it to `None` once the task is done) to avoid holding stale task references and make the task lifecycle easier to reason about.
- In `_run_handler`, exceptions are caught and `session.session_controller.stop(e)` is called, but since `await session._handler_task` also propagates exceptions to the outer `except Exception`, `stop` may be invoked twice; you may want to centralize the error handling to a single place to avoid duplicate stop calls.

## Individual Comments

### Comment 1
<location path="astrbot/core/utils/session_waiter.py" line_range="124-125" />
<code_context>
         self._lock = asyncio.Lock()
         """需要保证一个 session 同时只有一个 trigger"""

+        self._handler_task: asyncio.Task | None = None
+        """当前正在运行的 handler 任务"""
+
     async def register_wait(
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider how CancelledError is handled to avoid treating cancellations as generic errors.

With `_handler_task` cancellations now part of normal control flow (e.g., timeouts or new triggers), `CancelledError` is currently caught by the broad `except Exception` in `_run_handler` and sent to `session.session_controller.stop(e)`, incorrectly treating normal cancellations as errors. Please either re-raise `CancelledError` in `_run_handler` so the outer `except asyncio.CancelledError` can handle it, or narrow the inner `except` to exclude `CancelledError` so cancellations don’t trigger `stop` with an error.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[Qixuan112]

Fixed by re-raising CancelledError in _run_handler so cancellations are handled by the outer except block instead of being treated as errors.

🤖 Addressed by Claude Code

[xiaoyuyu6420]

Review

This PR addresses the TODO at line 217 — the original code await session.handler(...) inside _lock means a long-running handler blocks the lock, and after timeout cleanup the handler keeps running with no way to cancel it. Tracking the task is the right direction.

Strengths

1. Waiting outside the lock (task_to_await pattern) — correctly avoids holding _lock during handler execution, which prevents deadlock when _cleanup() tries to acquire the same lock to cancel the task.
2. Re-raising CancelledError inside _run_handler — correct, ensures the outer except clause can distinguish cancellation from other exceptions.
3. Cancelling previous task before creating new one — prevents overlapping handler execution if trigger() is called rapidly.

Issues

1. _cleanup() is now async — this is a breaking change for any caller that invokes it synchronously

_cleanup() was a sync method. Making it async means any code that calls self._cleanup() without await will silently create a coroutine that never executes. Within this file both call sites are updated, but this is a public-ish method on SessionWaiter — third-party code or plugins that call _cleanup() directly will break silently.

2. Deadlock risk: _cleanup() acquires _lock, but trigger() also holds _lock when the handler runs

Consider this sequence:

1. trigger() acquires _lock, creates _handler_task, releases _lock, then await task_to_await
2. While handler is running, timeout fires → register_wait() calls await self._cleanup()
3. _cleanup() tries async with self._lock to cancel _handler_task
4. If the handler itself calls controller.keep() (which doesn't touch _lock), this works. But if the handler somehow triggers another trigger() call on the same session (recursive), _lock would already be released by then, so this is probably OK.

Actually, on closer inspection, since trigger() releases _lock before awaiting task_to_await, and _cleanup() acquires _lock only to cancel the task, there shouldn't be a deadlock. But the interaction is subtle enough to warrant a comment explaining why it's safe.

3. No test for the task cancellation behavior

The core value of this PR — cancelling a handler after timeout — has no test. A test should verify:

• When timeout fires while handler is running, _handler_task is cancelled
• When trigger() is called again before previous handler finishes, previous task is cancelled

4. _handler_task exception handling asymmetry

In _run_handler, exceptions are caught and passed to session.session_controller.stop(e). But in the outer await task_to_await, only CancelledError is caught — if the task raises any other exception (which shouldn't happen because _run_handler catches them all), it would propagate to trigger()'s caller. This is probably fine but the double-layer exception handling is confusing.

Summary

Good direction — the task tracking pattern correctly addresses the TODO. The main concerns are: (1) the async _cleanup() breaking change, (2) missing tests for the cancellation behavior, and (3) the subtle lock interaction deserves documentation. Consider adding a test that verifies a timed-out handler actually gets cancelled.

[Qixuan112]

@xiaoyuyu6420 感谢详细审查。已逐项处理并推送：

1. _cleanup() async → 保留，已文档化

_cleanup 以 _ 前缀命名，全仓库 grep 确认无外部调用者。保持 async 是因为需要在 _lock 保护下取消 _handler_task（避免与 trigger 竞态）。已在 docstring 说明其为内部私有 async 方法。

2. 锁交互安全 → 已补注释

在 trigger（锁外 await）和 _cleanup（锁内 cancel）两处补充了不会死锁的说明：trigger 锁内只做同步操作、不在锁内 await handler；_cleanup 仅在锁内取消任务；两者不会互锁。

3. 单元测试 → 已补齐

新增 tests/unit/test_session_waiter.py，6 个用例覆盖：

• 基础流程（trigger → handler → register_wait）
• 重复 trigger 取消上一个 handler
• _cleanup 取消运行中 handler
• CancelledError 不流入 stop() — cleanup + double-trigger 两路径
• handler 普通异常经 stop(e) 传播
  全部 6 用例稳定通过（strict asyncio 模式，3 次重跑无 flaky）。

4. 双层异常处理 → 已补注释

分别在 _run_handler 内层（普通异常 → stop(e)；CancelledError re-raise）和外层 await task_to_await（静默处理取消）加了注释说明分工。

5. _handler_task 引用清理

在 task_to_await 完成后用 finally 块安全清理引用（if session._handler_task is task_to_await 守卫），防止误清并发 trigger 的新任务。

另外注意到 2 个 PR 之前就存在的问题（非本次范围）：register_wait 中 except 和 finally 都调 _cleanup（当前幂等故无害），以及 _run_handler 中 stop(e) 自身若抛异常会逃逸到外层（当前不可达）。这两项可留待后续。

🤖 Prepared by Claude Code