morluto/rea

Fork 0

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

[GH-ISSUE #6080] [Bug] JavaScript recovery reports success after cancellation during final handle closure #3720

New issue

Closed

opened 2026-10-31 00:43:21 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:43:21 +00:00

Owner

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

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

JavaScriptRecoveryService.recover and the real shared MCP Evidence handler report success after the request signal aborts during final closure of the recovery output directory handles. The output tree is already durably committed at this point, but the provider has not finished its awaited finalization. The handler takes the ordinary successful Evidence-recording path.

A deterministic real-filesystem run observed successful provider, service and handler results with the signal already aborted. The handler called the ordinary Evidence writer once. The uninterrupted control succeeded. Committed recovery files survived, and owned producer processes and private workspaces were gone after close.

The handler was invoked directly with an abortable server context; no cancelled SDK client was used, and this does not claim that such a client receives the reply. The CLI has a later completion check that compensates for this successful-service-result case.

Reproduction

In a prepared source checkout with locked dependencies, existing build artifacts and the repository-pinned Node.js, save this test as tests/process-global/javascriptRecoveryFinalization.test.ts and run:

node node_modules/vitest/vitest.mjs run tests/process-global/javascriptRecoveryFinalization.test.ts --maxWorkers=1

This uses the repository recovery producer simulation with real owned Node subprocesses. Only host admission is temporarily presented as Linux x64; the original host descriptors are restored before process launch. A local executable satisfies the limiter-path prerequisite. Real Wakaru and util-linux containment were not exercised. Real report validation, filesystem publication, file hashing and handle closure remain active. The close wrapper aborts only after the real tree reports published: true, then closes the actual handle. The four observed-behavior assertions passed; they describe the defective success, not a repair.

import { dirname, join } from 'node:path';
import { open, readdir } from 'node:fs/promises';
import { afterEach, expect, it, vi } from 'vitest';
import { SafeOutputTree } from '../../src/artifacts/SafeOutputTree.js';
import * as commandModule from '../../src/javascript/recovery/WakaruCommand.js';
import { recoveryFixture, assertRecoveryCleanup } from '../fixtures/javascript-recovery/provider.js';
import { createEvidenceToolHandler } from '../../src/server/contractToolHandler.js';
import { EvidenceMcpServer } from '../../src/server/EvidenceMcpServer.js';
import { ToolResultDelivery } from '../../src/server/toolResult.js';
import { toolContract } from '../../src/contracts/toolContracts.js';
import { silentLogger } from '../../src/logger.js';
import { ok } from '../../src/domain/result.js';
import { STDIO_DEFAULT_MAX_BUFFER_SIZE, type ServerContext } from '@modelcontextprotocol/server';
vi.mock('node:fs/promises', async (load) => {
  const actual = await load<typeof import('node:fs/promises')>();
  return { ...actual, open: vi.fn(actual.open) };
});
afterEach(() => vi.restoreAllMocks());
const originalResolve = commandModule.resolveWakaruCommand;
it.each(['control', 'provider', 'service', 'handler'])(
  'JavaScript cancellation during committed directory-handle close: %s', async mode => {
    const fixture = await recoveryFixture();
    Object.assign(fixture.provider.environment, { REA_JAVASCRIPT_PRLIMIT_COMMAND: join(fixture.root, 'fixture-wakaru') });
    vi.spyOn(commandModule, 'resolveWakaruCommand').mockImplementation(async environment => {
      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(environment); }
      finally { Object.defineProperty(process,'platform',platform); Object.defineProperty(process,'arch',arch); }
    });
    const controller = new AbortController();
    const reason = { phase: 'commit-descriptor-close' };
    const fs = await vi.importActual<typeof import('node:fs/promises')>('node:fs/promises');
    let committing: SafeOutputTree | undefined;
    let observedPublished = false;
    const commit = SafeOutputTree.prototype.commit;
    vi.spyOn(SafeOutputTree.prototype, 'commit').mockImplementation(async function(signal) {
      committing = this;
      try { return await commit.call(this,signal); } finally { committing = undefined; }
    });
    vi.mocked(open).mockImplementation(async (path,...args) => {
      const handle = await fs.open(path,...args);
      if (committing && [fixture.output,dirname(fixture.output)].includes(String(path))) {
        const tree = committing; const close = handle.close.bind(handle);
        handle.close = async () => {
          if (tree.published) {
            observedPublished = true;
            if (mode !== 'control') controller.abort(reason);
          }
          await close();
        };
      }
      return handle;
    });
    let recorded = 0;
    let server: EvidenceMcpServer | undefined;
    try {
      const input = {path:fixture.path,output_directory:fixture.output};
      let successful = false;
      if (mode === 'provider' || mode === 'control') {
        const result = await fixture.provider.recover({...input,extraction_mode:'structural',rewrite_level:'standard'}, {signal:controller.signal});
        successful = result.ok;
      } else if (mode === 'handler') {
        const writer = () => { recorded++; return ok('added' as const); };
        server = new EvidenceMcpServer({name:'recovery-finalization',version:'1'},{capabilities:{}},writer,new ToolResultDelivery(STDIO_DEFAULT_MAX_BUFFER_SIZE));
        const handler = createEvidenceToolHandler(server,(input,options)=>fixture.service.recover(input,options),silentLogger,writer)(toolContract('recover_javascript_sources'));
        const result = await handler(input,{mcpReq:{signal:controller.signal}} as ServerContext);
        successful = result.isError !== true;
        expect(recorded).toBe(1);
      } else {
        const result = await fixture.service.recover(input,{signal:controller.signal});
        successful = result.ok;
      }
      expect(observedPublished).toBe(true); expect(successful).toBe(true);
      expect(controller.signal.aborted).toBe(mode !== 'control');
      await fixture.provider.close();
      await assertRecoveryCleanup(fixture.launches);
      expect((await readdir(fixture.output)).sort()).toEqual(['inputs','manifest.json','modules','reports']);
      console.log(JSON.stringify({case:'javascript',mode,aborted:controller.signal.aborted,success:successful,recorded,published:true,ownedProcessesAndWorkspaceGone:true}));
    } finally { await server?.close(); await fixture.provider.close(); }
  }
);

