morluto/rea

Fork 0

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

[GH-ISSUE #6063] [Bug] Managed cleanup errors remain incomplete after their descriptor is released #3703

New issue

Closed

opened 2026-10-31 00:43:13 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

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

Owner

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

Before submitting

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

Area

Other

Observed behavior

Managed inspection errors retain cleanupIncomplete: true and their file path after the exact descriptor owner successfully closes. The first execution failure, a subsequent blocked-read failure, and a failed-close result all keep stale markers.

This also reaches public MCP: a controlled inspect_managed_artifact call returned cleanup_incomplete even though its one-shot final cleanup had already released the descriptor (fd === -1). A subsequent close_binary call succeeded.

Reproduction

With locked dependencies and prepared artifacts, save the following executed subset as tests/boundary/providers/managed/cleanupRecovery.repro.test.ts, then run:

node node_modules/vitest/vitest.mjs run tests/boundary/providers/managed/cleanupRecovery.repro.test.ts --maxWorkers=1

These assertions describe the affected behavior. Files, managed parsing, owner retention, retry, and successful close use production code. Only close refusal and its admission timing are controlled. The MCP case uses the actual server and SDK in-memory transport; its provider-creation spy calls through unchanged.

import { writeFile } from 'node:fs/promises';
import { join } from 'node:path';
import { expect, it, vi } from 'vitest';
import { createTestTempDirectory } from '../../../fixtures/temporaryDirectory.js';
import { parseBinaryTarget } from '../../../../src/application/BinaryTargetResolver.js';
import { ManagedStaticProvider } from '../../../../src/dotnet/ManagedStaticProvider.js';
import { buildManagedPeFixture } from '../../../../src/dotnet/ManagedPe.fixture.js';
import { OwnedFileHandle } from '../../../../src/filesystem/OwnedFileHandle.js';
import { RegularFileCleanupFailure } from '../../../../src/application/RegularFileRead.js';
import { projectAnalysisError } from '../../../../src/domain/analysisErrorProjection.js';
import { AnalysisCancelledError } from '../../../../src/domain/analysisErrorCore.js';
import { createManagedBinarySession } from '../../../../src/composition/binary.js';
import { runManagedProviderExecution } from '../../../../src/application/DirectAnalysis.js';

async function fixture() {
  const directory = await createTestTempDirectory('rea-managed-lifecycle-');
  const path = join(directory, 'assembly.dll');
  await writeFile(path, buildManagedPeFixture());
  return path;
}
function injectClose(deny: () => boolean, abort?: () => void) {
  const original = OwnedFileHandle.prototype.close;
  const owners: OwnedFileHandle[] = [];
  const spy = vi.spyOn(OwnedFileHandle.prototype, 'close').mockImplementation(async function(this: OwnedFileHandle) {
    owners.push(this);
    abort?.();
    if (deny()) throw new Error('controlled close refusal');
    return original.call(this);
  });
  return { owners, spy };
}

it('client retry, failed close, and successful close leave every managed cleanup error stale', async () => {
  const path = await fixture();
  const parsed = await parseBinaryTarget(path);
  if (!parsed.ok) throw parsed.error;
  const client = new ManagedStaticProvider().createClient(parsed.value);
  let deny = true;
  const { owners, spy } = injectClose(() => deny);
  try {
    const first = await client.execute('inspect_managed_artifact', {});
    const retry = await client.execute('inspect_managed_artifact', {});
    const refusedClose = await client.close();
    if (first.ok || retry.ok || refusedClose.ok) throw new Error('Expected close refusal');
    expect(owners.every(owner => owner === owners[0])).toBe(true);
    deny = false;
    expect((await client.close()).ok).toBe(true);
    expect(owners[0]?.handle.fd).toBe(-1);
    for (const error of [first.error, retry.error, refusedClose.error]) {
      expect(error.cleanupIncomplete).toBe(true);
      expect(error.cleanupResources).toEqual([path]);
      expect(projectAnalysisError(error).code).toBe('cleanup_incomplete');
    }
    console.log(JSON.stringify({case:'client-recovery', closeOk:true, fd:owners[0]?.handle.fd, sameOwner:true, staleErrors:3}));
  } finally { deny=false; await client.close(); spy.mockRestore(); }
});

it('MCP projects stale cleanup after one-shot close and close_binary cannot change it', async () => {
  const { Client, InMemoryTransport } = await import('@modelcontextprotocol/client');
  const { createServer } = await import('../../../../src/server/createServer.js');
  const { parseMcpToolError } = await import('../../../fixtures/mcpToolError.js');
  const path = await fixture();
  const session = createManagedBinarySession();
  const server = createServer({kind:'session',session}, {availabilityPolicy:()=>({processCaptureEnabled:false, investigationInputRoots:0,browserObservationEnabled:false,electronObservationEnabled:false})});
  const client = new Client({name:'managed-cleanup-repro',version:'1.0.0'});
  const [ct,st]=InMemoryTransport.createLinkedPair();
  let armed=false;
  let denied=0;
  const originalCreate=ManagedStaticProvider.prototype.createClient;
  const create=vi.spyOn(ManagedStaticProvider.prototype,'createClient').mockImplementation(function(this:ManagedStaticProvider,...args){
    const result=originalCreate.apply(this,args); armed=true; return result;
  });
  const {owners,spy}=injectClose(()=>armed && ++denied <=2);
  try {
    await server.connect(st); await client.connect(ct);
    const result=await client.callTool({name:'inspect_managed_artifact',arguments:{path}});
    const {error}=parseMcpToolError(result);
    expect(error.code).toBe('cleanup_incomplete');
    expect(owners.at(-1)?.handle.fd).toBe(-1);
    const closed=await client.callTool({name:'close_binary',arguments:{}});
    expect(closed.isError).not.toBe(true);
    expect(error.code).toBe('cleanup_incomplete');
    console.log(JSON.stringify({case:'mcp',code:error.code,finalFd:owners.at(-1)?.handle.fd,closeBinaryOk:closed.isError!==true}));
  } finally { armed=false; await client.close(); await server.close(); await session.close(); spy.mockRestore(); create.mockRestore(); }
});

Expected behavior

Bind every managed cleanup error to its exact retained owner. Once that owner confirms release, the error should have cleanupIncomplete: false, an empty resource list, and recover any existing typed primary. Preserve historical diagnostics. The lifecycle contract and MCP contract explicitly require this.

Impact and scope

P2: callers receive false machine-readable cleanup status and a resource list naming an already-closed file. This can misdirect recovery decisions. Physical cleanup and session closure succeeded in these controls; no descriptor leak, permanent admission blockage, target mutation, or downstream automation incident was established.

The final focused run passed six observation/control cases, including provider retry, session close, shared one-shot cleanup, and public MCP. Earlier five-case runs reproduced the same provider/workflow observations on both runtimes. Natural OS close-failure frequency, CLI transport delivery, and other platforms 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), locked Vitest 5.0.3. The five provider/workflow cases were also run on Node.js v26.12.0.

