diff --git a/package.json b/package.json index 2efe64d370..c4c192713e 100644 --- a/package.json +++ b/package.json @@ -46,7 +46,7 @@ "test:e2e:copy-tex": "node scripts/e2e-copy-tex.mjs", "test:e2e:transcript-disclosure": "node scripts/e2e-transcript-disclosure-anchor.mjs", "test:e2e:images": "node scripts/e2e-image-generation.mjs && node scripts/e2e-image-generation-ui.mjs && node scripts/e2e-image-chat.mjs", - "test:e2e:provider-client-certificate": "node scripts/e2e-provider-client-certificate.mjs", + "test:e2e:provider-client-certificate": "node --test scripts/e2e/wait-for-child-exit.test.mjs && node scripts/e2e-provider-client-certificate.mjs", "test:e2e:provider-order": "node scripts/e2e-provider-order.mjs", "test:e2e:project-edit": "node scripts/e2e-project-edit.mjs", "test:e2e:provider-api-style": "node scripts/e2e-provider-api-style.mjs", diff --git a/scripts/e2e-provider-client-certificate.mjs b/scripts/e2e-provider-client-certificate.mjs index 2876c74792..a0aac14957 100644 --- a/scripts/e2e-provider-client-certificate.mjs +++ b/scripts/e2e-provider-client-certificate.mjs @@ -8,6 +8,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { resolveElectronBinary } from "./e2e/boot.mjs"; +import { waitForChildExitWithEscalation } from "./e2e/wait-for-child-exit.mjs"; const root = fileURLToPath(new URL("..", import.meta.url)); const { build } = createRequire(join(root, "packages/agent-runtime/package.json"))("esbuild"); @@ -29,13 +30,17 @@ try { output += chunk; process.stdout.write(chunk); }); - const timer = setTimeout(() => child.kill(), 60_000); - try { - const code = await new Promise((resolve, reject) => { - child.once("error", reject); - child.once("close", resolve); - }); - assert.equal(code, 0, output); - assert.match(output, /provider client certificate E2E passed/); - } finally { clearTimeout(timer); } + // A probe that cannot start must fail this step, not hold the job open. + // SIGTERM alone is not enough: Electron can ignore it while a renderer is + // mid-init, and the `close` event then never arrives, so the step waits out + // the job's 30-minute budget and is cancelled with no failing step to read + // (#1751 hit exactly this). Escalate to SIGKILL after a short grace period. + const { code, killedBy } = await waitForChildExitWithEscalation(child); + assert.equal( + killedBy, + null, + `provider client certificate E2E hung and was killed by ${killedBy} after ${killedBy === "SIGKILL" ? 65 : 60}s: ${output}`, + ); + assert.equal(code, 0, output); + assert.match(output, /provider client certificate E2E passed/); } finally { await rm(temp, { recursive: true, force: true }); } diff --git a/scripts/e2e/wait-for-child-exit.mjs b/scripts/e2e/wait-for-child-exit.mjs new file mode 100644 index 0000000000..9bb26d6fc5 --- /dev/null +++ b/scripts/e2e/wait-for-child-exit.mjs @@ -0,0 +1,29 @@ +export async function waitForChildExitWithEscalation( + child, + { + termAfterMs = 60_000, + killAfterMs = 65_000, + setTimer = setTimeout, + clearTimer = clearTimeout, + } = {}, +) { + let killedBy = null; + const termTimer = setTimer(() => { + killedBy = "SIGTERM"; + child.kill("SIGTERM"); + }, termAfterMs); + const killTimer = setTimer(() => { + killedBy = "SIGKILL"; + child.kill("SIGKILL"); + }, killAfterMs); + try { + const code = await new Promise((resolve, reject) => { + child.once("error", reject); + child.once("close", resolve); + }); + return { code, killedBy }; + } finally { + clearTimer(termTimer); + clearTimer(killTimer); + } +} diff --git a/scripts/e2e/wait-for-child-exit.test.mjs b/scripts/e2e/wait-for-child-exit.test.mjs new file mode 100644 index 0000000000..78ca767607 --- /dev/null +++ b/scripts/e2e/wait-for-child-exit.test.mjs @@ -0,0 +1,87 @@ +import assert from "node:assert/strict"; +import { EventEmitter } from "node:events"; +import test from "node:test"; +import { waitForChildExitWithEscalation } from "./wait-for-child-exit.mjs"; + +function controlledTimers() { + let nextId = 0; + const pending = new Map(); + return { + setTimer(callback, delay) { + const id = ++nextId; + pending.set(id, { callback, delay }); + return id; + }, + clearTimer(id) { + pending.delete(id); + }, + fireAfter(delay) { + for (const [id, timer] of pending) { + if (timer.delay === delay) { + pending.delete(id); + timer.callback(); + } + } + }, + pending, + }; +} + +test("a normally closing child exits without signals and clears both timers", async () => { + const child = new EventEmitter(); + child.kill = () => assert.fail("a normally closing child must not be killed"); + const timers = controlledTimers(); + + const result = waitForChildExitWithEscalation(child, { + setTimer: timers.setTimer, + clearTimer: timers.clearTimer, + }); + child.emit("close", 0); + + assert.deepEqual(await result, { code: 0, killedBy: null }); + assert.equal(timers.pending.size, 0); +}); + +test("a child that ignores SIGTERM receives SIGKILL after the grace period", async () => { + const child = new EventEmitter(); + const signals = []; + child.kill = (signal) => signals.push(signal); + const timers = controlledTimers(); + + const result = waitForChildExitWithEscalation(child, { + termAfterMs: 60, + killAfterMs: 65, + setTimer: timers.setTimer, + clearTimer: timers.clearTimer, + }); + + timers.fireAfter(60); + assert.deepEqual(signals, ["SIGTERM"]); + timers.fireAfter(65); + assert.deepEqual(signals, ["SIGTERM", "SIGKILL"]); + child.emit("close", null); + + assert.deepEqual(await result, { code: null, killedBy: "SIGKILL" }); + assert.equal(timers.pending.size, 0); +}); + +test("a child that exits after SIGTERM is not escalated", async () => { + const child = new EventEmitter(); + const signals = []; + child.kill = (signal) => signals.push(signal); + const timers = controlledTimers(); + + const result = waitForChildExitWithEscalation(child, { + termAfterMs: 60, + killAfterMs: 65, + setTimer: timers.setTimer, + clearTimer: timers.clearTimer, + }); + + timers.fireAfter(60); + child.emit("close", 0); + + assert.deepEqual(await result, { code: 0, killedBy: "SIGTERM" }); + assert.deepEqual(signals, ["SIGTERM"]); + assert.equal(timers.pending.size, 0); +});