morluto/rea

Fork 0

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

[GH-ISSUE #5940] [Bug] Pcap retry loses cleanup status and resources after a failed version probe #3610

New issue

Closed

opened 2026-10-31 00:42:22 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-31 00:42:22 +00:00

Owner

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

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

Process capture or runtime observation

Observed behavior

After a failed packet-tool version probe whose cleanup remains unconfirmed, a second MCP inspect_pcap_client call refuses new work with error.code: "execution_failure" and no details.cleanup or details.resources. Its diagnostic still says Previous packet-capture resources remain owned.

This was reproduced through the production MCP server and real owned-process supervision, using a Node child to simulate a Wireshark version probe and a controlled cleanup failure. No Wireshark installation, capture file, or network traffic was used. A matching exit-zero control preserves cleanup_incomplete and three resource labels on the second call. Both cases launch only one child: the defect is the error classification and missing resource attribution while admission remains blocked.

Reproduction

From the source revision below with its locked dependencies installed, save this focused test as tests/boundary/mcp/pcapCleanupDependency.test.ts, then run:

./node_modules/.bin/vitest run tests/boundary/mcp/pcapCleanupDependency.test.ts --maxWorkers=1

The test keeps the production MCP tool/service/provider/command path. Only the external producer and final cleanup observation are controlled. The cleanup wrapper first performs normal owned-process cleanup, then reports injected uncertainty; teardown permits cleanup and awaits closure even if an assertion fails.

import { Client, InMemoryTransport } from "@modelcontextprotocol/client";
import { expect, it } from "vitest";
import { PcapProvider } from "../../../src/pcap/PcapProvider.js";
import { createServer } from "../../../src/server/createServer.js";
import { createTestBinarySession } from "../../fixtures/binarySession.js";
import { spawnOwnedProviderProcess } from "../../../src/process/ProviderProcess.js";
import { cleanupOwnedProcessGroup } from "../../../src/process/ProcessOwnership.js";

it.each([0, 7])("keeps cleanup visible after a version probe exits %i", async (exitCode) => {
  let mayClean = false;
  let launches = 0;
  let cleanupAttempts = 0;
  const provider = new PcapProvider({
    environment: {
      REA_TSHARK_COMMAND: process.execPath,
      REA_CAPINFOS_COMMAND: process.execPath,
    },
    launcher: async (spawn) => {
      launches++;
      const launched = await spawnOwnedProviderProcess({
        ...spawn,
        command: process.execPath,
        arguments: ["-e", `process.stdout.write("TShark (Wireshark) 4.6.6\\n"); process.exitCode = ${exitCode}`],
      });
      const cleanup = launched.cleanup ?? (() => cleanupOwnedProcessGroup(launched.ownership));
      return {
        ...launched,
        cleanup: async () => {
          cleanupAttempts++;
          const result = await cleanup();
          if (!mayClean) throw new Error("injected cleanup uncertainty");
          return result;
        },
      };
    },
  });
  const session = createTestBinarySession(() => {
    throw new Error("No native analysis provider should start");
  });
  const server = createServer({ kind: "session", session }, { pcapAnalysis: provider });
  const client = new Client({ name: "pcap-cleanup-test", version: "0.0.0" });
  const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
  try {
    await server.connect(serverTransport);
    await client.connect(clientTransport);
    const first = await client.callTool({ name: "inspect_pcap_client", arguments: {} });
    const second = await client.callTool({ name: "inspect_pcap_client", arguments: {} });
    const errorFrom = (reply: typeof first) => {
      const text = (reply.content as { type: string; text?: string }[])
        .map((part) => part.text ?? "").join("");
      return JSON.parse(text).error;
    };
    const firstError = errorFrom(first);
    const secondError = errorFrom(second);
    console.log(JSON.stringify({
      exitCode,
      firstCode: firstError.code,
      secondCode: secondError.code,
      secondCleanup: secondError.details?.cleanup ?? null,
      secondResourceCount: secondError.details?.resources?.length ?? null,
      reason: secondError.details?.diagnostics?.reason ?? null,
      launches,
      cleanupAttempts,
    }));
    expect(first.isError).toBe(true);
    expect(second.isError).toBe(true);
    expect(launches).toBe(1);
    expect(cleanupAttempts).toBeGreaterThan(0);
    expect(secondError.code).toBe("cleanup_incomplete");
    expect(secondError.details.cleanup).toBe("incomplete");
    expect(secondError.details.resources.length).toBeGreaterThan(0);
  } finally {
    mayClean = true;
    await client.close();
    await server.close();
    await provider.close();
  }
});

The exit-zero control passes. The exit-seven case fails the assertion expecting cleanup_incomplete from the second public call; it receives execution_failure. The corrected focused run reproduced this mismatch. An earlier attempt read the wrong MCP response field; errors intentionally arrive as JSON text, which the test above parses.

Expected behavior

While the pcap provider still retains an unresolved command owner, a refusal to admit another inspection should return cleanup_incomplete, details.cleanup: "incomplete", and the retained resource labels. No owner-confirmed release occurred between the two requests.

The MCP contract requires cleanup failures to use cleanup_incomplete and identify remaining owned resources. The lifecycle contract requires exact owner confirmation before resolving cleanup uncertainty. The pcap guide requires retained resources and retry on close.

Impact and scope

P2: callers lose the machine-readable cleanup classification and resource identities during this bounded failed-probe/failed-cleanup schedule. The generic cleanup-oriented message survives, and new work remains blocked. This does not establish data loss, a live-process leak, or a security-boundary crossing.

Observed scope is public in-memory MCP delivery on macOS with a simulated version probe and controlled cleanup uncertainty. The source path applies to version-probe failures before any capture snapshot exists. Naturally occurring Wireshark frequency, CLI delivery, other hosts, and real-provider recovery were not tested. No caller-facing workaround for recovering the omitted resource identities was demonstrated.

REA version

rea-agents 6.7.0; source revision 52415975942a4b9c53a817915b313ccc404f280e.

Runtime and platform

macOS 27.0.1, Darwin arm64; Node.js v26.12.0; Vitest 5.0.3. Source tests used the checkout's locked dependencies. No real TShark or capinfos process ran.

Target and artifact context

Synthetic version-probe output, no capture file or target binary. The producer is a real supervised Node child, emitting a Wireshark-shaped version line and exiting either zero or seven. Cleanup uncertainty is injected after normal process cleanup, so this test establishes reporting and retained-owner behavior rather than a live process leak.

Interface

MCP

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

The focused run completed with exit 1: one control passed and one regression assertion failed. These are the test's machine-independent observation lines:

{"exitCode":0,"firstCode":"cleanup_incomplete","secondCode":"cleanup_incomplete","secondCleanup":"incomplete","secondResourceCount":3,"reason":"Previous packet-capture resources remain owned.","launches":1,"cleanupAttempts":1}
{"exitCode":7,"firstCode":"capability_unavailable","secondCode":"execution_failure","secondCleanup":null,"secondResourceCount":null,"reason":"Previous packet-capture resources remain owned.","launches":1,"cleanupAttempts":1}

The failing assertion reports:

Expected: "cleanup_incomplete"
Received: "execution_failure"

The null values above denote missing cleanup/resource fields in the received MCP error. Both calls return isError: true. Teardown restores the cleanup observation and successfully awaits client/server/provider closure.

Typed outcome or recovery expected

Preserve the unresolved owner's current cleanup status and labels in the refusal:

{
  "error": {
    "code": "cleanup_incomplete",
    "details": {
      "provider_id": "pcap",
      "operation": "inspect_pcap_client",
      "cleanup": "incomplete",
      "resources": ["<retained run identity>", "<retained process identity>", "<retained process-group identity>"]
    }
  }
}

Retain ownership for close retry. Resolve the status only when the exact owner confirms release.

Analysis or suspected cause

Source-derived explanation of the executed path:

The command owner invokes retainCleanup for incomplete cleanup, then rethrows an ordinary producer failure with its original cleanupFailure value. For the exit-seven failure, that value remains null.
The pcap command conversion adds error-attached cleanup only when cause.cleanupFailure !== null. The retention callback already ran, but the failed version probe becomes a capability error without a cleanup marker.
PcapProvider.run stores that command with failure: undefined and only fills the failure in its catch when the error is already cleanup-incomplete. The exit-zero control instead reaches the explicit cleanup-error construction after the command returns.
On the next request, pendingCleanupFailure constructs a cleanup error from the retained command's resources, then unconditionally binds a dependency list computed from recorded failures. That list is empty in the failed-probe case.
The binding API resolves cleanup when the pending dependency set is empty. Error projection consequently omits the cleanup classification and resource fields.

Empty binding is explicitly documented as confirmed completion in the domain representation. The established inconsistency is that this pcap caller supplies it while a command remains owned. Preserving exact command cleanup dependencies at their producing boundary would address the lost fact; changing the shared API's empty-set semantics requires checking its other consumers.

Additional context

Read-only upstream comparison against origin/main at ce1f4b1bbaced57529d609f3007a76de583e11c4 found the relevant command retention, dependency binding, and cleanup classification unchanged. An unrelated error-projection change controls repeated captured-output delivery and does not change this cleanup branch.

Related reports checked: #5279 covers late cancellation and opaque abort reasons in pcap; #4578 covers resources retained after a child actually releases; #4628 covers typed-primary restoration after confirmed cleanup; #4866 covers artifact cleanup metadata without a capability; #2656 covers omitted process identities in the separate inspect_packet_capture provider. Those paths differ from a failed pcap client probe producing an empty cleanup dependency list. The broad lifecycle audit #4227 and merged lifecycle PR #4274 do not eliminate this path at the tested revision.

Originally created by @coygeek on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5940 ### Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ### Area Process capture or runtime observation ### Observed behavior After a failed packet-tool version probe whose cleanup remains unconfirmed, a second MCP `inspect_pcap_client` call refuses new work with `error.code: "execution_failure"` and no `details.cleanup` or `details.resources`. Its diagnostic still says `Previous packet-capture resources remain owned.` This was reproduced through the production MCP server and real owned-process supervision, using a Node child to simulate a Wireshark version probe and a controlled cleanup failure. No Wireshark installation, capture file, or network traffic was used. A matching exit-zero control preserves `cleanup_incomplete` and three resource labels on the second call. Both cases launch only one child: the defect is the error classification and missing resource attribution while admission remains blocked. ### Reproduction From the source revision below with its locked dependencies installed, save this focused test as `tests/boundary/mcp/pcapCleanupDependency.test.ts`, then run: ```sh ./node_modules/.bin/vitest run tests/boundary/mcp/pcapCleanupDependency.test.ts --maxWorkers=1 ``` The test keeps the production MCP tool/service/provider/command path. Only the external producer and final cleanup observation are controlled. The cleanup wrapper first performs normal owned-process cleanup, then reports injected uncertainty; teardown permits cleanup and awaits closure even if an assertion fails. ```ts import { Client, InMemoryTransport } from "@modelcontextprotocol/client"; import { expect, it } from "vitest"; import { PcapProvider } from "../../../src/pcap/PcapProvider.js"; import { createServer } from "../../../src/server/createServer.js"; import { createTestBinarySession } from "../../fixtures/binarySession.js"; import { spawnOwnedProviderProcess } from "../../../src/process/ProviderProcess.js"; import { cleanupOwnedProcessGroup } from "../../../src/process/ProcessOwnership.js"; it.each([0, 7])("keeps cleanup visible after a version probe exits %i", async (exitCode) => { let mayClean = false; let launches = 0; let cleanupAttempts = 0; const provider = new PcapProvider({ environment: { REA_TSHARK_COMMAND: process.execPath, REA_CAPINFOS_COMMAND: process.execPath, }, launcher: async (spawn) => { launches++; const launched = await spawnOwnedProviderProcess({ ...spawn, command: process.execPath, arguments: ["-e", `process.stdout.write("TShark (Wireshark) 4.6.6\\n"); process.exitCode = ${exitCode}`], }); const cleanup = launched.cleanup ?? (() => cleanupOwnedProcessGroup(launched.ownership)); return { ...launched, cleanup: async () => { cleanupAttempts++; const result = await cleanup(); if (!mayClean) throw new Error("injected cleanup uncertainty"); return result; }, }; }, }); const session = createTestBinarySession(() => { throw new Error("No native analysis provider should start"); }); const server = createServer({ kind: "session", session }, { pcapAnalysis: provider }); const client = new Client({ name: "pcap-cleanup-test", version: "0.0.0" }); const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); try { await server.connect(serverTransport); await client.connect(clientTransport); const first = await client.callTool({ name: "inspect_pcap_client", arguments: {} }); const second = await client.callTool({ name: "inspect_pcap_client", arguments: {} }); const errorFrom = (reply: typeof first) => { const text = (reply.content as { type: string; text?: string }[]) .map((part) => part.text ?? "").join(""); return JSON.parse(text).error; }; const firstError = errorFrom(first); const secondError = errorFrom(second); console.log(JSON.stringify({ exitCode, firstCode: firstError.code, secondCode: secondError.code, secondCleanup: secondError.details?.cleanup ?? null, secondResourceCount: secondError.details?.resources?.length ?? null, reason: secondError.details?.diagnostics?.reason ?? null, launches, cleanupAttempts, })); expect(first.isError).toBe(true); expect(second.isError).toBe(true); expect(launches).toBe(1); expect(cleanupAttempts).toBeGreaterThan(0); expect(secondError.code).toBe("cleanup_incomplete"); expect(secondError.details.cleanup).toBe("incomplete"); expect(secondError.details.resources.length).toBeGreaterThan(0); } finally { mayClean = true; await client.close(); await server.close(); await provider.close(); } }); ``` The exit-zero control passes. The exit-seven case fails the assertion expecting `cleanup_incomplete` from the second public call; it receives `execution_failure`. The corrected focused run reproduced this mismatch. An earlier attempt read the wrong MCP response field; errors intentionally arrive as JSON text, which the test above parses. ### Expected behavior While the pcap provider still retains an unresolved command owner, a refusal to admit another inspection should return `cleanup_incomplete`, `details.cleanup: "incomplete"`, and the retained resource labels. No owner-confirmed release occurred between the two requests. The [MCP contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/mcp-contracts.md#L670-L676) requires cleanup failures to use `cleanup_incomplete` and identify remaining owned resources. The [lifecycle contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/tool-design.md#L123-L131) requires exact owner confirmation before resolving cleanup uncertainty. The [pcap guide](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/pcap-analysis.md#L102-L105) requires retained resources and retry on close. ### Impact and scope P2: callers lose the machine-readable cleanup classification and resource identities during this bounded failed-probe/failed-cleanup schedule. The generic cleanup-oriented message survives, and new work remains blocked. This does not establish data loss, a live-process leak, or a security-boundary crossing. Observed scope is public in-memory MCP delivery on macOS with a simulated version probe and controlled cleanup uncertainty. The source path applies to version-probe failures before any capture snapshot exists. Naturally occurring Wireshark frequency, CLI delivery, other hosts, and real-provider recovery were not tested. No caller-facing workaround for recovering the omitted resource identities was demonstrated. ### REA version rea-agents 6.7.0; source revision `52415975942a4b9c53a817915b313ccc404f280e`. ### Runtime and platform macOS 27.0.1, Darwin arm64; Node.js v26.12.0; Vitest 5.0.3. Source tests used the checkout's locked dependencies. No real TShark or capinfos process ran. ### Target and artifact context Synthetic version-probe output, no capture file or target binary. The producer is a real supervised Node child, emitting a Wireshark-shaped version line and exiting either zero or seven. Cleanup uncertainty is injected after normal process cleanup, so this test establishes reporting and retained-owner behavior rather than a live process leak. ### Interface MCP ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence The focused run completed with exit 1: one control passed and one regression assertion failed. These are the test's machine-independent observation lines: ```json {"exitCode":0,"firstCode":"cleanup_incomplete","secondCode":"cleanup_incomplete","secondCleanup":"incomplete","secondResourceCount":3,"reason":"Previous packet-capture resources remain owned.","launches":1,"cleanupAttempts":1} {"exitCode":7,"firstCode":"capability_unavailable","secondCode":"execution_failure","secondCleanup":null,"secondResourceCount":null,"reason":"Previous packet-capture resources remain owned.","launches":1,"cleanupAttempts":1} ``` The failing assertion reports: ```text Expected: "cleanup_incomplete" Received: "execution_failure" ``` The `null` values above denote missing cleanup/resource fields in the received MCP error. Both calls return `isError: true`. Teardown restores the cleanup observation and successfully awaits client/server/provider closure. ### Typed outcome or recovery expected Preserve the unresolved owner's current cleanup status and labels in the refusal: ```json { "error": { "code": "cleanup_incomplete", "details": { "provider_id": "pcap", "operation": "inspect_pcap_client", "cleanup": "incomplete", "resources": ["<retained run identity>", "<retained process identity>", "<retained process-group identity>"] } } } ``` Retain ownership for close retry. Resolve the status only when the exact owner confirms release. ### Analysis or suspected cause Source-derived explanation of the executed path: 1. [The command owner](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/process/OwnedCommand.ts#L395-L436) invokes `retainCleanup` for incomplete cleanup, then rethrows an ordinary producer failure with its original `cleanupFailure` value. For the exit-seven failure, that value remains null. 2. [The pcap command conversion](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/pcap/PcapCommand.ts#L144-L187) adds error-attached cleanup only when `cause.cleanupFailure !== null`. The retention callback already ran, but the failed version probe becomes a capability error without a cleanup marker. 3. [PcapProvider.run](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/pcap/PcapProvider.ts#L209-L247) stores that command with `failure: undefined` and only fills the failure in its catch when the error is already cleanup-incomplete. The exit-zero control instead reaches the explicit cleanup-error construction after the command returns. 4. On the next request, [pendingCleanupFailure](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/pcap/PcapProvider.ts#L384-L406) constructs a cleanup error from the retained command's resources, then unconditionally binds a dependency list computed from recorded failures. That list is empty in the failed-probe case. 5. [The binding API](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/domain/analysisErrorBase.ts#L166-L201) resolves cleanup when the pending dependency set is empty. [Error projection](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/domain/analysisErrorProjection.ts#L69-L90) consequently omits the cleanup classification and resource fields. Empty binding is explicitly documented as confirmed completion in the domain representation. The established inconsistency is that this pcap caller supplies it while a command remains owned. Preserving exact command cleanup dependencies at their producing boundary would address the lost fact; changing the shared API's empty-set semantics requires checking its other consumers. ### Additional context Read-only upstream comparison against `origin/main` at `ce1f4b1bbaced57529d609f3007a76de583e11c4` found the relevant command retention, dependency binding, and cleanup classification unchanged. An unrelated error-projection change controls repeated captured-output delivery and does not change this cleanup branch. Related reports checked: #5279 covers late cancellation and opaque abort reasons in pcap; #4578 covers resources retained after a child actually releases; #4628 covers typed-primary restoration after confirmed cleanup; #4866 covers artifact cleanup metadata without a capability; #2656 covers omitted process identities in the separate `inspect_packet_capture` provider. Those paths differ from a failed pcap client probe producing an empty cleanup dependency list. The broad lifecycle audit #4227 and merged lifecycle PR #4274 do not eliminate this path at the tested revision.

bsakaguchi closed this issue

2026-10-31 00:42:22 +00:00

bsakaguchi commented

2026-10-31 00:42:23 +00:00

Author

Owner

@coygeek commented on GitHub (Oct 24, 2026):

Checked the reported fix on main at d87e73f2db.

PcapCommand now prefers cleanup captured by its retain callback when composing failure details, including when the failed command itself has cleanupFailure null. PcapProvider records that cleanup-bearing error, so pending admission binds a real unresolved dependency rather than an empty list. The regression tests exit 0 and exit 7 probes with controlled cleanup uncertainty, checks cleanup_incomplete/resources on the first and second calls, exactly one launch, and resolution of both historical errors only after owner close.

Implementation: PcapCommand.ts, PcapProvider.ts.

The relevant regression checks in pcapProviderCrossPlatform.test.ts passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

@coygeek commented on GitHub (Oct 24, 2026): Checked the reported fix on `main` at d87e73f2db50be72d33a4b3c7199af35518b441c. PcapCommand now prefers cleanup captured by its retain callback when composing failure details, including when the failed command itself has cleanupFailure null. PcapProvider records that cleanup-bearing error, so pending admission binds a real unresolved dependency rather than an empty list. The regression tests exit 0 and exit 7 probes with controlled cleanup uncertainty, checks cleanup_incomplete/resources on the first and second calls, exactly one launch, and resolution of both historical errors only after owner close. Implementation: [PcapCommand.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/pcap/PcapCommand.ts#L184), [PcapProvider.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/pcap/PcapProvider.ts#L355). The relevant regression checks in [pcapProviderCrossPlatform.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/tests/boundary/pcap/pcapProviderCrossPlatform.test.ts) passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

bsakaguchi referenced this issue

2026-10-31 01:09:56 +00:00

[PR #3610] [CLOSED] Update turbo.json #5854

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#3610

No description provided.

Rows
Columns