Target and artifact context

Synthetic PE/CLI assembly from buildManagedPeFixture, read from a real regular file. No assembly code or external analysis engine ran.

Interface

MCP

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Observed final pinned-runtime output:

{"case":"client-recovery","closeOk":true,"fd":-1,"sameOwner":true,"staleErrors":3}
{"case":"session-close","closeOk":true,"fd":-1,"code":"cleanup_incomplete"}
{"case":"one-shot","finalFd":-1,"code":"cleanup_incomplete"}
{"case":"mcp","code":"cleanup_incomplete","finalFd":-1,"closeBinaryOk":true}

All three retained client errors still named the selected file after successful real close. The session and shared workflow observations were additional cases in the executed six-case file; their test code is omitted here. The excerpt includes the client and public MCP cases. Public managed tools create isolated one-shot sessions, so close_binary operates on the server session rather than the earlier discarded managed session. The incorrect MCP code was already serialized after that managed session had closed successfully.

Typed outcome or recovery expected

After exact owner release, remove live cleanup blockers from originating and retry errors. Where an earlier typed primary exists, project that primary with historical cleanup diagnostics. Do not claim ordinary close failure never occurred.

Analysis or suspected cause

Source-derived: managedCleanupError constructs cleanup errors without binding a lease, including the ArtifactReaderFailure retry branch. scope retry failures carry cleanupLeases, but the managed translator ignores them. client close releases the scope without updating those errors. The one-shot wrapper returns the earlier error after successful close, so the stale state reaches projection. MCP managed dispatch uses the same workflow.

