From 38539a2f4a522243d6fc369ae34ef30e2a2f80cc Mon Sep 17 00:00:00 2001 From: Ysopwnable <161784452+N0zoM1z0@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:00:08 +0800 Subject: [PATCH] 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> --- src/process/OwnedCommand.ts | 24 ++++- src/process/ProcessOwnership.part2.test.ts | 2 +- src/process/ProcessOwnership.test.ts | 4 +- src/process/ProcessOwnership.ts | 32 +++--- .../ProcessOwnership.validation.test.ts | 8 +- src/process/ProviderProcess.ts | 19 +++- tests/boundary/process/ownedCommand.test.ts | 98 ++++++++++++++++++- .../boundary/process/providerProcess.test.ts | 51 ++++++++++ 8 files changed, 209 insertions(+), 29 deletions(-) diff --git a/src/process/OwnedCommand.ts b/src/process/OwnedCommand.ts index 33d06c5ea..a6b500b1b 100644 --- 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) diff --git a/src/process/ProcessOwnership.part2.test.ts b/src/process/ProcessOwnership.part2.test.ts index af34fde65..219b8095d 100644 --- 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" }], }, ); diff --git a/src/process/ProcessOwnership.test.ts b/src/process/ProcessOwnership.test.ts index 1589cce78..5f1906a3a 100644 --- 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(); diff --git a/src/process/ProcessOwnership.ts b/src/process/ProcessOwnership.ts index 2582dcf5d..f9fd5d99b 100644 --- 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, }; diff --git a/src/process/ProcessOwnership.validation.test.ts b/src/process/ProcessOwnership.validation.test.ts index ddbcb7fe4..c718f4b93 100644 --- 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({ diff --git a/src/process/ProviderProcess.ts b/src/process/ProviderProcess.ts index b6765b93f..1768c6c5b 100644 --- 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 { diff --git a/tests/boundary/process/ownedCommand.test.ts b/tests/boundary/process/ownedCommand.test.ts index 05ccdb81c..34205af3d 100644 --- 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> + | 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( diff --git a/tests/boundary/process/providerProcess.test.ts b/tests/boundary/process/providerProcess.test.ts index 3874be079..b2813ce46 100644 --- 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({