diff --git a/extension/src/AspireExtensionContext.ts b/extension/src/AspireExtensionContext.ts index 60eb4c61a63..42c02a26851 100644 --- a/extension/src/AspireExtensionContext.ts +++ b/extension/src/AspireExtensionContext.ts @@ -1,4 +1,5 @@ import * as vscode from 'vscode'; +import { ErrorCodes, ResponseError } from 'vscode-jsonrpc'; import { AspireDebugSession } from './debugger/AspireDebugSession'; import { AspireDebugConfigurationProvider } from './debugger/AspireDebugConfigurationProvider'; import { debugSessionAlreadyExists, extensionContextNotInitialized } from './loc/strings'; @@ -7,8 +8,11 @@ import AspireDcpServer from './dcp/AspireDcpServer'; import { AspireTerminalProvider } from './utils/AspireTerminalProvider'; import { AspireEditorCommandProvider } from './editor/AspireEditorCommandProvider'; import type { AspireDebugConsoleOutputEvent } from './types/extensionApi'; +import { extensionLogOutputChannel } from './utils/logging'; export class AspireExtensionContext implements vscode.Disposable { + private static readonly _cliStopTimeoutMs = 5_000; + private _rpcServer?: AspireRpcServer; private _dcpServer?: AspireDcpServer; private _extensionContext?: vscode.ExtensionContext; @@ -21,6 +25,9 @@ export class AspireExtensionContext implements vscode.Disposable { private readonly _debugSessionOutputSubscriptions = new Map(); private readonly _onDidChangeDebugSessions = new vscode.EventEmitter(); private readonly _onDidReceiveDebugConsoleOutput = new vscode.EventEmitter(); + private _shutdownPromise?: Promise; + private _isShuttingDown = false; + private _isDisposed = false; readonly onDidChangeDebugSessions = this._onDidChangeDebugSessions.event; readonly onDidReceiveDebugConsoleOutput = this._onDidReceiveDebugConsoleOutput.event; @@ -59,14 +66,27 @@ export class AspireExtensionContext implements vscode.Disposable { return null; } - return this._aspireDebugSessions.find(session => session.debugSessionId === debugSessionId) || null; + return this._aspireDebugSessions.find(session => session.debugSessionId === debugSessionId && !session.isDisposed) || null; } get aspireDebugSessions(): readonly AspireDebugSession[] { - return [...this._aspireDebugSessions]; + // 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); } addAspireDebugSession(debugSession: AspireDebugSession) { + if (this._isDisposed) { + // `_disposeCore` disposes exactly the sessions present when it takes its snapshot, and + // it never runs twice. Tracking a session that arrives afterwards would therefore mean + // never disposing it at all: its CLI would keep running with nothing left alive to stop + // it. Sessions arriving *before* teardown are still accepted — `_waitForCliStopRequests` + // re-scans for them, and `_disposeCore` then disposes them on the normal path. + extensionLogOutputChannel.warn(`Refusing Aspire debug session ${debugSession.debugSessionId} because the extension has already been torn down; disposing it immediately.`); + debugSession.dispose(); + return; + } + if (this._aspireDebugSessions.find(session => session.debugSessionId === debugSession.debugSessionId)) { throw new Error(debugSessionAlreadyExists(debugSession.debugSessionId)); } @@ -75,6 +95,17 @@ export class AspireExtensionContext implements vscode.Disposable { this._debugSessionStateSubscriptions.set(debugSession.debugSessionId, debugSession.onDidChangeState(() => this._onDidChangeDebugSessions.fire())); this._debugSessionOutputSubscriptions.set(debugSession.debugSessionId, debugSession.onDidSendDebugConsoleOutput(event => this._onDidReceiveDebugConsoleOutput.fire(event))); this._onDidChangeDebugSessions.fire(); + + if (this._isShuttingDown) { + // A session can be registered while deactivation is already awaiting an earlier stop + // request. Ask it to stop immediately rather than waiting for the next drain-loop scan: + // the shared deadline may expire first, in which case the loop goes straight to the + // force sweep and this otherwise healthy late session would never get cooperative + // cleanup for resources outside the CLI process tree, such as containers. + void debugSession.requestCliStopForExtensionShutdown().catch(error => { + extensionLogOutputChannel.warn(`Failed to stop Aspire CLI during extension deactivation: ${error}`); + }); + } } removeAspireDebugSession(debugSession: AspireDebugSession) { @@ -94,14 +125,153 @@ export class AspireExtensionContext implements vscode.Disposable { return this._debugConfigProvider; } - dispose() { - this._rpcServer?.dispose(); - this._dcpServer?.dispose(); + deactivate(): Promise { + if (this._isDisposed) { + return Promise.resolve(); + } + + if (this._shutdownPromise) { + return this._shutdownPromise; + } + + this._isShuttingDown = true; + // Schedule the async work after storing the shared promise so a reentrant dispose/deactivate + // call cannot begin synchronous teardown between the stop request and the first await. + this._shutdownPromise = Promise.resolve().then(() => this._deactivateCore()); + return this._shutdownPromise; + } + + dispose(): void { + if (this._isDisposed || this._isShuttingDown) { + return; + } + + this._disposeCore(); + } + + private async _deactivateCore(): Promise { + try { + await this._waitForCliStopRequests(); + } + finally { + // A timeout or failed RPC stop still has to run the established debug-session and + // terminal teardown path so extension deactivation cannot leave the CLI process alive. + this._disposeCore(); + } + } + + private async _waitForCliStopRequests(): Promise { + const requested = new Map>(); + const deadline = Date.now() + AspireExtensionContext._cliStopTimeoutMs; + + // Re-snapshot after every await. `_isShuttingDown` does not stop `addAspireDebugSession` + // from registering a session, so a debug-adapter descriptor or an RPC-triggered + // `startDebugSession` that lands mid-await would never be asked to stop if the array were + // captured only once. Requesting a stop is idempotent per session, so re-scanning is safe. + while (Date.now() < deadline && this._collectStopRequests(requested)) { + const timedOut = await this._settleStopRequests([...requested.values()], deadline); + if (timedOut) { + extensionLogOutputChannel.warn(`Timed out after ${AspireExtensionContext._cliStopTimeoutMs}ms waiting for Aspire CLI stop requests; continuing extension teardown.`); + break; + } + } + + // A cooperative stop that resolved, rejected or timed out proves only what happened to the + // RPC request; the CLI process can still be running. Signal any that are, so deactivation + // cannot leave an AppHost and its resource processes orphaned. + for (const session of [...this._aspireDebugSessions]) { + try { + // Force rather than signal-and-schedule: `terminateCliProcess` escalates to a hard + // kill on an `unref`'d timer, and `_deactivateCore` resolves immediately after this + // sweep, so the extension host can exit before that timer fires. The cooperative + // deadline above was this CLI's grace period; there is no second one. + session.terminateCliProcessTree({ force: true }); + } + catch (error) { + extensionLogOutputChannel.warn(`Failed to terminate the Aspire CLI process during extension deactivation: ${error}`); + } + } + } + + /** + * Requests a CLI stop for every registered session that has not been asked yet, returning + * whether any new session was found. + */ + private _collectStopRequests(requested: Map>): boolean { + let addedRequest = false; + for (const session of this._aspireDebugSessions) { + if (session.isDisposed) { + continue; + } + + if (requested.has(session.debugSessionId)) { + continue; + } + + addedRequest = true; + try { + requested.set(session.debugSessionId, session.requestCliStopForExtensionShutdown()); + } + catch (error) { + requested.set(session.debugSessionId, Promise.reject(error)); + } + } + + return addedRequest; + } + + private async _settleStopRequests(stopRequests: Promise[], deadline: number): Promise { + const allStops = Promise.allSettled(stopRequests); + let timeout: ReturnType | undefined; + const outcome = await Promise.race([ + allStops.then(results => ({ timedOut: false as const, results })), + new Promise<{ timedOut: true }>(resolve => { + timeout = setTimeout(() => { + timeout = undefined; + resolve({ timedOut: true }); + }, Math.max(0, deadline - Date.now())); + }), + ]); + + if (timeout) { + clearTimeout(timeout); + } + + if (outcome.timedOut) { + return true; + } + + const failures = outcome.results + .filter((result): result is PromiseRejectedResult => result.status === 'rejected') + .map(result => result.reason); + for (const failure of failures) { + // Closing the RPC transport rejects its outstanding stop request even though the + // synchronous debug-session and terminal teardown below has completed successfully. + if (failure instanceof ResponseError && failure.code === ErrorCodes.PendingResponseRejected) { + extensionLogOutputChannel.info(`Aspire CLI stop request ended after the RPC transport closed: ${failure}`); + } + else { + extensionLogOutputChannel.warn(`Failed to stop Aspire CLI during extension deactivation: ${failure}`); + } + } + + return false; + } + + private _disposeCore(): void { + if (this._isDisposed) { + return; + } + + this._isDisposed = true; this._debugSessionStateSubscriptions.forEach(disposable => disposable.dispose()); this._debugSessionStateSubscriptions.clear(); this._debugSessionOutputSubscriptions.forEach(disposable => disposable.dispose()); this._debugSessionOutputSubscriptions.clear(); - this._aspireDebugSessions.forEach(session => session.dispose()); + const sessions = this._aspireDebugSessions.splice(0); + sessions.forEach(session => session.dispose()); + this._rpcServer?.dispose(); + this._dcpServer?.dispose(); this._terminalProvider?.dispose(); this._editorCommandProvider?.dispose(); this._onDidChangeDebugSessions.dispose(); diff --git a/extension/src/debugger/AspireDebugSession.ts b/extension/src/debugger/AspireDebugSession.ts index c43b07c8b6c..37a2b49f169 100644 --- a/extension/src/debugger/AspireDebugSession.ts +++ b/extension/src/debugger/AspireDebugSession.ts @@ -5,7 +5,7 @@ import { createDebugAdapterTracker, AppHostOutputHandler, AppHostRestartHandler import { AspireResourceExtendedDebugConfiguration, AspireResourceDebugSession, EnvVar, AspireExtendedDebugConfiguration, NodeLaunchConfiguration, ProcessRestartedNotification, ProjectLaunchConfiguration, SessionTerminatedNotification, StartAppHostOptions } from "../dcp/types"; import { extensionLogOutputChannel } from "../utils/logging"; import AspireDcpServer, { generateDcpIdPrefix } from "../dcp/AspireDcpServer"; -import { spawnCliProcess } from "./languages/cli"; +import { spawnCliProcess, terminateCliProcess } from "./languages/cli"; import { disconnectingFromSession, launchingWithAppHost, launchingWithDirectory, processExceptionOccurred, processExitedWithCode, aspireDashboard, appHostSessionTerminated } from "../loc/strings"; import { projectDebuggerExtension } from "./languages/dotnet"; import { AnsiColors } from "../utils/AspireTerminalProvider"; @@ -20,6 +20,7 @@ import { ICliRpcClient } from "../server/rpcClient"; import path from "path"; import os from "os"; import { EnvironmentVariables } from "../utils/environment"; +import type { ChildProcessWithoutNullStreams } from "child_process"; import { sendTelemetryEvent } from "../utils/telemetry"; import { classifyAppHostPath, classifyAppHostDirectory } from "../utils/appHostLanguage"; import { bucketAspireCommand } from "../utils/telemetryBuckets"; @@ -53,6 +54,12 @@ export function getLoggableDebugConfiguration(debugConfig: AspireResourceExtende export class AspireDebugSession implements vscode.DebugAdapter { private static readonly _mauiDebugStartMaxAttempts = 3; private static readonly _mauiDebugStartRetryDelayMs = 5000; + /** + * How long the cooperative `stopCli` RPC has to bring the CLI down before its process group is + * signalled. Long enough for the CLI to stop containers and other resources cleanly, short + * enough that a wedged CLI does not keep the AppHost alive indefinitely. + */ + private static readonly _cliCooperativeStopGraceMs = 10_000; private readonly _onDidSendMessage = new EventEmitter(); private readonly _onDidSendDebugConsoleOutput = new EventEmitter(); private _messageSeq = 1; @@ -62,6 +69,7 @@ export class AspireDebugSession implements vscode.DebugAdapter { private readonly _rpcServer: AspireRpcServer; private readonly _dcpServer: AspireDcpServer; private readonly _terminalProvider: AspireTerminalProvider; + private readonly _removeAspireDebugSession: (session: AspireDebugSession) => void; private _appHostDebugSession?: AspireResourceDebugSession = undefined; private _resourceDebugSessions: AspireResourceDebugSession[] = []; @@ -73,7 +81,15 @@ export class AspireDebugSession implements vscode.DebugAdapter { private readonly _onDidChangeState = new EventEmitter(); private readonly _disposables: vscode.Disposable[] = []; private _disposed = false; + private _removedFromExtensionContext = false; private _parentStopPromise: Thenable | undefined; + private _cliStopPromise: Promise | undefined; + private _pendingCliStopWithoutRpcClient: { resolve: () => void; reject: (reason: unknown) => void } | undefined; + private _stopCliWhenRpcClientConnects: ((client: ICliRpcClient) => void) | undefined; + private _cliProcess: ChildProcessWithoutNullStreams | undefined; + private _cliTerminationTimer: ReturnType | undefined; + private _cliProcessTreeTerminationAttempted = false; + private _extensionShutdownRequested = false; // Timestamp for the `debug/apphost/end` duration measurement. Captured the first // time we observe a `launch` request so it covers the actual user-visible session // lifetime, not the moment the AspireDebugSession object was constructed. @@ -109,18 +125,23 @@ export class AspireDebugSession implements vscode.DebugAdapter { return this._startupCompleted; } + get isDisposed(): boolean { + return this._disposed; + } + + get cliProcessId(): number | undefined { + return this._cliProcess?.pid; + } + constructor(session: vscode.DebugSession, rpcServer: AspireRpcServer, dcpServer: AspireDcpServer, terminalProvider: AspireTerminalProvider, removeAspireDebugSession: (session: AspireDebugSession) => void, debugSessionId: string = generateDcpIdPrefix()) { this._session = session; this._rpcServer = rpcServer; this._dcpServer = dcpServer; this._terminalProvider = terminalProvider; + this._removeAspireDebugSession = removeAspireDebugSession; this.configuration = session.configuration as AspireExtendedDebugConfiguration; this.debugSessionId = debugSessionId; - - this._disposables.push({ - dispose: () => removeAspireDebugSession(this) - }); } async stopDebugging(): Promise { @@ -135,6 +156,122 @@ export class AspireDebugSession implements vscode.DebugAdapter { } } + requestCliStopForExtensionShutdown(): Promise { + this._extensionShutdownRequested = true; + if (this._cliStopPromise) { + return this._cliStopPromise; + } + + if (!this._rpcClient) { + this._cliStopPromise = new Promise((resolve, reject) => { + this._pendingCliStopWithoutRpcClient = { resolve, reject }; + this._stopCliWhenRpcClientConnects = client => { + client.stopCli().then(resolve, reject).finally(() => { + this._pendingCliStopWithoutRpcClient = undefined; + }); + }; + }); + + return this._cliStopPromise; + } + + this._cliStopPromise = this._rpcClient.stopCli(); + return this._cliStopPromise; + } + + /** + * Signals the `aspire` CLI process tree. + * + * The cooperative `stopCli` RPC resolving proves only that the request was accepted, and on a + * closed transport it resolves having done nothing at all. Neither outcome terminates the CLI, + * so the process it owns — the AppHost and every resource process beneath it — has to be + * signalled directly whenever the cooperative path did not finish the job. The leader may already + * have exited by then, but that does not prove its descendants exited too. + */ + terminateCliProcessTree(options?: { force?: boolean }): void { + this.cancelScheduledCliProcessTermination(); + const cliProcess = this._cliProcess; + if (!cliProcess) { + return; + } + + // A force sweep can run after the CLI leader has exited. Never aim another signal at that + // recorded PID afterward: on Windows the PID may already have been recycled, and `taskkill /t` + // would then target an unrelated process tree. + if (this._cliProcessTreeTerminationAttempted) { + return; + } + + // Deliberately not skipped once the leader has exited. `terminateCliProcess` reaps the surviving + // members of a managed process group in that case, and that is the only path that collects + // AppHost and resource processes which outlived the CLI that owned them. + this._cliProcessTreeTerminationAttempted = true; + terminateCliProcess(cliProcess, `Aspire CLI for debug session ${this.debugSessionId}`, options); + if (this._disposed) { + this.releaseExtensionContextOwnership(); + } + } + + private scheduleCliProcessTermination(): void { + if (!this._cliProcess || this._cliTerminationTimer || this._cliProcessTreeTerminationAttempted) { + return; + } + + // Give the cooperative stop the first chance so the CLI can shut its resources down cleanly; + // after this timer fires the session may be disposed and unowned except for the extension + // context, so use the hard-kill path rather than scheduling another unref'd escalation. + this._cliTerminationTimer = setTimeout(() => { + this._cliTerminationTimer = undefined; + this.terminateCliProcessTree({ force: true }); + this.releaseExtensionContextOwnership(); + }, AspireDebugSession._cliCooperativeStopGraceMs); + this._cliTerminationTimer.unref?.(); + } + + private cancelScheduledCliProcessTermination(): void { + if (this._cliTerminationTimer) { + clearTimeout(this._cliTerminationTimer); + this._cliTerminationTimer = undefined; + } + } + + /** + * Permanently gives up signalling the recorded CLI process tree, without signalling it. + * + * Windows has no equivalent of the POSIX process group: `taskkill /pid /t` walks the live + * process table to find children, so it can only reach descendants while the recorded PID still + * names the running leader. Once that PID is released the same number can be assigned to an + * unrelated process, and the sweep would then terminate that process and its children instead. + * + * Cancelling the pending timer is not enough on its own, because the disposable installed for the + * CLI schedules a new one every time it runs. Marking the PID as spent is what makes every later + * path — the scheduled escalation and any direct `terminateCliProcessTree` call — decline to aim + * at it. + */ + private abandonCliProcessTree(): void { + this.cancelScheduledCliProcessTermination(); + this._cliProcessTreeTerminationAttempted = true; + if (this._disposed) { + this.releaseExtensionContextOwnership(); + } + } + + private releaseExtensionContextOwnership(): void { + if (this._removedFromExtensionContext) { + return; + } + + this._removedFromExtensionContext = true; + this._removeAspireDebugSession(this); + } + + private completePendingCliStopWithoutRpcClient(): void { + this._stopCliWhenRpcClientConnects = undefined; + const pendingStop = this._pendingCliStopWithoutRpcClient; + this._pendingCliStopWithoutRpcClient = undefined; + pendingStop?.resolve(); + } + private stopParentDebugSessionOnce(): Thenable { if (this._parentStopPromise) { return this._parentStopPromise; @@ -367,6 +504,8 @@ export class AspireDebugSession implements vscode.DebugAdapter { if (client.debugSessionId === this.debugSessionId) { this._rpcClient = client; disposable.dispose(); + this._stopCliWhenRpcClientConnects?.(client); + this._stopCliWhenRpcClientConnects = undefined; } }); @@ -406,9 +545,21 @@ export class AspireDebugSession implements vscode.DebugAdapter { return partial; }; - spawnCliProcess( + const cliPath = await this._terminalProvider.getAspireCliExecutablePath(); + if (this._disposed || this._extensionShutdownRequested) { + // Resolving the CLI path is asynchronous, so extension deactivation can complete between the + // launch request and this point. Spawning now would produce an `aspire run` that no teardown + // path is left to stop — and because it is spawned detached as a process-group leader, it + // would not even die with the extension host. + extensionLogOutputChannel.info(`Skipping Aspire CLI launch for disposed or shutting-down debug session ${this.debugSessionId}.`); + disposable.dispose(); + this.completePendingCliStopWithoutRpcClient(); + return; + } + + this._cliProcess = spawnCliProcess( this._terminalProvider, - await this._terminalProvider.getAspireCliExecutablePath(), + cliPath, args, { stdoutCallback: (data) => { @@ -422,6 +573,20 @@ export class AspireDebugSession implements vscode.DebugAdapter { vscode.window.showErrorMessage(processExceptionOccurred(error.message, commandLabel)); }, exitCallback: (code) => { + // A detached POSIX leader's descendants can keep the process group alive after the + // leader exits, and the group id can be reused later, so collect that group immediately. + // Windows taskkill needs the target PID to still identify a live process tree; after the + // close event the CLI PID may already be reusable, so do not taskkill from this path. + // `dispose()` below re-runs the CLI disposable, which would otherwise schedule a forced + // sweep of that same spent PID once the grace period elapses, so retire it here instead + // of only skipping the immediate call. + if (process.platform !== 'win32') { + this.terminateCliProcessTree({ force: true }); + } + else { + this.abandonCliProcessTree(); + } + this.completePendingCliStopWithoutRpcClient(); this._dcpServer.recordAppHostProcessExit(this.debugSessionId, code); // Flush any partial line left in either buffer so trailing output isn't lost. if (stdoutBuffer.length > 0) { @@ -439,16 +604,26 @@ export class AspireDebugSession implements vscode.DebugAdapter { workingDirectory: workingDirectory, debugSessionId: this.debugSessionId, noDebug: noDebug, - env: env.length > 0 ? env : undefined + env: env.length > 0 ? env : undefined, + // `aspire run` owns the AppHost and every resource process beneath it. Spawning this + // long-lived CLI as a process-group leader is what lets `terminateCliProcess` signal the + // whole tree by negative PID when the cooperative `stopCli` RPC does not finish the job. + createProcessGroup: true, }, ); this._disposables.push({ dispose: () => { - this._rpcClient?.stopCli().catch((err) => { + void this.requestCliStopForExtensionShutdown().catch((err) => { extensionLogOutputChannel.info(`stopCli failed (connection may already be closed): ${err}`); }); extensionLogOutputChannel.info(`Requested Aspire CLI exit with args: ${args.join(' ')}`); + // `stopCli` is cooperative and cannot be the only stop mechanism: it resolves without + // effect when the transport is already closed, and never settles when the CLI has stopped + // servicing the connection. Escalate to signalling the process group once the CLI has had + // a chance to exit on its own, so a CLI that ignores the request cannot outlive the + // session and keep the AppHost and its resource processes alive. + this.scheduleCliProcessTermination(); } }); @@ -866,6 +1041,11 @@ export class AspireDebugSession implements vscode.DebugAdapter { this._trackedDebugAdapters = []; void this.stopParentDebugSessionOnce(); this._onDidSendDebugConsoleOutput.dispose(); + // Keep this disposed session tracked while its delayed CLI termination is pending, so + // extension deactivation can still force-drain the process tree before VS Code exits. + if (!this._cliTerminationTimer) { + this.releaseExtensionContextOwnership(); + } // Telemetry: emit `debug/apphost/end` after a short grace window so any // pending `sessionTerminated` notifications kicked off by the child-stop diff --git a/extension/src/debugger/languages/cli.ts b/extension/src/debugger/languages/cli.ts index 4030854c8fe..1e7a36dcccd 100644 --- a/extension/src/debugger/languages/cli.ts +++ b/extension/src/debugger/languages/cli.ts @@ -109,8 +109,9 @@ export function spawnCliProcess(terminalProvider: AspireTerminalProvider, comman return child; } -export function terminateCliProcess(childProcess: ChildProcessWithoutNullStreams, description: string, options?: { suppressTimeoutWarning?: boolean }): void { - const processGroupPid = process.platform !== 'win32' && managedPosixProcessGroups.has(childProcess) +export function terminateCliProcess(childProcess: ChildProcessWithoutNullStreams, description: string, options?: { suppressTimeoutWarning?: boolean; force?: boolean }): void { + const isWindows = process.platform === 'win32'; + const processGroupPid = !isWindows && managedPosixProcessGroups.has(childProcess) ? childProcess.pid : undefined; let exited = childProcess.exitCode !== null || childProcess.signalCode !== null; @@ -161,7 +162,26 @@ export function terminateCliProcess(childProcess: ChildProcessWithoutNullStreams forceTermination(); } managedPosixProcessGroups.delete(childProcess); + return; } + + if (!isWindows) { + return; + } + + // Windows taskkill walks the live process table from the PID. Once Node has reported the + // process exit, that PID can be reassigned to an unrelated process, so there is no safe + // process-tree sweep left to perform from this helper. + return; + } + + if (options?.force) { + // Skip the graceful signal and its escalation timer entirely. The timer below is `unref`'d, + // so a caller that is itself shutting down — extension deactivation, which resolves as soon + // as this returns — can have its host exit before the timer ever fires, leaving a process + // that ignored SIGTERM alive along with its whole tree. Such callers have already spent + // their own cooperative window, so the only signal left that means anything is the hard one. + forceTermination(); return; } diff --git a/extension/src/extension.ts b/extension/src/extension.ts index 66b4c78f8d3..ea52234a497 100644 --- a/extension/src/extension.ts +++ b/extension/src/extension.ts @@ -424,8 +424,8 @@ export async function activate(context: vscode.ExtensionContext) { return Object.freeze(api); } -export function deactivate() { - aspireExtensionContext.dispose(); +export function deactivate(): Promise { + return aspireExtensionContext.deactivate(); } function getExtensionModeForTelemetry(mode: vscode.ExtensionMode): string { diff --git a/extension/src/server/AspireRpcServer.ts b/extension/src/server/AspireRpcServer.ts index ae7481c63e3..72107197643 100644 --- a/extension/src/server/AspireRpcServer.ts +++ b/extension/src/server/AspireRpcServer.ts @@ -1,5 +1,5 @@ import * as vscode from 'vscode'; -import { createMessageConnection, MessageConnection } from 'vscode-jsonrpc'; +import { createMessageConnection, ErrorCodes, MessageConnection, ResponseError } from 'vscode-jsonrpc'; import { StreamMessageReader, StreamMessageWriter } from 'vscode-jsonrpc/node'; import { invalidTokenProvided, rpcServerAddressError, rpcServerError } from '../loc/strings'; import { addInteractionServiceEndpoints, IInteractionService } from './interactionService'; @@ -21,6 +21,8 @@ export default class AspireRpcServer { public connectionInfo: RpcServerConnectionInfo; public connections: ICliRpcClient[] = []; + private readonly _ownedConnections = new Set(); + private _disposed = false; private _onNewConnection = new vscode.EventEmitter(); public readonly onNewConnection = this._onNewConnection.event; @@ -33,24 +35,61 @@ export default class AspireRpcServer { return this.connections.find(connection => connection.debugSessionId === debugSessionId) || null; } - public addConnection(connection: ICliRpcClient) { + public addConnection(connection: ICliRpcClient): boolean { + if (this._disposed) { + connection.dispose(); + return false; + } + + this._ownedConnections.add(connection); + if (this.connections.includes(connection)) { + return true; + } + this.connections.push(connection); this._onNewConnection.fire(connection); + return true; } public removeConnection(connection: ICliRpcClient) { + this._ownedConnections.delete(connection); const index = this.connections.indexOf(connection); if (index !== -1) { this.connections.splice(index, 1); } + + connection.dispose(); } public dispose() { + if (this._disposed) { + return; + } + + this._disposed = true; extensionLogOutputChannel.info(`Disposing RPC server`); + // A client is owned before its debug-session handshake starts. That ensures a stalled + // handshake cannot outlive server teardown with its transport and UI state still active. + for (const connection of this._ownedConnections) { + connection.dispose(); + } + + this._ownedConnections.clear(); + this.connections.splice(0); this._onNewConnection.dispose(); this.server.close(); } + private _ownConnection(connection: ICliRpcClient): boolean { + if (this._disposed) { + connection.dispose(); + return false; + } + + this._ownedConnections.add(connection); + return true; + } + static async create(rpcClientFactory: (rpcServerConnectionInfo: RpcServerConnectionInfo, connection: MessageConnection, token: string, debugSessionId: string | null) => ICliRpcClient): Promise { const token = generateToken(); const { key, cert } = await createSelfSignedCertAsync(); @@ -113,17 +152,36 @@ export default class AspireRpcServer { // to avoid a race condition where the CLI sends requests (e.g. displayEmptyLine) // before handlers are registered. const rpcClient = rpcClientFactory(connectionInfo, connection, token, null); - addInteractionServiceEndpoints(connection, rpcClient.interactionService, rpcClient, withAuthentication); - - connection.listen(); - - const clientDebugSessionId = await connection.sendRequest('getDebugSessionId'); - rpcClient.debugSessionId = clientDebugSessionId; - - rpcServer.addConnection(rpcClient); - connection.onClose(() => rpcServer.removeConnection(rpcClient)); - + if (!rpcServer._ownConnection(rpcClient)) { + return; + } + + try { + addInteractionServiceEndpoints(connection, rpcClient.interactionService, rpcClient, withAuthentication); + connection.listen(); + + const clientDebugSessionId = await connection.sendRequest('getDebugSessionId'); + rpcClient.debugSessionId = clientDebugSessionId; + rpcServer.addConnection(rpcClient); + } + catch (error) { + // MessageConnection disposal rejects its outstanding request with + // PendingResponseRejected during normal CLI exit. The ownership check + // keeps the same response from a still-live client visible as a warning. + const transportDisposedDuringHandshake = + error instanceof ResponseError && + error.code === ErrorCodes.PendingResponseRejected && + !rpcServer._ownedConnections.has(rpcClient); + if (transportDisposedDuringHandshake) { + extensionLogOutputChannel.info(`RPC client transport closed during initialization: ${error}`); + } + else { + extensionLogOutputChannel.warn(`Failed to initialize RPC client: ${error}`); + } + + rpcServer.removeConnection(rpcClient); + } }); resolve(rpcServer); diff --git a/extension/src/server/interactionService.ts b/extension/src/server/interactionService.ts index f3ff30bf178..944d26132d7 100644 --- a/extension/src/server/interactionService.ts +++ b/extension/src/server/interactionService.ts @@ -16,7 +16,7 @@ import { isDirectory } from '../utils/io'; import { sendTelemetryEvent } from '../utils/telemetry'; import { dashboardDefaultChangedNotificationKey } from '../utils/dashboardNotificationState'; -export interface IInteractionService { +export interface IInteractionService extends vscode.Disposable { showStatus: (statusText: string | null) => void; clearProgressNotification: () => void; promptForString: (promptText: string, defaultValue: string | null, required: boolean, rpcClient: ICliRpcClient) => Promise; @@ -169,6 +169,7 @@ export class InteractionService implements IInteractionService { private _rpcClient?: ICliRpcClient; private _progressNotifier: ProgressNotifier; + private _isDisposed = false; constructor(getAspireDebugSession: () => AspireDebugSession | null, rpcClient: ICliRpcClient, private readonly _globalState?: vscode.Memento) { this._getAspireDebugSession = getAspireDebugSession; @@ -177,6 +178,13 @@ export class InteractionService implements IInteractionService { } showStatus(statusText: string | null) { + if (this._isDisposed) { + // The RPC connection owning this service is gone. A status message that was still in + // flight when the transport closed must not paint progress that nothing is left alive + // to clear, which would strand the indicator for the rest of the window's lifetime. + return; + } + delayStatusForE2E(); this._progressNotifier.show(statusText); } @@ -691,6 +699,14 @@ export class InteractionService implements IInteractionService { this._progressNotifier.clear(); } + dispose() { + // The RPC connection owning this service is going away, so tear down any progress it + // still has on screen. Otherwise a CLI that dies with the extension leaves a permanent + // "Building..." indicator that nothing is left alive to clear. + this._isDisposed = true; + this._progressNotifier.clear(); + } + /** * Closes the dashboard browser. Delegates to the current AspireDebugSession. */ diff --git a/extension/src/server/rpcClient.ts b/extension/src/server/rpcClient.ts index 9d8fa33c71f..e9edd6f0877 100644 --- a/extension/src/server/rpcClient.ts +++ b/extension/src/server/rpcClient.ts @@ -4,7 +4,7 @@ import { extensionLogOutputChannel, logAsyncOperation } from '../utils/logging'; import { IInteractionService, InteractionService } from './interactionService'; import { AspireDebugSession } from '../debugger/AspireDebugSession'; -export interface ICliRpcClient { +export interface ICliRpcClient extends vscode.Disposable { debugSessionId: string | null; interactionService: IInteractionService; getCliVersion(): Promise; @@ -21,6 +21,7 @@ export type ValidationResult = { export class RpcClient implements ICliRpcClient { private _messageConnection: MessageConnection; private _connectionClosed: boolean; + private _disposed = false; public debugSessionId: string | null; public interactionService: IInteractionService; @@ -32,12 +33,31 @@ export class RpcClient implements ICliRpcClient { this.interactionService = new InteractionService(getAspireDebugSession, this, globalState); this._messageConnection.onClose(() => { - this._connectionClosed = true; - this.interactionService.clearProgressNotification(); extensionLogOutputChannel.info('JSON-RPC connection closed'); + this.dispose(); }); } + dispose() { + if (this._disposed) { + return; + } + + this._disposed = true; + this._connectionClosed = true; + try { + this.interactionService.dispose(); + } + finally { + try { + this._messageConnection.end(); + } + finally { + this._messageConnection.dispose(); + } + } + } + getCliVersion(): Promise { return logAsyncOperation( `Requesting CLI version from CLI`, diff --git a/extension/src/test-e2e/edgeCases.e2e.test.ts b/extension/src/test-e2e/edgeCases.e2e.test.ts index a47c17131b9..e3026419905 100644 --- a/extension/src/test-e2e/edgeCases.e2e.test.ts +++ b/extension/src/test-e2e/edgeCases.e2e.test.ts @@ -2,11 +2,17 @@ import * as assert from 'assert'; import * as fs from 'fs'; import * as path from 'path'; import type { AspireExtensionE2EControlCommand } from '../types/extensionApi'; -import { getCommandInvocationCount, getDebugLaunchCount, isSamePath, waitForCommandOutcome, waitForDebugLaunch, waitForExtensionState, waitForRepositoryIdle, waitForWorkspaceAppHost } from './helpers/assertions'; -import { createExternalSingleFileAppHost, executeE2eControlCommand, removeExternalSingleFileAppHost, restoreWorkspaceCliPath, runE2eTeardown, setCliUnavailableForE2E, setDebugLaunchSuppressedForE2E, stopAppHostIfRunning, stopPrimaryAppHostIfRunning } from './helpers/fixtures'; +import { getCommandInvocationCount, getDebugLaunchCount, isSamePath, waitForCommandOutcome, waitForDebugLaunch, waitForDebugSessionStartup, waitForExtensionState, waitForNoDebugSessions, waitForNoRunningAppHost, waitForRepositoryIdle, waitForRunningAppHost, waitForWorkspaceAppHost } from './helpers/assertions'; +import { createExternalSingleFileAppHost, executeE2eControlCommand, isProcessAlive, removeExternalSingleFileAppHost, restoreWorkspaceCliPath, runE2eTeardown, setCliUnavailableForE2E, setDebugLaunchSuppressedForE2E, stopAppHostIfRunning, stopPrimaryAppHostIfRunning, waitForKnownProcessExit } from './helpers/fixtures'; import { getPrimaryAppHostProjectPath, getWorkspaceRoot } from './helpers/paths'; import { chooseActiveQuickPick, executeCommandFromPalette, openAspireView, waitForEditorTitle } from './helpers/vscode'; +interface DebugSessionProcessInfo { + appHostPath?: string; + cliPid?: number; + appHostPid?: number; +} + suite('Aspire extension edge case E2E', function () { this.timeout(240000); let externalAppHostPath: string | undefined; @@ -139,4 +145,34 @@ suite('Aspire extension edge case E2E', function () { 60000); assert.ok(backgrounded.state.appHosts.some(appHost => isSamePath(appHost.appHostPath, appHostPath))); }); + + test('process-owner cleanup stops the owned CLI and AppHost process tree', async () => { + await openAspireView(); + await waitForRepositoryIdle(); + const discovered = await waitForWorkspaceAppHost(); + const appHostPath = discovered.state.workspaceAppHostPath ?? getPrimaryAppHostProjectPath(); + + const beforeInvocation = getCommandInvocationCount('aspire-vscode.debugAppHost'); + await executeE2eControlCommand({ name: 'debugAppHost', appHostPath }, { waitFor: 'started' }); + await waitForCommandOutcome('aspire-vscode.debugAppHost', 'success', 180000, beforeInvocation); + await waitForDebugSessionStartup(appHostPath, 180000); + await waitForRunningAppHost(180000); + + const processInfoStatus = await executeE2eControlCommand({ name: 'getDebugSessionProcessInfo', appHostPath }); + const processInfo = processInfoStatus.result as DebugSessionProcessInfo | undefined; + assert.ok(processInfo?.cliPid, `Expected the E2E bridge to report the owned Aspire CLI pid: ${JSON.stringify(processInfoStatus)}`); + assert.ok(processInfo?.appHostPid, `Expected the E2E bridge to report the owned AppHost pid: ${JSON.stringify(processInfoStatus)}`); + assert.ok(isProcessAlive(processInfo.cliPid), `Expected the Aspire CLI process ${processInfo.cliPid} to be running before deactivation.`); + assert.ok(isProcessAlive(processInfo.appHostPid), `Expected the AppHost process ${processInfo.appHostPid} to be running before deactivation.`); + + // This exercises the process-owner cleanup methods with real CLI and AppHost processes while + // keeping the extension host alive. Workbench deactivation is covered separately by unit + // tests because a reload leaves ExTester unable to complete its own browser shutdown. + await executeE2eControlCommand({ name: 'stopOwnedDebugSessionProcesses', appHostPath }, { timeoutMs: 30000 }); + + await waitForKnownProcessExit(processInfo.cliPid, 'the Aspire CLI process owned by the debug session', 120000); + await waitForKnownProcessExit(processInfo.appHostPid, 'the AppHost process owned by the debug session', 120000); + await waitForNoDebugSessions(120000); + await waitForNoRunningAppHost(120000, appHostPath); + }); }); diff --git a/extension/src/test-e2e/helpers/fixtures.ts b/extension/src/test-e2e/helpers/fixtures.ts index e67a109a996..09bc70e3d8e 100644 --- a/extension/src/test-e2e/helpers/fixtures.ts +++ b/extension/src/test-e2e/helpers/fixtures.ts @@ -695,6 +695,19 @@ function getRunningAppHostFromState(appHostPath: string) { : state.appHosts.find(candidate => isSamePath(candidate.appHostPath, appHostPath)); } +export function isProcessAlive(pid: number): boolean { + return isProcessRunning(pid); +} + +export async function waitForKnownProcessExit(pid: number, description: string, timeoutMs: number): Promise { + try { + await waitForProcessExit(pid, timeoutMs); + } + catch (error) { + throw new Error(`Timed out after ${timeoutMs}ms waiting for ${description} ${pid} to exit. Last error: ${error instanceof Error ? error.message : String(error)}`); + } +} + async function waitForProcessExit(pid: number, timeoutMs: number): Promise { const started = Date.now(); while (Date.now() - started < timeoutMs) { diff --git a/extension/src/test/AspireExtensionContext.test.ts b/extension/src/test/AspireExtensionContext.test.ts new file mode 100644 index 00000000000..5fffd9fff49 --- /dev/null +++ b/extension/src/test/AspireExtensionContext.test.ts @@ -0,0 +1,494 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +import * as assert from 'assert'; +import type { ChildProcessWithoutNullStreams } from 'node:child_process'; +import { EventEmitter } from 'node:events'; +import { PassThrough } from 'node:stream'; +import * as sinon from 'sinon'; +import * as vscode from 'vscode'; +import { ErrorCodes, ResponseError } from 'vscode-jsonrpc'; +import { AspireExtensionContext } from '../AspireExtensionContext'; +import { AspireDebugSession } from '../debugger/AspireDebugSession'; +import * as cliModule from '../debugger/languages/cli'; +import { deactivate as deactivateExtension } from '../extension'; +import { extensionLogOutputChannel } from '../utils/logging'; + +suite('AspireExtensionContext', () => { + test('extension deactivate returns the AspireExtensionContext shutdown promise', () => { + const shutdown = Promise.resolve(); + const deactivateStub = sinon.stub(AspireExtensionContext.prototype, 'deactivate').returns(shutdown); + + try { + assert.strictEqual(deactivateExtension(), shutdown); + sinon.assert.calledOnce(deactivateStub); + } + finally { + deactivateStub.restore(); + } + }); + + test('deactivation waits for every CLI stop request before disposing transport', async () => { + const order: string[] = []; + const context = createContext(order); + const firstStop = createDeferred(); + const secondStop = createDeferred(); + addSession(context, 'first', () => { + order.push('stop first'); + return firstStop.promise; + }, () => order.push('dispose first')); + addSession(context, 'second', () => { + order.push('stop second'); + return secondStop.promise; + }, () => order.push('dispose second')); + + const shutdown = deactivateContext(context); + await Promise.resolve(); + + assert.deepStrictEqual(order, ['stop first', 'stop second']); + + firstStop.resolve(); + await Promise.resolve(); + assert.deepStrictEqual(order, ['stop first', 'stop second']); + + secondStop.resolve(); + await shutdown; + + assert.deepStrictEqual(order, [ + 'stop first', + 'stop second', + 'dispose first', + 'dispose second', + 'rpc server', + 'dcp server', + 'terminal provider', + 'editor command provider', + ]); + }); + + test('deactivation timeout falls back to synchronous session and terminal teardown', async () => { + const clock = sinon.useFakeTimers({ shouldClearNativeTimers: true }); + const order: string[] = []; + const context = createContext(order); + addSession(context, 'session', () => { + order.push('stop session'); + return new Promise(() => { }); + }, () => order.push('dispose session')); + + try { + const shutdown = deactivateContext(context); + await Promise.resolve(); + + assert.deepStrictEqual(order, ['stop session']); + + await clock.tickAsync(5_000); + await shutdown; + + assert.deepStrictEqual(order, [ + 'stop session', + 'dispose session', + 'rpc server', + 'dcp server', + 'terminal provider', + 'editor command provider', + ]); + } + finally { + clock.restore(); + } + }); + + test('deactivation does not start cooperative stop requests after the stop deadline', async () => { + const order: string[] = []; + const context = createContext(order); + const timeoutMs = (AspireExtensionContext as any)._cliStopTimeoutMs; + (AspireExtensionContext as any)._cliStopTimeoutMs = 0; + + addSession(context, 'expired', () => { + order.push('stop expired'); + return Promise.resolve(); + }, () => order.push('dispose expired'), () => order.push('terminate expired')); + + try { + await deactivateContext(context); + + assert.deepStrictEqual(order, [ + 'terminate expired', + 'dispose expired', + 'rpc server', + 'dcp server', + 'terminal provider', + 'editor command provider', + ]); + } + finally { + (AspireExtensionContext as any)._cliStopTimeoutMs = timeoutMs; + } + }); + + test('dispose does not race an in-flight deactivation and repeated shutdown is idempotent', async () => { + const order: string[] = []; + const context = createContext(order); + const stop = createDeferred(); + let stopCalls = 0; + addSession(context, 'session', () => { + stopCalls++; + order.push('stop session'); + return stop.promise; + }, () => order.push('dispose session')); + + const firstShutdown = deactivateContext(context); + const secondShutdown = deactivateContext(context); + await Promise.resolve(); + context.dispose(); + + assert.deepStrictEqual(order, ['stop session']); + assert.strictEqual(stopCalls, 1); + + stop.resolve(); + await Promise.all([firstShutdown, secondShutdown]); + context.dispose(); + await deactivateContext(context); + + assert.strictEqual(stopCalls, 1); + assert.deepStrictEqual(order, [ + 'stop session', + 'dispose session', + 'rpc server', + 'dcp server', + 'terminal provider', + 'editor command provider', + ]); + }); + + test('deactivation warns and absorbs CLI stop errors after completing teardown', async () => { + const order: string[] = []; + const context = createContext(order); + const expectedError = new Error('stop failed'); + const warnStub = sinon.stub(extensionLogOutputChannel, 'warn'); + addSession(context, 'session', async () => { + order.push('stop session'); + throw expectedError; + }, () => order.push('dispose session')); + + try { + await deactivateContext(context); + + sinon.assert.calledWithMatch(warnStub, 'Failed to stop Aspire CLI during extension deactivation: Error: stop failed'); + assert.deepStrictEqual(order, [ + 'stop session', + 'dispose session', + 'rpc server', + 'dcp server', + 'terminal provider', + 'editor command provider', + ]); + } + finally { + warnStub.restore(); + } + }); + + test('deactivation logs and absorbs PendingResponseRejected after the RPC transport closes', async () => { + const order: string[] = []; + const context = createContext(order); + const infoStub = sinon.stub(extensionLogOutputChannel, 'info'); + const warnStub = sinon.stub(extensionLogOutputChannel, 'warn'); + addSession(context, 'session', async () => { + order.push('stop session'); + throw new ResponseError(ErrorCodes.PendingResponseRejected, 'Pending response rejected since connection got disposed'); + }, () => order.push('dispose session')); + + try { + await deactivateContext(context); + + sinon.assert.calledWithMatch(infoStub, 'Aspire CLI stop request ended after the RPC transport closed:'); + assert.strictEqual(warnStub.calledWithMatch('Failed to stop Aspire CLI during extension deactivation:'), false); + assert.deepStrictEqual(order, [ + 'stop session', + 'dispose session', + 'rpc server', + 'dcp server', + 'terminal provider', + 'editor command provider', + ]); + } + finally { + infoStub.restore(); + warnStub.restore(); + } + }); + + test('deactivation stops a debug session registered while an earlier stop is in flight', async () => { + const order: string[] = []; + const context = createContext(order); + const firstStop = createDeferred(); + addSession(context, 'first', () => { + order.push('stop first'); + return firstStop.promise; + }, () => order.push('dispose first')); + + const shutdown = deactivateContext(context); + await Promise.resolve(); + assert.deepStrictEqual(order, ['stop first']); + + // `_isShuttingDown` does not gate `addAspireDebugSession`, so a debug-adapter descriptor + // or an RPC-triggered `startDebugSession` can still register a session at exactly this + // point. Snapshotting the session array once would leave this one running. + addSession(context, 'late', () => { + order.push('stop late'); + return Promise.resolve(); + }, () => order.push('dispose late')); + + firstStop.resolve(); + await shutdown; + + assert.ok(order.includes('stop late'), `A session registered during shutdown must still be asked to stop: ${JSON.stringify(order)}`); + assert.ok(order.indexOf('stop late') < order.indexOf('rpc server'), `The late stop must happen before the transport is disposed: ${JSON.stringify(order)}`); + }); + + test('deactivation asks a late debug session to stop even when the original batch times out', async () => { + const order: string[] = []; + const context = createContext(order); + const clock = sinon.useFakeTimers({ shouldClearNativeTimers: true }); + addSession(context, 'hung', () => { + order.push('stop hung'); + return new Promise(() => { }); + }, () => order.push('dispose hung'), () => order.push('terminate hung')); + + try { + const shutdown = deactivateContext(context); + await Promise.resolve(); + assert.deepStrictEqual(order, ['stop hung']); + + addSession(context, 'late', () => { + order.push('stop late'); + return Promise.resolve(); + }, () => order.push('dispose late'), () => order.push('terminate late')); + + await clock.tickAsync(5_000); + await shutdown; + + // The timeout exits the drain loop immediately, so a late session must receive its + // cooperative stop from addAspireDebugSession itself. Otherwise it would only be + // force-terminated and resources outside the process tree could keep running. + assert.deepStrictEqual(order, [ + 'stop hung', + 'stop late', + 'terminate hung', + 'terminate late', + 'dispose hung', + 'dispose late', + 'rpc server', + 'dcp server', + 'terminal provider', + 'editor command provider', + ]); + } + finally { + clock.restore(); + } + }); + + test('deactivation terminates the CLI process group after the cooperative stop resolves', async () => { + const order: string[] = []; + const context = createContext(order); + addSession(context, 'session', () => { + order.push('stop session'); + return Promise.resolve(); + }, () => order.push('dispose session'), () => order.push('terminate session')); + + await deactivateContext(context); + + // A resolved `stopCli` proves the request was accepted, not that the process exited, so + // the process group is signalled regardless before teardown continues. + assert.deepStrictEqual(order, [ + 'stop session', + 'terminate session', + 'dispose session', + 'rpc server', + 'dcp server', + 'terminal provider', + 'editor command provider', + ]); + }); + + test('deactivation terminates the CLI process group when the cooperative stop never settles', async () => { + const order: string[] = []; + const context = createContext(order); + const warnStub = sinon.stub(extensionLogOutputChannel, 'warn'); + const clock = sinon.useFakeTimers(); + // A CLI that stopped servicing its connection leaves the request pending forever. The + // timeout only ends the wait, so without an explicit signal the process would survive. + addSession(context, 'hung', () => { + order.push('stop hung'); + return new Promise(() => { }); + }, () => order.push('dispose hung'), () => order.push('terminate hung')); + + try { + const shutdown = deactivateContext(context); + await clock.tickAsync(5_000); + await shutdown; + + assert.deepStrictEqual(order, [ + 'stop hung', + 'terminate hung', + 'dispose hung', + 'rpc server', + 'dcp server', + 'terminal provider', + 'editor command provider', + ]); + sinon.assert.calledWithMatch(warnStub, 'Timed out after 5000ms waiting for Aspire CLI stop requests'); + } + finally { + clock.restore(); + warnStub.restore(); + } + }); + + test('deactivation force-terminates rather than relying on the unref-d escalation timer', async () => { + const order: string[] = []; + const context = createContext(order); + const terminateOptions: Array<{ force?: boolean } | undefined> = []; + addSession(context, 'session', () => { + order.push('stop session'); + return Promise.resolve(); + }, () => order.push('dispose session'), options => terminateOptions.push(options)); + + await deactivateContext(context); + + // `terminateCliProcess` escalates to a hard kill on an `unref`'d timer, and deactivation + // resolves as soon as this sweep returns, so the extension host can exit before that timer + // fires and leave a CLI that ignored SIGTERM alive. + assert.deepStrictEqual(terminateOptions, [{ force: true }]); + }); + + test('deactivation force-drains a disposed debug session with pending CLI termination', async () => { + const order: string[] = []; + const context = createContext(order); + const cliProcess = createFakeCliProcess(4321); + const spawnStub = sinon.stub(cliModule, 'spawnCliProcess').returns(cliProcess); + const terminateStub = sinon.stub(cliModule, 'terminateCliProcess'); + const stopDebuggingStub = sinon.stub(vscode.debug, 'stopDebugging').resolves(); + const aspireDebugSession = createSpawnedDebugSession(context); + context.addAspireDebugSession(aspireDebugSession); + + try { + await aspireDebugSession.spawnAspireCommand(['run'], '/workspace', false, 'aspire run'); + + aspireDebugSession.dispose(); + assert.deepStrictEqual(context.aspireDebugSessions, []); + + await deactivateContext(context); + + sinon.assert.calledOnceWithExactly( + terminateStub, + cliProcess, + `Aspire CLI for debug session ${aspireDebugSession.debugSessionId}`, + { force: true }); + } + finally { + spawnStub.restore(); + terminateStub.restore(); + stopDebuggingStub.restore(); + } + }); + + test('a debug session registered after teardown is refused and disposed rather than tracked forever', async () => { + const order: string[] = []; + const context = createContext(order); + const warnStub = sinon.stub(extensionLogOutputChannel, 'warn'); + + try { + await deactivateContext(context); + + // The drain loop above re-scans only until teardown starts. `_disposeCore` has since + // taken and emptied its snapshot, and it never runs again, so a session accepted here + // would keep its CLI alive with nothing left alive to stop it. + addSession(context, 'late', () => { + order.push('stop late'); + return Promise.resolve(); + }, () => order.push('dispose late')); + + assert.deepStrictEqual(context.aspireDebugSessions, []); + assert.deepStrictEqual(order, [ + 'rpc server', + 'dcp server', + 'terminal provider', + 'editor command provider', + 'dispose late', + ]); + sinon.assert.calledWithMatch(warnStub, 'Refusing Aspire debug session late because the extension has already been torn down'); + } + finally { + warnStub.restore(); + } + }); +}); + +function createContext(order: string[]): AspireExtensionContext { + const context = new AspireExtensionContext(); + context.initialize( + { dispose: () => order.push('rpc server') } as any, + { subscriptions: [] } as unknown as vscode.ExtensionContext, + { dispose: () => { } } as any, + { dispose: () => order.push('dcp server') } as any, + { dispose: () => order.push('terminal provider') } as any, + { dispose: () => order.push('editor command provider') } as any); + return context; +} + +function addSession(context: AspireExtensionContext, debugSessionId: string, stopCli: () => Promise, dispose: () => void, terminateCliProcessTree: (options?: { force?: boolean }) => void = () => { }): void { + context.addAspireDebugSession({ + debugSessionId, + onDidChangeState: () => ({ dispose: () => { } }), + onDidSendDebugConsoleOutput: () => ({ dispose: () => { } }), + requestCliStopForExtensionShutdown: stopCli, + terminateCliProcessTree, + dispose, + } as unknown as AspireDebugSession); +} + +function deactivateContext(context: AspireExtensionContext): Promise { + return context.deactivate(); +} + +function createDeferred(): { promise: Promise; resolve: (value: T) => void } { + let resolve!: (value: T) => void; + const promise = new Promise(promiseResolve => { + resolve = promiseResolve; + }); + + return { promise, resolve }; +} + +function createSpawnedDebugSession(context: AspireExtensionContext): AspireDebugSession { + const parentDebugSession = { + id: 'aspire-session', + configuration: {}, + } as unknown as vscode.DebugSession; + + return new AspireDebugSession( + parentDebugSession, + { onNewConnection: () => ({ dispose: () => { } }) } as any, + { recordAppHostProcessExit: () => { } } as any, + { + getAspireCliExecutablePath: async () => '/usr/local/bin/aspire', + createEnvironment: () => ({}), + } as any, + context.removeAspireDebugSession.bind(context)); +} + +function createFakeCliProcess(pid: number): ChildProcessWithoutNullStreams { + return Object.assign(new EventEmitter(), { + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + killed: false, + exitCode: null, + signalCode: null, + pid, + kill: sinon.stub().returns(true), + }) as unknown as ChildProcessWithoutNullStreams; +} diff --git a/extension/src/test/aspireDebugSession.test.ts b/extension/src/test/aspireDebugSession.test.ts index 643aefc7972..adb0b76bb02 100644 --- a/extension/src/test/aspireDebugSession.test.ts +++ b/extension/src/test/aspireDebugSession.test.ts @@ -1,10 +1,15 @@ import * as assert from 'assert'; import type { TelemetryReporter } from '@vscode/extension-telemetry'; import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { EventEmitter } from 'node:events'; +import { PassThrough } from 'node:stream'; +import type { ChildProcessWithoutNullStreams } from 'node:child_process'; import { join } from 'node:path'; import * as sinon from 'sinon'; import * as vscode from 'vscode'; +import * as cliModule from '../debugger/languages/cli'; import { AspireDebugSession, buildAspireCommandArgs, getLoggableDebugConfiguration } from '../debugger/AspireDebugSession'; +import { extensionLogOutputChannel } from '../utils/logging'; import { appHostTelemetryTargetPathConfigKey } from '../debugger/AspireDebugConfigurationMetadata'; import { AspireResourceExtendedDebugConfiguration } from '../dcp/types'; import { __resetCommonPropertiesForTests, __setReporterForTests } from '../utils/telemetry'; @@ -62,6 +67,313 @@ suite('AspireDebugSession tests', () => { tempDirs.length = 0; }); + test('extension shutdown reuses an in-flight CLI stop request', async () => { + let completeStop!: () => void; + const stopRequest = new Promise(resolve => { + completeStop = resolve; + }); + const stopCli = sinon.stub().returns(stopRequest); + const parentDebugSession = { + id: 'aspire-session', + configuration: {}, + } as unknown as vscode.DebugSession; + const aspireDebugSession = new AspireDebugSession(parentDebugSession, {} as any, {} as any, {} as any, () => { }); + (aspireDebugSession as any)._rpcClient = { stopCli }; + + const firstRequest = aspireDebugSession.requestCliStopForExtensionShutdown(); + const secondRequest = aspireDebugSession.requestCliStopForExtensionShutdown(); + + assert.strictEqual(secondRequest, firstRequest); + sinon.assert.calledOnce(stopCli); + + completeStop(); + await firstRequest; + }); + + test('extension shutdown waits for a CLI RPC client that connects after the stop request', async () => { + let onNewConnection!: (client: { debugSessionId: string; stopCli: () => Promise }) => void; + const stopCli = sinon.stub().resolves(); + const cliProcess = createFakeCliProcess(4320); + sinon.stub(cliModule, 'spawnCliProcess').returns(cliProcess); + const aspireDebugSession = createSessionForSpawn( + async () => '/usr/local/bin/aspire', + () => { }, + callback => { + onNewConnection = callback; + return { dispose: sinon.stub() }; + }); + + await aspireDebugSession.spawnAspireCommand(['run'], '/workspace', false, 'aspire run'); + + const stopRequest = aspireDebugSession.requestCliStopForExtensionShutdown(); + let stopSettled = false; + void stopRequest.then(() => { stopSettled = true; }); + await Promise.resolve(); + + assert.strictEqual(stopSettled, false); + + onNewConnection({ debugSessionId: aspireDebugSession.debugSessionId, stopCli }); + await stopRequest; + + sinon.assert.calledOnce(stopCli); + }); + + test('spawns the Aspire CLI as a process-group leader and retains the child process', async () => { + const cliProcess = createFakeCliProcess(4321); + const spawnStub = sinon.stub(cliModule, 'spawnCliProcess').returns(cliProcess); + try { + const aspireDebugSession = createSessionForSpawn(); + + await aspireDebugSession.spawnAspireCommand(['run'], '/workspace', false, 'aspire run'); + + const options = spawnStub.firstCall.args[3]; + // Without a process group there is no way to signal the AppHost and resource processes + // the CLI owns, and without retaining the child there is nothing to signal at all. + assert.strictEqual(options?.createProcessGroup, true); + assert.strictEqual((aspireDebugSession as any)._cliProcess, cliProcess); + } + finally { + spawnStub.restore(); + } + }); + + test('terminateCliProcessTree signals a running CLI process and still collects an exited one', () => { + // `terminateCliProcess` is stubbed rather than executed: on Windows it shells out to + // `taskkill /pid /t` instead of calling `child.kill`, so running it for real would + // both fail this assertion on the Windows CI agents and signal whatever process happens to + // own the made-up PID there. + const terminateStub = sinon.stub(cliModule, 'terminateCliProcess'); + const running = createFakeCliProcess(4322); + const aspireDebugSession = createSessionForSpawn(); + (aspireDebugSession as any)._cliProcess = running; + + aspireDebugSession.terminateCliProcessTree(); + + // The cooperative `stopCli` RPC cannot terminate the process, so the signal is what + // actually ends the CLI and the resource tree beneath it. + sinon.assert.calledOnce(terminateStub); + assert.strictEqual(terminateStub.firstCall.args[0], running); + + const exited = createFakeCliProcess(4323, 0); + const exitedAspireDebugSession = createSessionForSpawn(); + (exitedAspireDebugSession as any)._cliProcess = exited; + + exitedAspireDebugSession.terminateCliProcessTree(); + + // An exited leader is still forwarded: `terminateCliProcess` reaps the surviving members of + // its managed process group, which is the only path that collects an AppHost and resource + // processes that outlived the CLI. + sinon.assert.calledTwice(terminateStub); + assert.strictEqual(terminateStub.secondCall.args[0], exited); + }); + + test('terminateCliProcessTree is idempotent after signalling a CLI process', () => { + const terminateStub = sinon.stub(cliModule, 'terminateCliProcess'); + const cliProcess = createFakeCliProcess(4324); + const aspireDebugSession = createSessionForSpawn(); + (aspireDebugSession as any)._cliProcess = cliProcess; + + aspireDebugSession.terminateCliProcessTree({ force: true }); + aspireDebugSession.terminateCliProcessTree(); + + sinon.assert.calledOnce(terminateStub); + assert.strictEqual(terminateStub.firstCall.args[0], cliProcess); + assert.deepStrictEqual(terminateStub.firstCall.args[2], { force: true }); + }); + + test('a POSIX CLI process that exits on its own still has its process group collected', async () => { + // The behaviour under test is selected by `process.platform`, so it has to be pinned rather + // than inherited from whichever agent runs the suite. Without this the test asserts POSIX + // behaviour on the Windows agents, where the branch it covers deliberately does not run. + const platformStub = sinon.stub(process, 'platform').value('linux'); + // Already exited: the leader is gone by the time the exit callback runs, which is exactly + // the state the old early return skipped on. + const cliProcess = createFakeCliProcess(4325, 0); + const spawnStub = sinon.stub(cliModule, 'spawnCliProcess').returns(cliProcess); + const terminateStub = sinon.stub(cliModule, 'terminateCliProcess'); + sinon.stub(vscode.debug, 'stopDebugging').resolves(); + const aspireDebugSession = createSessionForSpawn(); + + try { + await aspireDebugSession.spawnAspireCommand(['run'], '/workspace', false, 'aspire run'); + + spawnStub.firstCall.args[3]?.exitCallback?.(0); + + // The CLI is gone but the AppHost and resource processes in its detached group need not + // be, and once the leader's PID is released the group id can be recycled — so the + // collection has to happen here rather than on a later timer. + sinon.assert.calledOnceWithExactly( + terminateStub, + cliProcess, + `Aspire CLI for debug session ${aspireDebugSession.debugSessionId}`, + { force: true }); + } + finally { + platformStub.restore(); + } + }); + + test('a Windows CLI process that exits on its own is not swept by stale PID', async () => { + const platformStub = sinon.stub(process, 'platform').value('win32'); + const cliProcess = createFakeCliProcess(4327, 0); + const spawnStub = sinon.stub(cliModule, 'spawnCliProcess').returns(cliProcess); + const terminateStub = sinon.stub(cliModule, 'terminateCliProcess'); + sinon.stub(vscode.debug, 'stopDebugging').resolves(); + const aspireDebugSession = createSessionForSpawn(); + + try { + await aspireDebugSession.spawnAspireCommand(['run'], '/workspace', false, 'aspire run'); + + spawnStub.firstCall.args[3]?.exitCallback?.(0); + + // On Windows, taskkill can only walk the tree while the target PID still names a live + // process. The close callback runs after that PID can be recycled, so a force sweep here + // is both unreliable for descendants and unsafe for an unrelated process that reused it. + sinon.assert.notCalled(terminateStub); + } + finally { + platformStub.restore(); + } + }); + + test('a Windows CLI process that exits on its own is not swept after the cooperative grace period', async () => { + // The exit callback runs `dispose()`, which re-runs the CLI disposable and schedules the + // forced escalation. Asserting only at exit time therefore proves nothing about the sweep + // that actually reaches taskkill, so this test has to advance past the grace period. + const clock = sinon.useFakeTimers({ shouldClearNativeTimers: true }); + const platformStub = sinon.stub(process, 'platform').value('win32'); + const cliProcess = createFakeCliProcess(4328, 0); + const spawnStub = sinon.stub(cliModule, 'spawnCliProcess').returns(cliProcess); + const terminateStub = sinon.stub(cliModule, 'terminateCliProcess'); + sinon.stub(vscode.debug, 'stopDebugging').resolves(); + const aspireDebugSession = createSessionForSpawn(); + + try { + await aspireDebugSession.spawnAspireCommand(['run'], '/workspace', false, 'aspire run'); + + spawnStub.firstCall.args[3]?.exitCallback?.(0); + + // Well past the 10s cooperative grace period, so any scheduled escalation has fired. + await clock.tickAsync(30_000); + + // The recorded PID named a process that has already exited, so Windows may have handed + // it to something unrelated by now. Nothing may aim taskkill at it. + sinon.assert.notCalled(terminateStub); + } + finally { + platformStub.restore(); + clock.restore(); + } + }); + + test('a disposed Windows CLI process that exits releases extension ownership', async () => { + const platformStub = sinon.stub(process, 'platform').value('win32'); + const cliProcess = createFakeCliProcess(4329, 0); + const spawnStub = sinon.stub(cliModule, 'spawnCliProcess').returns(cliProcess); + const removeAspireDebugSession = sinon.stub(); + sinon.stub(cliModule, 'terminateCliProcess'); + sinon.stub(vscode.debug, 'stopDebugging').resolves(); + const aspireDebugSession = createSessionForSpawn( + async () => '/usr/local/bin/aspire', + removeAspireDebugSession); + + try { + await aspireDebugSession.spawnAspireCommand(['run'], '/workspace', false, 'aspire run'); + + aspireDebugSession.dispose(); + spawnStub.firstCall.args[3]?.exitCallback?.(0); + + // dispose() keeps a stopped session registered while the delayed CLI termination is + // pending. When the Windows close callback retires that PID without signalling it, the + // callback has to release the same ownership because the later dispose() call is a no-op. + sinon.assert.calledOnceWithExactly(removeAspireDebugSession, aspireDebugSession); + } + finally { + platformStub.restore(); + } + }); + + test('a forced CLI process tree termination is not repeated by the exit callback', async () => { + const cliProcess = createFakeCliProcess(4326, 0); + const spawnStub = sinon.stub(cliModule, 'spawnCliProcess').returns(cliProcess); + const terminateStub = sinon.stub(cliModule, 'terminateCliProcess'); + sinon.stub(vscode.debug, 'stopDebugging').resolves(); + const aspireDebugSession = createSessionForSpawn(); + + await aspireDebugSession.spawnAspireCommand(['run'], '/workspace', false, 'aspire run'); + + aspireDebugSession.terminateCliProcessTree({ force: true }); + spawnStub.firstCall.args[3]?.exitCallback?.(0); + + sinon.assert.calledOnceWithExactly( + terminateStub, + cliProcess, + `Aspire CLI for debug session ${aspireDebugSession.debugSessionId}`, + { force: true }); + }); + + test('a launch that resolves the CLI path after disposal does not spawn an orphan CLI', async () => { + const spawnStub = sinon.stub(cliModule, 'spawnCliProcess'); + const infoStub = sinon.stub(extensionLogOutputChannel, 'info'); + let releaseCliPath!: (cliPath: string) => void; + const cliPath = new Promise(resolve => { + releaseCliPath = resolve; + }); + let cliPathRequested!: () => void; + const cliPathRequestObserved = new Promise(resolve => { + cliPathRequested = resolve; + }); + const aspireDebugSession = createSessionForSpawn(() => { + cliPathRequested(); + return cliPath; + }); + + const spawning = aspireDebugSession.spawnAspireCommand(['run'], '/workspace', false, 'aspire run'); + await cliPathRequestObserved; + // Deactivation can complete while the CLI path is still resolving. Set the state `dispose()` + // establishes rather than calling it, so this covers the spawn guard alone and not VS Code's + // parent-session teardown. + (aspireDebugSession as any)._disposed = true; + releaseCliPath('/usr/local/bin/aspire'); + await spawning; + + // A detached process group spawned here would outlive the extension host itself. + sinon.assert.notCalled(spawnStub); + assert.strictEqual((aspireDebugSession as any)._cliProcess, undefined); + sinon.assert.calledWithMatch(infoStub, 'Skipping Aspire CLI launch for disposed or shutting-down debug session'); + }); + + test('a launch that resolves the CLI path after extension shutdown was requested does not spawn an orphan CLI', async () => { + const spawnStub = sinon.stub(cliModule, 'spawnCliProcess'); + const infoStub = sinon.stub(extensionLogOutputChannel, 'info'); + let releaseCliPath!: (cliPath: string) => void; + const cliPath = new Promise(resolve => { + releaseCliPath = resolve; + }); + let cliPathRequested!: () => void; + const cliPathRequestObserved = new Promise(resolve => { + cliPathRequested = resolve; + }); + const aspireDebugSession = createSessionForSpawn(() => { + cliPathRequested(); + return cliPath; + }); + + const spawning = aspireDebugSession.spawnAspireCommand(['run'], '/workspace', false, 'aspire run'); + await cliPathRequestObserved; + const stopRequest = aspireDebugSession.requestCliStopForExtensionShutdown(); + releaseCliPath('/usr/local/bin/aspire'); + await spawning; + await stopRequest; + + // The extension context can request shutdown before it has disposed this session. The + // session must still remember that no later async continuation is allowed to create a + // detached CLI process after the deactivation force sweep has already run. + sinon.assert.notCalled(spawnStub); + assert.strictEqual((aspireDebugSession as any)._cliProcess, undefined); + sinon.assert.calledWithMatch(infoStub, 'Skipping Aspire CLI launch for disposed or shutting-down debug session'); + }); + test('suppresses the Aspire CLI first-run banner for extension-managed launches', async () => { const parentDebugSession = { id: 'aspire-session', @@ -1599,4 +1911,38 @@ var builder = Aspire.Hosting.DistributedApplication.CreateBuilder(args); await clock.tickAsync(10); } } + + function createSessionForSpawn( + getAspireCliExecutablePath: () => Promise = async () => '/usr/local/bin/aspire', + removeAspireDebugSession: (session: AspireDebugSession) => void = () => { }, + onNewConnection: (callback: (client: any) => void) => vscode.Disposable = () => ({ dispose: () => { } })): AspireDebugSession { + const parentDebugSession = { + id: 'aspire-session', + configuration: {}, + } as unknown as vscode.DebugSession; + + return new AspireDebugSession( + parentDebugSession, + { onNewConnection } as any, + { recordAppHostProcessExit: () => { } } as any, + { + getAspireCliExecutablePath, + createEnvironment: () => ({}), + } as any, + removeAspireDebugSession); + } + + function createFakeCliProcess(pid: number, exitCode: number | null = null): ChildProcessWithoutNullStreams & { kill: sinon.SinonStub } { + const kill = sinon.stub().returns(true); + return Object.assign(new EventEmitter(), { + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + killed: false, + exitCode, + signalCode: null, + pid, + kill, + }) as unknown as ChildProcessWithoutNullStreams & { kill: sinon.SinonStub }; + } }); diff --git a/extension/src/test/cli.test.ts b/extension/src/test/cli.test.ts new file mode 100644 index 00000000000..4c5b46f7bf1 --- /dev/null +++ b/extension/src/test/cli.test.ts @@ -0,0 +1,52 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +import * as assert from 'assert'; +import nodeChildProcess = require('child_process'); +import type { ChildProcessWithoutNullStreams } from 'child_process'; +import { EventEmitter } from 'node:events'; +import { PassThrough } from 'node:stream'; +import * as sinon from 'sinon'; +import { terminateCliProcess } from '../debugger/languages/cli'; + +suite('CLI process termination', () => { + teardown(() => { + sinon.restore(); + }); + + test('does not forcefully terminate the Windows process tree for an already-exited leader', () => { + sinon.stub(process, 'platform').value('win32'); + const childProcess = createFakeCliProcess(4242, 0); + const taskkillUnref = sinon.stub(); + const spawnStub = sinon.stub(nodeChildProcess, 'spawn').callsFake((command: string, args?: readonly string[], options?: nodeChildProcess.SpawnOptions) => { + return Object.assign(new EventEmitter(), { + command, + args: [...(args ?? [])], + options, + unref: taskkillUnref, + }) as unknown as nodeChildProcess.ChildProcess; + }); + + terminateCliProcess(childProcess, 'Aspire CLI', { force: true }); + + // After Node has observed exit, taskkill would resolve PID 4242 against the current process + // table rather than a durable handle to the former CLI tree. + sinon.assert.notCalled(spawnStub); + sinon.assert.notCalled(taskkillUnref); + sinon.assert.notCalled(childProcess.kill); + }); +}); + +function createFakeCliProcess(pid: number, exitCode: number | null): ChildProcessWithoutNullStreams & { kill: sinon.SinonStub } { + const kill = sinon.stub().returns(true); + return Object.assign(new EventEmitter(), { + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + killed: false, + exitCode, + signalCode: null, + pid, + kill, + }) as unknown as ChildProcessWithoutNullStreams & { kill: sinon.SinonStub }; +} diff --git a/extension/src/test/cliSpawn.test.ts b/extension/src/test/cliSpawn.test.ts index 199238327a3..8399597ed45 100644 --- a/extension/src/test/cliSpawn.test.ts +++ b/extension/src/test/cliSpawn.test.ts @@ -474,6 +474,56 @@ suite('spawnCliProcess tests', () => { platformStub.restore(); } }); + test('terminates the process tree with taskkill on Windows rather than signalling the child', () => { + // Regression coverage for the Windows CI break: `terminateCliProcess` deliberately never + // calls `child.kill` on Windows, because killing the leader there orphans its descendants. + // A test that asserts on `child.kill` therefore passes on POSIX and fails on Windows. + const platformStub = sinon.stub(process, 'platform').value('win32'); + const spawned: Array<{ command: string; args: readonly string[] }> = []; + const spawnStub = sinon.stub(nodeChildProcess, 'spawn').callsFake(((command: string, args: readonly string[]) => { + spawned.push({ command, args }); + return Object.assign(new EventEmitter(), { unref: () => { } }) as unknown as nodeChildProcess.ChildProcessWithoutNullStreams; + }) as unknown as typeof nodeChildProcess.spawn); + const child = createTestChildProcess(4747); + + try { + terminateCliProcess(child, 'test Aspire CLI'); + + assert.deepStrictEqual(spawned, [{ command: 'taskkill.exe', args: ['/pid', '4747', '/t'] }]); + assert.strictEqual(child.kill.callCount, 0); + } + finally { + spawnStub.restore(); + platformStub.restore(); + } + }); + + test('force terminates a POSIX process group immediately without waiting for the grace period', async () => { + const platformStub = sinon.stub(process, 'platform').value('linux'); + const processKillStub = sinon.stub(process, 'kill').returns(true); + const clock = sinon.useFakeTimers(); + const childProcess = createTestChildProcess(4646); + const spawnStub = sinon.stub(nodeChildProcess, 'spawn').returns(childProcess); + const terminalProvider = { createEnvironment: () => ({}) } as AspireTerminalProvider; + + try { + const child = spawnCliProcess(terminalProvider, '/usr/local/bin/aspire', ['run'], { createProcessGroup: true }); + terminateCliProcess(child, 'test Aspire CLI', { force: true }); + + // No SIGTERM and no escalation timer: a caller that is itself shutting down cannot rely + // on an `unref`'d timer still being there five seconds later. + assert.deepStrictEqual(processKillStub.args, [[-4646, 'SIGKILL']]); + + await clock.tickAsync(5000); + assert.strictEqual(processKillStub.callCount, 1); + } + finally { + spawnStub.restore(); + clock.restore(); + processKillStub.restore(); + platformStub.restore(); + } + }); }); function createTestChildProcess(pid: number, exitCode: number | null = null): nodeChildProcess.ChildProcessWithoutNullStreams & { kill: sinon.SinonStub } { diff --git a/extension/src/test/rpc/aspireRpcServer.test.ts b/extension/src/test/rpc/aspireRpcServer.test.ts new file mode 100644 index 00000000000..459876e900b --- /dev/null +++ b/extension/src/test/rpc/aspireRpcServer.test.ts @@ -0,0 +1,199 @@ +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import * as tls from 'tls'; +import { createMessageConnection, ErrorCodes, MessageConnection, ResponseError } from 'vscode-jsonrpc'; +import { StreamMessageReader, StreamMessageWriter } from 'vscode-jsonrpc/node'; + +import AspireRpcServer from '../../server/AspireRpcServer'; +import { ICliRpcClient, RpcClient } from '../../server/rpcClient'; +import { extensionLogOutputChannel } from '../../utils/logging'; + +suite('AspireRpcServer', () => { + test('server disposal owns a client while its debug-session handshake is pending', async function () { + this.timeout(10000); + + const handshakeStarted = createDeferred(); + const handshakeResult = createDeferred(); + let rpcClient: RpcClient | undefined; + const rpcServer = await AspireRpcServer.create((_connectionInfo, connection) => { + rpcClient = new RpcClient(connection, null, () => null); + return rpcClient; + }); + const transport = await connectClient(rpcServer, async () => { + handshakeStarted.resolve(); + return await handshakeResult.promise; + }); + const transportClosed = new Promise(resolve => transport.socket.once('close', () => resolve())); + + try { + await handshakeStarted.promise; + assert.ok(rpcClient); + + rpcClient.interactionService.showStatus('Building AppHost...'); + assert.strictEqual((rpcClient.interactionService as any)._progressNotifier.isActive, true); + + rpcServer.dispose(); + await transportClosed; + + assert.strictEqual((rpcClient.interactionService as any)._progressNotifier.isActive, false); + assert.strictEqual(transport.socket.destroyed, true); + } + finally { + handshakeResult.resolve(null); + await new Promise(resolve => setImmediate(resolve)); + transport.connection.end(); + transport.connection.dispose(); + transport.socket.destroy(); + rpcClient?.dispose(); + } + }); + + test('a rejected debug-session handshake on an open transport is warned and disposes the pending client', async function () { + this.timeout(10000); + + const handshakeStarted = createDeferred(); + const handshakeResult = createDeferred(); + const clientDisposed = createDeferred(); + const warnStub = sinon.stub(extensionLogOutputChannel, 'warn'); + let rpcClient: RpcClient | undefined; + const rpcServer = await AspireRpcServer.create((_connectionInfo, connection) => { + rpcClient = new RpcClient(connection, null, () => null); + const originalDispose = rpcClient.dispose.bind(rpcClient); + rpcClient.dispose = () => { + originalDispose(); + clientDisposed.resolve(); + }; + return rpcClient; + }); + const transport = await connectClient(rpcServer, async () => { + handshakeStarted.resolve(); + return await handshakeResult.promise; + }); + + try { + await handshakeStarted.promise; + assert.ok(rpcClient); + + rpcClient.interactionService.showStatus('Connecting to AppHost...'); + handshakeResult.reject(new ResponseError(ErrorCodes.PendingResponseRejected, 'handshake failed')); + await clientDisposed.promise; + + sinon.assert.calledWithMatch(warnStub, 'Failed to initialize RPC client:'); + assert.strictEqual((rpcClient.interactionService as any)._progressNotifier.isActive, false); + assert.deepStrictEqual(rpcServer.connections, []); + } + finally { + transport.connection.end(); + transport.connection.dispose(); + transport.socket.destroy(); + warnStub.restore(); + rpcServer.dispose(); + } + }); + + test('transport disposal during the handshake logs the expected pending rejection at info', async function () { + this.timeout(10000); + + const handshakeStarted = createDeferred(); + const handshakeResult = createDeferred(); + const clientDisposed = createDeferred(); + const infoStub = sinon.stub(extensionLogOutputChannel, 'info'); + const warnStub = sinon.stub(extensionLogOutputChannel, 'warn'); + let rpcClient: RpcClient | undefined; + const rpcServer = await AspireRpcServer.create((_connectionInfo, connection) => { + rpcClient = new RpcClient(connection, null, () => null); + const originalDispose = rpcClient.dispose.bind(rpcClient); + rpcClient.dispose = () => { + originalDispose(); + clientDisposed.resolve(); + }; + return rpcClient; + }); + const transport = await connectClient(rpcServer, async () => { + handshakeStarted.resolve(); + return await handshakeResult.promise; + }); + + try { + await handshakeStarted.promise; + assert.ok(rpcClient); + + rpcClient.interactionService.showStatus('Connecting to AppHost...'); + transport.connection.end(); + transport.connection.dispose(); + transport.socket.destroy(); + await clientDisposed.promise; + await new Promise(resolve => setImmediate(resolve)); + + sinon.assert.calledWithMatch(infoStub, 'RPC client transport closed during initialization:'); + assert.strictEqual(warnStub.calledWithMatch('Failed to initialize RPC client:'), false); + assert.strictEqual((rpcClient.interactionService as any)._progressNotifier.isActive, false); + assert.deepStrictEqual(rpcServer.connections, []); + } + finally { + handshakeResult.resolve(null); + infoStub.restore(); + warnStub.restore(); + rpcServer.dispose(); + } + }); + + test('connections added after disposal are rejected and disposed without publishing them', async () => { + const rpcServer = await AspireRpcServer.create(() => { + throw new Error('The test does not establish a connection.'); + }); + const onNewConnection = sinon.spy(); + rpcServer.onNewConnection(onNewConnection); + const closeSpy = sinon.spy(rpcServer.server, 'close'); + let clientDisposeCount = 0; + const client = { + dispose: () => clientDisposeCount++, + } as unknown as ICliRpcClient; + + rpcServer.dispose(); + rpcServer.dispose(); + const added = rpcServer.addConnection(client); + + assert.strictEqual(added, false); + assert.strictEqual(clientDisposeCount, 1); + assert.deepStrictEqual(rpcServer.connections, []); + sinon.assert.notCalled(onNewConnection); + sinon.assert.calledOnce(closeSpy); + }); +}); + +async function connectClient( + rpcServer: AspireRpcServer, + getDebugSessionId: () => Promise +): Promise<{ connection: MessageConnection; socket: tls.TLSSocket }> { + const port = Number(rpcServer.connectionInfo.address.replace('localhost:', '')); + const socket = tls.connect({ + port, + host: 'localhost', + rejectUnauthorized: false, + }); + await new Promise((resolve, reject) => { + socket.once('secureConnect', resolve); + socket.once('error', reject); + }); + + const connection = createMessageConnection( + new StreamMessageReader(socket), + new StreamMessageWriter(socket) + ); + connection.onRequest('getDebugSessionId', getDebugSessionId); + connection.listen(); + + return { connection, socket }; +} + +function createDeferred(): { promise: Promise; resolve: (value: T) => void; reject: (reason: unknown) => void } { + let resolve!: (value: T) => void; + let reject!: (reason: unknown) => void; + const promise = new Promise((promiseResolve, promiseReject) => { + resolve = promiseResolve; + reject = promiseReject; + }); + + return { promise, resolve, reject }; +} diff --git a/extension/src/test/rpc/interactionServiceTests.test.ts b/extension/src/test/rpc/interactionServiceTests.test.ts index f9e1e46619f..7ea34196b0b 100644 --- a/extension/src/test/rpc/interactionServiceTests.test.ts +++ b/extension/src/test/rpc/interactionServiceTests.test.ts @@ -254,7 +254,9 @@ suite('InteractionService endpoints', () => { closeHandler = handler; return { dispose: () => { } }; }, - sendRequest: sinon.stub() + sendRequest: sinon.stub(), + end: sinon.stub(), + dispose: sinon.stub() } as any; const rpcClient = new RpcClient(messageConnection, null, () => null); @@ -267,6 +269,39 @@ suite('InteractionService endpoints', () => { assert.strictEqual((rpcClient.interactionService as any)._progressNotifier.isActive, false); }); + test("RPC client disposal closes the transport and prevents late status resurrection", () => { + const end = sinon.stub(); + const dispose = sinon.stub(); + const messageConnection = { + onClose: () => ({ dispose: () => { } }), + sendRequest: sinon.stub(), + end, + dispose + } as any; + const rpcClient = new RpcClient(messageConnection, null, () => null); + + rpcClient.interactionService.showStatus('Connecting to AppHost...'); + rpcClient.dispose(); + rpcClient.interactionService.showStatus('Starting Dashboard...'); + rpcClient.dispose(); + + assert.strictEqual((rpcClient.interactionService as any)._progressNotifier.isActive, false); + sinon.assert.calledOnce(end); + sinon.assert.calledOnce(dispose); + }); + + test("RPC server disposal clears CLI status when the connection never closes", async () => { + const testInfo = await createTestRpcServer(); + testInfo.rpcServer.addConnection(testInfo.rpcClient); + + testInfo.interactionService.showStatus('Building AppHost...'); + assert.strictEqual((testInfo.interactionService as any)._progressNotifier.isActive, true); + + testInfo.rpcServer.dispose(); + + assert.strictEqual((testInfo.interactionService as any)._progressNotifier.isActive, false); + }); + test("displaySubtleMessage endpoint", async () => { const testInfo = await createTestRpcServer(); const setStatusBarMessageSpy = sinon.spy(vscode.window, 'setStatusBarMessage'); @@ -825,6 +860,7 @@ suite('InteractionService endpoints', () => { type RpcServerTestInfo = { rpcServerInfo: RpcServerConnectionInfo; + rpcServer: AspireRpcServer; rpcClient: ICliRpcClient; interactionService: IInteractionService; }; @@ -887,6 +923,10 @@ class TestCliRpcClient implements ICliRpcClient { this.interactionService = new InteractionService(getAspireDebugSession, this, globalState); } + dispose(): void { + this.interactionService.dispose(); + } + stopCli(): Promise { return Promise.resolve(); } @@ -927,6 +967,7 @@ async function createTestRpcServer(debugSessionId?: string | null, getAspireDebu return { rpcServerInfo: rpcServer.connectionInfo, + rpcServer: rpcServer, rpcClient: rpcClient, interactionService: rpcClient.interactionService }; diff --git a/extension/src/testing/e2eStateFileBridge.ts b/extension/src/testing/e2eStateFileBridge.ts index 1953fecb7bd..51a27d6bceb 100644 --- a/extension/src/testing/e2eStateFileBridge.ts +++ b/extension/src/testing/e2eStateFileBridge.ts @@ -542,6 +542,25 @@ async function executeE2eControlCommand( const commands = await vscode.commands.getCommands(true); return commands.filter(commandId => commandId.startsWith('aspire-vscode.')).sort(); } + case 'getDebugSessionProcessInfo': { + markStarted(); + const state = createStateSnapshot(dataRepository, appHostLaunchService, appHostTreeProvider, aspireContext, true); + const appHostPath = command.appHostPath; + const debugSession = aspireContext.aspireDebugSessions.find(session => + appHostPath === undefined || + (typeof session.appHostPath === 'string' && isSamePath(session.appHostPath, appHostPath))); + const appHost = state.appHosts.find(candidate => + appHostPath === undefined || isSamePath(candidate.appHostPath, appHostPath)) ?? + (state.workspaceAppHost && (appHostPath === undefined || isSamePath(state.workspaceAppHost.appHostPath, appHostPath)) + ? state.workspaceAppHost + : undefined); + + return { + appHostPath: debugSession?.appHostPath ?? appHost?.appHostPath, + cliPid: debugSession?.cliProcessId, + appHostPid: appHost?.appHostPid, + }; + } case 'getResourceDebuggerExtensions': { markStarted(); return getResourceDebuggerExtensions().map(extension => ({ @@ -643,6 +662,22 @@ async function executeE2eControlCommand( await vscode.commands.executeCommand('vscode.openFolder', vscode.Uri.file(folderPath), false); return undefined; } + case 'stopOwnedDebugSessionProcesses': { + markStarted(); + const appHostPath = command.appHostPath; + const debugSessions = aspireContext.aspireDebugSessions.filter(session => + appHostPath === undefined || + (typeof session.appHostPath === 'string' && isSamePath(session.appHostPath, appHostPath))); + await Promise.race([ + Promise.allSettled(debugSessions.map(session => session.requestCliStopForExtensionShutdown())), + delay(5000), + ]); + for (const session of debugSessions) { + session.terminateCliProcessTree({ force: true }); + } + + return undefined; + } case 'getWorkspaceFolders': { markStarted(); return vscode.workspace.workspaceFolders?.map(folder => ({ diff --git a/extension/src/types/extensionApi.ts b/extension/src/types/extensionApi.ts index 76b4455294d..913c2a088d7 100644 --- a/extension/src/types/extensionApi.ts +++ b/extension/src/types/extensionApi.ts @@ -196,6 +196,7 @@ export type AspireExtensionE2EControlCommand = | { name: 'stopDebugging' } | { name: 'closeAllEditors' } | { name: 'getRegisteredAspireCommands' } + | { name: 'getDebugSessionProcessInfo'; appHostPath?: string } | { name: 'getExtensionPackageJson' } | { name: 'getExtensionFileStatus'; relativePaths: readonly string[] } | { name: 'getDiagnostics'; filePath: string } @@ -205,6 +206,7 @@ export type AspireExtensionE2EControlCommand = | { name: 'assertClipboardMatchesLastExpectation' } | { name: 'openFile'; filePath: string } | { name: 'openWorkspaceFolder'; folderPath: string } + | { name: 'stopOwnedDebugSessionProcesses'; appHostPath?: string } | { name: 'getWorkspaceFolders' } | { name: 'getActiveEditor' } | { name: 'getResourceDebuggerExtensions' }