[GH-ISSUE #6215] [Bug] open_binary loses target descriptor cleanup after final close fails #3786

New issue

Closed

opened 2026-10-31 00:44:02 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:44:02 +00:00

Owner

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

Before submitting

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

Area

MCP or agent integration

Observed behavior

With a controlled final-close rejection, open_binary returned target_unavailable and file-read remediation after completing target identity reads. Repeating the request acquired another descriptor. After close was allowed, close_binary and session close reported success without retrying either earlier descriptor; both still accepted stat. The unchanged input then opened successfully.

Reproduction

With the locked dependencies installed, save the following test as tests/process-global/targetDescriptorClose.test.ts and run node node_modules/vitest/vitest.mjs run tests/process-global/targetDescriptorClose.test.ts --maxWorkers=1. Use the runtime stated below and an isolated test home/config/cache. One focused run exercised two faulted MCP requests, cleanup recovery attempts, and one successful control. The test asserts the observed defect, not a repair. Only close rejection on the selected real descriptor is injected; the mock delegates every other filesystem operation to production Node APIs.

import { writeFile } from 'node:fs/promises';
import type { FileHandle } from 'node:fs/promises';
import { join } from 'node:path';
import { Client, InMemoryTransport } from '@modelcontextprotocol/client';
import { expect, it, vi } from 'vitest';
import { createServer } from '../../src/server/createServer.js';
import { createTestBinarySession, createCacheProvider } from '../fixtures/binarySession.js';
import { createTestTempDirectory } from '../fixtures/temporaryDirectory.js';
import { parseMcpToolError } from '../fixtures/mcpToolError.js';

const fault = vi.hoisted(() => ({ path: '', deny: true, attempts: 0,
  handles: [] as FileHandle[], releases: [] as Array<() => Promise<void>> }));
vi.mock('node:fs/promises', async (importOriginal) => {
  const actual = await importOriginal<typeof import('node:fs/promises')>();
  return { ...actual, open: async (...args: Parameters<typeof actual.open>) => {
    const handle = await actual.open(...args);
    if (String(args[0]) === fault.path && fault.deny) {
      const close = handle.close.bind(handle);
      fault.handles.push(handle);
      fault.releases.push(close);
      handle.close = async () => {
        fault.attempts++;
        if (fault.deny) throw Object.assign(new Error('controlled descriptor close failure'), { code: 'EIO' });
        await close();
      };
    }
    return handle;
  }};
});

it('characterizes target admission close failure through registered MCP tools', async () => {
  const root = await createTestTempDirectory('rea-target-close-');
  const path = join(root, 'input.hop');
  await writeFile(path, 'fixture database');
  fault.path = path;
  const session = createTestBinarySession(createCacheProvider([]));
  const server = createServer({ kind: 'session', session });
  const client = new Client({ name: 'target-close-test', version: '1' });
  const [ct, st] = InMemoryTransport.createLinkedPair();
  try {
    await server.connect(st);
    await client.connect(ct);
    for (let n = 1; n <= 2; n++) {
      const result = await client.callTool({ name: 'open_binary', arguments: { path } });
      expect(result.isError).toBe(true);
      const { error } = parseMcpToolError(result);
      expect(error.code).toBe('target_unavailable');
      expect(error.message).toContain('target could not be read (EIO): controlled descriptor close failure');
      console.log(JSON.stringify({ attempt: n, error }));
    }
    expect(fault.attempts).toBe(2);
    expect(fault.handles).toHaveLength(2);
    fault.deny = false;
    const closed = await client.callTool({ name: 'close_binary', arguments: {} });
    expect(closed.isError).not.toBe(true);
    expect((await session.close()).ok).toBe(true);
    expect(fault.attempts).toBe(2);
    for (const handle of fault.handles) expect((await handle.stat()).isFile()).toBe(true);
    const control = await client.callTool({ name: 'open_binary', arguments: { path } });
    expect(control.isError).not.toBe(true);
    console.log(JSON.stringify({ closeBinaryOk: closed.isError !== true,
      closeAttemptsAfterRecovery: fault.attempts, liveLostDescriptors: fault.handles.length,
      controlOk: control.isError !== true }));
  } finally {
    fault.deny = false;
    await client.close();
    await server.close();
    await session.close();
    for (const close of fault.releases) await close();
    expect(fault.handles.every(h => h.fd === -1)).toBe(true);
  }
});

Expected behavior

Keep the exact failed descriptor owner and the completed admission observations, report typed incomplete cleanup, and retry unresolved cleanup before admitting replacement work. Session cleanup should close the retained handle once close is permitted and resolve its originating cleanup marker. The lifecycle contract requires these ownership and outcome guarantees; the lifecycle regression guide explicitly calls for injected final-close failures on real descriptor-backed reads.

Impact and scope

P2: misleading target-read diagnosis, lost cleanup ownership and two live descriptors under the controlled fault. One focused run reproduced the behavior on two faulted requests; the successful control restored target access but left the earlier handles live. The test released both itself. This models a rejection before native release; it does not establish natural close-failure incidence, native errno-specific descriptor usability, or descriptor exhaustion. No target code executed or target bytes changed. Public recovery of the original handles was not observed; process restart was not tested.

REA version

Source commit c465ff5a81 (main)

Runtime and platform

Node.js v24.22.0; macOS 27.0.1; arm64; locked Vitest 5.0.3. The repository pins Node 24.18.0, which was not exercised. No external provider engine.

Target and artifact context

Inert regular file named input.hop, selected through the existing analysis-database admission route; fixture provider for the successful control.

Interface

MCP

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Observed through actual registered MCP tools over the SDK in-memory transport:

faulted requests: 2
error.code: target_unavailable
error.retryable: false
error.details.reason: target could not be read (EIO): controlled descriptor close failure
close_binary: success
session.close().ok: true
close attempts after allowing cleanup: 2 (unchanged)
earlier descriptors still accepting stat: 2
unchanged-input control open_binary: success
final test-owned release: both fd values became -1

The production filesystem reads and target/session logic ran. Only the selected handle's close was denied; this was not a spontaneous OS close failure. CLI transport, earlier read failures and cancellation combinations were not executed.

Typed outcome or recovery expected

Attribute incomplete cleanup to the retained target-admission descriptor, preserve completed identity facts, and make session cleanup retry the exact owner. Do not require the entire open to return unqualified success while release is unresolved.

Analysis or suspected cause

Source-derived: the database target branch constructs identity after hashing. The raw finalizer and surrounding catch replace that return when close rejects, and the filesystem classifier constructs a target-read error. The shared session opening path propagates that failure before provider routing, without adopting a cleanup owner.

Additional context

Read related #4458 (stable artifact reader), #5184 (ASAR authorization), #6058 (directory enumeration), #6063 (managed cleanup markers) and #6065 (managed final-close cancellation); none owns this target-admission finalizer. Fetched main ce5bc8cb01 has no changes to the resolver or its session-open caller since the tested baseline. Newer main was inspected, not executed. No regression range was established.

Originally created by @coygeek on GitHub (Oct 22, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/6215 ### 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 MCP or agent integration ### Observed behavior With a controlled final-close rejection, `open_binary` returned `target_unavailable` and file-read remediation after completing target identity reads. Repeating the request acquired another descriptor. After close was allowed, `close_binary` and session close reported success without retrying either earlier descriptor; both still accepted stat. The unchanged input then opened successfully. ### Reproduction With the locked dependencies installed, save the following test as `tests/process-global/targetDescriptorClose.test.ts` and run `node node_modules/vitest/vitest.mjs run tests/process-global/targetDescriptorClose.test.ts --maxWorkers=1`. Use the runtime stated below and an isolated test home/config/cache. One focused run exercised two faulted MCP requests, cleanup recovery attempts, and one successful control. The test asserts the observed defect, not a repair. Only close rejection on the selected real descriptor is injected; the mock delegates every other filesystem operation to production Node APIs. ```ts import { writeFile } from 'node:fs/promises'; import type { FileHandle } from 'node:fs/promises'; import { join } from 'node:path'; import { Client, InMemoryTransport } from '@modelcontextprotocol/client'; import { expect, it, vi } from 'vitest'; import { createServer } from '../../src/server/createServer.js'; import { createTestBinarySession, createCacheProvider } from '../fixtures/binarySession.js'; import { createTestTempDirectory } from '../fixtures/temporaryDirectory.js'; import { parseMcpToolError } from '../fixtures/mcpToolError.js'; const fault = vi.hoisted(() => ({ path: '', deny: true, attempts: 0, handles: [] as FileHandle[], releases: [] as Array<() => Promise<void>> })); vi.mock('node:fs/promises', async (importOriginal) => { const actual = await importOriginal<typeof import('node:fs/promises')>(); return { ...actual, open: async (...args: Parameters<typeof actual.open>) => { const handle = await actual.open(...args); if (String(args[0]) === fault.path && fault.deny) { const close = handle.close.bind(handle); fault.handles.push(handle); fault.releases.push(close); handle.close = async () => { fault.attempts++; if (fault.deny) throw Object.assign(new Error('controlled descriptor close failure'), { code: 'EIO' }); await close(); }; } return handle; }}; }); it('characterizes target admission close failure through registered MCP tools', async () => { const root = await createTestTempDirectory('rea-target-close-'); const path = join(root, 'input.hop'); await writeFile(path, 'fixture database'); fault.path = path; const session = createTestBinarySession(createCacheProvider([])); const server = createServer({ kind: 'session', session }); const client = new Client({ name: 'target-close-test', version: '1' }); const [ct, st] = InMemoryTransport.createLinkedPair(); try { await server.connect(st); await client.connect(ct); for (let n = 1; n <= 2; n++) { const result = await client.callTool({ name: 'open_binary', arguments: { path } }); expect(result.isError).toBe(true); const { error } = parseMcpToolError(result); expect(error.code).toBe('target_unavailable'); expect(error.message).toContain('target could not be read (EIO): controlled descriptor close failure'); console.log(JSON.stringify({ attempt: n, error })); } expect(fault.attempts).toBe(2); expect(fault.handles).toHaveLength(2); fault.deny = false; const closed = await client.callTool({ name: 'close_binary', arguments: {} }); expect(closed.isError).not.toBe(true); expect((await session.close()).ok).toBe(true); expect(fault.attempts).toBe(2); for (const handle of fault.handles) expect((await handle.stat()).isFile()).toBe(true); const control = await client.callTool({ name: 'open_binary', arguments: { path } }); expect(control.isError).not.toBe(true); console.log(JSON.stringify({ closeBinaryOk: closed.isError !== true, closeAttemptsAfterRecovery: fault.attempts, liveLostDescriptors: fault.handles.length, controlOk: control.isError !== true })); } finally { fault.deny = false; await client.close(); await server.close(); await session.close(); for (const close of fault.releases) await close(); expect(fault.handles.every(h => h.fd === -1)).toBe(true); } }); ``` ### Expected behavior Keep the exact failed descriptor owner and the completed admission observations, report typed incomplete cleanup, and retry unresolved cleanup before admitting replacement work. Session cleanup should close the retained handle once close is permitted and resolve its originating cleanup marker. The [lifecycle contract](https://github.com/morluto/rea/blob/c465ff5a814b8e1a386fc547cbcd94df5c35f086/docs/tool-design.md#L132-L147) requires these ownership and outcome guarantees; [the lifecycle regression guide](https://github.com/morluto/rea/blob/c465ff5a814b8e1a386fc547cbcd94df5c35f086/docs/testing.md#L372-L395) explicitly calls for injected final-close failures on real descriptor-backed reads. ### Impact and scope P2: misleading target-read diagnosis, lost cleanup ownership and two live descriptors under the controlled fault. One focused run reproduced the behavior on two faulted requests; the successful control restored target access but left the earlier handles live. The test released both itself. This models a rejection before native release; it does not establish natural close-failure incidence, native errno-specific descriptor usability, or descriptor exhaustion. No target code executed or target bytes changed. Public recovery of the original handles was not observed; process restart was not tested. ### REA version Source commit c465ff5a814b8e1a386fc547cbcd94df5c35f086 (main) ### Runtime and platform Node.js v24.22.0; macOS 27.0.1; arm64; locked Vitest 5.0.3. The repository pins Node 24.18.0, which was not exercised. No external provider engine. ### Target and artifact context Inert regular file named input.hop, selected through the existing analysis-database admission route; fixture provider for the successful control. ### Interface MCP ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Observed through actual registered MCP tools over the SDK in-memory transport: ```text faulted requests: 2 error.code: target_unavailable error.retryable: false error.details.reason: target could not be read (EIO): controlled descriptor close failure close_binary: success session.close().ok: true close attempts after allowing cleanup: 2 (unchanged) earlier descriptors still accepting stat: 2 unchanged-input control open_binary: success final test-owned release: both fd values became -1 ``` The production filesystem reads and target/session logic ran. Only the selected handle's close was denied; this was not a spontaneous OS close failure. CLI transport, earlier read failures and cancellation combinations were not executed. ### Typed outcome or recovery expected Attribute incomplete cleanup to the retained target-admission descriptor, preserve completed identity facts, and make session cleanup retry the exact owner. Do not require the entire open to return unqualified success while release is unresolved. ### Analysis or suspected cause Source-derived: [the database target branch](https://github.com/morluto/rea/blob/c465ff5a814b8e1a386fc547cbcd94df5c35f086/src/application/BinaryTargetResolver.ts#L198-L208) constructs identity after hashing. [The raw finalizer and surrounding catch](https://github.com/morluto/rea/blob/c465ff5a814b8e1a386fc547cbcd94df5c35f086/src/application/BinaryTargetResolver.ts#L258-L278) replace that return when close rejects, and the filesystem classifier constructs a target-read error. [The shared session opening path](https://github.com/morluto/rea/blob/c465ff5a814b8e1a386fc547cbcd94df5c35f086/src/application/binary/BinarySessionOpen.ts#L85-L100) propagates that failure before provider routing, without adopting a cleanup owner. ### Additional context Read related #4458 (stable artifact reader), #5184 (ASAR authorization), #6058 (directory enumeration), #6063 (managed cleanup markers) and #6065 (managed final-close cancellation); none owns this target-admission finalizer. Fetched main ce5bc8cb0149c230e571126e2e7d9f43c86a823d has no changes to the resolver or its session-open caller since the tested baseline. Newer main was inspected, not executed. No regression range was established.

bsakaguchi closed this issue

2026-10-31 00:44:02 +00:00

bsakaguchi referenced this issue

2026-10-31 01:10:16 +00:00

[PR #3786] [CLOSED] Codex/windows runtime creation red green 20261015 #5898