Additional context

Related #4381 and merged PR #4446 fix hash-stream handle invalidation and preserve genuine cleanup ownership; this report observes successful physical release with stale error metadata. #4928 concerns artifact inventory/extraction translators; #4628 concerns projection after cleanup has resolved; #5294 concerns SQLite owners. None binds these managed errors.

Fetched upstream main 8e3bf56e11bcb56d0e8754fdfa32a7a9f004ca13: the relevant managed translation and lease-release logic remain unchanged. ArtifactResourceScope gained unrelated JavaScript close hooks. Newer main was inspected, not executed. No last-known-good revision was established.

Originally created by @coygeek on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/6063 ### 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 Other ### Observed behavior Managed inspection errors retain `cleanupIncomplete: true` and their file path after the exact descriptor owner successfully closes. The first execution failure, a subsequent blocked-read failure, and a failed-close result all keep stale markers. This also reaches public MCP: a controlled `inspect_managed_artifact` call returned `cleanup_incomplete` even though its one-shot final cleanup had already released the descriptor (`fd === -1`). A subsequent `close_binary` call succeeded. ### Reproduction With locked dependencies and prepared artifacts, save the following executed subset as `tests/boundary/providers/managed/cleanupRecovery.repro.test.ts`, then run: ```sh node node_modules/vitest/vitest.mjs run tests/boundary/providers/managed/cleanupRecovery.repro.test.ts --maxWorkers=1 ``` These assertions describe the affected behavior. Files, managed parsing, owner retention, retry, and successful close use production code. Only close refusal and its admission timing are controlled. The MCP case uses the actual server and SDK in-memory transport; its provider-creation spy calls through unchanged. ```ts import { writeFile } from 'node:fs/promises'; import { join } from 'node:path'; import { expect, it, vi } from 'vitest'; import { createTestTempDirectory } from '../../../fixtures/temporaryDirectory.js'; import { parseBinaryTarget } from '../../../../src/application/BinaryTargetResolver.js'; import { ManagedStaticProvider } from '../../../../src/dotnet/ManagedStaticProvider.js'; import { buildManagedPeFixture } from '../../../../src/dotnet/ManagedPe.fixture.js'; import { OwnedFileHandle } from '../../../../src/filesystem/OwnedFileHandle.js'; import { RegularFileCleanupFailure } from '../../../../src/application/RegularFileRead.js'; import { projectAnalysisError } from '../../../../src/domain/analysisErrorProjection.js'; import { AnalysisCancelledError } from '../../../../src/domain/analysisErrorCore.js'; import { createManagedBinarySession } from '../../../../src/composition/binary.js'; import { runManagedProviderExecution } from '../../../../src/application/DirectAnalysis.js'; async function fixture() { const directory = await createTestTempDirectory('rea-managed-lifecycle-'); const path = join(directory, 'assembly.dll'); await writeFile(path, buildManagedPeFixture()); return path; } function injectClose(deny: () => boolean, abort?: () => void) { const original = OwnedFileHandle.prototype.close; const owners: OwnedFileHandle[] = []; const spy = vi.spyOn(OwnedFileHandle.prototype, 'close').mockImplementation(async function(this: OwnedFileHandle) { owners.push(this); abort?.(); if (deny()) throw new Error('controlled close refusal'); return original.call(this); }); return { owners, spy }; } it('client retry, failed close, and successful close leave every managed cleanup error stale', async () => { const path = await fixture(); const parsed = await parseBinaryTarget(path); if (!parsed.ok) throw parsed.error; const client = new ManagedStaticProvider().createClient(parsed.value); let deny = true; const { owners, spy } = injectClose(() => deny); try { const first = await client.execute('inspect_managed_artifact', {}); const retry = await client.execute('inspect_managed_artifact', {}); const refusedClose = await client.close(); if (first.ok || retry.ok || refusedClose.ok) throw new Error('Expected close refusal'); expect(owners.every(owner => owner === owners[0])).toBe(true); deny = false; expect((await client.close()).ok).toBe(true); expect(owners[0]?.handle.fd).toBe(-1); for (const error of [first.error, retry.error, refusedClose.error]) { expect(error.cleanupIncomplete).toBe(true); expect(error.cleanupResources).toEqual([path]); expect(projectAnalysisError(error).code).toBe('cleanup_incomplete'); } console.log(JSON.stringify({case:'client-recovery', closeOk:true, fd:owners[0]?.handle.fd, sameOwner:true, staleErrors:3})); } finally { deny=false; await client.close(); spy.mockRestore(); } }); it('MCP projects stale cleanup after one-shot close and close_binary cannot change it', async () => { const { Client, InMemoryTransport } = await import('@modelcontextprotocol/client'); const { createServer } = await import('../../../../src/server/createServer.js'); const { parseMcpToolError } = await import('../../../fixtures/mcpToolError.js'); const path = await fixture(); const session = createManagedBinarySession(); const server = createServer({kind:'session',session}, {availabilityPolicy:()=>({processCaptureEnabled:false, investigationInputRoots:0,browserObservationEnabled:false,electronObservationEnabled:false})}); const client = new Client({name:'managed-cleanup-repro',version:'1.0.0'}); const [ct,st]=InMemoryTransport.createLinkedPair(); let armed=false; let denied=0; const originalCreate=ManagedStaticProvider.prototype.createClient; const create=vi.spyOn(ManagedStaticProvider.prototype,'createClient').mockImplementation(function(this:ManagedStaticProvider,...args){ const result=originalCreate.apply(this,args); armed=true; return result; }); const {owners,spy}=injectClose(()=>armed && ++denied <=2); try { await server.connect(st); await client.connect(ct); const result=await client.callTool({name:'inspect_managed_artifact',arguments:{path}}); const {error}=parseMcpToolError(result); expect(error.code).toBe('cleanup_incomplete'); expect(owners.at(-1)?.handle.fd).toBe(-1); const closed=await client.callTool({name:'close_binary',arguments:{}}); expect(closed.isError).not.toBe(true); expect(error.code).toBe('cleanup_incomplete'); console.log(JSON.stringify({case:'mcp',code:error.code,finalFd:owners.at(-1)?.handle.fd,closeBinaryOk:closed.isError!==true})); } finally { armed=false; await client.close(); await server.close(); await session.close(); spy.mockRestore(); create.mockRestore(); } }); ``` ### Expected behavior Bind every managed cleanup error to its exact retained owner. Once that owner confirms release, the error should have `cleanupIncomplete: false`, an empty resource list, and recover any existing typed primary. Preserve historical diagnostics. The [lifecycle contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/tool-design.md#L123-L147) and [MCP contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/mcp-contracts.md#L670-L676) explicitly require this. ### Impact and scope P2: callers receive false machine-readable cleanup status and a resource list naming an already-closed file. This can misdirect recovery decisions. Physical cleanup and session closure succeeded in these controls; no descriptor leak, permanent admission blockage, target mutation, or downstream automation incident was established. The final focused run passed six observation/control cases, including provider retry, session close, shared one-shot cleanup, and public MCP. Earlier five-case runs reproduced the same provider/workflow observations on both runtimes. Natural OS close-failure frequency, CLI transport delivery, and other platforms 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), locked Vitest 5.0.3. The five provider/workflow cases were also run on Node.js v26.12.0. ### Target and artifact context Synthetic PE/CLI assembly from buildManagedPeFixture, read from a real regular file. No assembly code or external analysis engine ran. ### Interface MCP ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Observed final pinned-runtime output: ```json {"case":"client-recovery","closeOk":true,"fd":-1,"sameOwner":true,"staleErrors":3} {"case":"session-close","closeOk":true,"fd":-1,"code":"cleanup_incomplete"} {"case":"one-shot","finalFd":-1,"code":"cleanup_incomplete"} {"case":"mcp","code":"cleanup_incomplete","finalFd":-1,"closeBinaryOk":true} ``` All three retained client errors still named the selected file after successful real close. The session and shared workflow observations were additional cases in the executed six-case file; their test code is omitted here. The excerpt includes the client and public MCP cases. Public managed tools create isolated one-shot sessions, so `close_binary` operates on the server session rather than the earlier discarded managed session. The incorrect MCP code was already serialized after that managed session had closed successfully. ### Typed outcome or recovery expected After exact owner release, remove live cleanup blockers from originating and retry errors. Where an earlier typed primary exists, project that primary with historical cleanup diagnostics. Do not claim ordinary close failure never occurred. ### Analysis or suspected cause Source-derived: [managedCleanupError](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/dotnet/ManagedSnapshot.ts#L144-L178) constructs cleanup errors without binding a lease, including the ArtifactReaderFailure retry branch. [scope retry failures](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/artifacts/ArtifactResourceScope.ts#L69-L84) carry cleanupLeases, but the managed translator ignores them. [client close](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/dotnet/ManagedStaticProvider.ts#L162-L170) releases the scope without updating those errors. The [one-shot wrapper](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/DirectAnalysisCleanup.ts#L12-L32) returns the earlier error after successful close, so the stale state reaches projection. [MCP managed dispatch](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/server/registerManagedTools.ts#L54-L62) uses the same workflow. ### Additional context Related #4381 and merged PR #4446 fix hash-stream handle invalidation and preserve genuine cleanup ownership; this report observes successful physical release with stale error metadata. #4928 concerns artifact inventory/extraction translators; #4628 concerns projection after cleanup has resolved; #5294 concerns SQLite owners. None binds these managed errors. Fetched upstream main `8e3bf56e11bcb56d0e8754fdfa32a7a9f004ca13`: the relevant managed translation and lease-release logic remain unchanged. ArtifactResourceScope gained unrelated JavaScript close hooks. Newer main was inspected, not executed. No last-known-good revision was established.

