[GH-ISSUE #2541] fix(process): retain the specific reason when analysis cleanup fails #857

New issue

Closed

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

bsakaguchi commented

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

Owner

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

Summary

When analysis finishes with a retained producer that cannot be cleaned up, the caller receives cleanup_incomplete and the retained resource path, but loses the supervisor's specific failure reason. The resulting generic adapter message cannot distinguish why that resource remains open.

Steps to reproduce

Use the source runtime below. Add this test beside the resource module as src/process/cleanup-reason.regression.test.ts and run npm run test:focused -- src/process/cleanup-reason.regression.test.ts.

import { EventEmitter } from "node:events";
import { PassThrough } from "node:stream";
import { expect, it } from "vitest";
import { ProviderAdapterError } from "../domain/providerAdapterError.js";
import { projectAnalysisError } from "../domain/analysisErrorProjection.js";
import { err } from "../domain/result.js";
import { readStableArtifact } from "../artifacts/readStableArtifact.js";
import { ArtifactAnalysisResources } from "./ArtifactAnalysisResources.js";
import { readLinuxResourceLimits } from "./LinuxResourceLimits.js";
import { ProviderProcessSupervisor } from "./ProviderProcess.js";

it("exposes the producer cleanup reason", async () => {
  const producer = Object.assign(new EventEmitter(), {
    stdout: new PassThrough(), stderr: new PassThrough(),
    exitCode: null, signalCode: null, kill: () => true,
  });
  const owner = new ProviderProcessSupervisor({
    process: producer, ownsProcessLifetime: true,
    cleanup: async () => ({ cleaned: false as const,
      reason: "synthetic process-group cleanup did not stop the launcher" }),
  });
  const resources = new ArtifactAnalysisResources("fixture-provider", "fixture-operation",
    readStableArtifact, readLinuxResourceLimits, { message: "busy", details: {} });
  resources.retain({ path: "runtime-root", close: async () => {} }, owner);
  const result = await resources.finish(
    err(new ProviderAdapterError("fixture-provider", "fixture-operation")),
    () => { throw new Error("unexpected success observation"); },
  );
  owner.dispose();
  if (result.ok) throw new Error("expected cleanup failure");
  expect(JSON.stringify(projectAnalysisError(result.error)))
    .toContain("synthetic process-group cleanup did not stop the launcher");
});

This uses the real supervisor and resource finalizer with a simulated producer. It launches no external program. The cleanup callback reports a specific incomplete result immediately, so there is no exit-timing dependency.

Expected behavior

The projected incomplete-cleanup result must retain the reason supplied by the supervisor, together with the retained resource path and the original analysis failure. Callers should not need access to an internal error cause chain to learn why cleanup failed.

Actual behavior

The focused probe failed because the projected result contained diagnostics.reason: "Error: Provider fixture-provider adapter failed during fixture-operation" and no specific launcher failure reason. It still reported cleanup: "incomplete" and the retained resource path. A control with successful cleanup returned the original analysis result unchanged.

Evidence

Expected source: AGENTS.md, Architecture and Cleanup, requires meaningful failure reasons and partial facts to survive typed boundaries; src/process/ArtifactAnalysisResources.ts:115-127 receives and records the specific supervisor reason.
Failure source: src/process/ArtifactAnalysisResources.ts:204-231 replaces the inner cleanup error with a second error whose reason stringifies its generic message; src/domain/analysisErrorProjection.ts:48-76 projects only the outer diagnostics and does not expose its cause.
Evidence provenance: observed
Local verification: reproduced
Reproduction completeness: complete

Supported failed-command paths retain the supervisor before invoking this finalizer in src/captures/TsharkCaptureProvider.ts:349-366 and src/signals/SigrokRecordingProvider.ts:323-340. The probe verifies that shared resource boundary; it does not certify either external decoder. The nearby diagnostic work in issue #1441 concerns the JADX teardown test fixture rather than this finalizer.

Restoration check

Repeat the fixture and verify that the projection contains the supervisor's exact reason, the incomplete cleanup status and the retained resource path. Preserve the original analysis failure. A successful-cleanup control must continue returning the original result without a cleanup error.

Runtime or environment

Source revision 8b181716aca2cb467be499cba226abfb5ae19141, Node v26.10.0, npm 11.19.1, installed lockfile dependencies and Vitest v5.0.3. The focused source test used a simulated producer.

Triage assessment

Impact level: P2
Assessment status: supported
Impact basis: The caller-facing error discards the concrete reason for a retained resource's failed cleanup, removing information needed to diagnose recovery. The probe confirms that cleanup remains reported incomplete and the resource remains identified.
Workaround status: unverified
Workaround basis: Internal callers can inspect the cause chain, but the projected CLI/MCP result does not expose it. A practical recovery diagnostic for those callers was not verified.

Originally created by @coygeek on GitHub (Oct 13, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/2541 ## Summary When analysis finishes with a retained producer that cannot be cleaned up, the caller receives `cleanup_incomplete` and the retained resource path, but loses the supervisor's specific failure reason. The resulting generic adapter message cannot distinguish why that resource remains open. ## Steps to reproduce Use the source runtime below. Add this test beside the resource module as `src/process/cleanup-reason.regression.test.ts` and run `npm run test:focused -- src/process/cleanup-reason.regression.test.ts`. ```ts import { EventEmitter } from "node:events"; import { PassThrough } from "node:stream"; import { expect, it } from "vitest"; import { ProviderAdapterError } from "../domain/providerAdapterError.js"; import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; import { err } from "../domain/result.js"; import { readStableArtifact } from "../artifacts/readStableArtifact.js"; import { ArtifactAnalysisResources } from "./ArtifactAnalysisResources.js"; import { readLinuxResourceLimits } from "./LinuxResourceLimits.js"; import { ProviderProcessSupervisor } from "./ProviderProcess.js"; it("exposes the producer cleanup reason", async () => { const producer = Object.assign(new EventEmitter(), { stdout: new PassThrough(), stderr: new PassThrough(), exitCode: null, signalCode: null, kill: () => true, }); const owner = new ProviderProcessSupervisor({ process: producer, ownsProcessLifetime: true, cleanup: async () => ({ cleaned: false as const, reason: "synthetic process-group cleanup did not stop the launcher" }), }); const resources = new ArtifactAnalysisResources("fixture-provider", "fixture-operation", readStableArtifact, readLinuxResourceLimits, { message: "busy", details: {} }); resources.retain({ path: "runtime-root", close: async () => {} }, owner); const result = await resources.finish( err(new ProviderAdapterError("fixture-provider", "fixture-operation")), () => { throw new Error("unexpected success observation"); }, ); owner.dispose(); if (result.ok) throw new Error("expected cleanup failure"); expect(JSON.stringify(projectAnalysisError(result.error))) .toContain("synthetic process-group cleanup did not stop the launcher"); }); ``` This uses the real supervisor and resource finalizer with a simulated producer. It launches no external program. The cleanup callback reports a specific incomplete result immediately, so there is no exit-timing dependency. ## Expected behavior The projected incomplete-cleanup result must retain the reason supplied by the supervisor, together with the retained resource path and the original analysis failure. Callers should not need access to an internal error cause chain to learn why cleanup failed. ## Actual behavior The focused probe failed because the projected result contained `diagnostics.reason: "Error: Provider fixture-provider adapter failed during fixture-operation"` and no specific launcher failure reason. It still reported `cleanup: "incomplete"` and the retained resource path. A control with successful cleanup returned the original analysis result unchanged. ## Evidence - Expected source: `AGENTS.md`, Architecture and Cleanup, requires meaningful failure reasons and partial facts to survive typed boundaries; `src/process/ArtifactAnalysisResources.ts:115-127` receives and records the specific supervisor reason. - Failure source: `src/process/ArtifactAnalysisResources.ts:204-231` replaces the inner cleanup error with a second error whose reason stringifies its generic message; `src/domain/analysisErrorProjection.ts:48-76` projects only the outer diagnostics and does not expose its cause. - Evidence provenance: observed - Local verification: reproduced - Reproduction completeness: complete Supported failed-command paths retain the supervisor before invoking this finalizer in `src/captures/TsharkCaptureProvider.ts:349-366` and `src/signals/SigrokRecordingProvider.ts:323-340`. The probe verifies that shared resource boundary; it does not certify either external decoder. The nearby diagnostic work in [issue #1441](https://github.com/morluto/rea/issues/1441) concerns the JADX teardown test fixture rather than this finalizer. ## Restoration check Repeat the fixture and verify that the projection contains the supervisor's exact reason, the incomplete cleanup status and the retained resource path. Preserve the original analysis failure. A successful-cleanup control must continue returning the original result without a cleanup error. ## Runtime or environment Source revision `8b181716aca2cb467be499cba226abfb5ae19141`, Node v26.10.0, npm 11.19.1, installed lockfile dependencies and Vitest v5.0.3. The focused source test used a simulated producer. ## Triage assessment - Impact level: P2 - Assessment status: supported - Impact basis: The caller-facing error discards the concrete reason for a retained resource's failed cleanup, removing information needed to diagnose recovery. The probe confirms that cleanup remains reported incomplete and the resource remains identified. - Workaround status: unverified - Workaround basis: Internal callers can inspect the cause chain, but the projected CLI/MCP result does not expose it. A practical recovery diagnostic for those callers was not verified.

bsakaguchi

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

closed this issue
added the
area: runtime

bug
labels

bsakaguchi commented

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

Author

Owner

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

Cleanup projection now retains the supervisor's specific failure reason and member diagnostics with the retained resource identities. The focused regression passes on current main.

Root cause: ArtifactAnalysisResources.release wrapped cleanup and replaced supervisor diagnostics with a generic cause string.

Validation: src/process/ArtifactAnalysisResources.test.ts: 'projects the specific reason and member evidence for retained cleanup' checks cleanup_incomplete, the specific reason, failures, unverified PID, and resource paths. These assertions passed in CI at 500e32399; the reviewed implementation is present on current main. Fix: 599a8ee11de6b1a7fb270e669b88d78b6be982d2.

Verification scope: The implementation preserves previous_error, satisfying that part of the issue body, but this named assertion does not assert previous_error itself. Test uses a simulated producer.

@morluto commented on GitHub (Oct 14, 2026): Cleanup projection now retains the supervisor's specific failure reason and member diagnostics with the retained resource identities. The focused regression passes on current main. Root cause: ArtifactAnalysisResources.release wrapped cleanup and replaced supervisor diagnostics with a generic cause string. Validation: src/process/ArtifactAnalysisResources.test.ts: 'projects the specific reason and member evidence for retained cleanup' checks cleanup_incomplete, the specific reason, failures, unverified PID, and resource paths. These assertions passed in [CI at 500e32399](https://github.com/morluto/rea/actions/runs/38514789942); the reviewed implementation is present on current main. Fix: [599a8ee11de6b1a7fb270e669b88d78b6be982d2](https://github.com/morluto/rea/commit/599a8ee11de6b1a7fb270e669b88d78b6be982d2). Verification scope: The implementation preserves previous_error, satisfying that part of the issue body, but this named assertion does not assert previous_error itself. Test uses a simulated producer.

bsakaguchi referenced this issue

2026-10-16 21:16:51 +00:00

[PR #857] [MERGED] fix(target): decode app Info.plist executables with an XML parser #2294

bsakaguchi referenced this issue

2026-10-16 21:17:11 +00:00

[PR #885] [MERGED] fix(plist): read XML plists containing a __proto__ key #2317