mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-11-07 06:19:43 -10:00
fix(node-repl): let the kernel exit promptly on shutdown (#15160)
* fix(node-repl): let the kernel exit promptly on shutdown The kernel reads its input pipe through a file stream, and a pending read on that pipe keeps process.exit() from completing until the pipe reaches EOF. Since the manager began waiting 2.5 s for a graceful exit before signalling, every reset and disposal cost about 3 seconds and ended in SIGTERM, which also slowed the node-repl suite from ~35 s to ~3.8 min in CI. End the pipe right after the shutdown frame so the kernel can exit as soon as its cleanup finishes. * fix(node-repl): stop session processes when the kernel exits on its own Ending the kernel's input pipe lets it exit within milliseconds, so the group signals that used to reach processes the session started without detaching were no longer sent: they kept running after reset, disposal and a cancellation timeout. When the kernel exits on its own, signal its process group the way the signalled shutdown did: SIGTERM, then SIGKILL if anything is still in the group after 500 ms. The group keeps its ID while any member lives, and a kernel that was already gone before termination began is left alone. Tests start a child from session code that holds a socket to the test, so its exit is seen even when nothing reaps it, and cover reset, disposal, a timed-out cancellation barrier and a child that ignores SIGTERM. The idle-kernel test now checks that no signal reaches a running kernel rather than that no signal is sent at all. * fix(node-repl): stop leftover session processes after a SIGTERM exit too The process group stop ran only when the kernel exited within the first grace period. A kernel whose own cleanup outlasts it (a slow Computer Use disposal, say) exits during the SIGTERM wait instead, so neither the group stop nor the SIGKILL ran, and a session process that ignores SIGTERM kept running. Before the pipe was closed that kernel could not finish exiting, so the old path always reached SIGKILL. Run the group stop whenever the kernel exits before the hard stop. A test blocks Computer Use disposal past the grace period and checks that a SIGTERM-ignoring child is still stopped. * test(node-repl): pin the SIGTERM step of the leftover-process sweep The slow-cleanup case was named for a kernel that exits on SIGTERM, but the kernel ignores SIGTERM once shutdown has started; it exits when the fixture's cleanup ends. Rename it and assert the group got SIGTERM while the kernel was still running, so a kernel that exits in the first wait no longer passes for this path. Add a case whose session process handles SIGTERM by writing a marker and exiting, so replacing the SIGTERM-then-SIGKILL sweep with a bare SIGKILL fails a test.
This commit is contained in:
1 parent
9e16932bee
commit
249ab50d84
2 files changed
+227
-1
No files matched your search
@@ -5,10 +5,11 @@
|
||||
*/
|
||||
|
||||
import fs from 'node:fs';
|
||||
import net from 'node:net';
|
||||
import os from 'node:os';
|
||||
import path from 'node:path';
|
||||
import { pathToFileURL } from 'node:url';
|
||||
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import {
|
||||
NodeReplKernelManager,
|
||||
type KernelManagerOptions,
|
||||
@@ -75,6 +76,59 @@ async function run(code: string, timeoutMs = EXEC_TIMEOUT) {
|
||||
return manager.exec({ code, timeoutMs });
|
||||
}
|
||||
|
||||
// Starts a process from session code. It holds a connection to this test,
|
||||
// so its exit is seen even if nothing reaps it.
|
||||
async function startSessionProcess(script = '') {
|
||||
const server = net.createServer();
|
||||
const connection = new Promise<net.Socket>((resolve) =>
|
||||
server.once('connection', resolve),
|
||||
);
|
||||
await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve));
|
||||
const { port } = server.address() as net.AddressInfo;
|
||||
let pid = 0;
|
||||
let exited = false;
|
||||
const stop = () => {
|
||||
if (pid > 0 && !exited) {
|
||||
try {
|
||||
process.kill(pid, 'SIGKILL');
|
||||
} catch {
|
||||
// Already gone.
|
||||
}
|
||||
}
|
||||
server.close();
|
||||
};
|
||||
try {
|
||||
const started = await run(
|
||||
`const { spawn } = await import('node:child_process');
|
||||
const child = spawn(${JSON.stringify(process.execPath)}, ['-e', ${JSON.stringify(
|
||||
`require('node:net').connect(${port}, '127.0.0.1'); ${script} setInterval(() => {}, 1000);`,
|
||||
)}], { stdio: 'ignore' });
|
||||
nodeRepl.write(String(child.pid));`,
|
||||
);
|
||||
expect(started.status).toBe('ok');
|
||||
pid = Number(texts(started)[0]);
|
||||
const socket = await connection;
|
||||
socket.on('error', () => undefined);
|
||||
const closed = new Promise<boolean>((resolve) =>
|
||||
socket.once('close', () => {
|
||||
exited = true;
|
||||
resolve(true);
|
||||
}),
|
||||
);
|
||||
const exitedWithin = (timeoutMs: number) =>
|
||||
Promise.race([
|
||||
closed,
|
||||
new Promise<boolean>((resolve) =>
|
||||
setTimeout(resolve, timeoutMs, false),
|
||||
),
|
||||
]);
|
||||
return { exitedWithin, stop };
|
||||
} catch (error) {
|
||||
stop();
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
workDir = fs.mkdtempSync(path.join(os.tmpdir(), 'node-repl-km-'));
|
||||
manager = makeManager();
|
||||
@@ -1618,6 +1672,149 @@ describe('NodeReplKernelManager', () => {
|
||||
TEST_TIMEOUT,
|
||||
);
|
||||
|
||||
it.skipIf(process.platform === 'win32')(
|
||||
'lets an idle kernel exit on reset and disposal before any signal',
|
||||
async () => {
|
||||
await run('nodeRepl.write("up");');
|
||||
const realKill = process.kill.bind(process);
|
||||
const signalledRunningKernels: number[] = [];
|
||||
const kill = vi
|
||||
.spyOn(process, 'kill')
|
||||
.mockImplementation((pid, signal) => {
|
||||
try {
|
||||
realKill(Math.abs(pid), 0);
|
||||
signalledRunningKernels.push(pid);
|
||||
} catch {
|
||||
// The kernel had already exited.
|
||||
}
|
||||
return realKill(pid, signal);
|
||||
});
|
||||
try {
|
||||
await manager.reset();
|
||||
await run('nodeRepl.write("up");');
|
||||
await manager.dispose();
|
||||
expect(signalledRunningKernels).toEqual([]);
|
||||
} finally {
|
||||
kill.mockRestore();
|
||||
}
|
||||
},
|
||||
TEST_TIMEOUT,
|
||||
);
|
||||
|
||||
it.skipIf(process.platform === 'win32').each([
|
||||
{ name: 'reset', terminate: () => manager.reset() },
|
||||
{ name: 'disposal', terminate: () => manager.dispose() },
|
||||
{
|
||||
name: 'a timed-out cancellation barrier',
|
||||
terminate: async () => {
|
||||
const outcome = await run(
|
||||
'await nodeRepl.signal.waitUntil(new Promise(() => {}));',
|
||||
2_000,
|
||||
);
|
||||
expect(outcome.status).toBe('timeout');
|
||||
expect(outcome.stats.kernelReplaced).toBe(true);
|
||||
},
|
||||
},
|
||||
])(
|
||||
'stops processes the session started on $name',
|
||||
async ({ terminate }) => {
|
||||
const child = await startSessionProcess();
|
||||
try {
|
||||
await terminate();
|
||||
await expect(child.exitedWithin(10_000)).resolves.toBe(true);
|
||||
} finally {
|
||||
child.stop();
|
||||
}
|
||||
},
|
||||
TEST_TIMEOUT,
|
||||
);
|
||||
|
||||
it.skipIf(process.platform === 'win32')(
|
||||
'kills a session process that ignores SIGTERM on reset',
|
||||
async () => {
|
||||
const child = await startSessionProcess(
|
||||
"process.on('SIGTERM', () => {});",
|
||||
);
|
||||
try {
|
||||
await manager.reset();
|
||||
await expect(child.exitedWithin(10_000)).resolves.toBe(true);
|
||||
} finally {
|
||||
child.stop();
|
||||
}
|
||||
},
|
||||
TEST_TIMEOUT,
|
||||
);
|
||||
|
||||
it.skipIf(process.platform === 'win32')(
|
||||
'lets a session process handle SIGTERM before it is killed on reset',
|
||||
async () => {
|
||||
const marker = path.join(workDir, 'sigterm-marker');
|
||||
const child = await startSessionProcess(
|
||||
`process.on('SIGTERM', () => { require('node:fs').writeFileSync(${JSON.stringify(marker)}, 'flushed'); process.exit(0); });`,
|
||||
);
|
||||
try {
|
||||
await manager.reset();
|
||||
await expect(child.exitedWithin(10_000)).resolves.toBe(true);
|
||||
expect(fs.readFileSync(marker, 'utf8')).toBe('flushed');
|
||||
} finally {
|
||||
child.stop();
|
||||
}
|
||||
},
|
||||
TEST_TIMEOUT,
|
||||
);
|
||||
|
||||
it.skipIf(process.platform === 'win32')(
|
||||
'kills a session process that ignores SIGTERM when the kernel exits on its own after the grace period',
|
||||
async () => {
|
||||
// Cleanup outlasts the 2500 ms grace period, so the group gets SIGTERM
|
||||
// while the kernel is still running. The kernel ignores it once shutdown
|
||||
// has started and exits when cleanup ends, during the 500 ms that follow.
|
||||
createEsmPackage(
|
||||
path.join(workDir, 'node_modules'),
|
||||
'cua-slow-cleanup-fixture',
|
||||
`export const cua = { async [Symbol.asyncDispose]() {
|
||||
const until = Date.now() + 2750;
|
||||
while (Date.now() < until);
|
||||
} };`,
|
||||
);
|
||||
const child = await startSessionProcess(
|
||||
"process.on('SIGTERM', () => {});",
|
||||
);
|
||||
try {
|
||||
const install = await run(
|
||||
`globalThis.cua = (await import('cua-slow-cleanup-fixture')).cua;`,
|
||||
);
|
||||
expect(install.status).toBe('ok');
|
||||
const kernelPid = manager.getKernelPid()!;
|
||||
const realKill = process.kill.bind(process);
|
||||
const signalledRunningKernel: Array<[number, unknown]> = [];
|
||||
const kill = vi
|
||||
.spyOn(process, 'kill')
|
||||
.mockImplementation((pid, signal) => {
|
||||
try {
|
||||
realKill(kernelPid, 0);
|
||||
signalledRunningKernel.push([pid, signal]);
|
||||
} catch {
|
||||
// The kernel had already exited.
|
||||
}
|
||||
return realKill(pid, signal);
|
||||
});
|
||||
try {
|
||||
await manager.reset();
|
||||
} finally {
|
||||
kill.mockRestore();
|
||||
}
|
||||
// Without this the kernel could exit before any signal and the case
|
||||
// would pass without reaching the path it is named for.
|
||||
expect(signalledRunningKernel).toContainEqual([-kernelPid, 'SIGTERM']);
|
||||
await expect(child.exitedWithin(10_000)).resolves.toBe(true);
|
||||
} finally {
|
||||
child.stop();
|
||||
}
|
||||
},
|
||||
TEST_TIMEOUT,
|
||||
);
|
||||
|
||||
it(
|
||||
'disposes the process and temp directory and settles in-flight work',
|
||||
async () => {
|
||||
|
||||
@@ -995,8 +995,14 @@ export class NodeReplKernelManager {
|
||||
}
|
||||
|
||||
private async terminateHandle(handle: KernelHandle): Promise<void> {
|
||||
const running =
|
||||
handle.child.exitCode === null && handle.child.signalCode === null;
|
||||
try {
|
||||
handle.toKernel.write(encodeFrame({ type: 'shutdown' }));
|
||||
// The kernel's pending read on this pipe keeps its process.exit()
|
||||
// from completing until the pipe reaches EOF; without it every
|
||||
// shutdown waits out the grace period and ends in a signal.
|
||||
handle.toKernel.end();
|
||||
} catch {
|
||||
// The hard stop below remains authoritative.
|
||||
}
|
||||
@@ -1010,12 +1016,35 @@ export class NodeReplKernelManager {
|
||||
hardKill = true;
|
||||
this.signalProcessTree(handle, true);
|
||||
exited = await this.waitForExit(handle.child, TERMINATE_GRACE_MS);
|
||||
} else if (running && !IS_WINDOWS) {
|
||||
// Processes the session started without detaching outlive a kernel
|
||||
// that exits before the hard stop, whether on its own or on SIGTERM
|
||||
// they ignored. They are still in its process group, which keeps its
|
||||
// ID while any of them lives, so stop them as the hard stop would.
|
||||
hardKill = await this.stopLeftoverProcessGroup(handle.pid);
|
||||
}
|
||||
debugLogger.debug(
|
||||
`[node-repl] process tree termination completed (generation=${handle.generation}, pid=${handle.pid}, hardKill=${hardKill}, exited=${exited})`,
|
||||
);
|
||||
}
|
||||
|
||||
private async stopLeftoverProcessGroup(pgid: number): Promise<boolean> {
|
||||
const signalGroup = (signal: NodeJS.Signals | 0): boolean => {
|
||||
try {
|
||||
process.kill(-pgid, signal);
|
||||
return true;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
};
|
||||
if (!signalGroup('SIGTERM')) return false;
|
||||
for (let waited = 0; waited < 500; waited += 50) {
|
||||
await new Promise((resolve) => setTimeout(resolve, 50));
|
||||
if (!signalGroup(0)) return false;
|
||||
}
|
||||
return signalGroup('SIGKILL');
|
||||
}
|
||||
|
||||
private signalProcessTree(handle: KernelHandle, hard: boolean): void {
|
||||
if (IS_WINDOWS) {
|
||||
const fallback = () => {
|
||||
|
||||
Reference in new issue
Block a user