morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #2656] fix(process): cleanup diagnostics lose retained capture process identity #957

New issue

Closed

opened 2026-10-15 18:29:52 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-15 18:29:52 +00:00

Owner

Originally created by @coygeek on GitHub (Oct 13, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/2656

Summary

A caller of inspect_packet_capture can receive cleanup_incomplete naming only a private workspace even though the provider still owns an unresolved process group. The command layer has already recorded the run ID, leader PID and process-group ID, but these identities do not survive the provider's cleanup error conversion. This prevents the caller from identifying that retained process group from the reported resources. Ownership and cleanup retry still work.

Steps to reproduce

This is an executed inert source reproduction through the shared service and TShark provider's launcher dependency seam. It starts no process and calls no process-table or signaling operation. It verifies error reporting and retained retries, not actual TShark execution or whether a real process remains alive.

In the source checkout at the baseline below, use the repository's pinned toolchain and prepared dependencies.
Save the following fixture as src/process/retainedResourceVerification.test.ts.

import { EventEmitter } from "node:events";
import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises";
import { join } from "node:path";
import { tmpdir } from "node:os";
import { expect, it } from "vitest";
import { PacketCaptureService } from "../application/captures/PacketCaptureService.js";
import { TsharkCaptureProvider } from "../captures/TsharkCaptureProvider.js";
import { projectAnalysisError } from "../domain/analysisErrorProjection.js";
import { readStableArtifact } from "../artifacts/readStableArtifact.js";
import type { OwnedProviderProcessSpawnOptions } from "./ProviderProcess.js";

it("reports the retained workspace but omits retained launch identity", async () => {
  const base = await mkdtemp(join(process.env.REA_VERIFICATION_DIR ?? tmpdir(), "retained-fixture-"));
  const root = join(base, "workspace");
  await mkdir(root);
  const command = join(base, "inert-tool");
  const capture = join(base, "recording.pcap");
  await writeFile(command, "inert fixture, never executed", { mode: 0o700 });
  await writeFile(capture, Buffer.from("d4c3b2a1020004000000000000000000ffff000001000000", "hex"));
  let cleanupCalls = 0;
  let rootCloses = 0;
  let launches = 0;
  let settled = false;
  let runId = "";
  const provider = new TsharkCaptureProvider(
    { REA_TSHARK_COMMAND: command, REA_TSHARK_PRLIMIT_COMMAND: command },
    async (spawn: OwnedProviderProcessSpawnOptions) => {
      launches++;
      runId = spawn.runId;
      const handle = Object.assign(new EventEmitter(), {
        pid: 4242, stdout: null, stderr: null, exitCode: 0, signalCode: null,
        kill: () => { throw new Error("No process signals allowed"); },
      });
      setTimeout(() => handle.emit("close", 0, null), 0);
      return {
        process: handle,
        ownership: { runId, leaderPid: 4242, processGroupId: 4242 },
        cleanup: async () => {
          cleanupCalls++;
          return settled ? { cleaned: true as const, signaled: false } : { cleaned: false as const, reason: "fixture observation unavailable" };
        },
      };
    },
    async () => ({ path: root, close: async () => { rootCloses++; } }),
    "linux",
    readStableArtifact,
    async (limits) => limits,
  );
  const service = new PacketCaptureService(provider);
  try {
    const result = await service.inspect({ path: capture, fields: ["ip.addr"] });
    expect(result.ok).toBe(false);
    if (result.ok) throw new Error("Expected cleanup failure");
    const projected = projectAnalysisError(result.error);
    expect(projected.code).toBe("cleanup_incomplete");
    expect(projected.details?.resources).toEqual([root]);
    expect(JSON.stringify(projected)).not.toContain("pid:4242");
    expect(JSON.stringify(projected)).not.toContain("process-group:4242");
    expect(JSON.stringify(projected)).not.toContain(runId);
    expect(launches).toBe(1);
    expect(cleanupCalls).toBe(2);
    expect(rootCloses).toBe(0);
    let closeError: unknown;
    try { await service.close(); } catch (cause) { closeError = cause; }
    expect(closeError).toMatchObject({ cleanupResources: [root] });
    expect(cleanupCalls).toBe(3);
    expect(rootCloses).toBe(0);
    console.log(JSON.stringify({ code: projected.code, resources: ["workspace"], cleanupCalls, rootCloses, identityPresent: false }));
    settled = true;
    await service.close();
    expect(cleanupCalls).toBe(4);
    expect(rootCloses).toBe(1);
    console.log(JSON.stringify({ retry: "completed", cleanupCalls, rootCloses }));
  } finally {
    settled = true;
    await service.close();
    await rm(base, { recursive: true, force: true });
  }
});

Run the exact source test:

npm run test:focused -- src/process/retainedResourceVerification.test.ts

Inspect the output. The fixture asserts that the service result and a failed close() retry contain only the workspace, despite the launch ownership containing a run ID and synthetic leader/group ID. It then changes the inert cleanup observation to success and verifies that the retained workspace is released.

For the production path, source inspection establishes the corresponding conditional trigger: on Linux, initial process-ownership preparation succeeds, an owned command launches, and its cleanup observation later becomes unavailable and remains unavailable during the provider's immediate retry. This real-launcher scenario was not executed. A process-table failure already present before launch is excluded because the Linux preparation guard rejects it.

Expected behavior

The documented cleanup result identifies the resources whose shutdown or removal could not be verified. The repository also requires preserving identity and uncertainty across boundaries. When the provider retains an unresolved owned group, callers should receive the known process identity alongside the workspace identity. The reporting must preserve uncertainty about whether anything remains alive; it must not turn those coordinates into proof that a process exists or may be signaled.

Actual behavior

The executed source fixture produces a projected cleanup_incomplete result with only the workspace path in details.resources. The projected result contains neither the run ID nor the leader/group coordinates, including in the nested previous error. A failed close() retry reports only the same workspace. Root cleanup is withheld while group cleanup remains unresolved. After the fixture reports successful group cleanup, the retained owner is retried and the root closes once.

{"code":"cleanup_incomplete","resources":["workspace"],"cleanupCalls":3,"rootCloses":0,"identityPresent":false}
{"retry":"completed","cleanupCalls":4,"rootCloses":1}
Test Files 1 passed (1)
Tests 1 passed (1)

The output normalizes the fixture's temporary path to workspace. This is a missing-resource diagnostic, not an observed live-process leak. The sigrok provider uses the same helper and analogous retention guards; its applicability was inspected in source, and that provider was not executed.

Evidence

Expected source: AGENTS.md:19 requires preserving identity and uncertainty across boundaries and retaining failed-cleanup ownership; docs/cli.md:149 says cleanup_incomplete identifies resources whose shutdown or removal could not be verified. docs/migration-6.2.md:118 applies cleanup reporting to one-shot analysis. The original command error already contains its run ID, workspace, PID and group ID at src/process/OwnedCommand.ts:199.
Failure source: The inline executed fixture exercises PacketCaptureService.inspect, TsharkCaptureProvider, runOwnedCommand, the real supervisor, and error projection. src/process/ArtifactAnalysisResources.ts:112 reports only the root on failed release, and :204 builds the final error from directory-only pendingPaths. :151 maps the original command failure without preserving its resource list.
Evidence provenance: observed
Local verification: reproduced
Reproduction completeness: complete

The complete reproduced case is the source reporting failure through the injected inert launcher and cleanup observer. Production reachability and sigrok applicability are separate static evidence; no real-provider runtime reproduction was run.

Public entry-point references are src/server/registerCaptureTools.ts:18, src/cli/captureCommands.ts:61, src/application/captures/PacketCaptureService.ts:37, and src/composition/captures.ts:3. The conditional production case requires successful Linux launch preparation followed by repeated unavailable cleanup observations. Its source references are src/process/ProcessOwnershipObservation.ts:362 and src/process/ProcessOwnership.ts:231 and :322. The analogous sigrok case is at src/signals/SigrokRecordingProvider.ts:322. These production cases were inspected in source only.

The related pwntools cleanup report concerns a separate retirement implementation. It is background only; the contract and contrary behavior above were established independently. Related: #2541 concerns loss of the cleanup reason, and #2542 concerns loss of observed member records; neither covers the launch identities omitted when the cleanup observation contains no member records.

Restoration check

For the same valid service request and inert incomplete cleanup observations, inspect the projected service error and failed close() result. Both should identify the retained group using its known launch identity as well as its workspace, without asserting liveness. When the cleanup observer subsequently reports success, cleanup should retry the retained owner and release the workspace. Also distinguish a workspace-only removal failure after verified process cleanup; that case should not describe an unresolved process. These checks restore the same cleanup-reporting contract without prescribing where or how the identities must be represented.

Runtime or environment

Source commit a2d8b94dd5. The focused source fixture ran with Node 24.18.0, npm 11.16.0 and Vitest 5.0.3. Linux selection and inherited limits were supplied through the provider's supported test dependencies. No released package runtime or real TShark/sigrok process was executed. Current upstream recheck at 3ae826ceba found the same omission by source comparison: command ownership, capture-provider retention, shared resource release and finalization remain unchanged, and the error-projection change affects capability retryability rather than retained resources. The source fixture was not rerun during this recheck; the latest registry release, rea-agents 6.4.0, was not executed.

Triage assessment

Impact level: P2
Assessment status: supported
Impact basis: The failure omits known retained process identity from cleanup diagnostics for callers whose owned-group cleanup remains unresolved. It affects investigation of that failed cleanup while preserving ownership and retries. It does not establish data loss, a live-process leak or failure for every cleanup error.
Workaround status: unverified
Workaround basis: The fixture confirms that later close() can retry and release resources once cleanup succeeds, but no caller-facing mechanism was demonstrated that recovers the missing process identity while cleanup remains incomplete. Actual occurrence rates and real-provider retry success were not measured.

Originally created by @coygeek on GitHub (Oct 13, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/2656 ## Summary A caller of `inspect_packet_capture` can receive `cleanup_incomplete` naming only a private workspace even though the provider still owns an unresolved process group. The command layer has already recorded the run ID, leader PID and process-group ID, but these identities do not survive the provider's cleanup error conversion. This prevents the caller from identifying that retained process group from the reported resources. Ownership and cleanup retry still work. ## Steps to reproduce This is an executed inert source reproduction through the shared service and TShark provider's launcher dependency seam. It starts no process and calls no process-table or signaling operation. It verifies error reporting and retained retries, not actual TShark execution or whether a real process remains alive. 1. In the source checkout at the baseline below, use the repository's pinned toolchain and prepared dependencies. 2. Save the following fixture as `src/process/retainedResourceVerification.test.ts`. ```ts import { EventEmitter } from "node:events"; import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises"; import { join } from "node:path"; import { tmpdir } from "node:os"; import { expect, it } from "vitest"; import { PacketCaptureService } from "../application/captures/PacketCaptureService.js"; import { TsharkCaptureProvider } from "../captures/TsharkCaptureProvider.js"; import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; import { readStableArtifact } from "../artifacts/readStableArtifact.js"; import type { OwnedProviderProcessSpawnOptions } from "./ProviderProcess.js"; it("reports the retained workspace but omits retained launch identity", async () => { const base = await mkdtemp(join(process.env.REA_VERIFICATION_DIR ?? tmpdir(), "retained-fixture-")); const root = join(base, "workspace"); await mkdir(root); const command = join(base, "inert-tool"); const capture = join(base, "recording.pcap"); await writeFile(command, "inert fixture, never executed", { mode: 0o700 }); await writeFile(capture, Buffer.from("d4c3b2a1020004000000000000000000ffff000001000000", "hex")); let cleanupCalls = 0; let rootCloses = 0; let launches = 0; let settled = false; let runId = ""; const provider = new TsharkCaptureProvider( { REA_TSHARK_COMMAND: command, REA_TSHARK_PRLIMIT_COMMAND: command }, async (spawn: OwnedProviderProcessSpawnOptions) => { launches++; runId = spawn.runId; const handle = Object.assign(new EventEmitter(), { pid: 4242, stdout: null, stderr: null, exitCode: 0, signalCode: null, kill: () => { throw new Error("No process signals allowed"); }, }); setTimeout(() => handle.emit("close", 0, null), 0); return { process: handle, ownership: { runId, leaderPid: 4242, processGroupId: 4242 }, cleanup: async () => { cleanupCalls++; return settled ? { cleaned: true as const, signaled: false } : { cleaned: false as const, reason: "fixture observation unavailable" }; }, }; }, async () => ({ path: root, close: async () => { rootCloses++; } }), "linux", readStableArtifact, async (limits) => limits, ); const service = new PacketCaptureService(provider); try { const result = await service.inspect({ path: capture, fields: ["ip.addr"] }); expect(result.ok).toBe(false); if (result.ok) throw new Error("Expected cleanup failure"); const projected = projectAnalysisError(result.error); expect(projected.code).toBe("cleanup_incomplete"); expect(projected.details?.resources).toEqual([root]); expect(JSON.stringify(projected)).not.toContain("pid:4242"); expect(JSON.stringify(projected)).not.toContain("process-group:4242"); expect(JSON.stringify(projected)).not.toContain(runId); expect(launches).toBe(1); expect(cleanupCalls).toBe(2); expect(rootCloses).toBe(0); let closeError: unknown; try { await service.close(); } catch (cause) { closeError = cause; } expect(closeError).toMatchObject({ cleanupResources: [root] }); expect(cleanupCalls).toBe(3); expect(rootCloses).toBe(0); console.log(JSON.stringify({ code: projected.code, resources: ["workspace"], cleanupCalls, rootCloses, identityPresent: false })); settled = true; await service.close(); expect(cleanupCalls).toBe(4); expect(rootCloses).toBe(1); console.log(JSON.stringify({ retry: "completed", cleanupCalls, rootCloses })); } finally { settled = true; await service.close(); await rm(base, { recursive: true, force: true }); } }); ``` 3. Run the exact source test: ```sh npm run test:focused -- src/process/retainedResourceVerification.test.ts ``` 4. Inspect the output. The fixture asserts that the service result and a failed `close()` retry contain only the workspace, despite the launch ownership containing a run ID and synthetic leader/group ID. It then changes the inert cleanup observation to success and verifies that the retained workspace is released. For the production path, source inspection establishes the corresponding conditional trigger: on Linux, initial process-ownership preparation succeeds, an owned command launches, and its cleanup observation later becomes unavailable and remains unavailable during the provider's immediate retry. This real-launcher scenario was not executed. A process-table failure already present before launch is excluded because the Linux preparation guard rejects it. ## Expected behavior The documented cleanup result identifies the resources whose shutdown or removal could not be verified. The repository also requires preserving identity and uncertainty across boundaries. When the provider retains an unresolved owned group, callers should receive the known process identity alongside the workspace identity. The reporting must preserve uncertainty about whether anything remains alive; it must not turn those coordinates into proof that a process exists or may be signaled. ## Actual behavior The executed source fixture produces a projected `cleanup_incomplete` result with only the workspace path in `details.resources`. The projected result contains neither the run ID nor the leader/group coordinates, including in the nested previous error. A failed `close()` retry reports only the same workspace. Root cleanup is withheld while group cleanup remains unresolved. After the fixture reports successful group cleanup, the retained owner is retried and the root closes once. ```text {"code":"cleanup_incomplete","resources":["workspace"],"cleanupCalls":3,"rootCloses":0,"identityPresent":false} {"retry":"completed","cleanupCalls":4,"rootCloses":1} Test Files 1 passed (1) Tests 1 passed (1) ``` The output normalizes the fixture's temporary path to `workspace`. This is a missing-resource diagnostic, not an observed live-process leak. The sigrok provider uses the same helper and analogous retention guards; its applicability was inspected in source, and that provider was not executed. ## Evidence - Expected source: `AGENTS.md:19` requires preserving identity and uncertainty across boundaries and retaining failed-cleanup ownership; `docs/cli.md:149` says `cleanup_incomplete` identifies resources whose shutdown or removal could not be verified. `docs/migration-6.2.md:118` applies cleanup reporting to one-shot analysis. The original command error already contains its run ID, workspace, PID and group ID at `src/process/OwnedCommand.ts:199`. - Failure source: The inline executed fixture exercises `PacketCaptureService.inspect`, `TsharkCaptureProvider`, `runOwnedCommand`, the real supervisor, and error projection. `src/process/ArtifactAnalysisResources.ts:112` reports only the root on failed release, and `:204` builds the final error from directory-only `pendingPaths`. `:151` maps the original command failure without preserving its resource list. - Evidence provenance: observed - Local verification: reproduced - Reproduction completeness: complete The complete reproduced case is the source reporting failure through the injected inert launcher and cleanup observer. Production reachability and sigrok applicability are separate static evidence; no real-provider runtime reproduction was run. Public entry-point references are `src/server/registerCaptureTools.ts:18`, `src/cli/captureCommands.ts:61`, `src/application/captures/PacketCaptureService.ts:37`, and `src/composition/captures.ts:3`. The conditional production case requires successful Linux launch preparation followed by repeated unavailable cleanup observations. Its source references are `src/process/ProcessOwnershipObservation.ts:362` and `src/process/ProcessOwnership.ts:231` and `:322`. The analogous sigrok case is at `src/signals/SigrokRecordingProvider.ts:322`. These production cases were inspected in source only. The related [pwntools cleanup report](https://github.com/morluto/rea/issues/2274) concerns a separate retirement implementation. It is background only; the contract and contrary behavior above were established independently. Related: #2541 concerns loss of the cleanup reason, and #2542 concerns loss of observed member records; neither covers the launch identities omitted when the cleanup observation contains no member records. ## Restoration check For the same valid service request and inert incomplete cleanup observations, inspect the projected service error and failed `close()` result. Both should identify the retained group using its known launch identity as well as its workspace, without asserting liveness. When the cleanup observer subsequently reports success, cleanup should retry the retained owner and release the workspace. Also distinguish a workspace-only removal failure after verified process cleanup; that case should not describe an unresolved process. These checks restore the same cleanup-reporting contract without prescribing where or how the identities must be represented. ## Runtime or environment Source commit a2d8b94dd50dab8d966239dd63ab84718aa8b4b4. The focused source fixture ran with Node 24.18.0, npm 11.16.0 and Vitest 5.0.3. Linux selection and inherited limits were supplied through the provider's supported test dependencies. No released package runtime or real TShark/sigrok process was executed. Current upstream recheck at 3ae826ceba6f25b08bce50bcb51001209bc16b57 found the same omission by source comparison: command ownership, capture-provider retention, shared resource release and finalization remain unchanged, and the error-projection change affects capability retryability rather than retained resources. The source fixture was not rerun during this recheck; the latest registry release, rea-agents 6.4.0, was not executed. ## Triage assessment - Impact level: P2 - Assessment status: supported - Impact basis: The failure omits known retained process identity from cleanup diagnostics for callers whose owned-group cleanup remains unresolved. It affects investigation of that failed cleanup while preserving ownership and retries. It does not establish data loss, a live-process leak or failure for every cleanup error. - Workaround status: unverified - Workaround basis: The fixture confirms that later `close()` can retry and release resources once cleanup succeeds, but no caller-facing mechanism was demonstrated that recovers the missing process identity while cleanup remains incomplete. Actual occurrence rates and real-provider retry success were not measured.

bsakaguchi

2026-10-15 18:29:52 +00:00

closed this issue
added the
area: runtime

bug
labels

bsakaguchi commented

2026-10-15 18:29:53 +00:00

Author

Owner

@morluto commented on GitHub (Oct 14, 2026):

Verified the reported defect is addressed on main by 599a8ee11d.

Root cause and resolution: Retained cleanup projection now includes launch ownership identity alongside workspace and cleanup failure evidence.

Validation: src/process/ArtifactAnalysisResources.test.ts — projects the specific reason and member evidence for retained cleanup checks retained paths plus run ID, leader PID, group ID, typed member failure/errno, and a distinct unverified member after projection.

Evidence: CI run 500e: owner unchanged and assertion passed.

Verification scope: Controlled process-supervisor observations verify reporting; no real process liveness/cleanup failure is claimed.

Current main CI has separate failures under repair; this closure does not claim the complete main workflow is green.

@morluto commented on GitHub (Oct 14, 2026): Verified the reported defect is addressed on main by 599a8ee11de6b1a7fb270e669b88d78b6be982d2. Root cause and resolution: Retained cleanup projection now includes launch ownership identity alongside workspace and cleanup failure evidence. Validation: `src/process/ArtifactAnalysisResources.test.ts` — `projects the specific reason and member evidence for retained cleanup` checks retained paths plus run ID, leader PID, group ID, typed member failure/errno, and a distinct unverified member after projection. Evidence: CI run 500e: owner unchanged and assertion passed. Verification scope: Controlled process-supervisor observations verify reporting; no real process liveness/cleanup failure is claimed. Current main CI has separate failures under repair; this closure does not claim the complete main workflow is green.

bsakaguchi referenced this issue

2026-10-17 21:07:15 +00:00

[PR #957] [MERGED] fix(windows): remove cancelled snapshot outputs #2533

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#957

No description provided.

Rows
Columns