mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-04 22:51:08 +00:00
fix: clean up MCP subprocesses on ACP shutdown to prevent orphan processes
PR #2472 added SIGTERM/SIGINT handlers in runAcpAgent() that destroy stdin/stdout streams, allowing the ACP connection to close. However, it did not call runExitCleanup() to terminate child processes (MCP servers, etc.) spawned by the CLI. This caused orphan subprocesses to persist after the IDE disconnects, especially noticeable in VSCode with multiple tabs. Changes: - acpAgent.ts: Call runExitCleanup() in the shutdown signal handler before process.exit(0), ensuring MCP server subprocesses are killed - gemini.tsx: Call runExitCleanup() + process.exit(0) after runAcpAgent() returns normally, matching the cleanup pattern used by other non-interactive exit paths - Add 4 unit tests covering SIGTERM cleanup, SIGINT cleanup, idempotent shutdown, and cleanup-failure resilience Closes #1884
This commit is contained in:
parent
28e62882f0
commit
38d0acb01c
3 changed files with 219 additions and 1 deletions
207
packages/cli/src/acp-integration/acpAgent.test.ts
Normal file
207
packages/cli/src/acp-integration/acpAgent.test.ts
Normal file
|
|
@ -0,0 +1,207 @@
|
|||
/**
|
||||
* @license
|
||||
* Copyright 2025 Qwen Team
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
|
||||
|
||||
// Mock cleanup module before importing anything else
|
||||
const { mockRunExitCleanup } = vi.hoisted(() => ({
|
||||
mockRunExitCleanup: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
vi.mock('../utils/cleanup.js', () => ({
|
||||
runExitCleanup: mockRunExitCleanup,
|
||||
}));
|
||||
|
||||
// Mock the ACP SDK
|
||||
const { mockConnectionState } = vi.hoisted(() => {
|
||||
const state = {
|
||||
resolve: () => {},
|
||||
promise: null as unknown as Promise<void>,
|
||||
reset() {
|
||||
state.promise = new Promise<void>((r) => {
|
||||
state.resolve = r;
|
||||
});
|
||||
},
|
||||
};
|
||||
state.reset();
|
||||
return { mockConnectionState: state };
|
||||
});
|
||||
|
||||
vi.mock('@agentclientprotocol/sdk', () => ({
|
||||
AgentSideConnection: vi.fn().mockImplementation(() => ({
|
||||
get closed() {
|
||||
return mockConnectionState.promise;
|
||||
},
|
||||
})),
|
||||
ndJsonStream: vi.fn().mockReturnValue({}),
|
||||
RequestError: class RequestError extends Error {},
|
||||
PROTOCOL_VERSION: '1.0.0',
|
||||
}));
|
||||
|
||||
// Mock stream conversion
|
||||
vi.mock('node:stream', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('node:stream')>();
|
||||
return {
|
||||
...actual,
|
||||
Writable: { ...actual.Writable, toWeb: vi.fn().mockReturnValue({}) },
|
||||
Readable: { ...actual.Readable, toWeb: vi.fn().mockReturnValue({}) },
|
||||
};
|
||||
});
|
||||
|
||||
// Mock core dependencies
|
||||
vi.mock('@qwen-code/qwen-code-core', () => ({
|
||||
createDebugLogger: () => ({
|
||||
debug: vi.fn(),
|
||||
error: vi.fn(),
|
||||
warn: vi.fn(),
|
||||
}),
|
||||
APPROVAL_MODE_INFO: {},
|
||||
APPROVAL_MODES: [],
|
||||
AuthType: {},
|
||||
clearCachedCredentialFile: vi.fn(),
|
||||
QwenOAuth2Event: {},
|
||||
qwenOAuth2Events: { on: vi.fn(), off: vi.fn() },
|
||||
MCPServerConfig: {},
|
||||
SessionService: vi.fn(),
|
||||
tokenLimit: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock('./authMethods.js', () => ({ buildAuthMethods: vi.fn() }));
|
||||
vi.mock('./service/filesystem.js', () => ({
|
||||
AcpFileSystemService: vi.fn(),
|
||||
}));
|
||||
vi.mock('../config/settings.js', () => ({ SettingScope: {} }));
|
||||
vi.mock('../config/config.js', () => ({ loadCliConfig: vi.fn() }));
|
||||
vi.mock('./session/Session.js', () => ({ Session: vi.fn() }));
|
||||
vi.mock('../utils/acpModelUtils.js', () => ({
|
||||
formatAcpModelId: vi.fn(),
|
||||
}));
|
||||
|
||||
import { runAcpAgent } from './acpAgent.js';
|
||||
import type { Config } from '@qwen-code/qwen-code-core';
|
||||
import type { LoadedSettings } from '../config/settings.js';
|
||||
import type { CliArgs } from '../config/config.js';
|
||||
|
||||
describe('runAcpAgent shutdown cleanup', () => {
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
let processExitSpy: any;
|
||||
let sigTermListeners: NodeJS.SignalsListener[];
|
||||
let sigIntListeners: NodeJS.SignalsListener[];
|
||||
|
||||
const mockConfig = {} as Config;
|
||||
const mockSettings = { merged: {} } as LoadedSettings;
|
||||
const mockArgv = {} as CliArgs;
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
mockRunExitCleanup.mockResolvedValue(undefined);
|
||||
mockConnectionState.reset();
|
||||
sigTermListeners = [];
|
||||
sigIntListeners = [];
|
||||
|
||||
// Intercept signal handler registration
|
||||
vi.spyOn(process, 'on').mockImplementation(((
|
||||
event: string,
|
||||
listener: (...args: unknown[]) => void,
|
||||
) => {
|
||||
if (event === 'SIGTERM')
|
||||
sigTermListeners.push(listener as NodeJS.SignalsListener);
|
||||
if (event === 'SIGINT')
|
||||
sigIntListeners.push(listener as NodeJS.SignalsListener);
|
||||
return process;
|
||||
}) as typeof process.on);
|
||||
|
||||
vi.spyOn(process, 'off').mockImplementation(
|
||||
(() => process) as typeof process.off,
|
||||
);
|
||||
|
||||
// Mock process.exit to prevent actually exiting
|
||||
processExitSpy = vi
|
||||
.spyOn(process, 'exit')
|
||||
.mockImplementation((() => undefined) as unknown as typeof process.exit);
|
||||
|
||||
// Mock stdin/stdout destroy
|
||||
vi.spyOn(process.stdin, 'destroy').mockImplementation(() => process.stdin);
|
||||
vi.spyOn(process.stdout, 'destroy').mockImplementation(
|
||||
() => process.stdout,
|
||||
);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
processExitSpy.mockRestore();
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it('calls runExitCleanup and process.exit on SIGTERM', async () => {
|
||||
// Start runAcpAgent (it will await connection.closed)
|
||||
const agentPromise = runAcpAgent(mockConfig, mockSettings, mockArgv);
|
||||
|
||||
// Simulate SIGTERM from IDE
|
||||
expect(sigTermListeners.length).toBeGreaterThan(0);
|
||||
sigTermListeners[0]('SIGTERM');
|
||||
|
||||
// runExitCleanup is async, wait for it
|
||||
await vi.waitFor(() => {
|
||||
expect(mockRunExitCleanup).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(processExitSpy).toHaveBeenCalledWith(0);
|
||||
});
|
||||
|
||||
// Resolve connection.closed so the promise settles
|
||||
mockConnectionState.resolve();
|
||||
await agentPromise;
|
||||
});
|
||||
|
||||
it('calls runExitCleanup and process.exit on SIGINT', async () => {
|
||||
const agentPromise = runAcpAgent(mockConfig, mockSettings, mockArgv);
|
||||
|
||||
expect(sigIntListeners.length).toBeGreaterThan(0);
|
||||
sigIntListeners[0]('SIGINT');
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(mockRunExitCleanup).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(processExitSpy).toHaveBeenCalledWith(0);
|
||||
});
|
||||
|
||||
mockConnectionState.resolve();
|
||||
await agentPromise;
|
||||
});
|
||||
|
||||
it('only runs shutdown once even if multiple signals arrive', async () => {
|
||||
const agentPromise = runAcpAgent(mockConfig, mockSettings, mockArgv);
|
||||
|
||||
// Send SIGTERM twice
|
||||
sigTermListeners[0]('SIGTERM');
|
||||
sigTermListeners[0]('SIGTERM');
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(mockRunExitCleanup).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
mockConnectionState.resolve();
|
||||
await agentPromise;
|
||||
});
|
||||
|
||||
it('still exits even if runExitCleanup throws', async () => {
|
||||
mockRunExitCleanup.mockRejectedValueOnce(new Error('cleanup failed'));
|
||||
|
||||
const agentPromise = runAcpAgent(mockConfig, mockSettings, mockArgv);
|
||||
|
||||
sigTermListeners[0]('SIGTERM');
|
||||
|
||||
// process.exit should still be called via .finally()
|
||||
await vi.waitFor(() => {
|
||||
expect(processExitSpy).toHaveBeenCalledWith(0);
|
||||
});
|
||||
|
||||
mockConnectionState.resolve();
|
||||
await agentPromise;
|
||||
});
|
||||
});
|
||||
|
|
@ -65,6 +65,7 @@ import { loadCliConfig } from '../config/config.js';
|
|||
import { Session } from './session/Session.js';
|
||||
import { formatAcpModelId } from '../utils/acpModelUtils.js';
|
||||
import { runWithAcpRuntimeOutputDir } from './runtimeOutputDirContext.js';
|
||||
import { runExitCleanup } from '../utils/cleanup.js';
|
||||
|
||||
const debugLogger = createDebugLogger('ACP_AGENT');
|
||||
|
||||
|
|
@ -107,6 +108,14 @@ export async function runAcpAgent(
|
|||
} catch {
|
||||
// stdout may already be closed
|
||||
}
|
||||
// Clean up child processes (MCP servers, etc.) and force exit.
|
||||
// Without this, orphan subprocesses keep the Node.js event loop alive
|
||||
// and the CLI process never terminates after the IDE disconnects.
|
||||
runExitCleanup()
|
||||
.catch(() => {})
|
||||
.finally(() => {
|
||||
process.exit(0);
|
||||
});
|
||||
};
|
||||
process.on('SIGTERM', shutdownHandler);
|
||||
process.on('SIGINT', shutdownHandler);
|
||||
|
|
|
|||
|
|
@ -408,7 +408,9 @@ export async function main() {
|
|||
const initializationResult = await initializeApp(config, settings);
|
||||
|
||||
if (config.getExperimentalZedIntegration()) {
|
||||
return runAcpAgent(config, settings, argv);
|
||||
await runAcpAgent(config, settings, argv);
|
||||
await runExitCleanup();
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
let input = config.getQuestion();
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue