fix: dispatch OnAfterMessageSentEvent after streaming delivery - #10121
bingchengcc wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/pipeline/respond/stage.py" line_range="237" />
<code_context>
)
logger.info(f"Applying streaming output ({event.get_platform_id()}).")
await event.send_streaming(result.async_stream, realtime_segmenting)
+ if await self._after_sent_cleanup(event):
+ return
return
</code_context>
<issue_to_address>
**issue (bug_risk):** `_after_sent_cleanup(event)` runs as soon as `event.send_streaming(...)` returns, but streaming adapters such as Telegram schedule the actual stream delivery in a background task before returning. `OnAfterMessageSentEvent` is therefore dispatched, and the result is cleared, before the message has actually finished sending.
**Triggers:** When a platform adapter implements streaming delivery with a background task.
**Suggested fix:** Await the adapter's complete streaming-delivery operation before invoking `_after_sent_cleanup`, or provide an explicit completion callback/future for the adapter.
</issue_to_address>
### Comment 2
<location path="astrbot/core/pipeline/respond/stage.py" line_range="237" />
<code_context>
)
logger.info(f"Applying streaming output ({event.get_platform_id()}).")
await event.send_streaming(result.async_stream, realtime_segmenting)
+ if await self._after_sent_cleanup(event):
+ return
return
</code_context>
<issue_to_address>
**issue (bug_risk):** The streaming branch invokes `_after_sent_cleanup` even when `send_streaming` delivers no message. For example, the aiocqhttp implementation returns immediately when the stream is empty, so `OnAfterMessageSentEvent` fires and the result is cleared despite no message being sent.
**Triggers:** When `result.async_stream` is empty or a streaming adapter skips delivery.
**Suggested fix:** Only dispatch the after-sent hook and clear the result when the streaming adapter reports that at least one message was delivered.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and streaming responses now invoke registered OnAfterMessageSentEvent handlers and clear the event result, changing the post-send lifecycle for every streaming delivery. A faulty handler could perform an externally visible action or leave incorrect state behind; reverting stops future invocations, but actions already taken by a handler would need separate cleanup.
Blocking findings: astrbot/core/pipeline/respond/stage.py:237, astrbot/core/pipeline/respond/stage.py:237
…eaming delivery
…eaming delivery
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Empty streams are reported as sent
At head 2f91e54d8a90a5a81156c3d1ef6c4684ad19d080, the streaming branch now always calls _after_sent_cleanup(event) after await event.send_streaming(...). However, the existing aiocqhttp implementation returns early when its async generator yields no chains, before it sends anything. The new hook then dispatches OnAfterMessageSentEvent and clears event.result even though no message was delivered.
That changes the event contract for empty/filtered responses: plugins can perform post-send work (logging, persistence, follow-up actions) for a message that never existed, and the result is lost. Please make the streaming API return an explicit delivery outcome (or otherwise retain the existing has_sent signal) and invoke cleanup only when at least one message was actually delivered. A regression test should exercise an empty async stream through the real stage path and assert that the hook is not called and the result remains available.
This is separate from the timing concern in the automated review: it applies even when the adapter awaits all delivery work. I did not claim a full adapter matrix or run the project suite; the finding is based on the current aiocqhttp.send_streaming early return and the changed stage control flow.
|
Thanks for the detailed review! Fixed in the new commit. What changed:
delivered = await event.send_streaming(result.async_stream, realtime_segmenting)
if delivered:
if await self._after_sent_cleanup(event):
return
returnSo an empty/filtered stream (e.g. aiocqhttp's generator yielding no chains) no longer dispatches Affected adapters: aiocqhttp, dingtalk, discord, line, misskey, slack, mattermost, wecom, weixin_official_account, weixin_oc (buffering pattern), plus lark/telegram/qqofficial/webchat/wecom_ai_bot (complex streaming, explicit Happy to add a dedicated regression test for the empty-stream path if you'd like, though the existing pipeline tests cover the stage flow. |
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Reviewed updated head 450b02c. The stage now gates _after_sent_cleanup on the bool returned by send_streaming, and buffering adapters return False on empty streams while successful delivery paths return True or delegate to the base implementation. This resolves the empty-stream lifecycle issue I reported. I did not claim a full local test run because the repository dependencies are unavailable in this environment.
Problem
When streaming output is active,
OnAfterMessageSentEventwas never dispatched because the streaming path returned early before reaching the event hook at the bottom ofprocess().Solution
_after_sent_cleanup()helper that dispatchesOnAfterMessageSentEventand clears the resultRebased onto latest master.
Test plan
OnAfterMessageSentEventSummary by Sourcery
Ensure streaming message delivery completes the same post-send lifecycle as regular responses.
Bug Fixes:
OnAfterMessageSentEventconsistently after successfully delivered streaming messages, matching regular message delivery behavior.Enhancements: