From 018fcea1dadeea6704937413982b661d43368fd0 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Wed, 1 Apr 2026 18:13:30 +0800 Subject: [PATCH] Revert "Merge pull request #2666 from qqqys/feat/vscode-acp-reconnect-logic" This reverts commit cd47e9b653abbaa845494c256c930e0bc9be3c03, reversing changes made to 449a421ca7cfacdf1eda2db77eee83de4799d3bc. --- .../src/services/acpConnection.test.ts | 176 +----------------- .../src/services/acpConnection.ts | 174 +---------------- .../src/services/qwenAgentManager.ts | 67 ------- .../services/qwenConnectionHandler.test.ts | 30 ++- .../src/services/qwenConnectionHandler.ts | 2 +- .../src/webview/providers/WebViewProvider.ts | 35 +--- 6 files changed, 19 insertions(+), 465 deletions(-) diff --git a/packages/vscode-ide-companion/src/services/acpConnection.test.ts b/packages/vscode-ide-companion/src/services/acpConnection.test.ts index 6b6a1dec10..5785a945bc 100644 --- a/packages/vscode-ide-companion/src/services/acpConnection.test.ts +++ b/packages/vscode-ide-companion/src/services/acpConnection.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, expect, it, vi, beforeEach } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { RequestError } from '@agentclientprotocol/sdk'; import type { ContentBlock } from '@agentclientprotocol/sdk'; @@ -21,11 +21,8 @@ type AcpConnectionInternal = { sessionId: string | null; lastExitCode: number | null; lastExitSignal: string | null; - intentionalDisconnect: boolean; - autoReconnectAttempts: number; mapReadTextFileError: (error: unknown, filePath: string) => unknown; ensureConnection: () => unknown; - cleanupForRetry: () => void; }; function createConnection(overrides?: Partial) { @@ -227,174 +224,3 @@ describe('AcpConnection lastExitCode/lastExitSignal', () => { expect(conn.lastExitSignal).toBeNull(); }); }); - -describe('AcpConnection.connectWithRetry', () => { - let acpConn: AcpConnection; - - beforeEach(() => { - acpConn = new AcpConnection(); - }); - - it('succeeds on first attempt without retrying', async () => { - const connectSpy = vi - .spyOn(acpConn, 'connect') - .mockResolvedValueOnce(undefined); - - await acpConn.connectWithRetry('/path/to/cli.js', '/workdir', [], 3); - - expect(connectSpy).toHaveBeenCalledTimes(1); - expect(connectSpy).toHaveBeenCalledWith('/path/to/cli.js', '/workdir', []); - }); - - it('retries on failure and succeeds on second attempt', async () => { - const connectSpy = vi - .spyOn(acpConn, 'connect') - .mockRejectedValueOnce(new Error('SIGTERM')) - .mockResolvedValueOnce(undefined); - - await acpConn.connectWithRetry('/path/to/cli.js', '/workdir', [], 3); - - expect(connectSpy).toHaveBeenCalledTimes(2); - }); - - it('throws after all retries are exhausted', async () => { - const error = new Error('persistent failure'); - const connectSpy = vi.spyOn(acpConn, 'connect').mockRejectedValue(error); - - await expect( - acpConn.connectWithRetry('/path/to/cli.js', '/workdir', [], 2), - ).rejects.toThrow('persistent failure'); - - // 1 initial + 2 retries = 3 total - expect(connectSpy).toHaveBeenCalledTimes(3); - }); - - it('cleans up state between retry attempts', async () => { - const internal = acpConn as unknown as AcpConnectionInternal; - const cleanupSpy = vi.spyOn(internal, 'cleanupForRetry' as never); - - vi.spyOn(acpConn, 'connect') - .mockRejectedValueOnce(new Error('fail 1')) - .mockResolvedValueOnce(undefined); - - await acpConn.connectWithRetry('/path/to/cli.js', '/workdir', [], 3); - - // cleanupForRetry called once for the failed attempt - expect(cleanupSpy).toHaveBeenCalledTimes(1); - }); - - it('resets autoReconnectAttempts on successful connect', async () => { - const internal = acpConn as unknown as AcpConnectionInternal; - internal.autoReconnectAttempts = 5; - - vi.spyOn(acpConn, 'connect').mockResolvedValueOnce(undefined); - - await acpConn.connectWithRetry('/path/to/cli.js', '/workdir', [], 3); - - expect(acpConn.currentAutoReconnectAttempts).toBe(0); - }); -}); - -describe('AcpConnection.cleanupForRetry', () => { - it('kills zombie child process and resets state', () => { - const mockKill = vi.fn(); - const conn = createConnection({ - child: createMockChild({ kill: mockKill, killed: false }), - sdkConnection: { fake: true }, - sessionId: 'test-session', - lastExitCode: 1, - lastExitSignal: 'SIGTERM', - }); - - conn.cleanupForRetry(); - - expect(mockKill).toHaveBeenCalledOnce(); - expect(conn.child).toBeNull(); - expect(conn.sdkConnection).toBeNull(); - expect(conn.sessionId).toBeNull(); - expect(conn.lastExitCode).toBeNull(); - expect(conn.lastExitSignal).toBeNull(); - }); - - it('handles already-killed child process gracefully', () => { - const conn = createConnection({ - child: createMockChild({ killed: true }), - sdkConnection: { fake: true }, - sessionId: 'test', - }); - - expect(() => conn.cleanupForRetry()).not.toThrow(); - expect(conn.child).toBeNull(); - }); - - it('handles null child process gracefully', () => { - const conn = createConnection({ - child: null, - sdkConnection: { fake: true }, - sessionId: 'test', - }); - - expect(() => conn.cleanupForRetry()).not.toThrow(); - }); -}); - -describe('AcpConnection intentionalDisconnect flag', () => { - it('defaults to false', () => { - const acpConn = new AcpConnection(); - expect(acpConn.wasIntentionalDisconnect).toBe(false); - }); - - it('is set to true by disconnect()', () => { - const conn = createConnection({ - child: createMockChild(), - sdkConnection: {}, - sessionId: 'test', - }); - const acpConn = conn as unknown as AcpConnection; - - acpConn.disconnect(); - - expect(acpConn.wasIntentionalDisconnect).toBe(true); - }); - - it('is reset to false when connect() is called', async () => { - const internal = new AcpConnection() as unknown as AcpConnectionInternal; - internal.intentionalDisconnect = true; - - // connect() will throw because we haven't set up a real subprocess, - // but the flag should be reset before the error - try { - await (internal as unknown as AcpConnection).connect( - '/nonexistent/cli.js', - '/workdir', - ); - } catch { - // Expected to fail - } - - expect(internal.intentionalDisconnect).toBe(false); - }); -}); - -describe('AcpConnection auto-reconnect counter', () => { - it('defaults to 0', () => { - const acpConn = new AcpConnection(); - expect(acpConn.currentAutoReconnectAttempts).toBe(0); - }); - - it('increments via incrementAutoReconnectAttempts()', () => { - const acpConn = new AcpConnection(); - acpConn.incrementAutoReconnectAttempts(); - expect(acpConn.currentAutoReconnectAttempts).toBe(1); - acpConn.incrementAutoReconnectAttempts(); - expect(acpConn.currentAutoReconnectAttempts).toBe(2); - }); - - it('resets via resetAutoReconnectAttempts()', () => { - const acpConn = new AcpConnection(); - acpConn.incrementAutoReconnectAttempts(); - acpConn.incrementAutoReconnectAttempts(); - acpConn.resetAutoReconnectAttempts(); - expect(acpConn.currentAutoReconnectAttempts).toBe(0); - }); -}); diff --git a/packages/vscode-ide-companion/src/services/acpConnection.ts b/packages/vscode-ide-companion/src/services/acpConnection.ts index e7b3b15591..deb04f24cd 100644 --- a/packages/vscode-ide-companion/src/services/acpConnection.ts +++ b/packages/vscode-ide-companion/src/services/acpConnection.ts @@ -55,10 +55,6 @@ export class AcpConnection { private fileHandler = new AcpFileHandler(); private lastExitCode: number | null = null; private lastExitSignal: string | null = null; - /** Set to true when disconnect() is called intentionally by the extension. */ - private intentionalDisconnect: boolean = false; - /** Tracks auto-reconnect attempts to prevent infinite loops. */ - private autoReconnectAttempts: number = 0; onSessionUpdate: (data: SessionNotification) => void = () => {}; onPermissionRequest: (data: RequestPermissionRequest) => Promise<{ @@ -90,7 +86,6 @@ export class AcpConnection { this.lastExitCode = null; this.lastExitSignal = null; - this.intentionalDisconnect = false; this.workingDir = workingDir; const env = { ...process.env }; @@ -186,57 +181,7 @@ export class AcpConnection { } }); - // Wait for readiness: resolve on first stdout data, reject on exit or timeout. - const READINESS_TIMEOUT_MS = 10_000; - await new Promise((resolve, reject) => { - const child = this.child!; - let settled = false; - - const cleanup = () => { - child.stdout?.removeListener('data', onData); - child.removeListener('exit', onExit); - clearTimeout(timer); - }; - - const onData = () => { - if (settled) return; - settled = true; - cleanup(); - resolve(); - }; - - const onExit = (code: number | null, signal: string | null) => { - if (settled) return; - settled = true; - cleanup(); - reject( - new Error( - `Qwen ACP process exited before becoming ready (exit code: ${code}, signal: ${signal})`, - ), - ); - }; - - const timer = setTimeout(() => { - if (settled) return; - settled = true; - cleanup(); - reject( - new Error( - `Qwen ACP process did not become ready within ${READINESS_TIMEOUT_MS / 1000}s`, - ), - ); - }, READINESS_TIMEOUT_MS); - - child.stdout?.on('data', onData); - child.on('exit', onExit); - - // Also handle spawn errors that occurred before this point - if (spawnError) { - settled = true; - cleanup(); - reject(spawnError); - } - }); + await new Promise((resolve) => setTimeout(resolve, 1000)); if (spawnError) { throw spawnError; @@ -407,12 +352,10 @@ export class AcpConnection { stream, ); - // Initialize protocol via SDK with timeout // Race the SDK initialize against process exit so we don't hang forever // if the CLI crashes before responding. console.log('[ACP] Sending initialize request...'); - const INITIALIZE_TIMEOUT_MS = 15_000; - const initPromise = Promise.race([ + const initResponse = await Promise.race([ this.sdkConnection.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: { @@ -425,27 +368,6 @@ export class AcpConnection { processExitPromise, ]); - const timeoutPromise = new Promise((_resolve, reject) => { - setTimeout(() => { - reject( - new Error( - `ACP initialize handshake timed out after ${INITIALIZE_TIMEOUT_MS / 1000}s`, - ), - ); - }, INITIALIZE_TIMEOUT_MS); - }); - - let initResponse; - try { - initResponse = await Promise.race([initPromise, timeoutPromise]); - } catch (error) { - // On timeout or init failure, kill the subprocess to avoid orphans - if (this.child && !this.child.killed) { - this.child.kill(); - } - throw error; - } - console.log('[ACP] Initialize successful'); console.log('[ACP] Initialization response:', initResponse); try { @@ -657,79 +579,7 @@ export class AcpConnection { return res; } - /** - * Connect with retry logic. Retries the full connect() call up to - * {@link maxRetries} times with exponential backoff on failure. - * Cleans up any partial state between attempts. - */ - async connectWithRetry( - cliEntryPath: string, - workingDir: string = process.cwd(), - extraArgs: string[] = [], - maxRetries: number = 3, - ): Promise { - const backoffDelays = [1000, 2000, 4000]; - let lastError: Error | undefined; - - for (let attempt = 0; attempt <= maxRetries; attempt++) { - try { - if (attempt > 0) { - console.log(`[ACP] Spawn retry attempt ${attempt}/${maxRetries}...`); - } - await this.connect(cliEntryPath, workingDir, extraArgs); - // Success — reset auto-reconnect counter - this.autoReconnectAttempts = 0; - return; - } catch (error) { - lastError = error instanceof Error ? error : new Error(String(error)); - console.error( - `[ACP] Connect attempt ${attempt + 1} failed:`, - lastError.message, - ); - - // Clean up any partial state before retry - this.cleanupForRetry(); - - if (attempt < maxRetries) { - const delay = - backoffDelays[attempt] ?? backoffDelays[backoffDelays.length - 1]; - console.log(`[ACP] Retrying in ${delay}ms...`); - await new Promise((resolve) => setTimeout(resolve, delay)); - } - } - } - - throw ( - lastError ?? - new Error( - `ACP connection failed after ${maxRetries + 1} attempts. The Qwen CLI subprocess could not be started.`, - ) - ); - } - - /** - * Clean up partial state after a failed connect attempt, - * preparing for a clean retry. - */ - private cleanupForRetry(): void { - if (this.child) { - try { - if (!this.child.killed) { - this.child.kill(); - } - } catch { - // Ignore kill errors during cleanup - } - this.child = null; - } - this.sdkConnection = null; - this.sessionId = null; - this.lastExitCode = null; - this.lastExitSignal = null; - } - disconnect(): void { - this.intentionalDisconnect = true; if (this.child) { this.child.kill(); this.child = null; @@ -744,26 +594,6 @@ export class AcpConnection { ); } - /** Whether the last disconnect was intentionally triggered by the extension. */ - get wasIntentionalDisconnect(): boolean { - return this.intentionalDisconnect; - } - - /** Current auto-reconnect attempt count. */ - get currentAutoReconnectAttempts(): number { - return this.autoReconnectAttempts; - } - - /** Increment the auto-reconnect attempt counter. */ - incrementAutoReconnectAttempts(): void { - this.autoReconnectAttempts++; - } - - /** Reset the auto-reconnect attempt counter (e.g., after successful reconnection). */ - resetAutoReconnectAttempts(): void { - this.autoReconnectAttempts = 0; - } - get hasActiveSession(): boolean { return this.sessionId !== null; } diff --git a/packages/vscode-ide-companion/src/services/qwenAgentManager.ts b/packages/vscode-ide-companion/src/services/qwenAgentManager.ts index 92561c1c6b..883a226eea 100644 --- a/packages/vscode-ide-companion/src/services/qwenAgentManager.ts +++ b/packages/vscode-ide-companion/src/services/qwenAgentManager.ts @@ -91,10 +91,6 @@ export class QwenAgentManager { private connectionHandler: QwenConnectionHandler; private sessionUpdateHandler: QwenSessionUpdateHandler; private currentWorkingDir: string = process.cwd(); - /** Last CLI entry path used for connecting, needed for auto-reconnect. */ - private lastCliEntryPath: string | null = null; - /** Callback invoked when auto-reconnect fails and user action is needed. */ - onAutoReconnectFailed: (errorMessage: string) => void = () => {}; // When loading a past session via ACP, the CLI replays history through // session/update notifications. We set this flag to route message chunks // (user/assistant) as discrete chat messages instead of live streaming. @@ -298,68 +294,6 @@ export class QwenAgentManager { console.warn('[QwenAgentManager] onInitialized parse error:', err); } }; - - // Auto-reconnect on unexpected subprocess exit - this.connection.onDisconnected = ( - code: number | null, - signal: string | null, - ) => { - // Skip reconnection for intentional disconnects or clean exits - if (this.connection.wasIntentionalDisconnect) { - console.log( - '[QwenAgentManager] Intentional disconnect, skipping auto-reconnect.', - ); - return; - } - if (code === 0 && signal === null) { - console.log( - '[QwenAgentManager] Clean exit (code 0), skipping auto-reconnect.', - ); - return; - } - - // Limit auto-reconnect to 1 attempt - if (this.connection.currentAutoReconnectAttempts >= 1) { - console.warn( - '[QwenAgentManager] Auto-reconnect limit reached. User action required.', - ); - this.onAutoReconnectFailed( - `Qwen ACP process exited unexpectedly (exit code: ${code}, signal: ${signal}). Automatic reconnection failed.`, - ); - return; - } - - console.log( - `[QwenAgentManager] Unexpected subprocess exit (code: ${code}, signal: ${signal}). Attempting auto-reconnect...`, - ); - this.connection.incrementAutoReconnectAttempts(); - - if (!this.lastCliEntryPath) { - console.error( - '[QwenAgentManager] Cannot auto-reconnect: no CLI entry path stored.', - ); - this.onAutoReconnectFailed( - `Qwen ACP process exited unexpectedly (exit code: ${code}, signal: ${signal}). Cannot reconnect automatically.`, - ); - return; - } - - // Attempt reconnection asynchronously - this.connectionHandler - .connect(this.connection, this.currentWorkingDir, this.lastCliEntryPath) - .then(() => { - console.log('[QwenAgentManager] Auto-reconnect succeeded.'); - // Reset counter on success so future crashes can also auto-reconnect - this.connection.resetAutoReconnectAttempts(); - }) - .catch((err) => { - const errorMsg = getErrorMessage(err); - console.error('[QwenAgentManager] Auto-reconnect failed:', errorMsg); - this.onAutoReconnectFailed( - `Qwen ACP process exited unexpectedly (exit code: ${code}, signal: ${signal}). Reconnection failed: ${errorMsg}`, - ); - }); - }; } /** @@ -374,7 +308,6 @@ export class QwenAgentManager { options?: AgentConnectOptions, ): Promise { this.currentWorkingDir = workingDir; - this.lastCliEntryPath = cliEntryPath; const res = await this.connectionHandler.connect( this.connection, workingDir, diff --git a/packages/vscode-ide-companion/src/services/qwenConnectionHandler.test.ts b/packages/vscode-ide-companion/src/services/qwenConnectionHandler.test.ts index 1fa9ff36ef..21f2f4b0f4 100644 --- a/packages/vscode-ide-companion/src/services/qwenConnectionHandler.test.ts +++ b/packages/vscode-ide-companion/src/services/qwenConnectionHandler.test.ts @@ -30,7 +30,6 @@ describe('QwenConnectionHandler', () => { handler = new QwenConnectionHandler(); mockConnection = { connect: vi.fn().mockResolvedValue(undefined), - connectWithRetry: vi.fn().mockResolvedValue(undefined), newSession: vi.fn().mockResolvedValue({ sessionId: 'test-session' }), authenticate: vi.fn().mockResolvedValue({}), } as unknown as AcpConnection; @@ -53,10 +52,9 @@ describe('QwenConnectionHandler', () => { await handler.connect(mockConnection, '/workspace', '/path/to/cli.js'); - expect(mockConnection.connectWithRetry).toHaveBeenCalled(); - const connectArgs = ( - mockConnection.connectWithRetry as ReturnType - ).mock.calls[0]; + expect(mockConnection.connect).toHaveBeenCalled(); + const connectArgs = (mockConnection.connect as ReturnType) + .mock.calls[0]; expect(connectArgs[2]).toContain('--proxy'); expect(connectArgs[2]).toContain('http://proxy.example.com:8080'); }); @@ -76,10 +74,9 @@ describe('QwenConnectionHandler', () => { await handler.connect(mockConnection, '/workspace', '/path/to/cli.js'); - expect(mockConnection.connectWithRetry).toHaveBeenCalled(); - const connectArgs = ( - mockConnection.connectWithRetry as ReturnType - ).mock.calls[0]; + expect(mockConnection.connect).toHaveBeenCalled(); + const connectArgs = (mockConnection.connect as ReturnType) + .mock.calls[0]; expect(connectArgs[2]).toContain('--proxy'); expect(connectArgs[2]).toContain('http://https-proxy.example.com:8080'); }); @@ -99,9 +96,8 @@ describe('QwenConnectionHandler', () => { await handler.connect(mockConnection, '/workspace', '/path/to/cli.js'); - const connectArgs = ( - mockConnection.connectWithRetry as ReturnType - ).mock.calls[0]; + const connectArgs = (mockConnection.connect as ReturnType) + .mock.calls[0]; expect(connectArgs[2]).toContain('http://http-proxy.example.com:8080'); expect(connectArgs[2]).not.toContain( 'http://https-proxy.example.com:8080', @@ -115,9 +111,8 @@ describe('QwenConnectionHandler', () => { await handler.connect(mockConnection, '/workspace', '/path/to/cli.js'); - const connectArgs = ( - mockConnection.connectWithRetry as ReturnType - ).mock.calls[0]; + const connectArgs = (mockConnection.connect as ReturnType) + .mock.calls[0]; expect(connectArgs[2]).not.toContain('--proxy'); }); @@ -133,9 +128,8 @@ describe('QwenConnectionHandler', () => { await handler.connect(mockConnection, '/workspace', '/path/to/cli.js'); - const connectArgs = ( - mockConnection.connectWithRetry as ReturnType - ).mock.calls[0]; + const connectArgs = (mockConnection.connect as ReturnType) + .mock.calls[0]; expect(connectArgs[2]).not.toContain('--proxy'); }); }); diff --git a/packages/vscode-ide-companion/src/services/qwenConnectionHandler.ts b/packages/vscode-ide-companion/src/services/qwenConnectionHandler.ts index f674ed7b93..6ba990317f 100644 --- a/packages/vscode-ide-companion/src/services/qwenConnectionHandler.ts +++ b/packages/vscode-ide-companion/src/services/qwenConnectionHandler.ts @@ -85,7 +85,7 @@ export class QwenConnectionHandler { ); } - await connection.connectWithRetry(cliEntryPath!, workingDir, extraArgs); + await connection.connect(cliEntryPath!, workingDir, extraArgs); // Try to restore existing session or create new session // Note: Auto-restore on connect is disabled to avoid surprising loads diff --git a/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts b/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts index 8459483034..b1cebfe4c5 100644 --- a/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts +++ b/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts @@ -206,25 +206,6 @@ export class WebViewProvider { }); }); - // Handle auto-reconnect failure: show VS Code notification with retry button - this.agentManager.onAutoReconnectFailed = (errorMessage: string) => { - vscode.window - .showWarningMessage(errorMessage, 'Retry Connection') - .then((selection) => { - if (selection === 'Retry Connection') { - this.doInitializeAgentConnection({ autoAuthenticate: true }); - } - }); - - // Notify webview that connection was lost - this.sendMessageToWebView({ - type: 'agentConnectionError', - data: { - message: errorMessage, - }, - }); - }; - // Setup end-turn handler from ACP stopReason notifications this.agentManager.onEndTurn((reason) => { // Ensure WebView exits streaming state even if no explicit streamEnd was emitted elsewhere @@ -898,19 +879,9 @@ export class WebViewProvider { } catch (_error) { const errorMsg = getErrorMessage(_error); console.error('[WebViewProvider] Agent connection error:', _error); - - // Show warning with a "Retry Connection" action button - vscode.window - .showWarningMessage( - `Failed to connect to Qwen CLI: ${errorMsg}`, - 'Retry Connection', - ) - .then((selection) => { - if (selection === 'Retry Connection') { - this.doInitializeAgentConnection({ autoAuthenticate: true }); - } - }); - + vscode.window.showWarningMessage( + `Failed to connect to Qwen CLI: ${errorMsg}\nYou can still use the chat UI, but messages won't be sent to AI.`, + ); // Fallback to empty conversation await this.initializeEmptyConversation();