Stop the Aspire CLI and own its RPC connections during extension deactivation - #19152
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19152Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19152" |
There was a problem hiding this comment.
Pull request overview
Hardens VS Code extension shutdown and prevents absolute paths from appearing in debug configuration names.
Changes:
- Awaits CLI stop requests during deactivation with bounded teardown.
- Owns and disposes RPC connections and status notifications.
- Adds cross-platform workspace-path handling and regression tests.
Show a summary per file
| File | Description |
|---|---|
extension/src/extension.ts |
Awaits extension deactivation. |
extension/src/AspireExtensionContext.ts |
Coordinates bounded CLI shutdown and disposal. |
extension/src/debugger/AspireDebugSession.ts |
Deduplicates CLI stop requests. |
extension/src/server/AspireRpcServer.ts |
Tracks and disposes RPC clients. |
extension/src/server/rpcClient.ts |
Adds idempotent transport disposal. |
extension/src/server/interactionService.ts |
Clears and suppresses disposed status updates. |
extension/src/utils/workspace.ts |
Adds cross-platform path filtering. |
extension/src/test/AspireExtensionContext.test.ts |
Tests deactivation sequencing and failures. |
extension/src/test/aspireDebugSession.test.ts |
Tests stop-request deduplication. |
extension/src/test/rpc/aspireRpcServer.test.ts |
Tests RPC ownership during races. |
extension/src/test/rpc/interactionServiceTests.test.ts |
Tests transport and status cleanup. |
extension/src/test/workspace.test.ts |
Tests path fallback behavior. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
Extension deactivation was fire-and-forget: `deactivate()` returned void, so VS Code never waited for the CLI stop requests it triggered, and connections whose debug-session handshake was still pending were never owned by the RPC server. A window close could therefore leave `aspire run` processes alive and leave progress indicators on screen with nothing left to clear them. - `deactivate()` now returns a promise and awaits `AspireExtensionContext.deactivate()`, which asks every live debug session to stop its CLI (deduplicating in-flight requests) with a bounded 5s timeout before disposing the rest of the extension. - `AspireRpcServer` tracks the connections it creates, including ones still inside the handshake, and disposes them on server disposal. - `RpcClient.dispose()` is idempotent and closes the transport. - `InteractionService` is disposable and latches disposal so a status message still in flight when the transport closed cannot resurrect progress. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`asRelativePath` returns its input unchanged when the path cannot be made relative, and that value was returned verbatim. It resolves against the workspace, which may not share the extension host's path semantics, so a Windows absolute path (`C:\Users\...` or `\\server\share\...`) passes the host's `path.isAbsolute` on POSIX — the case for remote SSH, WSL and Codespaces — and the full path leaked into the debug configuration name. Reject both POSIX and Win32 absolute forms and fall back to the workspace folder name, and use the file name rather than the full path when the target is outside every workspace folder. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The cross-platform absolute-path rejection sat after the getWorkspaceFolder early return, so it was unreachable for the input it existed to catch. A Windows path is inside no workspace folder on a POSIX host, so getWorkspaceFolder returns undefined and control reached path.basename, which on POSIX does not split on '\' and returned C:\Users\me\secret\AppHost.csproj whole as the debug configuration name. Move the rejection ahead of that early return and reduce the path with path.win32.basename, which splits on both separators. Only Win32 forms can be foreign, because path.win32.isAbsolute also accepts a leading '/'. The existing regression test stubbed getWorkspaceFolder to return a folder for the Windows paths, which cannot happen on a POSIX host, so it was green against a code path that never ran. It now covers the asRelativePath guard with a host-native path, and a new test drives the foreign paths with getWorkspaceFolder returning undefined, asserting both the file name and that no separator survives on either host platform. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a0cb46e to
f7e616a
Compare
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
… not `stopCli` is an RPC request, not a kill. It resolves without effect when the transport is already closed and never settles when the CLI has stopped servicing the connection, so neither outcome proves the process exited. `spawnAspireCommand` discarded the ChildProcess that `spawnCliProcess` returns and did not request a process group, so there was nothing to signal as a fallback. Every other CLI spawn site in the extension already retains its child and calls `terminateCliProcess`; the longest-lived one did not. Retain the child, spawn `aspire run` as a process-group leader, and add `terminateCliProcessTree()`. Session disposal escalates to it after a 10s grace period so a cooperative stop still gets the first chance to shut resources down cleanly, and deactivation calls it directly once the stop requests settle or time out. Also re-snapshot the session array between awaits during deactivation. `_isShuttingDown` does not gate `addAspireDebugSession`, so a debug-adapter descriptor or an RPC-triggered `startDebugSession` landing mid-await was never asked to stop. Requesting a stop is idempotent per session, so re-scanning until no new session appears is safe. Verified red-green: with the escalation, the process group and the re-snapshot loop reverted, 4 of the 5 new tests fail and the existing 5 deactivation tests still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The re-snapshot loop asks every session that appears *before* teardown to stop, but `_disposeCore` disposes exactly the sessions present when it takes its snapshot and never runs again. A session registered after that point was still tracked forever and never disposed, so its CLI kept running with nothing left alive to stop it. `addAspireDebugSession` now refuses and disposes once `_isDisposed` is set; the pre-teardown window is unchanged and still handled by the drain. `spawnAspireCommand` awaits the CLI path before spawning, so deactivation can complete inside that await. Spawning afterwards produced an `aspire run` that no teardown path could reach — and now that it is spawned detached as a process-group leader, one that would not even die with the extension host. Two fixes to the tests added alongside the process-group change: - `terminateCliProcessTree signals a running CLI process` ran the real `terminateCliProcess`, which on Windows shells out to `taskkill /pid <pid> /t` rather than calling `child.kill`. The assertion would have failed on the Windows CI agents, and the run would have signalled whatever process owned PID 4322 there. It now stubs the module function. - Restore the newline that was lost from the `reuses an in-flight CLI stop request` test declaration. Also drops the `if (deactivate)` fallback in the test helper. `deactivate` is a declared method, so the fallback could never run, and had it ever run it would have silently retargeted the suite at `dispose()`. Verified red-green: reverting the two guards fails exactly the two new tests and nothing else. 1476 passing, 0 failing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
extension/src/AspireExtensionContext.ts:171
- A session can dispose/remove itself while
_settleStopRequestsis awaiting (for example when its AppHost session terminates). This final loop then loses that session's process handle, so a hung stop receives no immediate termination; only the session's unref'd 10-second timer remains, even though deactivation resolves after 5 seconds and the extension host may exit first. Retain the session objects associated with every collected stop request and terminate the union of those sessions and the currently registered sessions. A regression test should remove a session while its stop request is pending.
for (const session of this._aspireDebugSessions) {
try {
session.terminateCliProcessTree();
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…tion An Aspire CLI spawned with createProcessGroup leads a detached group that the AppHost and every resource process joins. Two paths let that group outlive the extension: - When the CLI exited on its own, the exit callback only cancelled the escalation timer and terminateCliProcessTree early-returned on an exited leader, so nothing ever signalled the surviving descendants. terminateCliProcess already reaps a managed group whose leader has exited; it just was not being invoked. Collect synchronously from the exit callback, because once the leader's PID is released the OS may recycle it as another group's id. - The deactivation sweep sent SIGTERM and scheduled the hard kill on an unref'd timer, but _deactivateCore resolves as soon as the sweep returns, so the host could exit first and leave a CLI that ignored SIGTERM alive. Deactivation has already spent its 5s cooperative window, so it now forces immediately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
`terminateCliProcess` never calls `child.kill` on Windows: it spawns `taskkill.exe /pid <pid> /t` so the descendants come down with the leader, and only falls back to `child.kill` from taskkill's error handler. That branch had no coverage anywhere, which is how a test asserting `child.kill` reached CI — it passed on macOS and Linux and could only fail on the Windows unit-test job, the one leg with no counterpart on another platform. Assert the taskkill invocation and that the child is not signalled directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
There was a problem hiding this comment.
Review details
Suppressed comments (2)
extension/src/AspireExtensionContext.ts:75
- Disposed sessions are now hidden here but remain in
_aspireDebugSessionsfor up to the 10-second termination grace period, whileaddAspireDebugSessionstill treats every backing-array entry as an active duplicate.TestRunSessionManagerfirst observesgetAspireDebugSession(lease.sessionId) === nulland can then create a replacement with that same lease ID, only for registration to throwdebugSessionAlreadyExistsbecause the old process owner is retained. Keep pending process owners separate from active-session identity/subscription bookkeeping (or otherwise make lookup, duplicate detection, and removal consistently distinguish the session instance).
// Disposed sessions can remain tracked only as CLI process owners. They must still be
// visible to deactivation, but not to RPC lookups or extension-state snapshots.
return this._aspireDebugSessions.filter(session => !session.isDisposed);
extension/src/debugger/AspireDebugSession.ts:173
- If deactivation lands after
createDebugAdapterDescriptorregisters the session but before VS Code sends the DAPlaunchrequest, there is no RPC client, no CLI process, and nospawnAspireCommandcontinuation that can callcompletePendingCliStopWithoutRpcClient(). This promise therefore remains pending until the context's full 5-second deadline even though nothing was launched, unnecessarily delaying window shutdown. Track whether CLI launch is actually in flight and resolve immediately when shutdown prevents a not-yet-started launch; preserve this pending path only when a CLI may already exist/connect.
if (!this._rpcClient) {
this._cliStopPromise = new Promise<void>((resolve, reject) => {
this._pendingCliStopWithoutRpcClient = { resolve, reject };
this._stopCliWhenRpcClientConnects = client => {
client.stopCli().then(resolve, reject).finally(() => {
this._pendingCliStopWithoutRpcClient = undefined;
});
};
});
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
extension/src/debugger/AspireDebugSession.ts:165
- A debug session is registered before its launch has necessarily spawned a CLI. If deactivation lands in that window, both
_rpcClientand_cliProcessare absent, so this promise can only be completed by a connection or process exit that will never occur; shutdown therefore always burns the full 5-second timeout despite having no process to stop. Resolve immediately when no child exists (the shutdown latch already prevents a later spawn), and cover the pre-launch session case.
if (!this._rpcClient) {
this._cliStopPromise = new Promise<void>((resolve, reject) => {
this._pendingCliStopWithoutRpcClient = { resolve, reject };
this._stopCliWhenRpcClientConnects = client => {
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d1cf760-b1bc-46ba-a6d4-628354b00f2c
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Ella Hathaway (ellahathaway)
left a comment
There was a problem hiding this comment.
I reviewed the shutdown flow end to end, including cooperative CLI stop, timeout/force termination, RPC ownership, and disposal ordering. I didn't find a new correctness issue beyond the concerns already addressed in resolved threads. I left two nonblocking coverage/description notes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0b8e5743-60bf-456e-b673-f64c0182bb7a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0b8e5743-60bf-456e-b673-f64c0182bb7a
There was a problem hiding this comment.
Review details
Suppressed comments (1)
extension/src/test/rpc/aspireRpcServer.test.ts:48
- If an assertion before line 35 fails, this
finallycloses only the client transport and leaves the TLS server listening, which can keep the Mocha process alive and obscure the original failure. DisposerpcServerin the unconditional cleanup path as the other server tests do.
finally {
handshakeResult.resolve(null);
await new Promise<void>(resolve => setImmediate(resolve));
transport.connection.end();
transport.connection.dispose();
transport.socket.destroy();
rpcClient?.dispose();
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Description
Split out of #19124, which combined notification presentation with extension shutdown/lifecycle hardening. This branch now contains only the shutdown work.
Extension deactivation was fire-and-forget:
deactivate()returnedvoid, so VS Code did not wait for CLI stop requests and closing a window could leaveaspire runprocesses alive. RPC connections still inside the debug-session handshake were not owned by the server, and late status updates could repaint progress after transport disposal.The extension now has one bounded shutdown path:
deactivate()returnsAspireExtensionContext.deactivate()so VS Code can await it.AspireRpcServerowns connections from creation, including pending handshakes, and rejects connections added after disposal.The E2E now starts a real AppHost through the generated VSIX and repo CLI, records the owned CLI and AppHost process IDs, invokes the process-owner cleanup path while keeping the extension host alive, and verifies both processes and the AppHost/debug-session state stop.
Verification
Additional exact-head proof:
edgeCases.e2e.test.js: 5/5 passing against the generated VSIX and repo CLIdeactivate()wiring: focused unit coverage verifies VS Code receives the context shutdown promiseChecklist