Browse Source

fix(subprocess): keep awaited tree waits and the pending SIGKILL ref'd (Codex round 2)

An awaited waitForExit()/dispose() must hold the event loop open until the
tree really exits: with the liveness tick and the escalation timer unref'd,
a parent with no other live handles could exit claiming quiescence and
orphan the survivors it promised to reap. The escalation's pending SIGKILL
is a commitment; it self-bounds at graceMs. Module graph picks up the
subprocess-local -> timeout edge.
Tianyi Cui 2 months ago
parent
commit
f2972e846d
2 changed files with 13 additions and 6 deletions
  1. 2 1
      docs/module-graph.md
  2. 11 5
      packages/subprocess/subprocess-local/src/spawn.ts

+ 2 - 1
docs/module-graph.md

@@ -283,6 +283,7 @@ flowchart TD
   pkg_storage_sqlite --> pkg_storage
   pkg_subprocess_local --> pkg_invariants
   pkg_subprocess_local --> pkg_subprocess
+  pkg_subprocess_local --> pkg_timeout
   pkg_llm_deepseek --> pkg_invariants
   pkg_llm_deepseek --> pkg_llm
   pkg_llm_deepseek --> pkg_timeout
@@ -852,7 +853,7 @@ flowchart TD
 | [`storage-domain`](../packages/storage/storage-domain) | `storage` | [`invariants`](../packages/support/invariants), [`storage`](../packages/storage/storage) |
 | [`storage-json`](../packages/storage/storage-json) | `storage` | [`invariants`](../packages/support/invariants), [`storage`](../packages/storage/storage) |
 | [`storage-sqlite`](../packages/storage/storage-sqlite) | `storage` | [`invariants`](../packages/support/invariants), [`storage`](../packages/storage/storage) |
-| [`subprocess-local`](../packages/subprocess/subprocess-local) | `subprocess` | [`invariants`](../packages/support/invariants), [`subprocess`](../packages/subprocess/subprocess) |
+| [`subprocess-local`](../packages/subprocess/subprocess-local) | `subprocess` | [`invariants`](../packages/support/invariants), [`subprocess`](../packages/subprocess/subprocess), [`timeout`](../packages/util/timeout) |
 | [`llm-deepseek`](../packages/llm/llm-deepseek) | `llm` | [`invariants`](../packages/support/invariants), [`llm`](../packages/llm/llm), [`timeout`](../packages/util/timeout) |
 | [`llm-pi-ai`](../packages/llm/llm-pi-ai) | `llm` | [`invariants`](../packages/support/invariants), [`llm`](../packages/llm/llm), [`timeout`](../packages/util/timeout) |
 | [`session`](../packages/core/session) | `core` | [`brand`](../packages/util/brand), [`invariants`](../packages/support/invariants), [`llm`](../packages/llm/llm), [`scope`](../packages/core/scope) |

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

@@ -66,9 +66,14 @@ export interface SpawnInternals {
 /** Timeout code marking a dispose-ladder tier bound (vs an external abort). */
 const DISPOSE_TIER_TIMEOUT = 'SUBPROCESS_DISPOSE_TIER'
 
-/** Liveness-poll cadence for tree-exit waits; unref'd so an abandoned wait cannot hold the parent's loop open. */
+/**
+ * Liveness-poll cadence for tree-exit waits. The timer stays ref'd: an
+ * awaited teardown must keep the event loop alive until the tree really
+ * exits, or the parent can exit while claiming quiescence and orphan the
+ * survivors it promised to reap.
+ */
 function sleepTick(): Promise<void> {
-  return sleepMs(15, undefined, { ref: false })
+  return sleepMs(15)
 }
 
 let spillCounter = 0
@@ -401,12 +406,13 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
     if (!treeAlive()) return
     signalTree(platform, pid, 'SIGTERM', child, taskkill)
     // The escalation must survive direct-child settlement — the leader dying
-    // does not mean the tree died — so the timer is unref'd rather than
-    // cleared at settle, and re-checks tree liveness before force-killing.
+    // does not mean the tree died — so settle does not clear this timer, and
+    // it re-probes tree liveness before force-killing. It stays ref'd: the
+    // pending SIGKILL is a commitment, and a parent exiting before it fires
+    // would orphan a trapped survivor. Self-bounds at graceMs.
     graceTimer = setTimeout(() => {
       if (treeAlive()) signalTree(platform, pid, 'SIGKILL', child, taskkill)
     }, spec.graceMs)
-    graceTimer.unref()
   }
 
   // The caller owns timeout classification; this layer only reacts to abort.