fix(tui): settle execFileNoThrow on timeout even when a daemon holds stdio
The timeout handler only called settle(124) when resolveOnExit was true. In the default path the promise waits for 'close', which requires every inherited stdio handle to close — a daemonized grandchild that kept the pipes open meant 'close' never fired, and after the timeout SIGTERM (which only reaches the direct child) nothing settled the promise. The await hung forever: the clipboard path (setClipboard -> tmuxLoadBuffer -> osc.ts spawn without resolveOnExit) leaked a pending promise whenever a spawned tool forked a stdio-inheriting daemon (#93134). Settle(124) unconditionally in the timeout handler. The settled-guard makes it a no-op when the child's own 'exit'/'close' won the race, so normal timeout behavior is unchanged; in the daemon case it becomes the only exit and returns the same 124 the close path would have. Also un-skips the documented-hang regression test, with a 30s daemon sleeper so it genuinely outlives the timeout (and vitest's own 5s test timeout — before the fix the test fails by timing out, not asserting), plus an elapsed bound.
This commit is contained in:
@@ -65,19 +65,26 @@ afterEach(() => {
|
||||
})
|
||||
|
||||
describe.skipIf(onWindows)('execFileNoThrow with daemon-style children', () => {
|
||||
// Skipped because the bug it documents is a forever-hang. Without
|
||||
// resolveOnExit, the 'close' event doesn't fire when the immediate
|
||||
// child has exited but a forked daemon still holds stdio open. Even
|
||||
// SIGTERM at the timeout doesn't help — the daemon survives it. To
|
||||
// verify by hand: remove `it.skip` and watch the test timeout. This
|
||||
// test is here so a reviewer reading the resolveOnExit option knows
|
||||
// *why* every clipboard-tool spawn in osc.ts wires it on.
|
||||
it.skip('(documented hang) without resolveOnExit, await never resolves when daemon inherits stdio', async () => {
|
||||
// Formerly a documented forever-hang: without resolveOnExit, the 'close'
|
||||
// event doesn't fire when the immediate child has exited but a forked
|
||||
// daemon still holds stdio open, and even the SIGTERM at timeout used to
|
||||
// leave the promise unsettled (#93134). The unconditional settle(124) in
|
||||
// the timeout handler now guarantees the await resolves with 124.
|
||||
// The daemon script's sleeper lives 30s so it genuinely outlives the
|
||||
// timeout (and vitest's own 5s test timeout — before the fix this test
|
||||
// fails by timing out, not by asserting).
|
||||
it('settles with code=124 on timeout when a daemon inherits stdio and resolveOnExit is off', async () => {
|
||||
const pidFile = join(scriptDir, 'sleeper-skip.pid')
|
||||
const result = await execFileNoThrow(daemonScript, [pidFile], { timeout: 300 })
|
||||
const longDaemonScript = join(scriptDir, 'fake-daemonizer-long.sh')
|
||||
writeFileSync(longDaemonScript, '#!/bin/sh\nsleep 30 &\necho $! > "$1"\nexit 0\n')
|
||||
chmodSync(longDaemonScript, 0o755)
|
||||
const start = Date.now()
|
||||
|
||||
const result = await execFileNoThrow(longDaemonScript, [pidFile], { timeout: 300 })
|
||||
trackSleeperPid(pidFile)
|
||||
|
||||
expect(result.code).toBe(124)
|
||||
expect(Date.now() - start).toBeLessThan(2000)
|
||||
})
|
||||
|
||||
it("settles immediately on 'exit' when resolveOnExit is true, regardless of daemon stdio", async () => {
|
||||
|
||||
@@ -69,12 +69,14 @@ export function execFileNoThrow(
|
||||
timedOut = true
|
||||
child.kill('SIGTERM')
|
||||
|
||||
// When resolving on exit, SIGTERM-ing a child that has already
|
||||
// exited is a no-op and `'exit'` won't fire again — settle here
|
||||
// so the promise doesn't leak. Safe under settled-guard.
|
||||
if (options.resolveOnExit) {
|
||||
settle(124)
|
||||
}
|
||||
// Settle unconditionally: SIGTERM-ing the child does not
|
||||
// guarantee 'close'/'exit' will fire. In the default
|
||||
// (non-resolveOnExit) path a daemonized grandchild that
|
||||
// inherited the stdio pipes keeps them open forever, so
|
||||
// 'close' never fires and the promise leaked (#93134).
|
||||
// The settled-guard makes this a no-op when the child's own
|
||||
// 'exit'/'close' won the race.
|
||||
settle(124)
|
||||
}, options.timeout)
|
||||
: null
|
||||
|
||||
|
||||
Reference in New Issue
Block a user