Browse Source

fix(subprocess): stop escalation after tree exit

pku-xht 2 months ago
parent
commit
8067701b92

+ 42 - 11
packages/subprocess/subprocess-local/src/spawn.ts

@@ -384,6 +384,8 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
 
   let graceTimer: ReturnType<typeof scheduleFiniteTimeout> | undefined
   let terminationStarted = false
+  let treeExitObserved = false
+  let treeExitObservation: Promise<void> | undefined
   let settled = false
 
   // Failed spawns use pid -1 so signalling remains a no-op.
@@ -391,6 +393,9 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
 
   /** Whether the detached tree's root (or POSIX group) is still alive. */
   const treeAlive = (): boolean => {
+    /* v8 ignore next -- only a timer callback already queued when the observer settles can enter here;
+       the guard is the final defense against probing an id after its tree was confirmed absent. */
+    if (treeExitObserved) return false
     if (pid <= 0) return false
     if (platform === 'win32') {
       // Windows has no group-liveness probe; the direct child's exit is the
@@ -413,13 +418,29 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
     }
   }
 
+  /**
+   * Start or reuse the handle's single whole-tree exit observer. The first
+   * confirmed absence is a permanent no-more-signals boundary: it cancels a
+   * pending escalation before this process-group id can be reused.
+   */
+  const observeTreeExit = (): Promise<void> => {
+    treeExitObservation ??= (async () => {
+      while (treeAlive()) await sleepTick()
+      treeExitObserved = true
+      terminationStarted = true
+      graceTimer?.cancel()
+      graceTimer = undefined
+    })()
+    return treeExitObservation
+  }
+
   // The escalation's tier primitive (not on the handle — terminate() is the
   // only consumer-facing termination verb). Guards on TREE liveness, not
   // outcome settlement: a TERM-trapping helper can outlive the settled direct
   // child and must stay signalable, while a fully-dead tree (possible pid
   // reuse) must not be re-signalled by a later tier.
   const kill = (sig: NodeJS.Signals): void => {
-    /* v8 ignore next -- a successful consumer wait cancels the ordinary dead-tree timer;
+    /* v8 ignore next -- the shared exit observer cancels the ordinary dead-tree timer;
        this remains the timer/death race guard and cannot be staged deterministically. */
     if (!treeAlive()) return
     signalTree(platform, pid, sig, child, taskkill)
@@ -428,7 +449,10 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
   const terminate = (): void => {
     if (terminationStarted) return
     terminationStarted = true
-    if (!treeAlive()) return
+    // Observe from the first termination tier onward, even when inherited
+    // pipes delay `done` and no consumer has begun its own teardown wait.
+    void observeTreeExit()
+    if (treeExitObserved) return
     kill('SIGTERM')
     // The escalation must survive direct-child settlement — the leader dying
     // does not mean the tree died — so settle does not clear this timer, and
@@ -487,16 +511,23 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
   })
 
   const waitForExit = async (signal?: AbortSignal): Promise<boolean> => {
-    while (treeAlive()) {
-      if (signal?.aborted) return false
-      await sleepTick()
+    const observed = observeTreeExit()
+    if (treeExitObserved) return true
+    if (signal?.aborted) return false
+    if (signal === undefined) {
+      await observed
+      return true
+    }
+    const aborted = Promise.withResolvers<boolean>()
+    const onAbort = (): void => { aborted.resolve(false) }
+    signal.addEventListener('abort', onAbort, { once: true })
+    /* v8 ignore next -- closes the event-loop race between the preceding aborted check and listener registration. */
+    if (signal.aborted) onAbort()
+    try {
+      return await Promise.race([observed.then(() => true), aborted.promise])
+    } finally {
+      signal.removeEventListener('abort', onAbort)
     }
-    // Successful observation is the permanent no-more-signals boundary. It
-    // also cancels an escalation whose TERM tier already removed the tree.
-    terminationStarted = true
-    graceTimer?.cancel()
-    graceTimer = undefined
-    return true
   }
 
   return {

+ 48 - 0
packages/subprocess/subprocess-local/tests/spawn.spec.ts

@@ -205,6 +205,54 @@ describe('spawnSubprocess', () => {
     await expect(running.waitForExit()).resolves.toBe(true)
   })
 
+  it('cancels escalation when the terminated group vanishes before collected pipes drain', async () => {
+    const pidFile = join(spillDir, `escaped-pipe-holder-${Date.now()}.pid`)
+    const graceMs = 160
+    const childScript = `
+      const { spawn } = require('node:child_process')
+      const { writeFileSync } = require('node:fs')
+      const helper = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], {
+        detached: true,
+        stdio: ['ignore', 1, 2],
+      })
+      writeFileSync(${JSON.stringify(pidFile)}, String(helper.pid))
+      helper.unref()
+      setInterval(() => {}, 1000)
+    `
+    const running = spawnSubprocess({
+      ...spec('unused', { graceMs }),
+      argv: [process.execPath, '-e', childScript],
+    })
+    const helper = await waitForPidFile(pidFile)
+    const realKill: typeof process.kill = process.kill.bind(process)
+    let termAt = 0
+    let forceSignals = 0
+    const killSpy = vi.spyOn(process, 'kill').mockImplementation((target, signal) => {
+      if (target !== -running.pid) return realKill(target, signal)
+      if (signal === 'SIGTERM') {
+        termAt = Date.now()
+        return realKill(target, signal)
+      }
+      if (signal === 'SIGKILL') {
+        forceSignals += 1
+        return true
+      }
+      if (signal === 0 && termAt !== 0 && Date.now() - termAt < graceMs / 2) {
+        throw Object.assign(new Error('simulated vanished process group'), { code: 'ESRCH' })
+      }
+      return true // Before TERM the original group is live; later its pgid is reused.
+    })
+    try {
+      running.terminate()
+      await running.done
+      expect(forceSignals).toBe(0)
+    } finally {
+      killSpy.mockRestore()
+      process.kill(helper, 'SIGKILL')
+      await waitGone(helper)
+    }
+  })
+
   it('terminates the whole process group (grandchildren die too)', async () => {
     // The subshell writes the sleep's pid then waits on it; terminating the
     // group must take the sleep down with bash.