PR 010 F1: Settle post-hardening failures safely

src/adapters/process-transport.ts

@@ -381,6 +381,49 @@ function waitForExit(child: ChildProcess, ms: number): Promise<boolean> {
   });
 }
 
+/**
+ * Run one best-effort cleanup step after a mandatory hardening failure.
+ *
+ * Cleanup on that path is best effort by definition. The handle it operates on
+ * has already been observed to be hostile, so reading `stdout`, destroying a
+ * pipe, or clearing listeners can each throw. None of those secondary failures
+ * may displace the hardening error the caller is owed, and none of them may
+ * abandon the steps that follow, so every step is isolated here.
+ */
+function attemptCleanup(step: () => void): void {
+  try {
+    step();
+  } catch {
+    // Deliberately absorbed; see the doc comment. The mandatory hardening
+    // failure is still what the scheduling path reports to the caller.
+  }
+}
+
+/**
+ * Clear a failed exchange's listeners without leaving the handle uncovered.
+ *
+ * {@link removeAllEvents} also removes the spawn-failure absorber, so restoring
+ * it belongs to the same synchronous step: nothing can be dispatched between
+ * the two calls, and the handle is therefore never observably uncovered. A
+ * hostile handle can make the restore itself throw, which would leave a queued
+ * spawn failure with no listener and take the host process down with it, so the
+ * absorber is installed once *before* the clear as well. That first call is the
+ * evidence that the captured `on` intrinsic still works on this handle; when it
+ * does not, the listeners are left exactly as they are, because a handle that
+ * kept its absorber is strictly safer than one left uncovered.
+ */
+function clearEventsKeepingAbsorber(child: ChildProcess): void {
+  try {
+    rearmSpawnFailureAbsorber(child);
+  } catch {
+    return;
+  }
+  attemptCleanup(() => {
+    removeAllEvents(child);
+    rearmSpawnFailureAbsorber(child);
+  });
+}
+
 /** Kill and reap a helper whose post-spawn dispatch hardening failed. */
 async function reapUnprotectedHelper(child: ChildProcess): Promise<void> {
   killDirectChild(child);
@@ -455,9 +498,18 @@ function runTaskkill(
     try {
       protectChildDispatch(killer);
     } catch {
-      void reapUnprotectedHelper(killer).then(() => {
-        resolve(false);
-      });
+      // The reap operates on a handle whose hardening already failed, so its
+      // own steps can throw. Resolving from both settlement paths keeps this
+      // helper's promise — and therefore every termination that awaits it —
+      // total, and leaves no discarded rejection unhandled.
+      void reapUnprotectedHelper(killer).then(
+        () => {
+          resolve(false);
+        },
+        () => {
+          resolve(false);
+        },
+      );
       return;
     }
 
@@ -596,6 +648,70 @@ async function terminate(
     : terminateWindows(child, pid, graceMs);
 }
 
+/**
+ * Release a child whose mandatory post-spawn dispatch hardening failed.
+ *
+ * Both halves of this operate on a handle already observed to be hostile:
+ * {@link terminate} can reject when an accessor it consults throws, and each
+ * cleanup step can throw either while reading `stdout`/`stderr` or on the value
+ * such an accessor yields. This function therefore **never rejects and never
+ * abandons a later step**, which is what lets its caller reach the one
+ * rejection the exchange owes on every hostile path.
+ *
+ * Nothing here strengthens any guarantee. Termination stays a bounded
+ * *attempt*, a failed kill stays a failed kill, and cleanup stays best effort;
+ * only the obligation to settle is absolute. The one thing this does insist on
+ * is that the attempt is actually *made*: when the platform strategy faults
+ * before it can signal, a single non-ignorable direct-child signal follows, and
+ * no process group, tree, or descendant is claimed on that path.
+ */
+async function releaseUnprotectedChild(
+  child: ChildProcess,
+  platform: TransportPlatform,
+  graceMs: number,
+): Promise<void> {
+  try {
+    await terminate(child, platform, graceMs);
+  } catch {
+    // A bounded termination attempt that fails is still only an attempt. The
+    // exchange's obligation is to settle, not to prove the child is gone.
+    //
+    // *No* attempt is a different thing. Termination consults the handle's own
+    // `exitCode`/`signalCode` before it signals anything, so a hostile accessor
+    // can abort the attempt on its very first observation — before any signal
+    // has been delivered, and on Windows before the helper that would deliver
+    // one has even been started. Releasing responsibility there would abandon a
+    // live direct child, so exactly one guarded direct-child signal is
+    // delivered here first. {@link killDirectChild} is the same primitive
+    // {@link reapUnprotectedHelper} already relies on: it goes through the
+    // captured `kill` intrinsic, reads no property of the handle, and absorbs
+    // its own failure. Nothing is waited on and nothing beyond the direct child
+    // is attempted, so this can neither re-enter a hostile accessor nor defer
+    // the rejection the caller is owed, and a fallback that fails stays a
+    // failure rather than becoming a claim.
+    //
+    // The signal is named rather than left to the default because this attempt
+    // gets exactly one shot. The graceful path is an *escalating* one — signal,
+    // wait out the grace window, escalate — and waiting is precisely what this
+    // fallback may not do. A lone `SIGTERM` is a request a POSIX child may
+    // catch or ignore outright, so a child that does would predictably outlive
+    // the one attempt on offer here; `SIGKILL` is the signal POSIX does not
+    // allow the target to handle, block, or ignore. On Windows the choice
+    // changes nothing: every signal Node accepts there terminates the target
+    // unconditionally, so this is the same operation the default already was.
+    // It is still only the direct child — `SIGKILL` is delivered to one
+    // process, is not inherited by descendants, and claims nothing about them.
+    killDirectChild(child, 'SIGKILL');
+  }
+  attemptCleanup(() => {
+    destroyReadable(child.stdout);
+  });
+  attemptCleanup(() => {
+    destroyReadable(child.stderr);
+  });
+  clearEventsKeepingAbsorber(child);
+}
+
 /**
  * Run one process exchange.
  *
@@ -712,19 +828,27 @@ export function invokeAgentProcess(
       if (invocation.signal !== null) {
         removeAbortListener(invocation.signal, onAbort);
       }
-      void terminate(child, platform, invocation.graceMs).then(() => {
-        destroyReadable(child.stdout);
-        destroyReadable(child.stderr);
-        removeAllEvents(child);
-        // Clearing the listeners also cleared the absorber; the child's own
-        // spawn failure may still be queued, so cover the handle again.
-        rearmSpawnFailureAbsorber(child);
-        reject(
-          error instanceof Error
-            ? error
-            : new Error('Process dispatch hardening failed', { cause: error }),
-        );
-      });
+      // Normalised here, before the asynchronous release, so the reason this
+      // exchange rejects with is already fixed and cannot itself be lost to a
+      // later hostile read.
+      const hardeningFailure =
+        error instanceof Error
+          ? error
+          : new Error('Process dispatch hardening failed', { cause: error });
+      // `releaseUnprotectedChild` runs every step and never rejects, and the
+      // rejection is scheduled on *both* settlement paths of the chain anyway,
+      // so neither a termination failure nor a cleanup step that throws on a
+      // poisoned `stdout`/`stderr` value can leave this exchange pending or
+      // leave an internal rejection unhandled. The mandatory hardening failure
+      // stays the externally visible reason on every one of those paths.
+      void releaseUnprotectedChild(child, platform, invocation.graceMs).then(
+        () => {
+          reject(hardeningFailure);
+        },
+        () => {
+          reject(hardeningFailure);
+        },
+      );
       return;
     }