From 394bf4136fcc005663cc5cd2b2c9de534622efc9 Mon Sep 17 00:00:00 2001 From: Neil Date: Mon, 14 Sep 2026 14:02:29 -0700 Subject: [PATCH] fix(hooks): keep a timed-out hook's output instead of discarding it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The timeout settle passed `stdout: '', stderr: ''`, so everything the hook printed was dropped — exactly when the removal gate reports `unverifiable` and the user has nothing else to go on. The buffers are local variables one scope up now, so this is passing them instead of empty strings. Also drops a comment that still explained why `exec` was being kept. It isn't. Refs #19334 --- src/main/hook-termination-real-process.test.ts | 10 ++++++---- src/main/hooks.ts | 7 ++++--- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/main/hook-termination-real-process.test.ts b/src/main/hook-termination-real-process.test.ts index 0460d7292d9..20d5cffb882 100644 --- a/src/main/hook-termination-real-process.test.ts +++ b/src/main/hook-termination-real-process.test.ts @@ -18,7 +18,7 @@ const alive = (pid: number): boolean => { /** Run a hook past its deadline and report which of its real processes survived. */ async function survivorsAfterDeadline( script: string -): Promise<{ shell: boolean; child: boolean; pids: number[] }> { +): Promise<{ shell: boolean; child: boolean; output: string; pids: number[] }> { const { runHook } = await import('./hooks') const dir = mkdtempSync(join(tmpdir(), 'orca-term-')) const pidFile = join(dir, 'pids') @@ -34,7 +34,7 @@ async function survivorsAfterDeadline( await new Promise((resolve) => setTimeout(resolve, 3_500)) expect(existsSync(pidFile)).toBe(true) pids = readFileSync(pidFile, 'utf8').trim().split(/\s+/).map(Number) - return { shell: alive(pids[0]!), child: alive(pids[1]!), pids } + return { shell: alive(pids[0]!), child: alive(pids[1]!), output: result.output, pids } } finally { for (const pid of pids) { try { @@ -53,10 +53,12 @@ async function survivorsAfterDeadline( // or not a real group exists. That is the precise condition the bug turns on. describe.skipIf(process.platform === 'win32')('hook termination against real processes', () => { it('kills the shell and its child when the deadline expires', async () => { - const { shell, child } = await survivorsAfterDeadline( - 'sleep 120 &\necho "$$ $!" > "$PWD/pids"\nwait' + const { shell, child, output } = await survivorsAfterDeadline( + 'echo "archive step 3 of 7"\nsleep 120 &\necho "$$ $!" > "$PWD/pids"\nwait' ) expect({ shell, child }).toEqual({ shell: false, child: false }) + // The gate reports this run as `unverifiable`; what the hook printed is the only clue why. + expect(output).toContain('archive step 3 of 7') }, 30_000) it('kills a descendant that ignores SIGTERM', async () => { diff --git a/src/main/hooks.ts b/src/main/hooks.ts index e1a00ed7d3f..bec92fc7d2f 100644 --- a/src/main/hooks.ts +++ b/src/main/hooks.ts @@ -248,8 +248,7 @@ export function runHook( // reports whatever it chose to do, so a hook that traps SIGTERM and exits 0 came back as a // PASS — a hook cut off mid-archive, indistinguishable from one that finished. Settle on the // deadline instead, and settle AT it, so a hook that traps and keeps running cannot hold a - // removal open. `exec` stays because it owns the per-platform shell invocation (`cmd.exe` - // wants `/d /s /c`, not `-c`), which is not this change's to re-derive. + // removal open. let settled = false let deadline: NodeJS.Timeout | undefined const settle = (result: HookProcessOutcome): void => { @@ -304,7 +303,9 @@ export function runHook( deadline = setTimeout(() => { settle( classifyHookProcessResult( - { code: null, stdout: '', stderr: '', timedOut: true }, + // Keep what the hook printed: it is the only clue to why the removal gate says + // `unverifiable`. + { code: null, stdout, stderr, timedOut: true }, { hookName, cwd, timeoutMs } ) )