fix(core): stop per-subagent ToolRegistry on foreground-fork path#3887
Merged
Conversation
Follow-up to #3873 review: the foreground-fork branch in `agent.ts` fires the fork body via `void runInForkContext(runFramedFork)` and returns the placeholder result synchronously, with no try/finally around the fork body. The other three spawn paths (foreground non-fork, background fork, background non-fork) added in #3873 already stop the per-subagent ToolRegistry in their finally blocks — this one was missed, so any AgentTool / SkillTool the fork's model later instantiated leaked its change-listener on the shared SubagentManager / SkillManager for the rest of the session. Wraps the inner body in `try { await runSubagentWithHooks(...) } finally { void agentConfig.getToolRegistry().stop().catch(() => {}) }` — same shape as the background bgBody finally added in #3873. Adds a regression test in `agent.test.ts` Fork dispatch describe that drains the detached fork body and asserts the stop spy was invoked exactly once.
6 tasks
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
tanzhenxin
approved these changes
May 7, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Closes the listener-leak hole left by #3873 with a minimal, parity-shaped patch — the new try/finally in the foreground-fork branch is byte-for-byte identical to the bgBody finally added in #3873. I verified the regression test reliably fails without the fix and passes with it (exactly one stopSpy call, since runSubagentWithHooks swallows errors so the finally fires once on normal completion). Multi-fork isolation holds because each execute() rebuilds a fresh agentConfig via the approval-mode override path, so each fork stops only its own registry.
Verdict
APPROVE — surgical, correct.
3 tasks
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 7, 2026
DragonnZhang
pushed a commit
that referenced
this pull request
May 8, 2026
…#3887) Follow-up to #3873 review: the foreground-fork branch in `agent.ts` fires the fork body via `void runInForkContext(runFramedFork)` and returns the placeholder result synchronously, with no try/finally around the fork body. The other three spawn paths (foreground non-fork, background fork, background non-fork) added in #3873 already stop the per-subagent ToolRegistry in their finally blocks — this one was missed, so any AgentTool / SkillTool the fork's model later instantiated leaked its change-listener on the shared SubagentManager / SkillManager for the rest of the session. Wraps the inner body in `try { await runSubagentWithHooks(...) } finally { void agentConfig.getToolRegistry().stop().catch(() => {}) }` — same shape as the background bgBody finally added in #3873. Adds a regression test in `agent.test.ts` Fork dispatch describe that drains the detached fork body and asserts the stop spy was invoked exactly once.
46 tasks
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…QwenLM#3887) Follow-up to QwenLM#3873 review: the foreground-fork branch in `agent.ts` fires the fork body via `void runInForkContext(runFramedFork)` and returns the placeholder result synchronously, with no try/finally around the fork body. The other three spawn paths (foreground non-fork, background fork, background non-fork) added in QwenLM#3873 already stop the per-subagent ToolRegistry in their finally blocks — this one was missed, so any AgentTool / SkillTool the fork's model later instantiated leaked its change-listener on the shared SubagentManager / SkillManager for the rest of the session. Wraps the inner body in `try { await runSubagentWithHooks(...) } finally { void agentConfig.getToolRegistry().stop().catch(() => {}) }` — same shape as the background bgBody finally added in QwenLM#3873. Adds a regression test in `agent.test.ts` Fork dispatch describe that drains the detached fork body and asserts the stop spy was invoked exactly once.
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
2 participants
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.
Summary
Follow-up to #3873 review. The foreground-fork branch in
agent.ts:executefires the fork body viavoid runInForkContext(runFramedFork)and returns a placeholder synchronously, with no try/finally around the fork body. The other three spawn paths added in #3873 (foreground non-fork, background fork, background non-fork) already stop the per-subagentToolRegistryin their finally blocks — this one was missed.Without it, any
AgentTool/SkillToolthe fork's model later instantiates leaks its change-listener on the sharedSubagentManager/SkillManagerfor the rest of the session. The other three paths' fix already proves the cleanup is safe (stop()is fire-and-forget; the disposed tools have only just released their listeners; discovered tools shared from parent are skipped becauseDiscoveredTool/DiscoveredMCPTooldon't implementdispose).Change
Same shape as the background
bgBodyfinally added in #3873.Tests
agent.test.ts > Fork dispatch (subagent_type omitted) > stops the per-subagent ToolRegistry after the fork body finishes— drains the detached fork body and asserts the stop spy was invoked exactly once.npx vitest run packages/core/src→ 268 files / 6966 passed.中文
概要
#3873 review follow-up。
agent.ts:execute的 foreground-fork 分支用void runInForkContext(runFramedFork)fire-and-forget 启动 fork body，同步 return placeholder，没有 try/finally。#3873 加的其他三条 spawn 路径（fg 非 fork、bg fork、bg 非 fork）都已经在 finally 里 stop per-subagentToolRegistry——这条漏了。不修的话，fork 的 model 后续实例化的
AgentTool/SkillTool在共享SubagentManager/SkillManager上挂的 change-listener 会泄漏到 session 结束。其他三条路径的修复已经证明清理是安全的（stop()fire-and-forget；被 dispose 的 tool 此刻刚释放 listener；从 parent 复制过来的 discovered tool 因为DiscoveredTool/DiscoveredMCPTool不实现dispose会被跳过）。修改
跟 #3873 的 background
bgBodyfinally 同形：try { 跑 subagent } finally { 异步 stop registry }。测试
agent.test.ts > Fork dispatch ... > stops the per-subagent ToolRegistry after the fork body finishes—— 等 detached fork body 跑完，断言 stop spy 被调一次。npx vitest run packages/core/src→ 268 文件 / 6966 通过。