bsakaguchi closed this issue

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

bsakaguchi commented

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

Author

Owner

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

Checked on main at d87e73f2db50be72d33a4b3c7199af35518b441c: Managed cleanup translation now binds initial and retry errors to retained cleanup leases. The ownership regression checks the original, blocked-read and failed-close errors all clear cleanupIncomplete after the same owner closes. ManagedSnapshot.ts ManagedSnapshot.ts RegularFileRead.ts managedStaticProvider.test.ts

I ran the cited regression file on this revision; all passed as part of the focused 28-file, 603-test run. Provider-facing checks here use controlled fixtures; this does not claim a run with the real analysis engine. Closing as completed on main.

@coygeek commented on GitHub (Oct 24, 2026): Checked on main at `d87e73f2db50be72d33a4b3c7199af35518b441c`: Managed cleanup translation now binds initial and retry errors to retained cleanup leases. The ownership regression checks the original, blocked-read and failed-close errors all clear cleanupIncomplete after the same owner closes. [ManagedSnapshot.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/dotnet/ManagedSnapshot.ts#L200) [ManagedSnapshot.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/dotnet/ManagedSnapshot.ts#L245) [RegularFileRead.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/application/RegularFileRead.ts#L56) [managedStaticProvider.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/tests/boundary/providers/managed/managedStaticProvider.test.ts#L343) I ran the cited regression file on this revision; all passed as part of the focused 28-file, 603-test run. Provider-facing checks here use controlled fixtures; this does not claim a run with the real analysis engine. Closing as completed on main.

bsakaguchi referenced this issue

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

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

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

No description provided.

Rows
Columns