diff --git a/scripts/e2e/parallels/host-command.ts b/scripts/e2e/parallels/host-command.ts index 69ed482e61e3..a1cf3eae1638 100644 --- a/scripts/e2e/parallels/host-command.ts +++ b/scripts/e2e/parallels/host-command.ts @@ -14,6 +14,10 @@ export const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url) const HOST_COMMAND_MAX_BUFFER_BYTES = 50 * 1024 * 1024; const HOST_COMMAND_WRAPPER_EXTRA_BUFFER_BYTES = 1024 * 1024; const HOST_COMMAND_WRAPPER_BACKSTOP_MS = 5_000; +const HOST_COMMAND_TIMEOUT_KILL_GRACE_MS = 100; +const HOST_COMMAND_STREAMING_TIMEOUT_KILL_GRACE_MS = 2_000; +const HOST_COMMAND_PROCESS_GROUP_EXIT_POLL_MS = 25; +const HOST_COMMAND_POST_FORCE_KILL_WAIT_MS = 100; const HOST_COMMAND_CHILD_PID_PREFIX = "__OPENCLAW_HOST_COMMAND_CHILD_PID__"; const HOST_COMMAND_SPAWN_ERROR_PREFIX = "__OPENCLAW_HOST_COMMAND_SPAWN_ERROR__"; const HOST_COMMAND_TIMEOUT_PREFIX = "__OPENCLAW_HOST_COMMAND_TIMEOUT__"; @@ -108,6 +112,7 @@ writeSync( let timedOut = false; let killTimer; +let killDeadlineAt = 0; let outputExceeded = false; let stderrBytes = 0; let stdoutBytes = 0; @@ -132,6 +137,58 @@ function signalGroup(signal) { } } +function groupAlive() { + if (!child.pid) { + return false; + } + try { + process.kill(-child.pid, 0); + return true; + } catch (error) { + return Boolean(error && error.code === "EPERM"); + } +} + +function finishTimedOut() { + if (killTimer) { + clearTimeout(killTimer); + } + writeSync(3, ${JSON.stringify(HOST_COMMAND_TIMEOUT_PREFIX)} + "{}\n"); + process.exit(124); +} + +function finishTimedOutAfterCleanup() { + if (!groupAlive()) { + finishTimedOut(); + return; + } + const pollMs = Math.max(1, Math.min(25, payload.timeoutKillGraceMs)); + let pollTimer; + let forceFinishTimer; + let postForceFinishTimer; + const finish = () => { + if (pollTimer) { + clearInterval(pollTimer); + } + if (forceFinishTimer) { + clearTimeout(forceFinishTimer); + } + if (postForceFinishTimer) { + clearTimeout(postForceFinishTimer); + } + finishTimedOut(); + }; + pollTimer = setInterval(() => { + if (!groupAlive()) { + finish(); + } + }, pollMs); + forceFinishTimer = setTimeout(() => { + signalGroup("SIGKILL"); + postForceFinishTimer = setTimeout(finish, pollMs); + }, Math.max(0, killDeadlineAt - Date.now())); +} + function forwardBounded(stream, chunk) { const currentBytes = stream === "stdout" ? stdoutBytes : stderrBytes; const nextBytes = currentBytes + chunk.byteLength; @@ -170,7 +227,8 @@ for (const signal of ["SIGHUP", "SIGINT", "SIGTERM"]) { const timeout = setTimeout(() => { timedOut = true; signalGroup("SIGTERM"); - killTimer = setTimeout(() => signalGroup("SIGKILL"), 100); + killDeadlineAt = Date.now() + payload.timeoutKillGraceMs; + killTimer = setTimeout(() => signalGroup("SIGKILL"), payload.timeoutKillGraceMs); killTimer.unref(); }, payload.timeoutMs); timeout.unref(); @@ -199,14 +257,13 @@ child.on("error", (error) => { }); child.on("close", (code, signal) => { clearTimeout(timeout); + if (timedOut) { + finishTimedOutAfterCleanup(); + return; + } if (killTimer) { clearTimeout(killTimer); } - if (timedOut) { - signalGroup("SIGKILL"); - writeSync(3, ${JSON.stringify(HOST_COMMAND_TIMEOUT_PREFIX)} + "{}\n"); - process.exit(124); - } if (outputExceeded) { process.exit(1); } @@ -407,6 +464,7 @@ function runPosixTimedCommandSync( input: options.input, maxBufferBytes: HOST_COMMAND_MAX_BUFFER_BYTES, shell: invocation.shell, + timeoutKillGraceMs: HOST_COMMAND_TIMEOUT_KILL_GRACE_MS, timeoutMs: options.timeoutMs, }); return spawnSync(process.execPath, ["-e", POSIX_TIMEOUT_WRAPPER], { @@ -461,6 +519,29 @@ export async function runStreaming( } } }; + const streamingProcessGroupAlive = (): boolean => { + if (!detached || !childPid) { + return false; + } + try { + process.kill(-childPid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException).code === "EPERM"; + } + }; + const waitForStreamingProcessGroupExit = async (timeoutMs: number): Promise => { + const deadlineAt = Date.now() + timeoutMs; + while (Date.now() < deadlineAt) { + if (!streamingProcessGroupAlive()) { + return true; + } + await new Promise((resolvePoll) => { + setTimeout(resolvePoll, HOST_COMMAND_PROCESS_GROUP_EXIT_POLL_MS); + }); + } + return !streamingProcessGroupAlive(); + }; logStream?.on("error", (error) => { logStreamError = error; signalStreamingChild("SIGTERM"); @@ -520,13 +601,32 @@ export async function runStreaming( let timedOut = false; let killTimer: NodeJS.Timeout | undefined; + let killDeadlineAt = 0; + const waitForStreamingTimeoutCleanup = async (): Promise => { + if (!detached) { + signalStreamingChild("SIGKILL"); + return; + } + const remainingGraceMs = Math.max(0, killDeadlineAt - Date.now()); + if (remainingGraceMs > 0) { + await waitForStreamingProcessGroupExit(remainingGraceMs); + } + if (streamingProcessGroupAlive()) { + signalStreamingChild("SIGKILL"); + await waitForStreamingProcessGroupExit(HOST_COMMAND_POST_FORCE_KILL_WAIT_MS); + } + }; const timer = options.timeoutMs == null ? undefined : setTimeout(() => { timedOut = true; signalHostCommandProcess(childPid, "SIGTERM"); - killTimer = setTimeout(() => signalHostCommandProcess(childPid, "SIGKILL"), 2_000); + killDeadlineAt = Date.now() + HOST_COMMAND_STREAMING_TIMEOUT_KILL_GRACE_MS; + killTimer = setTimeout( + () => signalHostCommandProcess(childPid, "SIGKILL"), + HOST_COMMAND_STREAMING_TIMEOUT_KILL_GRACE_MS, + ); killTimer.unref(); }, options.timeoutMs); @@ -546,12 +646,12 @@ export async function runStreaming( if (timer) { clearTimeout(timer); } - if (killTimer) { - clearTimeout(killTimer); - } removeParentSignalHandlers(); if (timedOut) { - signalStreamingChild("SIGKILL"); + await waitForStreamingTimeoutCleanup(); + } + if (killTimer) { + clearTimeout(killTimer); } if (logStream) { logStream.end(); diff --git a/test/scripts/parallels-smoke-model.test.ts b/test/scripts/parallels-smoke-model.test.ts index 0161c52e5225..58db4777f1d6 100644 --- a/test/scripts/parallels-smoke-model.test.ts +++ b/test/scripts/parallels-smoke-model.test.ts @@ -15,7 +15,7 @@ import { tmpdir } from "node:os"; import { basename, delimiter, join, win32 } from "node:path"; import { setTimeout as delay } from "node:timers/promises"; import { pathToFileURL } from "node:url"; -import { beforeAll, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { extractLastOpenClawVersionFromLog, modelProviderConfigBatchJson, @@ -50,6 +50,7 @@ import { import { parseArgs as parseWindowsSmokeArgs } from "../../scripts/e2e/parallels/windows-smoke.ts"; import { withEnv } from "../../src/test-utils/env.js"; import { spawnNodeEvalSync } from "../../src/test-utils/node-process.js"; +import { cleanupTempDirs, makeTempDir } from "../helpers/temp-dir.js"; const WRAPPERS = { linux: "scripts/e2e/parallels-linux-smoke.sh", @@ -82,6 +83,11 @@ const TS_PATHS = { }; const OS_TS_PATHS = [TS_PATHS.linux, TS_PATHS.macos, TS_PATHS.windows]; +const tempDirs: string[] = []; + +afterEach(() => { + cleanupTempDirs(tempDirs); +}); function countNonEmptyLines(value: string): number { let count = 0; @@ -1300,28 +1306,50 @@ if (isPrlctl) { expect(result.stdout).toBeTypeOf("string"); }); - it("does not wait for host commands that trap SIGTERM after a timeout", () => { - const startedAt = Date.now(); - const result = run( - process.execPath, - [ - "-e", - [ - "process.on('SIGTERM', () => {});", - "setTimeout(() => process.exit(77), 700);", - "setInterval(() => {}, 1000);", - ].join(""), - ], - { - check: false, - quiet: true, - timeoutMs: 50, - }, - ); + it.runIf(process.platform !== "win32")( + "lets timed host command descendants drain before force kill", + () => { + const tempDir = makeTempDir(tempDirs, "openclaw-parallels-host-command-drain-"); + const readyFile = join(tempDir, "ready"); + const drainFile = join(tempDir, "drained"); + const descendantScript = [ + "const { writeFileSync } = require('node:fs');", + "writeFileSync(process.env.READY_FILE, 'ready');", + "process.on('SIGTERM', () => {", + " setTimeout(() => {", + " writeFileSync(process.env.DRAIN_FILE, 'drained');", + " process.exit(0);", + " }, 25);", + "});", + "setInterval(() => {}, 1000);", + ].join("\n"); + const parentScript = [ + "const { spawn } = require('node:child_process');", + `spawn(process.execPath, ['-e', ${JSON.stringify(descendantScript)}], { env: process.env, stdio: 'ignore' });`, + "process.on('SIGTERM', () => process.exit(0));", + "setInterval(() => {}, 1000);", + ].join("\n"); - expect(result.status).toBe(124); - expect(Date.now() - startedAt).toBeLessThan(500); - }); + try { + const result = run(process.execPath, ["-e", parentScript], { + check: false, + env: { + ...process.env, + DRAIN_FILE: drainFile, + READY_FILE: readyFile, + }, + quiet: true, + timeoutMs: 1_000, + }); + + expect(result.status).toBe(124); + expect(existsSync(readyFile)).toBe(true); + expect(readFileSync(drainFile, "utf8")).toBe("drained"); + } finally { + cleanupTempDirs(tempDirs); + } + }, + ); it.runIf(process.platform !== "win32")("throws checked timed host command timeouts", () => { expect(() => @@ -1424,6 +1452,52 @@ setInterval(() => {}, 1000); } }); + it.runIf(process.platform !== "win32")( + "lets timed streaming host command descendants drain before force kill", + async () => { + const tempDir = makeTempDir(tempDirs, "openclaw-parallels-streaming-host-command-drain-"); + const readyFile = join(tempDir, "ready"); + const drainFile = join(tempDir, "drained"); + const logPath = join(tempDir, "stream.log"); + const descendantScript = [ + "const { writeFileSync } = require('node:fs');", + "writeFileSync(process.env.READY_FILE, 'ready');", + "process.on('SIGTERM', () => {", + " setTimeout(() => {", + " writeFileSync(process.env.DRAIN_FILE, 'drained');", + " process.exit(0);", + " }, 50);", + "});", + "setInterval(() => {}, 1000);", + ].join("\n"); + const parentScript = [ + "const { spawn } = require('node:child_process');", + `spawn(process.execPath, ['-e', ${JSON.stringify(descendantScript)}], { env: process.env, stdio: 'ignore' });`, + "process.on('SIGTERM', () => process.exit(0));", + "setInterval(() => {}, 1000);", + ].join("\n"); + + try { + const statusPromise = runStreaming(process.execPath, ["-e", parentScript], { + env: { + ...process.env, + DRAIN_FILE: drainFile, + READY_FILE: readyFile, + }, + logPath, + quiet: true, + timeoutMs: 500, + }); + + await waitFor(() => existsSync(readyFile), 2_000); + await expect(statusPromise).resolves.toBe(124); + expect(readFileSync(drainFile, "utf8")).toBe("drained"); + } finally { + cleanupTempDirs(tempDirs); + } + }, + ); + it("streams host command logs instead of retaining them in memory", async () => { const source = readFileSync(TS_PATHS.hostCommand, "utf8"); const runStreamingBlock = source.slice(source.indexOf("export async function runStreaming"));