fix(process): retain owned command cleanup diagnostics (#1907)

Preserve per-member verification failures and sweep uncertainty across supervisor stop and owned-command failures. Report the observed token mismatch with its PID coordinates instead of implying PID reuse. Keep unresolved ownership unsignaled and retain the supervisor for a later verified stop. Exercise cancellation, timeout and cleanup revalidation with source-owned process fixtures and cooperative settlement. Closes #1856 Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com>
2026-10-12 01:00:08 +08:00 · 2026-10-12 01:00:08 +08:00 · 38539a2f4a
parent 23d50ea3be
commit 38539a2f4a
8 changed files with 209 additions and 29 deletions
--- a/src/process/OwnedCommand.ts
+++ b/src/process/OwnedCommand.ts
@ -7,7 +7,10 @@ import {
  ProviderProcessSupervisor,
  spawnOwnedProviderProcess,
 } from "./ProviderProcess.js";
-import { cleanupOwnedProcessGroup } from "./ProcessOwnership.js";
+import {
+  cleanupOwnedProcessGroup,
+  type ProcessCleanupFailure,
+} from "./ProcessOwnership.js";
 import {
  ProviderStartupDeadline,
  waitForAbortableDelay,
@ -15,16 +18,19 @@ import {

 /** A short-lived command failed its lifecycle or complete-output contract. */
 export class OwnedCommandFailure extends Error {
+  readonly cleanupResult: ProcessCleanupFailure | undefined;
+
  constructor(
    readonly reason: "cancelled" | "timeout" | "output-limit" | "process",
    message: string,
    readonly snapshot: ProviderProcessSnapshot | null = null,
    readonly cleanupFailure: string | null = null,
-    options?: ErrorOptions,
+    options?: ErrorOptions & { readonly cleanupResult?: ProcessCleanupFailure },
    readonly resources: readonly string[] = [],
    readonly cleanupOwner?: ProviderProcessSupervisor,
  ) {
    super(message, options);
+    this.cleanupResult = options?.cleanupResult;
  }
 }

@ -190,7 +196,12 @@ const collectOwnedCommand = async (
      failure?.message ?? "Command cleanup failed.",
      snapshot,
      stopped.reason,
-      failure === undefined ? undefined : { cause: failure },
+      {
+        ...(failure === undefined ? {} : { cause: failure }),
+        ...(stopped.cleanupResult === undefined
+          ? {}
+          : { cleanupResult: stopped.cleanupResult }),
+      },
      [
        spawn.runId,
        ...(spawn.cwd === undefined ? [] : [spawn.cwd]),
@ -205,7 +216,12 @@ const collectOwnedCommand = async (
      failure.message,
      snapshot,
      failure.cleanupFailure,
-      { cause: failure },
+      {
+        cause: failure,
+        ...(failure.cleanupResult === undefined
+          ? {}
+          : { cleanupResult: failure.cleanupResult }),
+      },
      failure.resources,
    );
  if (callerSignal?.aborted)
--- a/src/process/ProcessOwnership.part2.test.ts
+++ b/src/process/ProcessOwnership.part2.test.ts
@ -111,7 +111,7 @@ describe("owned process-group cleanup discovery", () => {
    await expect(cleanupOwnedProcessGroup(ownership, adapter)).resolves.toEqual(
      {
        cleaned: false,
-        reason: "process tree contains an unowned or PID-reused process",
+        reason: "process ownership token mismatch for PID(s): 104",
        failures: [{ pid: 104, reason: "run-token-mismatch" }],
      },
    );
--- a/src/process/ProcessOwnership.test.ts
+++ b/src/process/ProcessOwnership.test.ts
@ -331,7 +331,7 @@ describe("sampled detached process groups during cleanup", () => {

    expect(result).toMatchObject({
      cleaned: false,
-      reason: "process tree contains an unowned or PID-reused process",
+      reason: "process ownership token mismatch for PID(s): 200",
      failures: [{ pid: 200, reason: "run-token-mismatch" }],
    });
    expect(signalGroup).toHaveBeenCalledTimes(1);
@ -369,7 +369,7 @@ describe("sampled detached process groups during cleanup", () => {

    expect(result).toMatchObject({
      cleaned: false,
-      reason: "process tree contains an unowned or PID-reused process",
+      reason: "process ownership token mismatch for PID(s): 200",
      failures: [{ pid: 200, reason: "run-token-mismatch" }],
    });
    expect(signalGroup).not.toHaveBeenCalled();
--- a/src/process/ProcessOwnership.ts
+++ b/src/process/ProcessOwnership.ts
@ -157,19 +157,15 @@ export type ProcessCleanupResult =
      readonly signaled: boolean;
      readonly unverified?: readonly OwnershipSweepUnverifiedProcess[];
    }
-  | {
-      readonly cleaned: false;
-      readonly reason: string;
-      readonly failures?: readonly {
-        readonly pid: number;
-        readonly reason:
-          | "environment-unreadable"
-          | "run-token-mismatch"
-          | "process-identity-unavailable";
-        readonly diagnostic?: string;
-      }[];
-      readonly unverified?: readonly OwnershipSweepUnverifiedProcess[];
-    };
+  | ProcessCleanupFailure;
+
+/** Retained verification failures for one cleanup attempt, without additional signaling authority. */
+export interface ProcessCleanupFailure {
+  readonly cleaned: false;
+  readonly reason: string;
+  readonly failures?: readonly ProcessOwnershipValidationFailure[];
+  readonly unverified?: readonly OwnershipSweepUnverifiedProcess[];
+}

 /** Token-verified liveness result used by post-root-exit settlement. */
 export type ProcessGroupObservation =
@ -591,7 +587,15 @@ const cleanupValidationFailure = (
  if (failures.some(({ reason }) => reason === "run-token-mismatch"))
    return {
      cleaned: false,
-      reason: "process tree contains an unowned or PID-reused process",
+      reason: `process ownership token mismatch for PID(s): ${[
+        ...new Set(
+          failures
+            .filter(({ reason }) => reason === "run-token-mismatch")
+            .map(({ pid }) => pid),
+        ),
+      ]
+        .sort((left, right) => left - right)
+        .join(", ")}`,
      failures,
      ...unverifiedEntry,
    };
--- a/src/process/ProcessOwnership.validation.test.ts
+++ b/src/process/ProcessOwnership.validation.test.ts
@ -74,7 +74,7 @@ describe("owned process-group cleanup validation: ownership and lineage", () =>
    });
    expect(await cleanupOwnedProcessGroup(ownership, adapter)).toEqual({
      cleaned: false,
-      reason: "process tree contains an unowned or PID-reused process",
+      reason: "process ownership token mismatch for PID(s): 101",
      failures: [{ pid: 101, reason: "run-token-mismatch" }],
    });
    expect(signalGroup).not.toHaveBeenCalled();
@ -106,7 +106,7 @@ describe("owned process-group cleanup validation: ownership and lineage", () =>
    };
    expect(await cleanupOwnedProcessGroup(ownership, adapter)).toEqual({
      cleaned: false,
-      reason: "process tree contains an unowned or PID-reused process",
+      reason: "process ownership token mismatch for PID(s): 101",
      failures: [
        expect.objectContaining({
          pid: 100,
@ -189,7 +189,7 @@ describe("owned process-group cleanup liveness rechecks", () => {
        cleanupOwnedProcessGroup(ownership, adapter),
      ).resolves.toEqual({
        cleaned: false,
-        reason: "process tree contains an unowned or PID-reused process",
+        reason: "process ownership token mismatch for PID(s): 101",
        failures: recheckFails
          ? [
              {
@ -345,7 +345,7 @@ describe("owned process-group cleanup validation: launcher exit races", () => {
        if (remaining === "foreign")
          expect(result).toMatchObject({
            cleaned: false,
-            reason: "process tree contains an unowned or PID-reused process",
+            reason: "process ownership token mismatch for PID(s): 101",
          });
        else
          expect(result).toEqual({
--- a/src/process/ProviderProcess.ts
+++ b/src/process/ProviderProcess.ts
@ -6,6 +6,7 @@ import { WindowsOwnedProcess } from "../windows/WindowsOwnedProcess.js";

 import type {
  OwnedProcessGroup,
+  ProcessCleanupFailure,
  ProcessCleanupResult,
 } from "./ProcessOwnership.js";
 import { prepareProcessOwnershipInspection } from "./ProcessOwnershipObservation.js";
@ -143,7 +144,11 @@ export type ProviderProcessStopResult =
        | "terminated"
        | "killed";
    }
-  | { readonly status: "incomplete"; readonly reason: string };
+  | {
+      readonly status: "incomplete";
+      readonly reason: string;
+      readonly cleanupResult?: ProcessCleanupFailure;
+    };

 /**
 * Spawn a provider in an owned Windows job or dedicated POSIX process group.
@ -373,13 +378,21 @@ export class ProviderProcessSupervisor {
      if (this.launch.cleanup !== undefined) {
        const cleanup = await this.#ownedCleanup(options.cleanupResult);
        if (!cleanup.cleaned)
-          return { status: "incomplete", reason: cleanup.reason };
+          return {
+            status: "incomplete",
+            reason: cleanup.reason,
+            cleanupResult: cleanup,
+          };
        const graceMs = options.killGraceMs ?? DEFAULT_KILL_GRACE_MS;
        if (await this.waitForExit(graceMs))
          return { status: "verified-cleanup" };
        const retried = await this.launch.cleanup();
        if (!retried.cleaned)
-          return { status: "incomplete", reason: retried.reason };
+          return {
+            status: "incomplete",
+            reason: retried.reason,
+            cleanupResult: retried,
+          };
        if (await this.waitForExit(graceMs))
          return { status: "verified-cleanup" };
        return {
--- a/tests/boundary/process/ownedCommand.test.ts
+++ b/tests/boundary/process/ownedCommand.test.ts
@ -8,7 +8,10 @@ import {
  runOwnedCommand,
 } from "../../../src/process/OwnedCommand.js";
 import { spawnOwnedProviderProcess } from "../../../src/process/ProviderProcess.js";
-import { cleanupOwnedProcessGroup } from "../../../src/process/ProcessOwnership.js";
+import {
+  cleanupOwnedProcessGroup,
+  type ProcessCleanupResult,
+} from "../../../src/process/ProcessOwnership.js";
 import { prepareProcessOwnershipInspection } from "../../../src/process/ProcessOwnershipObservation.js";
 import { waitForProviderProcessReady } from "../../fixtures/providerProcess.js";

@ -186,6 +189,99 @@ it("returns a timeout for a running process after independent cleanup", async ()
  ).rejects.toMatchObject({ reason: "timeout", cleanupFailure: null });
 });

+it.skipIf(process.platform === "win32").each(["cancelled", "timeout"] as const)(
+  "retains the real per-member cleanup ledger after %s and settles after cooperative exit",
+  async (reason) => {
+    const childScript =
+      'process.stdin.once("data", () => process.exit(0)); process.stdout.write("child-ready\\n")';
+    const request = {
+      ...command(`
+        const { spawn } = require("node:child_process");
+        const child = spawn(process.execPath, ["-e", ${JSON.stringify(childScript)}], {
+          env: { ...process.env, REA_PROCESS_RUN_ID: "different-child-owner" },
+          stdio: ["pipe", "inherit", "inherit"],
+        });
+        const release = () => child.stdin.end("release");
+        process.stdin.once("data", release);
+        const failsafe = setTimeout(release, 10000);
+        child.once("exit", () => { clearTimeout(failsafe); process.stdin.destroy(); });
+      `),
+      stdin: "pipe" as const,
+    };
+    const controller = new AbortController();
+    let launched:
+      | Awaited<ReturnType<typeof spawnOwnedProviderProcess>>
+      | undefined;
+    let cleanup: ProcessCleanupResult | undefined;
+    let failure: OwnedCommandFailure | undefined;
+    try {
+      await runOwnedCommand(
+        request,
+        { timeoutMs: 2000, diagnosticBytes: 1024 },
+        {
+          signal: controller.signal,
+          launcher: async (input) => {
+            const acquired = await spawnOwnedProviderProcess(input);
+            launched = acquired;
+            acquired.process.stdout?.on("data", (chunk: Buffer) => {
+              if (
+                reason === "cancelled" &&
+                chunk.toString().includes("child-ready")
+              )
+                controller.abort();
+            });
+            return {
+              ...acquired,
+              cleanup: async () => {
+                const observed = await cleanupOwnedProcessGroup(
+                  acquired.ownership,
+                );
+                cleanup ??= observed;
+                return observed;
+              },
+            };
+          },
+        },
+      ).catch((cause: unknown) => {
+        if (!(cause instanceof OwnedCommandFailure)) throw cause;
+        failure = cause;
+      });
+      if (failure === undefined || launched === undefined)
+        throw new Error(
+          "The acquired command must retain its failed cleanup owner",
+        );
+      expect(failure.reason).toBe(reason);
+      expect(failure.snapshot?.stdout.text).toContain("child-ready");
+      expect(cleanup).toMatchObject({
+        cleaned: false,
+        failures: [expect.objectContaining({ reason: "run-token-mismatch" })],
+      });
+      expect(failure.cleanupResult).toEqual(cleanup);
+      expect(failure.resources).toEqual(
+        expect.arrayContaining([request.runId]),
+      );
+      // An unresolved member does not grant authority to signal the shared group.
+      const acquiredPid = launched.ownership.leaderPid;
+      expect(() => process.kill(acquiredPid, 0)).not.toThrow();
+      if (cleanup?.cleaned === false) {
+        for (const member of cleanup.failures ?? []) {
+          expect(() => process.kill(member.pid, 0)).not.toThrow();
+          expect(failure.cleanupFailure).toContain(String(member.pid));
+        }
+      }
+    } finally {
+      // Release through the fixture's acquired input channel, never by an unverified PID.
+      launched?.process.stdin?.end("release");
+      if (failure?.cleanupOwner !== undefined) {
+        expect(await failure.cleanupOwner.waitForOutputClose(5000)).toBe(true);
+        expect(await failure.cleanupOwner.stop()).toEqual({
+          status: "verified-cleanup",
+        });
+      }
+    }
+  },
+);
+
 it("honors cancellation during cleanup after a successful command", async () => {
  const controller = new AbortController();
  await expect(
--- a/tests/boundary/process/providerProcess.test.ts
+++ b/tests/boundary/process/providerProcess.test.ts
@ -358,6 +358,57 @@ describe("provider process output and cleanup primitives", () => {
  });
 });

+it("retains member failures and sweep uncertainty from a failed cleanup recheck", async () => {
+  const child = spawnProviderProcessFixture("stubborn");
+  await waitForProviderProcessReady(child);
+  const pid = child.pid;
+  if (pid === undefined) {
+    await stopProviderProcessFixture(child);
+    throw new Error("The acquired ready fixture must have a process ID");
+  }
+  const failed = {
+    cleaned: false as const,
+    reason: "fixture identity unavailable during recheck",
+    failures: [
+      {
+        pid,
+        reason: "process-identity-unavailable" as const,
+        diagnostic: "fixture identity reader unavailable",
+      },
+    ],
+    unverified: [
+      { pid: 777, diagnostic: "fixture foreign process token unavailable" },
+    ],
+  };
+  let attempts = 0;
+  let release = false;
+  const supervisor = new ProviderProcessSupervisor({
+    process: child,
+    ownsProcessLifetime: true,
+    cleanup: async () => {
+      if (release) child.kill("SIGKILL");
+      attempts += 1;
+      return release || attempts === 1
+        ? { cleaned: true as const, signaled: release }
+        : failed;
+    },
+  });
+  try {
+    await expect(supervisor.stop({ killGraceMs: 20 })).resolves.toEqual({
+      status: "incomplete",
+      reason: failed.reason,
+      cleanupResult: failed,
+    });
+    expect(child.exitCode).toBeNull();
+    release = true;
+    await expect(supervisor.stop()).resolves.toEqual({
+      status: "verified-cleanup",
+    });
+  } finally {
+    await stopProviderProcessFixture(child);
+  }
+});
+
 describe("provider process host injection", () => {
  it("uses injected host platform and environment when spawning", async () => {
    const spawned = await spawnOwnedProviderProcess({