mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-01 21:20:44 +00:00
Merge pull request #1146 from QwenLM/fix/windows-background-terminal-execute-x
fix: improve windows background process handling and cleanup
This commit is contained in:
tanzhenxin
GitHub
commit
3e695cd82b
4 changed files with 158 additions and 28 deletions
|
|
@ -589,7 +589,7 @@ describe('ShellExecutionService child_process fallback', () => {
|
|||
expect(result.error).toBeNull();
|
||||
expect(result.aborted).toBe(false);
|
||||
expect(result.output).toBe('file1.txt\na warning');
|
||||
expect(handle.pid).toBe(undefined);
|
||||
expect(handle.pid).toBe(12345);
|
||||
|
||||
expect(onOutputEventMock).toHaveBeenCalledWith({
|
||||
type: 'data',
|
||||
|
|
@ -829,7 +829,7 @@ describe('ShellExecutionService child_process fallback', () => {
|
|||
[],
|
||||
expect.objectContaining({
|
||||
shell: true,
|
||||
detached: false,
|
||||
detached: true,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@
|
|||
import stripAnsi from 'strip-ansi';
|
||||
import type { PtyImplementation } from '../utils/getPty.js';
|
||||
import { getPty } from '../utils/getPty.js';
|
||||
import { spawn as cpSpawn } from 'node:child_process';
|
||||
import { spawn as cpSpawn, spawnSync } from 'node:child_process';
|
||||
import { TextDecoder } from 'node:util';
|
||||
import os from 'node:os';
|
||||
import type { IPty } from '@lydell/node-pty';
|
||||
|
|
@ -98,6 +98,48 @@ const getFullBufferText = (terminal: pkg.Terminal): string => {
|
|||
return lines.join('\n').trimEnd();
|
||||
};
|
||||
|
||||
interface ProcessCleanupStrategy {
|
||||
killPty(pid: number, pty: ActivePty): void;
|
||||
killChildProcesses(pids: Set<number>): void;
|
||||
}
|
||||
|
||||
const windowsStrategy: ProcessCleanupStrategy = {
|
||||
killPty: (_pid, pty) => {
|
||||
pty.ptyProcess.kill();
|
||||
},
|
||||
killChildProcesses: (pids) => {
|
||||
if (pids.size > 0) {
|
||||
try {
|
||||
const args = ['/f', '/t'];
|
||||
for (const pid of pids) {
|
||||
args.push('/pid', pid.toString());
|
||||
}
|
||||
spawnSync('taskkill', args);
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
}
|
||||
},
|
||||
};
|
||||
|
||||
const posixStrategy: ProcessCleanupStrategy = {
|
||||
killPty: (pid, _pty) => {
|
||||
process.kill(-pid, 'SIGKILL');
|
||||
},
|
||||
killChildProcesses: (pids) => {
|
||||
for (const pid of pids) {
|
||||
try {
|
||||
process.kill(-pid, 'SIGKILL');
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
}
|
||||
},
|
||||
};
|
||||
|
||||
const getCleanupStrategy = () =>
|
||||
os.platform() === 'win32' ? windowsStrategy : posixStrategy;
|
||||
|
||||
/**
|
||||
* A centralized service for executing shell commands with robust process
|
||||
* management, cross-platform compatibility, and streaming output capabilities.
|
||||
|
|
@ -106,6 +148,29 @@ const getFullBufferText = (terminal: pkg.Terminal): string => {
|
|||
|
||||
export class ShellExecutionService {
|
||||
private static activePtys = new Map<number, ActivePty>();
|
||||
private static activeChildProcesses = new Set<number>();
|
||||
|
||||
static cleanup() {
|
||||
const strategy = getCleanupStrategy();
|
||||
// Cleanup PTYs
|
||||
for (const [pid, pty] of this.activePtys) {
|
||||
try {
|
||||
strategy.killPty(pid, pty);
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
}
|
||||
|
||||
// Cleanup child processes
|
||||
strategy.killChildProcesses(this.activeChildProcesses);
|
||||
}
|
||||
|
||||
static {
|
||||
process.on('exit', () => {
|
||||
ShellExecutionService.cleanup();
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Executes a shell command using `node-pty`, capturing all output and lifecycle events.
|
||||
*
|
||||
|
|
@ -164,7 +229,7 @@ export class ShellExecutionService {
|
|||
stdio: ['ignore', 'pipe', 'pipe'],
|
||||
windowsVerbatimArguments: true,
|
||||
shell: isWindows ? true : 'bash',
|
||||
detached: !isWindows,
|
||||
detached: true,
|
||||
env: {
|
||||
...process.env,
|
||||
QWEN_CODE: '1',
|
||||
|
|
@ -281,9 +346,13 @@ export class ShellExecutionService {
|
|||
|
||||
abortSignal.addEventListener('abort', abortHandler, { once: true });
|
||||
|
||||
if (child.pid) {
|
||||
this.activeChildProcesses.add(child.pid);
|
||||
}
|
||||
|
||||
child.on('exit', (code, signal) => {
|
||||
if (child.pid) {
|
||||
this.activePtys.delete(child.pid);
|
||||
this.activeChildProcesses.delete(child.pid);
|
||||
}
|
||||
handleExit(code, signal);
|
||||
});
|
||||
|
|
@ -310,7 +379,7 @@ export class ShellExecutionService {
|
|||
}
|
||||
});
|
||||
|
||||
return { pid: undefined, result };
|
||||
return { pid: child.pid, result };
|
||||
} catch (e) {
|
||||
const error = e as Error;
|
||||
return {
|
||||
|
|
|
|||
|
|
@ -248,7 +248,7 @@ describe('ShellTool', () => {
|
|||
wrappedCommand,
|
||||
'/test/dir',
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -275,7 +275,7 @@ describe('ShellTool', () => {
|
|||
wrappedCommand,
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -300,7 +300,7 @@ describe('ShellTool', () => {
|
|||
wrappedCommand,
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -325,7 +325,7 @@ describe('ShellTool', () => {
|
|||
wrappedCommand,
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -350,7 +350,7 @@ describe('ShellTool', () => {
|
|||
wrappedCommand,
|
||||
'/test/dir/subdir',
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -378,7 +378,7 @@ describe('ShellTool', () => {
|
|||
'dir',
|
||||
'/test/dir',
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -471,7 +471,7 @@ describe('ShellTool', () => {
|
|||
expect(summarizer.summarizeToolOutput).toHaveBeenCalledWith(
|
||||
expect.any(String),
|
||||
mockConfig.getGeminiClient(),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
1000,
|
||||
);
|
||||
expect(result.llmContent).toBe('summarized output');
|
||||
|
|
@ -580,7 +580,7 @@ describe('ShellTool', () => {
|
|||
),
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -610,7 +610,7 @@ describe('ShellTool', () => {
|
|||
),
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -640,7 +640,7 @@ describe('ShellTool', () => {
|
|||
),
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -699,7 +699,7 @@ describe('ShellTool', () => {
|
|||
expect.stringContaining('npm install'),
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -728,7 +728,7 @@ describe('ShellTool', () => {
|
|||
expect.stringContaining('git commit'),
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -758,7 +758,7 @@ describe('ShellTool', () => {
|
|||
),
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -794,7 +794,7 @@ describe('ShellTool', () => {
|
|||
expect.stringContaining('git commit -m "Initial commit"'),
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -831,7 +831,7 @@ describe('ShellTool', () => {
|
|||
),
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
mockAbortSignal,
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
|
|
@ -962,4 +962,41 @@ spanning multiple lines"`;
|
|||
expect(shellTool.description).toMatchSnapshot();
|
||||
});
|
||||
});
|
||||
|
||||
describe('Windows background execution', () => {
|
||||
it('should clean up trailing ampersand on Windows for background tasks', async () => {
|
||||
vi.mocked(os.platform).mockReturnValue('win32');
|
||||
const mockAbortSignal = new AbortController().signal;
|
||||
|
||||
const invocation = shellTool.build({
|
||||
command: 'npm start &',
|
||||
is_background: true,
|
||||
});
|
||||
|
||||
const promise = invocation.execute(mockAbortSignal);
|
||||
|
||||
// Simulate immediate success (process started)
|
||||
resolveExecutionPromise({
|
||||
rawOutput: Buffer.from(''),
|
||||
output: '',
|
||||
exitCode: 0,
|
||||
signal: null,
|
||||
error: null,
|
||||
aborted: false,
|
||||
pid: 12345,
|
||||
executionMethod: 'child_process',
|
||||
});
|
||||
|
||||
await promise;
|
||||
|
||||
expect(mockShellExecutionService).toHaveBeenCalledWith(
|
||||
'npm start',
|
||||
expect.any(String),
|
||||
expect.any(Function),
|
||||
expect.any(AbortSignal),
|
||||
false,
|
||||
{},
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -143,11 +143,24 @@ export class ShellToolInvocation extends BaseToolInvocation<
|
|||
const shouldRunInBackground = this.params.is_background;
|
||||
let finalCommand = processedCommand;
|
||||
|
||||
// If explicitly marked as background and doesn't already end with &, add it
|
||||
if (shouldRunInBackground && !finalCommand.trim().endsWith('&')) {
|
||||
// On non-Windows, use & to run in background.
|
||||
// On Windows, we don't use start /B because it creates a detached process that
|
||||
// doesn't die when the parent dies. Instead, we rely on the race logic below
|
||||
// to return early while keeping the process attached (detached: false).
|
||||
if (
|
||||
!isWindows &&
|
||||
shouldRunInBackground &&
|
||||
!finalCommand.trim().endsWith('&')
|
||||
) {
|
||||
finalCommand = finalCommand.trim() + ' &';
|
||||
}
|
||||
|
||||
// On Windows, we rely on the race logic below to handle background tasks.
|
||||
// We just ensure the command string is clean.
|
||||
if (isWindows && shouldRunInBackground) {
|
||||
finalCommand = finalCommand.trim().replace(/&+$/, '').trim();
|
||||
}
|
||||
|
||||
// pgrep is not available on Windows, so we can't get background PIDs
|
||||
const commandToExecute = isWindows
|
||||
? finalCommand
|
||||
|
|
@ -169,10 +182,6 @@ export class ShellToolInvocation extends BaseToolInvocation<
|
|||
commandToExecute,
|
||||
cwd,
|
||||
(event: ShellOutputEvent) => {
|
||||
if (!updateOutput) {
|
||||
return;
|
||||
}
|
||||
|
||||
let shouldUpdate = false;
|
||||
|
||||
switch (event.type) {
|
||||
|
|
@ -201,7 +210,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
|
|||
}
|
||||
}
|
||||
|
||||
if (shouldUpdate) {
|
||||
if (shouldUpdate && updateOutput) {
|
||||
updateOutput(
|
||||
typeof cumulativeOutput === 'string'
|
||||
? cumulativeOutput
|
||||
|
|
@ -219,6 +228,21 @@ export class ShellToolInvocation extends BaseToolInvocation<
|
|||
setPidCallback(pid);
|
||||
}
|
||||
|
||||
if (shouldRunInBackground) {
|
||||
// For background tasks, return immediately with PID info
|
||||
// Note: We cannot reliably detect startup errors for background processes
|
||||
// since their stdio is typically detached/ignored
|
||||
const pidMsg = pid ? ` PID: ${pid}` : '';
|
||||
const killHint = isWindows
|
||||
? ' (Use taskkill /F /T /PID <pid> to stop)'
|
||||
: ' (Use kill <pid> to stop)';
|
||||
|
||||
return {
|
||||
llmContent: `Background command started.${pidMsg}${killHint}`,
|
||||
returnDisplay: `Background command started.${pidMsg}${killHint}`,
|
||||
};
|
||||
}
|
||||
|
||||
const result = await resultPromise;
|
||||
|
||||
const backgroundPIDs: number[] = [];
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue