fix: dispatch OnAfterMessageSentEvent after streaming delivery - #9733
Open
bingchengcc wants to merge 2 commits into
Open
fix: dispatch OnAfterMessageSentEvent after streaming delivery#9733bingchengcc wants to merge 2 commits into
bingchengcc wants to merge 2 commits into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- To keep the streaming and non-streaming paths from diverging again, consider extracting the shared OnAfterMessageSentEvent + clear_result logic into a small helper so both branches call the same code.
- Double-check that calling event.clear_result() after send_streaming matches the non-streaming path ordering (i.e., hooks can still access any required response data before it is cleared); if not, you may want to clear earlier or pass required data into the hook explicitly.
- It may be worth wrapping send_streaming and the subsequent hook invocation in a try/finally to guarantee event.clear_result() is called even if streaming or hook execution raises, avoiding potential state leaks between requests.
Please address the comments from this code review:
## Overall Comments
- To keep the streaming and non-streaming paths from diverging again, consider extracting the shared `OnAfterMessageSentEvent` + `clear_result` logic into a small helper so both branches call the same code.
- Double-check that calling `event.clear_result()` after `send_streaming` matches the non-streaming path ordering (i.e., hooks can still access any required response data before it is cleared); if not, you may want to clear earlier or pass required data into the hook explicitly.
- It may be worth wrapping `send_streaming` and the subsequent hook invocation in a `try`/`finally` to guarantee `event.clear_result()` is called even if streaming or hook execution raises, avoiding potential state leaks between requests.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.
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
At least 1 approving review is required to merge this pull request.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The streaming output path in RespondStage early-returns after send_streaming(), skipping the OnAfterMessageSentEvent dispatch that the non-streaming path performs.
This means plugins hooking after_message_sent (e.g. simple_memory _capture) never fire for streaming platforms such as webchat.
Change
Add the same OnAfterMessageSentEvent dispatch after send_streaming() completes, matching the non-streaming path:
Test plan
Summary by Sourcery
Bug Fixes:
Fixes #9740