fix(hooks): keep a timed-out hook's output instead of discarding it

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
This commit is contained in:
Neil
2026-09-14 14:02:29 -07:00
parent cd560e81e1
commit 394bf4136f
2 changed files with 10 additions and 7 deletions
@@ -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 () => {
+4 -3
View File
@@ -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 }
)
)