morluto/rea

Fork 0

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

[GH-ISSUE #5964] [Bug] JavaScript recovery discards the caller's cancellation reason #3631

New issue

Closed

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

bsakaguchi commented

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

Owner

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

Before submitting

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

Area

Evidence, artifacts, or reconstruction

Observed behavior

Executed source-recovery service calls returned AnalysisCancelledError with cause === undefined in three cases: an already-aborted request, cancellation while a simulated recovery process was running, and a launcher rejection immediately after the caller aborted. Each controller was aborted with a distinct object reason. Cancellation classification remained cancelled.

These were in-process service calls, not public transport cancellation tests. An ordinary, non-cancelled control recovered successfully through the real MCP registration and SDK in-memory transport. Active cancellation left no output directory, owned process group, or private recovery workspace.

Reproduction

In a prepared source checkout with locked dependencies and existing build artifacts, place the following test at tests/boundary/javascript-recovery/recoveryErrorClassification.repro.test.ts, then run it with Node.js 24.18.0:

node node_modules/vitest/vitest.mjs run tests/boundary/javascript-recovery/recoveryErrorClassification.repro.test.ts --maxWorkers=1

This uses the repository's simulated producer and real owned Node subprocesses. Wakaru was not installed. The test temporarily simulates Linux x64 only while the real command-admission function runs, restoring the host descriptors before subprocess creation. A local executable substitutes for limiter admission; no util-linux resource containment or real Wakaru behavior is claimed. The normal recovery control succeeds through the same MCP registration and SDK transports. The complete observation run contained seven passing assertions; the relevant subset is below. The assertions record the current incorrect result so a passing test here means the behavior reproduced.

import { Client, InMemoryTransport } from '@modelcontextprotocol/client';
import { access, chmod, readFile } from 'node:fs/promises';
import { join } from 'node:path';
import { expect, it, onTestFinished, vi } from 'vitest';
import * as commandModule from '../../../src/javascript/recovery/WakaruCommand.js';
import { WakaruProvider } from '../../../src/javascript/recovery/WakaruProvider.js';
import { JavaScriptRecoveryService } from '../../../src/application/javascript/JavaScriptRecoveryService.js';
import { projectAnalysisError } from '../../../src/domain/analysisErrorProjection.js';
import { createServer } from '../../../src/server/createServer.js';
import { createTestBinarySession } from '../../fixtures/binarySession.js';
import { recoveryFixture, assertRecoveryCleanup } from '../../fixtures/javascript-recovery/provider.js';

// Only platform admission is simulated; real filesystem and owned Node processes follow.
const originalResolve = commandModule.resolveWakaruCommand;
const setup = async (mode = 'normal', limiter?: string) => {
  const fixture = await recoveryFixture(mode);
  const provider = fixture.provider as WakaruProvider;
  Object.assign(provider.environment, {
    REA_JAVASCRIPT_PRLIMIT_COMMAND: limiter ?? join(fixture.root, 'fixture-wakaru'),
  });
  const spy = vi.spyOn(commandModule, 'resolveWakaruCommand').mockImplementation(async env => {
    const platform = Object.getOwnPropertyDescriptor(process, 'platform')!;
    const arch = Object.getOwnPropertyDescriptor(process, 'arch')!;
    Object.defineProperty(process, 'platform', { ...platform, value: 'linux' });
    Object.defineProperty(process, 'arch', { ...arch, value: 'x64' });
    try { return await originalResolve(env); }
    finally {
      Object.defineProperty(process, 'platform', platform);
      Object.defineProperty(process, 'arch', arch);
    }
  });
  onTestFinished(async () => { spy.mockRestore(); await provider.close(); });
  return { ...fixture, provider };
};
const callMcp = async (fixture: Awaited<ReturnType<typeof setup>>) => {
  const session = createTestBinarySession(() => { throw new Error('No native provider expected'); });
  const server = createServer({ kind: 'session', session }, { javascriptRecovery: fixture.provider });
  const client = new Client({ name: 'recovery-error-test', version: '1' });
  const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
  try {
    await server.connect(serverTransport); await client.connect(clientTransport);
    return await client.callTool({ name: 'recover_javascript_sources', arguments: {
      path: fixture.path, output_directory: fixture.output,
    }});
  } finally { await client.close(); await server.close(); await session.close(); }
};
it('control: normal simulated recovery succeeds through MCP', async () => {
  const fixture = await setup();
  const response = await callMcp(fixture);
  console.log('control', JSON.stringify({ isError: response.isError ?? false }));
  expect(response.isError).not.toBe(true);
  await assertRecoveryCleanup(fixture.launches);
});
it('service admission loses the caller reason', async () => {
  const fixture = await setup();
  const controller = new AbortController(); const reason = { requested: 'stop' };
  controller.abort(reason);
  const result = await fixture.service.recover({ path: fixture.path, output_directory: fixture.output }, { signal: controller.signal });
  if (result.ok) throw new Error('Expected cancellation');
  console.log('service-admission', JSON.stringify({ tag: result.error._tag, causeAbsent: result.error.cause === undefined, exactReason: result.error.cause === reason }));
  expect(projectAnalysisError(result.error).code).toBe('cancelled');
  expect(result.error.cause).toBeUndefined();
});
it('active recovery cancellation loses the caller reason with owned cleanup', async () => {
  const fixture = await setup('hang');
  const controller = new AbortController(); const reason = { requested: 'stop' };
  const pending = fixture.service.recover({ path: fixture.path, output_directory: fixture.output }, { signal: controller.signal });
  await vi.waitFor(async () => {
    const launch = fixture.launches[1];
    expect(launch?.cwd).toBeDefined();
    await access(join(launch!.cwd!, 'recovery-started'));
  }, { timeout: 10000 });
  controller.abort(reason);
  const result = await pending;
  if (result.ok) throw new Error('Expected cancellation');
  console.log('active-cancellation', JSON.stringify({ tag: result.error._tag, causeAbsent: result.error.cause === undefined, exactReason: result.error.cause === reason }));
  expect(projectAnalysisError(result.error).code).toBe('cancelled');
  expect(result.error.cause).toBeUndefined();
  await expect(access(fixture.output)).rejects.toMatchObject({ code: 'ENOENT' });
  await assertRecoveryCleanup(fixture.launches);
});
it('launcher rejection after abort loses caller reason', async () => {
  const fixture = await setup();
  const controller = new AbortController(); const reason = { requested: 'stop' };
  const provider = new WakaruProvider(fixture.provider.environment, async () => {
    controller.abort(reason); throw new Error('Simulated launch rejected after abort');
  });
  const service = new JavaScriptRecoveryService(provider);
  try {
    const result = await service.recover({ path: fixture.path, output_directory: fixture.output }, { signal: controller.signal });
    if (result.ok) throw new Error('Expected cancellation');
    console.log('launcher-cancellation', JSON.stringify({ tag: result.error._tag, causeAbsent: result.error.cause === undefined, exactReason: result.error.cause === reason }));
    expect(result.error.cause).toBeUndefined();
    expect(projectAnalysisError(result.error).code).toBe('cancelled');
  } finally { await service.close(); }
});

Expected behavior

The lifecycle contract requires preserving the exact caller abort reason as the typed cancellation error's cause. Its testing guidance requires reason-identity checks. The recovery operation should preserve that value at service admission and provider checkpoints while retaining its cancellation classification and cleanup semantics.

Impact and scope

P2: a required diagnostic/provenance value disappears from typed errors, preventing downstream in-process consumers from inspecting the originating reason. A caller retaining the original signal can still consult its reason. No incorrect successful result, corruption, or residual process was observed. The shared projection does not serialize the opaque cause, so this does not establish a missing JSON field or changed client-facing message. Natural race frequency and real-provider behavior were not measured.

REA version

rea-agents 6.7.0; source revision 52415975942a4b9c53a817915b313ccc404f280e.

Runtime and platform

macOS 27.0.1 arm64; Node.js v24.18.0 (repository pin), Vitest 5.0.3. An earlier observation run also completed on Node.js v24.22.0. No real Wakaru binary was used.

Target and artifact context

Synthetic regular UTF-8 JavaScript input; repository-supplied Wakaru producer simulation, including its hanging mode. Selected application code was not executed.

Interface

Other

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Observed diagnostics:

{"phase":"service-admission","tag":"AnalysisCancelledError","causeAbsent":true,"exactReason":false}
{"phase":"active-cancellation","tag":"AnalysisCancelledError","causeAbsent":true,"exactReason":false}
{"phase":"launcher-cancellation","tag":"AnalysisCancelledError","causeAbsent":true,"exactReason":false}
{"phase":"control","isError":false}

The active-cancellation check also verified that the output was absent and owned subprocess groups/workspaces were removed.

Typed outcome or recovery expected

Keep AnalysisCancelledError / cancelled and set its cause to the exact caller-owned cancellation reason. This does not require serializing an arbitrary reason in public JSON.

Analysis or suspected cause

The public MCP registration forwards inputs and execution options to the shared recovery service. Source inspection found cause-less constructors in service admission, the shared recovery checkpoint, and launcher rejection. The post-stop checkpoint has the same omission, but that exact timing was not separately executed.

Additional context

Related #5935 concerns the JavaScript artifact reader, #5944 concerns derived MCP operations, and PR #5671 changes reverse/pcap/SQLite admission. Those repairs do not change recovery's local constructors. Upstream main 25e1a24a3ed3671a0148757555f18d16c29d936a was fetched and source-compared; these constructors are unchanged. That newer revision was not executed. No fix or regression range was established.

Originally created by @coygeek on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5964 ### 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 Evidence, artifacts, or reconstruction ### Observed behavior Executed source-recovery service calls returned `AnalysisCancelledError` with `cause === undefined` in three cases: an already-aborted request, cancellation while a simulated recovery process was running, and a launcher rejection immediately after the caller aborted. Each controller was aborted with a distinct object reason. Cancellation classification remained `cancelled`. These were in-process service calls, not public transport cancellation tests. An ordinary, non-cancelled control recovered successfully through the real MCP registration and SDK in-memory transport. Active cancellation left no output directory, owned process group, or private recovery workspace. ### Reproduction In a prepared source checkout with locked dependencies and existing build artifacts, place the following test at `tests/boundary/javascript-recovery/recoveryErrorClassification.repro.test.ts`, then run it with Node.js 24.18.0: ```sh node node_modules/vitest/vitest.mjs run tests/boundary/javascript-recovery/recoveryErrorClassification.repro.test.ts --maxWorkers=1 ``` This uses the repository's simulated producer and real owned Node subprocesses. Wakaru was not installed. The test temporarily simulates Linux x64 only while the real command-admission function runs, restoring the host descriptors before subprocess creation. A local executable substitutes for limiter admission; no util-linux resource containment or real Wakaru behavior is claimed. The normal recovery control succeeds through the same MCP registration and SDK transports. The complete observation run contained seven passing assertions; the relevant subset is below. The assertions record the current incorrect result so a passing test here means the behavior reproduced. ```ts import { Client, InMemoryTransport } from '@modelcontextprotocol/client'; import { access, chmod, readFile } from 'node:fs/promises'; import { join } from 'node:path'; import { expect, it, onTestFinished, vi } from 'vitest'; import * as commandModule from '../../../src/javascript/recovery/WakaruCommand.js'; import { WakaruProvider } from '../../../src/javascript/recovery/WakaruProvider.js'; import { JavaScriptRecoveryService } from '../../../src/application/javascript/JavaScriptRecoveryService.js'; import { projectAnalysisError } from '../../../src/domain/analysisErrorProjection.js'; import { createServer } from '../../../src/server/createServer.js'; import { createTestBinarySession } from '../../fixtures/binarySession.js'; import { recoveryFixture, assertRecoveryCleanup } from '../../fixtures/javascript-recovery/provider.js'; // Only platform admission is simulated; real filesystem and owned Node processes follow. const originalResolve = commandModule.resolveWakaruCommand; const setup = async (mode = 'normal', limiter?: string) => { const fixture = await recoveryFixture(mode); const provider = fixture.provider as WakaruProvider; Object.assign(provider.environment, { REA_JAVASCRIPT_PRLIMIT_COMMAND: limiter ?? join(fixture.root, 'fixture-wakaru'), }); const spy = vi.spyOn(commandModule, 'resolveWakaruCommand').mockImplementation(async env => { const platform = Object.getOwnPropertyDescriptor(process, 'platform')!; const arch = Object.getOwnPropertyDescriptor(process, 'arch')!; Object.defineProperty(process, 'platform', { ...platform, value: 'linux' }); Object.defineProperty(process, 'arch', { ...arch, value: 'x64' }); try { return await originalResolve(env); } finally { Object.defineProperty(process, 'platform', platform); Object.defineProperty(process, 'arch', arch); } }); onTestFinished(async () => { spy.mockRestore(); await provider.close(); }); return { ...fixture, provider }; }; const callMcp = async (fixture: Awaited<ReturnType<typeof setup>>) => { const session = createTestBinarySession(() => { throw new Error('No native provider expected'); }); const server = createServer({ kind: 'session', session }, { javascriptRecovery: fixture.provider }); const client = new Client({ name: 'recovery-error-test', version: '1' }); const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); try { await server.connect(serverTransport); await client.connect(clientTransport); return await client.callTool({ name: 'recover_javascript_sources', arguments: { path: fixture.path, output_directory: fixture.output, }}); } finally { await client.close(); await server.close(); await session.close(); } }; it('control: normal simulated recovery succeeds through MCP', async () => { const fixture = await setup(); const response = await callMcp(fixture); console.log('control', JSON.stringify({ isError: response.isError ?? false })); expect(response.isError).not.toBe(true); await assertRecoveryCleanup(fixture.launches); }); it('service admission loses the caller reason', async () => { const fixture = await setup(); const controller = new AbortController(); const reason = { requested: 'stop' }; controller.abort(reason); const result = await fixture.service.recover({ path: fixture.path, output_directory: fixture.output }, { signal: controller.signal }); if (result.ok) throw new Error('Expected cancellation'); console.log('service-admission', JSON.stringify({ tag: result.error._tag, causeAbsent: result.error.cause === undefined, exactReason: result.error.cause === reason })); expect(projectAnalysisError(result.error).code).toBe('cancelled'); expect(result.error.cause).toBeUndefined(); }); it('active recovery cancellation loses the caller reason with owned cleanup', async () => { const fixture = await setup('hang'); const controller = new AbortController(); const reason = { requested: 'stop' }; const pending = fixture.service.recover({ path: fixture.path, output_directory: fixture.output }, { signal: controller.signal }); await vi.waitFor(async () => { const launch = fixture.launches[1]; expect(launch?.cwd).toBeDefined(); await access(join(launch!.cwd!, 'recovery-started')); }, { timeout: 10000 }); controller.abort(reason); const result = await pending; if (result.ok) throw new Error('Expected cancellation'); console.log('active-cancellation', JSON.stringify({ tag: result.error._tag, causeAbsent: result.error.cause === undefined, exactReason: result.error.cause === reason })); expect(projectAnalysisError(result.error).code).toBe('cancelled'); expect(result.error.cause).toBeUndefined(); await expect(access(fixture.output)).rejects.toMatchObject({ code: 'ENOENT' }); await assertRecoveryCleanup(fixture.launches); }); it('launcher rejection after abort loses caller reason', async () => { const fixture = await setup(); const controller = new AbortController(); const reason = { requested: 'stop' }; const provider = new WakaruProvider(fixture.provider.environment, async () => { controller.abort(reason); throw new Error('Simulated launch rejected after abort'); }); const service = new JavaScriptRecoveryService(provider); try { const result = await service.recover({ path: fixture.path, output_directory: fixture.output }, { signal: controller.signal }); if (result.ok) throw new Error('Expected cancellation'); console.log('launcher-cancellation', JSON.stringify({ tag: result.error._tag, causeAbsent: result.error.cause === undefined, exactReason: result.error.cause === reason })); expect(result.error.cause).toBeUndefined(); expect(projectAnalysisError(result.error).code).toBe('cancelled'); } finally { await service.close(); } }); ``` ### Expected behavior The [lifecycle contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/tool-design.md#L101-L110) requires preserving the exact caller abort reason as the typed cancellation error's cause. Its [testing guidance](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/testing.md#L268-L275) requires reason-identity checks. The recovery operation should preserve that value at service admission and provider checkpoints while retaining its cancellation classification and cleanup semantics. ### Impact and scope P2: a required diagnostic/provenance value disappears from typed errors, preventing downstream in-process consumers from inspecting the originating reason. A caller retaining the original signal can still consult its reason. No incorrect successful result, corruption, or residual process was observed. The shared projection does not serialize the opaque cause, so this does not establish a missing JSON field or changed client-facing message. Natural race frequency and real-provider behavior were not measured. ### REA version rea-agents 6.7.0; source revision `52415975942a4b9c53a817915b313ccc404f280e`. ### Runtime and platform macOS 27.0.1 arm64; Node.js v24.18.0 (repository pin), Vitest 5.0.3. An earlier observation run also completed on Node.js v24.22.0. No real Wakaru binary was used. ### Target and artifact context Synthetic regular UTF-8 JavaScript input; repository-supplied Wakaru producer simulation, including its hanging mode. Selected application code was not executed. ### Interface Other ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Observed diagnostics: ```json {"phase":"service-admission","tag":"AnalysisCancelledError","causeAbsent":true,"exactReason":false} {"phase":"active-cancellation","tag":"AnalysisCancelledError","causeAbsent":true,"exactReason":false} {"phase":"launcher-cancellation","tag":"AnalysisCancelledError","causeAbsent":true,"exactReason":false} {"phase":"control","isError":false} ``` The active-cancellation check also verified that the output was absent and owned subprocess groups/workspaces were removed. ### Typed outcome or recovery expected Keep `AnalysisCancelledError` / `cancelled` and set its cause to the exact caller-owned cancellation reason. This does not require serializing an arbitrary reason in public JSON. ### Analysis or suspected cause The public [MCP registration](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/server/registerJavaScriptRecoveryTool.ts#L19-L33) forwards inputs and execution options to the shared recovery service. Source inspection found cause-less constructors in [service admission](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/javascript/JavaScriptRecoveryService.ts#L30-L36), the [shared recovery checkpoint](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/javascript/recovery/RecoveryFiles.ts#L340-L352), and [launcher rejection](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/javascript/recovery/WakaruCommand.ts#L135-L145). The [post-stop checkpoint](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/javascript/recovery/WakaruCommand.ts#L197-L199) has the same omission, but that exact timing was not separately executed. ### Additional context Related #5935 concerns the JavaScript artifact reader, #5944 concerns derived MCP operations, and PR #5671 changes reverse/pcap/SQLite admission. Those repairs do not change recovery's local constructors. Upstream main `25e1a24a3ed3671a0148757555f18d16c29d936a` was fetched and source-compared; these constructors are unchanged. That newer revision was not executed. No fix or regression range was established.

bsakaguchi closed this issue

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

bsakaguchi commented

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

Author

Owner

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

Checked the reported fix on main at d87e73f2db.

All four cited constructors now preserve signal.reason: service pre-admission, recovery checkpoint, launcher rejection after abort and post-stop cancellation. Tests check exact pre-aborted reason identity (including null/false/object) and launcher-interruption reasons; descriptor-finalization coverage also retains exact reason and completed hashes. The original active Wakaru-simulator probe should still be rerun by the parent; no real Wakaru is claimed.

Implementation: JavaScriptRecoveryService.ts, RecoveryFiles.ts.

The relevant regression checks in JavaScriptRecoveryService.test.ts, WakaruCommand.test.ts, RecoveryFiles.ownership.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. All four cited constructors now preserve signal.reason: service pre-admission, recovery checkpoint, launcher rejection after abort and post-stop cancellation. Tests check exact pre-aborted reason identity (including null/false/object) and launcher-interruption reasons; descriptor-finalization coverage also retains exact reason and completed hashes. The original active Wakaru-simulator probe should still be rerun by the parent; no real Wakaru is claimed. Implementation: [JavaScriptRecoveryService.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/application/javascript/JavaScriptRecoveryService.ts#L38), [RecoveryFiles.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/javascript/recovery/RecoveryFiles.ts#L438). The relevant regression checks in [JavaScriptRecoveryService.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/application/javascript/JavaScriptRecoveryService.test.ts), [WakaruCommand.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/javascript/recovery/WakaruCommand.test.ts), [RecoveryFiles.ownership.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/javascript/recovery/RecoveryFiles.ownership.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:58 +00:00

[PR #3634] [MERGED] feat(artifacts): inspect and extract Godot PCK with retained facts #5859

bsakaguchi referenced this issue

2026-11-01 21:10:57 +00:00

[GH-ISSUE #3796] [Tracker] Open feature requests by area (October 2026) #1471