Expected behavior

After validating the completed provider report and binding its source/engine identity, check cancellation before returning successful Evidence. On a late abort, return typed cancellation with the exact caller reason and the completed Evidence retained as a partial observation. Keep the committed files and their path/digest facts. A malformed report or genuinely earlier failure must retain its output/failure classification.

The lifecycle contract explicitly includes cancellation observed after provider completion. The successful-report regression contract requires the application-consumption check and Evidence retention. The recovery publication guide documents validated output but no earlier publication-as-request-cancellation cutoff. The CLI finalizer already implements the corresponding late-success conversion. Durable publication can remain complete while the caller's request is classified as cancelled.

Impact and scope

P2: a cancelled recovery can be classified and recorded as an ordinary completed MCP operation rather than cancellation with retained partial Evidence. Consumers that track their own cancellation can discard the late success. The CLI's finalizer limits this specific successful-result manifestation. No source corruption, output rollback, target execution or residual producer process was observed. Each deterministic JavaScript case ran once in the independent reduced run and in the combined run. Natural timing frequency, real Wakaru, Linux containment and SDK cancellation/reply delivery 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; locked Vitest 5.0.3. Linux-x64 command admission and Wakaru producer reports were simulated as described above.

Target and artifact context

Synthetic regular UTF-8 JavaScript input and repository-supplied producer reports/modules. Input and recovered application code were never executed.

Interface

MCP

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Observed diagnostics from the production boundaries:

{"case":"javascript","mode":"control","aborted":false,"success":true,"recorded":0,"published":true,"ownedProcessesAndWorkspaceGone":true}
{"case":"javascript","mode":"provider","aborted":true,"success":true,"recorded":0,"published":true,"ownedProcessesAndWorkspaceGone":true}
{"case":"javascript","mode":"service","aborted":true,"success":true,"recorded":0,"published":true,"ownedProcessesAndWorkspaceGone":true}
{"case":"javascript","mode":"handler","aborted":true,"success":true,"recorded":1,"published":true,"ownedProcessesAndWorkspaceGone":true}

