From 434e06f5d2382482bcc6bf065474b221d657a968 Mon Sep 17 00:00:00 2001 From: muzimu217 Date: Tue, 20 Oct 2026 17:02:14 -0700 Subject: [PATCH 1/2] fix(e2e): escalate to SIGKILL when the certificate probe hangs #1751's Electron E2E job ran 30m18s and was cancelled by the job budget with no failing step to read. The cause is this runner's only timeout: const timer = setTimeout(() => child.kill(), 60_000); SIGTERM is a request. An Electron child still initialising its renderer can ignore it, and the 'close' event this script awaits then never arrives, so the step holds the job until GitHub cancels it. The runner's own 60s budget is never enforced. Escalate: SIGTERM at 60s, SIGKILL at 65s, and report which one fired. A probe that cannot start now fails this step in about a minute with its output attached, instead of burning the job budget. Verified: normal path still passes ("provider client certificate E2E passed"); with a child that ignores SIGTERM the escalation force-kills it at ~800ms in an isolated harness, so the timeout path terminates rather than hanging. --- scripts/e2e-provider-client-certificate.mjs | 25 +++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/scripts/e2e-provider-client-certificate.mjs b/scripts/e2e-provider-client-certificate.mjs index 2876c74792..f29cb14846 100644 --- a/scripts/e2e-provider-client-certificate.mjs +++ b/scripts/e2e-provider-client-certificate.mjs @@ -29,13 +29,34 @@ try { output += chunk; process.stdout.write(chunk); }); - const timer = setTimeout(() => child.kill(), 60_000); + // 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. + let killedBy = null; + const termTimer = setTimeout(() => { + killedBy = "SIGTERM"; + child.kill("SIGTERM"); + }, 60_000); + const killTimer = setTimeout(() => { + killedBy = "SIGKILL"; + child.kill("SIGKILL"); + }, 65_000); try { const code = await new Promise((resolve, reject) => { child.once("error", reject); child.once("close", resolve); }); + 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 { clearTimeout(timer); } + } finally { + clearTimeout(termTimer); + clearTimeout(killTimer); + } } finally { await rm(temp, { recursive: true, force: true }); } From 02aa5678ddb3d836230e0f2e7c569a8dcbc1154c Mon Sep 17 00:00:00 2001 From: vastsa Date: Wed, 21 Oct 2026 20:14:46 +0800 Subject: [PATCH 2/2] fix(e2e): test certificate probe kill escalation Keep the normal Electron path unchanged while asserting that a child ignoring SIGTERM receives SIGKILL after the grace period. --- package.json | 2 +- scripts/e2e-provider-client-certificate.mjs | 34 +++----- scripts/e2e/wait-for-child-exit.mjs | 29 +++++++ scripts/e2e/wait-for-child-exit.test.mjs | 87 +++++++++++++++++++++ 4 files changed, 126 insertions(+), 26 deletions(-) create mode 100644 scripts/e2e/wait-for-child-exit.mjs create mode 100644 scripts/e2e/wait-for-child-exit.test.mjs 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 f29cb14846..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"); @@ -34,29 +35,12 @@ try { // 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. - let killedBy = null; - const termTimer = setTimeout(() => { - killedBy = "SIGTERM"; - child.kill("SIGTERM"); - }, 60_000); - const killTimer = setTimeout(() => { - killedBy = "SIGKILL"; - child.kill("SIGKILL"); - }, 65_000); - try { - const code = await new Promise((resolve, reject) => { - child.once("error", reject); - child.once("close", resolve); - }); - 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 { - clearTimeout(termTimer); - clearTimeout(killTimer); - } + 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); +});