-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(vscode): shut the ACP CLI down gracefully instead of killing it #11642
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
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
d3eb393
fix(vscode): shut the ACP CLI down gracefully instead of killing it
yiliang114 94c10c8
fix(acp): close shutdown and superseded-session races
yiliang114 99a72dd
test(acp): pin shutdown and retry invariants
yiliang114 315c34a
test(acp): satisfy strict companion type checks
yiliang114 dd58503
test(acp): await startup supersede assertion
yiliang114 71c3577
fix(acp): bound signal shutdown hooks
yiliang114 bbe224c
docs(acp): update escalation timing comment
yiliang114 67bd4c7
Merge remote-tracking branch 'origin/main' into fix/acp-graceful-shut…
yiliang114 2c835b3
fix(vscode): narrow ACP teardown to graceful shutdown
yiliang114 4cfe551
fix(vscode): narrow ACP teardown to graceful shutdown
yiliang114 edbdac4
Merge remote-tracking branch 'origin/fix/acp-graceful-shutdown' into …
yiliang114 c0aaaca
test(vscode): cover same-session stale responses
yiliang114 b462fdf
fix(cli): fail ACP shutdown when a SessionEnd hook is cancelled
yiliang114 7f8c118
fix(vscode): deliver prompts that finish after a same-connection sess…
yiliang114 5914fac
test(vscode): pin deferred ACP shutdown coverage gaps
yiliang114 02a4908
style(cli): satisfy prettier on acpAgent's SessionEnd failure message
yiliang114 42be100
docs(cli): say what a cancelled SessionEnd hook costs on each path
yiliang114 ebbeffe
docs(vscode): narrow the escalation Verification bullet to what each …
yiliang114 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # VS Code ACP graceful shutdown E2E plan | ||
|
|
||
| ## Scenario | ||
|
|
||
| 1. Start a VS Code Companion chat that launches an MCP stdio child. | ||
| 2. Close or reload the companion while the chat is idle. | ||
| 3. Confirm the ACP CLI exits normally and its MCP child does not remain alive. | ||
| 4. Repeat with an intentionally unresponsive ACP child and confirm the host escalates only after the graceful deadline. | ||
| 5. Reopen the companion during teardown and confirm the replacement connection remains usable after the old child exits. | ||
|
|
||
| ## Expected result | ||
|
|
||
| - Ordinary teardown reaches the CLI cleanup path before the host uses a forced termination. | ||
| - No old child exit, notification, or response clears the replacement connection. | ||
| - POSIX escalation targets the ACP process group; Windows escalation targets the process tree. | ||
|
|
||
| ## Automated coverage | ||
|
|
||
| The focused companion and CLI tests cover the shutdown ladder, replacement races, overlapping shutdown phases, and hook deadline. Native Windows process-tree behavior and a real VS Code host remain manual validation boundaries. | ||
|
|
||
| The global `qwen` CLI cannot reproduce the companion-owned child lifecycle, so a standalone CLI dry run is not a faithful baseline for this scenario. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| # VS Code ACP graceful shutdown | ||
|
|
||
| [中文版](./vscode-acp-graceful-shutdown.zh-CN.md) | ||
|
|
||
| ## Problem | ||
|
|
||
| The VS Code companion terminated its ACP child immediately when the panel disconnected. On Windows this bypassed the CLI shutdown path and could leave shells and ConPTY descendants alive. A graceful child can also overlap a replacement connection, so an old exit or response must not clear state owned by the replacement. | ||
|
|
||
| ## Decision | ||
|
|
||
| The companion closes the ACP child's stdin first. The CLI treats the closed transport as a normal shutdown, fires SessionEnd hooks, drains MCP clients, disposes sessions, and runs process cleanup. | ||
|
|
||
| Shutdown is bounded: | ||
|
|
||
| - On POSIX, the ACP child leads a process group. After 75 seconds the companion sends SIGTERM to that group, then SIGKILL after another 75 seconds. | ||
| - On Windows, after 75 seconds the companion invokes the absolute System32 `taskkill.exe` path with `/f /t`. | ||
| - Exit handlers and asynchronous responses are tied to the child and connection that created them, so a retired process cannot mutate a replacement connection. | ||
| - Overlapping EOF and signal shutdown paths share the same SessionEnd, MCP drain, session-disposal, and registered process-cleanup work. SessionEnd hooks start concurrently and share a 30-second abort budget. | ||
|
|
||
| ## Scope | ||
|
|
||
| This change covers ACP process teardown and the connection races introduced by graceful teardown. It does not close sessions when the user switches conversations and does not change hook process-tree ownership, which is handled separately. | ||
|
|
||
| ## Verification | ||
|
|
||
| - Disconnect closes stdin before any forced termination. | ||
| - POSIX escalation targets the ACP child's process group; Windows escalation targets the process tree via `taskkill /f /t`, degrading to the direct child if taskkill fails. | ||
| - A normal child exit cancels escalation. | ||
| - A retired child or response cannot clear or update its replacement. | ||
| - EOF and signal shutdown execute each cleanup phase once, and all SessionEnd hooks begin within the shared deadline. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| # VS Code ACP 优雅退出 | ||
|
|
||
| [English](./vscode-acp-graceful-shutdown.md) | ||
|
|
||
| ## 问题 | ||
|
|
||
| VS Code companion 在面板断开时会立即终止 ACP 子进程。Windows 上这种方式会绕过 CLI 的退出流程,可能遗留 shell 和 ConPTY 后代进程。优雅退出期间旧进程还可能与新连接短暂并存,因此旧进程的退出事件或异步响应不能清理新连接持有的状态。 | ||
|
|
||
| ## 决策 | ||
|
|
||
| companion 首先关闭 ACP 子进程的 stdin。CLI 将传输关闭作为正常退出处理,触发 SessionEnd hooks、排空 MCP 客户端、释放会话并执行进程清理。 | ||
|
|
||
| 退出流程有明确上限: | ||
|
|
||
| - POSIX 上 ACP 子进程作为进程组组长。75 秒后 companion 向该进程组发送 SIGTERM,再等待 75 秒后发送 SIGKILL。 | ||
| - Windows 上等待 75 秒后,companion 使用 System32 下的绝对 `taskkill.exe` 路径和 `/f /t` 参数终止进程树。 | ||
| - 退出处理器和异步响应绑定到创建它们的子进程与连接,已退役进程不能修改替代连接。 | ||
| - EOF 与信号触发的重叠退出共享同一次 SessionEnd、MCP 排空、会话释放和已注册进程清理。所有 SessionEnd hooks 并发启动,并共享 30 秒的中止时限。 | ||
|
|
||
| ## 范围 | ||
|
|
||
| 本次只处理 ACP 进程退出,以及优雅退出引入的连接竞态。不处理用户切换会话时关闭旧会话,也不改变 hook 进程树的归属;后者由独立改动处理。 | ||
|
|
||
| ## 验证 | ||
|
|
||
| - 断开连接时先关闭 stdin,不立即强制终止。 | ||
| - POSIX 升级路径针对 ACP 子进程所在的进程组;Windows 升级路径通过 `taskkill /f /t` 针对进程树,taskkill 失败时退化为只终止直接子进程。 | ||
| - 子进程正常退出后取消升级计时器。 | ||
| - 已退役子进程的退出或响应不能清理或更新替代连接。 | ||
| - EOF 与信号重叠时每个清理阶段只执行一次,且所有 SessionEnd hooks 都在共享时限内启动。 |
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.
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.
Uh oh!
There was an error while loading. Please reload this page.