recorded counts the supplied ordinary handler Evidence writer. published was observed on the actual output tree before aborting; process-group and private-workspace absence were checked using the repository fixture. These are direct handler/service observations, not wire receipts.

Typed outcome or recovery expected

Return AnalysisCancelledError("recover_javascript_sources", { cause: signal.reason, partialObservation: evidence }) after accepting a valid completed report. Use the existing partial-error recording/delivery path and preserve committed recovery artifacts. Retain existing failure and cleanup precedence.

Analysis or suspected cause

Source-derived: the recovery service awaits the provider, validates its result and identity, then unconditionally returns successful Evidence. It lacks the completion check already centralized for other services in completeProviderEvidence.

The actual finalization interval is SafeOutputTree directory closure, after its final pre-publication signal check and published assignment. WakaruProvider awaits that commit; it later returns its execution. The named recovery registration forwards the signal into the service, and the shared handler records/delivers its result without a further completion check.

This does not imply that the atomic output owner should erase committed files or that every atomic publisher must use the same cutoff. The missing decision is the recovery operation's typed completion after validated result consumption.

Additional context

#5964 concerns already-classified cancellation errors losing their cause at existing checkpoints. Adding those causes does not cover this successful-result branch. #5969/#5970/#4629 concern input-change/read-denial/size classification; #4948 and PRs #4958/#5324 concern host/limiter admission. #4939 is CLI final-close signal forwarding, which this registration already supplies. #3927 and PR #4274 apply the same completion policy in a different service; #6048/#6049 involve different producers. #4276 tracks raw partial recovery policy, not this valid completed Evidence result.

