From 44ffd24236f47a50c551633ab326106c4d60f654 Mon Sep 17 00:00:00 2001 From: Adam Cheng <63501289+627150795@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:36:26 +0800 Subject: [PATCH 1/2] =?UTF-8?q?fix(background-terminals):=20Windows=20task?= =?UTF-8?q?kill=20=E8=B6=85=E6=97=B6=E9=81=BF=E5=85=8D=E8=AF=AF=E6=8A=A5?= =?UTF-8?q?=E8=BF=9B=E7=A8=8B=E6=A0=91=E5=B7=B2=E7=BB=88=E6=AD=A2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../background-terminals/src/manager.ts | 8 +++--- .../background-terminals/manager.test.ts | 26 +++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/extensions/background-terminals/src/manager.ts b/extensions/background-terminals/src/manager.ts index a8639400..43bf76cc 100644 --- a/extensions/background-terminals/src/manager.ts +++ b/extensions/background-terminals/src/manager.ts @@ -395,11 +395,9 @@ export async function signalWindowsProcessTree( : attempt.outcome === "timed_out" ? `taskkill timed out after ${attempt.timeoutMs}ms; helper ${attempt.helperClosed ? "closed after SIGKILL" : `did not close within an additional ${attempt.helperCloseTimeoutMs}ms`}` : `taskkill exited ${attempt.exitCode ?? "without a code"}${attempt.signal ? ` (${attempt.signal})` : ""}`; - if ( - attempt.outcome === "timed_out" && - !attempt.helperClosed && - !targetExited() - ) { + // ponytail: a closed helper proves only that taskkill stopped, not that the + // target tree was removed; retry or Job Objects can provide stronger proof. + if (attempt.outcome === "timed_out") { return { outcome: "unresolved", detail }; } // A failed graceful taskkill must leave the shell PID alive for the diff --git a/tests/extensions/background-terminals/manager.test.ts b/tests/extensions/background-terminals/manager.test.ts index b37e617c..0c543783 100644 --- a/tests/extensions/background-terminals/manager.test.ts +++ b/tests/extensions/background-terminals/manager.test.ts @@ -204,6 +204,32 @@ test("Windows taskkill timeout waits for the stopped helper to close", async () }); }); +test("Windows taskkill timeout never falls back to a direct shell signal", async () => { + const killer = new EventEmitter() as ChildProcess; + killer.kill = () => { + queueMicrotask(() => killer.emit("close", null, "SIGKILL")); + return true; + }; + const signals: Array = []; + + const result = await signalWindowsProcessTree( + { + pid: 47, + kill(signal) { + signals.push(signal); + return true; + }, + }, + "SIGKILL", + () => false, + () => killer, + ); + + assert.equal(result.outcome, "unresolved"); + assert.match(result.detail, /helper closed after SIGKILL/); + assert.deepEqual(signals, []); +}); + test("Windows taskkill helper close has a second explicit bound", async () => { const killer = new EventEmitter() as ChildProcess; killer.kill = () => true; From a2c23a150c4910b26aba0dee134c42903f7d222c Mon Sep 17 00:00:00 2001 From: tt-a1i Date: Sun, 6 Sep 2026 09:46:34 +0800 Subject: [PATCH 2/2] test(background-terminals): cover Windows timeout exit races --- .../background-terminals/src/manager.ts | 4 +- .../background-terminals/manager.test.ts | 52 +++++++++++-------- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/extensions/background-terminals/src/manager.ts b/extensions/background-terminals/src/manager.ts index 43bf76cc..844dcb28 100644 --- a/extensions/background-terminals/src/manager.ts +++ b/extensions/background-terminals/src/manager.ts @@ -395,8 +395,8 @@ export async function signalWindowsProcessTree( : attempt.outcome === "timed_out" ? `taskkill timed out after ${attempt.timeoutMs}ms; helper ${attempt.helperClosed ? "closed after SIGKILL" : `did not close within an additional ${attempt.helperCloseTimeoutMs}ms`}` : `taskkill exited ${attempt.exitCode ?? "without a code"}${attempt.signal ? ` (${attempt.signal})` : ""}`; - // ponytail: a closed helper proves only that taskkill stopped, not that the - // target tree was removed; retry or Job Objects can provide stronger proof. + // Closing the helper or observing the shell exit does not prove that all + // descendants exited. Preserve uncertainty instead of killing only the shell. if (attempt.outcome === "timed_out") { return { outcome: "unresolved", detail }; } diff --git a/tests/extensions/background-terminals/manager.test.ts b/tests/extensions/background-terminals/manager.test.ts index 0c543783..8b9114da 100644 --- a/tests/extensions/background-terminals/manager.test.ts +++ b/tests/extensions/background-terminals/manager.test.ts @@ -204,31 +204,37 @@ test("Windows taskkill timeout waits for the stopped helper to close", async () }); }); -test("Windows taskkill timeout never falls back to a direct shell signal", async () => { - const killer = new EventEmitter() as ChildProcess; - killer.kill = () => { - queueMicrotask(() => killer.emit("close", null, "SIGKILL")); - return true; - }; - const signals: Array = []; - - const result = await signalWindowsProcessTree( - { - pid: 47, - kill(signal) { - signals.push(signal); +for (const signal of ["SIGTERM", "SIGKILL"] as const) { + for (const shellExitsDuringTimeout of [false, true]) { + test(`Windows ${signal} taskkill timeout stays unresolved when shell exit is ${shellExitsDuringTimeout}`, async () => { + let targetExited = false; + const killer = new EventEmitter() as ChildProcess; + killer.kill = () => { + targetExited = shellExitsDuringTimeout; + queueMicrotask(() => killer.emit("close", null, "SIGKILL")); return true; - }, - }, - "SIGKILL", - () => false, - () => killer, - ); + }; + const signals: Array = []; + + const result = await signalWindowsProcessTree( + { + pid: 47, + kill(signal) { + signals.push(signal); + return true; + }, + }, + signal, + () => targetExited, + () => killer, + ); - assert.equal(result.outcome, "unresolved"); - assert.match(result.detail, /helper closed after SIGKILL/); - assert.deepEqual(signals, []); -}); + assert.equal(result.outcome, "unresolved"); + assert.match(result.detail, /helper closed after SIGKILL/); + assert.deepEqual(signals, []); + }); + } +} test("Windows taskkill helper close has a second explicit bound", async () => { const killer = new EventEmitter() as ChildProcess;