-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core,cli): drain background notifications outside the subagent's ALS frame #7194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
wenshao
merged 2 commits into
QwenLM:main
from
zjunothing:fix/7156-notification-als-leak
Jul 21, 2026
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The consumer-side
runOutsideAgentContextguard — described in the PR as the "decisive" fix for #7156 — has no targeted unit regression test. The existing notification test inuseGeminiStream.test.tsx(line 6388, regression for #7114) invokes the drain without any agent ALS frame, sostorage.exit(fn)is a transparent no-op in that test. Removing this wrapping would not change that test's outcome.Failure scenario: a future refactor that removes or restructures the
runOutsideAgentContextwrapping around the drain effect would silently re-introduce the model leak — notification turns resolve to the subagent's model, causing 400 errors on smaller-context models. No automated test would catch this regression.The producer-side guard in
background-tasks.tshas a well-constructed regression test that proves the mechanism. A similar test here — invoking the drain from insiderunWithAgentContext/runWithRuntimeContentGeneratorand asserting the drainedsubmitQueryuses the main session's model — would close the gap.— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 5a1a6aa:
drains a notification outside a background agent ALS frame(useGeminiStream.test.tsx). It drives the notification callback with the wholeact()flush insiderunWithRuntimeContentGenerator— mirroring the contaminated React commit — bypassing the producer-side guard, and asserts the drainedsendMessageStreamcall observes no runtime view. Verified it fails when therunOutsideAgentContextwrapping around the drain effect is removed (captured view = the subagent's). The branch is also rebased onto latest main: the earlier Test-job failure was the NOTICES.txt drift guard from #7161 tripping on this branch's pre-#7161 base, unrelated to the changed files.中文:已在 5a1a6aa 补上该回归测试——整个 act() flush 在 runWithRuntimeContentGenerator 内执行以复刻被污染的 React commit(绕过生产端防线),断言汇入的 sendMessageStream 观察不到 runtime view;移除 drain effect 的 runOutsideAgentContext 包裹后该测试确实失败。分支已 rebase 到最新 main:此前 Test job 失败是 #7161 引入的 NOTICES.txt 漂移守卫在旧基线上触发,与本 PR 改动无关。