Fetched upstream main f86206535f8ec71014781f9d432a84b01e183e5d; the recovery service, provider, output-tree completion and recovery guide are unchanged. That newer revision was source-compared, 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/6080 ### 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 `JavaScriptRecoveryService.recover` and the real shared MCP Evidence handler report success after the request signal aborts during final closure of the recovery output directory handles. The output tree is already durably committed at this point, but the provider has not finished its awaited finalization. The handler takes the ordinary successful Evidence-recording path. A deterministic real-filesystem run observed successful provider, service and handler results with the signal already aborted. The handler called the ordinary Evidence writer once. The uninterrupted control succeeded. Committed recovery files survived, and owned producer processes and private workspaces were gone after close. The handler was invoked directly with an abortable server context; no cancelled SDK client was used, and this does not claim that such a client receives the reply. The CLI has a later completion check that compensates for this successful-service-result case. ### Reproduction In a prepared source checkout with locked dependencies, existing build artifacts and the repository-pinned Node.js, save this test as `tests/process-global/javascriptRecoveryFinalization.test.ts` and run: ```sh node node_modules/vitest/vitest.mjs run tests/process-global/javascriptRecoveryFinalization.test.ts --maxWorkers=1 ``` This uses the repository recovery producer simulation with real owned Node subprocesses. Only host admission is temporarily presented as Linux x64; the original host descriptors are restored before process launch. A local executable satisfies the limiter-path prerequisite. Real Wakaru and util-linux containment were not exercised. Real report validation, filesystem publication, file hashing and handle closure remain active. The close wrapper aborts only after the real tree reports `published: true`, then closes the actual handle. The four observed-behavior assertions passed; they describe the defective success, not a repair. ```ts import { dirname, join } from 'node:path'; import { open, readdir } from 'node:fs/promises'; import { afterEach, expect, it, vi } from 'vitest'; import { SafeOutputTree } from '../../src/artifacts/SafeOutputTree.js'; import * as commandModule from '../../src/javascript/recovery/WakaruCommand.js'; import { recoveryFixture, assertRecoveryCleanup } from '../fixtures/javascript-recovery/provider.js'; import { createEvidenceToolHandler } from '../../src/server/contractToolHandler.js'; import { EvidenceMcpServer } from '../../src/server/EvidenceMcpServer.js'; import { ToolResultDelivery } from '../../src/server/toolResult.js'; import { toolContract } from '../../src/contracts/toolContracts.js'; import { silentLogger } from '../../src/logger.js'; import { ok } from '../../src/domain/result.js'; import { STDIO_DEFAULT_MAX_BUFFER_SIZE, type ServerContext } from '@modelcontextprotocol/server'; vi.mock('node:fs/promises', async (load) => { const actual = await load<typeof import('node:fs/promises')>(); return { ...actual, open: vi.fn(actual.open) }; }); afterEach(() => vi.restoreAllMocks()); const originalResolve = commandModule.resolveWakaruCommand; it.each(['control', 'provider', 'service', 'handler'])( 'JavaScript cancellation during committed directory-handle close: %s', async mode => { const fixture = await recoveryFixture(); Object.assign(fixture.provider.environment, { REA_JAVASCRIPT_PRLIMIT_COMMAND: join(fixture.root, 'fixture-wakaru') }); vi.spyOn(commandModule, 'resolveWakaruCommand').mockImplementation(async environment => { 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(environment); } finally { Object.defineProperty(process,'platform',platform); Object.defineProperty(process,'arch',arch); } }); const controller = new AbortController(); const reason = { phase: 'commit-descriptor-close' }; const fs = await vi.importActual<typeof import('node:fs/promises')>('node:fs/promises'); let committing: SafeOutputTree | undefined; let observedPublished = false; const commit = SafeOutputTree.prototype.commit; vi.spyOn(SafeOutputTree.prototype, 'commit').mockImplementation(async function(signal) { committing = this; try { return await commit.call(this,signal); } finally { committing = undefined; } }); vi.mocked(open).mockImplementation(async (path,...args) => { const handle = await fs.open(path,...args); if (committing && [fixture.output,dirname(fixture.output)].includes(String(path))) { const tree = committing; const close = handle.close.bind(handle); handle.close = async () => { if (tree.published) { observedPublished = true; if (mode !== 'control') controller.abort(reason); } await close(); }; } return handle; }); let recorded = 0; let server: EvidenceMcpServer | undefined; try { const input = {path:fixture.path,output_directory:fixture.output}; let successful = false; if (mode === 'provider' || mode === 'control') { const result = await fixture.provider.recover({...input,extraction_mode:'structural',rewrite_level:'standard'}, {signal:controller.signal}); successful = result.ok; } else if (mode === 'handler') { const writer = () => { recorded++; return ok('added' as const); }; server = new EvidenceMcpServer({name:'recovery-finalization',version:'1'},{capabilities:{}},writer,new ToolResultDelivery(STDIO_DEFAULT_MAX_BUFFER_SIZE)); const handler = createEvidenceToolHandler(server,(input,options)=>fixture.service.recover(input,options),silentLogger,writer)(toolContract('recover_javascript_sources')); const result = await handler(input,{mcpReq:{signal:controller.signal}} as ServerContext); successful = result.isError !== true; expect(recorded).toBe(1); } else { const result = await fixture.service.recover(input,{signal:controller.signal}); successful = result.ok; } expect(observedPublished).toBe(true); expect(successful).toBe(true); expect(controller.signal.aborted).toBe(mode !== 'control'); await fixture.provider.close(); await assertRecoveryCleanup(fixture.launches); expect((await readdir(fixture.output)).sort()).toEqual(['inputs','manifest.json','modules','reports']); console.log(JSON.stringify({case:'javascript',mode,aborted:controller.signal.aborted,success:successful,recorded,published:true,ownedProcessesAndWorkspaceGone:true})); } finally { await server?.close(); await fixture.provider.close(); } } ); ``` ### Expected behavior After validating the completed provider report and binding its source/engine identity, check cancellation before returning successful Evidence. On a late abort, return typed cancellation with the exact caller reason and the completed Evidence retained as a partial observation. Keep the committed files and their path/digest facts. A malformed report or genuinely earlier failure must retain its output/failure classification. The [lifecycle contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/tool-design.md#L101-L113) explicitly includes cancellation observed after provider completion. The [successful-report regression contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/testing.md#L321-L326) requires the application-consumption check and Evidence retention. The [recovery publication guide](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/javascript-recovery.md#L77-L109) documents validated output but no earlier publication-as-request-cancellation cutoff. [The CLI finalizer](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/cli/finishAnalysisCommand.ts#L72-L78) already implements the corresponding late-success conversion. Durable publication can remain complete while the caller's request is classified as cancelled. ### Impact and scope P2: a cancelled recovery can be classified and recorded as an ordinary completed MCP operation rather than cancellation with retained partial Evidence. Consumers that track their own cancellation can discard the late success. The CLI's finalizer limits this specific successful-result manifestation. No source corruption, output rollback, target execution or residual producer process was observed. Each deterministic JavaScript case ran once in the independent reduced run and in the combined run. Natural timing frequency, real Wakaru, Linux containment and SDK cancellation/reply delivery 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; locked Vitest 5.0.3. Linux-x64 command admission and Wakaru producer reports were simulated as described above. ### Target and artifact context Synthetic regular UTF-8 JavaScript input and repository-supplied producer reports/modules. Input and recovered application code were never executed. ### Interface MCP ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Observed diagnostics from the production boundaries: ```jsonl {"case":"javascript","mode":"control","aborted":false,"success":true,"recorded":0,"published":true,"ownedProcessesAndWorkspaceGone":true} {"case":"javascript","mode":"provider","aborted":true,"success":true,"recorded":0,"published":true,"ownedProcessesAndWorkspaceGone":true} {"case":"javascript","mode":"service","aborted":true,"success":true,"recorded":0,"published":true,"ownedProcessesAndWorkspaceGone":true} {"case":"javascript","mode":"handler","aborted":true,"success":true,"recorded":1,"published":true,"ownedProcessesAndWorkspaceGone":true} ``` `recorded` counts the supplied ordinary handler Evidence writer. `published` was observed on the actual output tree before aborting; process-group and private-workspace absence were checked using the repository fixture. These are direct handler/service observations, not wire receipts. ### Typed outcome or recovery expected Return `AnalysisCancelledError("recover_javascript_sources", { cause: signal.reason, partialObservation: evidence })` after accepting a valid completed report. Use the existing partial-error recording/delivery path and preserve committed recovery artifacts. Retain existing failure and cleanup precedence. ### Analysis or suspected cause Source-derived: [the recovery service](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/javascript/JavaScriptRecoveryService.ts#L80-L121) awaits the provider, validates its result and identity, then unconditionally returns successful Evidence. It lacks the completion check already centralized for other services in [completeProviderEvidence](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/ProviderOperationExecution.ts#L91-L111). The actual finalization interval is [SafeOutputTree directory closure](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/artifacts/SafeOutputTree.ts#L309-L321), after its final pre-publication signal check and `published` assignment. [WakaruProvider](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/javascript/recovery/WakaruProvider.ts#L142-L147) awaits that commit; it later returns its execution. [The named recovery registration](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/server/registerJavaScriptRecoveryTool.ts#L19-L33) forwards the signal into the service, and [the shared handler](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/server/contractToolHandler.ts#L89-L104) records/delivers its result without a further completion check. This does not imply that the atomic output owner should erase committed files or that every atomic publisher must use the same cutoff. The missing decision is the recovery operation's typed completion after validated result consumption. ### Additional context #5964 concerns already-classified cancellation errors losing their cause at existing checkpoints. Adding those causes does not cover this successful-result branch. #5969/#5970/#4629 concern input-change/read-denial/size classification; #4948 and PRs #4958/#5324 concern host/limiter admission. #4939 is CLI final-close signal forwarding, which this registration already supplies. #3927 and PR #4274 apply the same completion policy in a different service; #6048/#6049 involve different producers. #4276 tracks raw partial recovery policy, not this valid completed Evidence result. Fetched upstream main `f86206535f8ec71014781f9d432a84b01e183e5d`; the recovery service, provider, output-tree completion and recovery guide are unchanged. That newer revision was source-compared, not executed. No fix or regression range was established.

bsakaguchi closed this issue

2026-10-31 00:43:21 +00:00

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

No description provided.

Rows
Columns