morluto/rea

Fork 0

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

[GH-ISSUE #5083] [Bug] Cancelling Inspector observation during final close loses unresolved transport cleanup #3120

New issue

Closed

opened 2026-10-31 00:36:51 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-31 00:36:51 +00:00

Owner

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

Before submitting

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

Area

Process capture or runtime observation

Observed behavior

Executed mocked-close production-boundary reproduction: observeJavaScriptRuntime(new V8InspectorProvider(), input, {signal}) returns typed cancellation and retained partial Evidence when aborted during final transport close, but reports cleanupIncomplete: false and no cleanup resources while the close is still pending. Rejecting that close afterward does not update the error. The non-aborted control returns cleanup_incomplete with browser_transport for the same injected close failure.

Discovery, authorization, protocol setup, capture, and Evidence construction use production code against an authored loopback HTTP/WebSocket fixture. Only the final CdpConnection.close() promise is controlled. No actual Node/Electron target or natural socket failure was exercised.

Reproduction

In a disposable source checkout at the revision below with locked dependencies installed, save the following as $WORK/probe.mjs, where WORK=$(mktemp -d). Run from the checkout root:

mkdir -p "$WORK/home" "$WORK/tmp"
env -i PATH="$PATH" HOME="$WORK/home" TMPDIR="$WORK/tmp" node --import tsx "$WORK/probe.mjs"

The executed probe uses a close-start barrier, then compares rejection without abort against abort before rejection. It restores the real close and awaits fixture teardown:

import { createServer } from 'node:http';
import assert from 'node:assert/strict';
import { createRequire } from 'node:module';
import { resolve } from 'node:path';
import { pathToFileURL } from 'node:url';
const require = createRequire(resolve('package.json'));
const { WebSocketServer } = require('ws');
const { CdpConnection } = await import(pathToFileURL(resolve('src/browser/CdpConnection.ts')).href);
const { V8InspectorProvider } = await import(pathToFileURL(resolve('src/inspector/V8InspectorProvider.ts')).href);
const { observeJavaScriptRuntime } = await import(pathToFileURL(resolve('src/application/javascript/JavaScriptRuntimeObservationService.ts')).href);
const { observeJavaScriptRuntimeInputSchema } = await import(pathToFileURL(resolve('src/domain/javascript/javascriptRuntimeObservation.ts')).href);
const { projectAnalysisError } = await import(pathToFileURL(resolve('src/domain/analysisErrorProjection.ts')).href);

for (const abort of [false, true]) {
  let endpoint = '';
  const server = createServer((req, res) => {
    res.setHeader('content-type', 'application/json');
    if (req.url === '/json/version') res.end(JSON.stringify({Browser: 'Node.js/v24.18.0', 'Protocol-Version': '1.3', 'V8-Version': 'fixture'}));
    else if (req.url === '/json/list') res.end(JSON.stringify([{id: 'fixture', type: 'node', url: `${endpoint}/app.js`, attached: false, webSocketDebuggerUrl: endpoint.replace('http:', 'ws:') + '/fixture'}]));
    else res.writeHead(404).end();
  });
  const sockets = new WebSocketServer({server});
  sockets.on('connection', socket => socket.on('message', raw => {
    const {id} = JSON.parse(raw.toString());
    socket.send(JSON.stringify({id, result: {}}));
  }));
  await new Promise(resolve => server.listen(0, '127.0.0.1', resolve));
  const addr = server.address();
  if (!addr || typeof addr === 'string') throw new Error('no port');
  endpoint = `http://127.0.0.1:${addr.port}`;
  const started = Promise.withResolvers();
  const close = Promise.withResolvers();
  const originalClose = CdpConnection.prototype.close;
  let connection;
  let settled = false;
  CdpConnection.prototype.close = function() {
    connection = this;
    started.resolve();
    return close.promise;
  };
  try {
    const controller = new AbortController();
    const input = observeJavaScriptRuntimeInputSchema.parse({inspector_endpoint: endpoint, target_id: 'fixture', observation_ms: 0});
    const observing = observeJavaScriptRuntime(new V8InspectorProvider(), input, {signal: controller.signal});
    await started.promise;
    if (abort) controller.abort(new Error('fixture cancellation'));
    else { settled = true; close.reject(new Error('fixture close failed')); }
    const result = await observing;
    assert.equal(result.ok, false);
    if (result.ok) throw new Error('expected error');
    const error = result.error;
    const before = {abort, closeSettled: settled, code: projectAnalysisError(error).code, cleanupIncomplete: error.cleanupIncomplete, resources: error.cleanupResources, partialEvidence: error.partialObservation !== undefined};
    console.log(JSON.stringify(before));
    assert.equal(error.cleanupIncomplete, !abort);
    assert.deepEqual(error.cleanupResources, abort ? [] : ['browser_transport']);
    assert.ok(error.partialObservation);
    if (abort) {
      settled = true;
      close.reject(new Error('fixture close failed'));
      await new Promise(resolve => setImmediate(resolve));
      assert.equal(error.cleanupIncomplete, false);
      console.log(JSON.stringify({afterLateRejection: true, code: projectAnalysisError(error).code, cleanupIncomplete: error.cleanupIncomplete, resources: error.cleanupResources}));
    }
  } finally {
    close.resolve();
    CdpConnection.prototype.close = originalClose;
    if (connection) await originalClose.call(connection);
    for (const client of sockets.clients) client.terminate();
    await new Promise(resolve => sockets.close(() => resolve()));
    await new Promise(resolve => server.close(() => resolve()));
  }
}

Expected behavior

Caller cancellation may stop waiting promptly, but it must not imply confirmed resource release. Preserve unresolved transport cleanup and its owner/recovery information alongside cancellation and completed Evidence, or retain and surface the eventual close outcome. The lifecycle contract requires unresolved cleanup to remain attributed to its owner and one-shot cleanup to preserve completed results and remaining resources.

Impact and scope

The controlled boundary run loses a genuine injected cleanup failure and initially omits a pending transport. Callers cannot distinguish that state from cleanup completed successfully using the returned cleanup status. Cancellation and partial Evidence themselves remain correct.

NOT_ENOUGH_INFO: frequency with real Inspector peers, persistent socket leakage, and native close-failure conditions. No target mutation or data loss was observed. Production close has its own graceful and forced-close bounds; this report does not claim cancellation prevents that background attempt.

REA version

Package metadata 6.7.0; tested source main 3537e58c1ab29309f06eabf50bc52adba9817a0c. The relevant provider, connection, and application-service source files have no diff from rea-agents-6.7.0, but the published release runtime was not tested.

Runtime and platform

Node.js v26.12.0; Darwin 27.0.0; arm64. Source execution through locked tsx; focused Vitest 5.0.3 control also passed both characterization cases. The repository-pinned Node 24.18.0 runtime was not exercised. Fixture-reported Inspector runtime metadata is synthetic.

Target and artifact context

Authored loopback HTTP/WebSocket Inspector protocol fixture with one node target and an HTTP script location; zero-duration observation. No real provider app was launched or attached.

Interface

Other

The reproduction exercises the shared application workflow used by the public Inspector tool. Source tracing confirms the MCP registration forwards its signal to this workflow. Actual MCP-client cancellation/result delivery was not tested.

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Executed standalone output:

{"abort":false,"closeSettled":true,"code":"cleanup_incomplete","cleanupIncomplete":true,"resources":["browser_transport"],"partialEvidence":true}
{"abort":true,"closeSettled":false,"code":"cancelled","cleanupIncomplete":false,"resources":[],"partialEvidence":true}
{"afterLateRejection":true,"code":"cancelled","cleanupIncomplete":false,"resources":[]}

This establishes loss of the controlled close outcome through the application boundary, not a real socket leak.

Typed outcome or recovery expected

Retain typed cancellation and partial Evidence, together with truthful unresolved browser_transport cleanup and recovery ownership until release is confirmed. A later close failure must remain observable rather than being consumed without updating the result or its owner.

Analysis or suspected cause

The provider memoizes the close helper's promise. The helper resolves on abort and suppresses later close rejection, so the final cleanup classification sees no failure. The application correctly classifies late cancellation, but has no retained close outcome to add.

SOURCE-ONLY supporting reachability: the real transport close can reject when closure remains unconfirmed. Its graceful-close timer allows one second before termination, followed if needed by another one-second confirmation timer. Neither native failure path was faulted in this reproduction.

Additional context

Open and closed issue bodies were searched for Inspector, provider/helper identifiers, transport cleanup, and cancellation/close combinations. #4227 is the umbrella lifecycle audit. #4261 addressed retained Evidence and validation after provider completion, #3553 addressed setup deadlines, and #4869 addressed browser-scenario cleanup markers after successful recovery. Their triggers and closure conditions differ from this Inspector final-close abort race.

Originally created by @coygeek on GitHub (Oct 18, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5083 ## Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ## Area Process capture or runtime observation ## Observed behavior Executed mocked-close production-boundary reproduction: `observeJavaScriptRuntime(new V8InspectorProvider(), input, {signal})` returns typed cancellation and retained partial Evidence when aborted during final transport close, but reports `cleanupIncomplete: false` and no cleanup resources while the close is still pending. Rejecting that close afterward does not update the error. The non-aborted control returns `cleanup_incomplete` with `browser_transport` for the same injected close failure. Discovery, authorization, protocol setup, capture, and Evidence construction use production code against an authored loopback HTTP/WebSocket fixture. Only the final `CdpConnection.close()` promise is controlled. No actual Node/Electron target or natural socket failure was exercised. ## Reproduction In a disposable source checkout at the revision below with locked dependencies installed, save the following as `$WORK/probe.mjs`, where `WORK=$(mktemp -d)`. Run from the checkout root: ```sh mkdir -p "$WORK/home" "$WORK/tmp" env -i PATH="$PATH" HOME="$WORK/home" TMPDIR="$WORK/tmp" node --import tsx "$WORK/probe.mjs" ``` The executed probe uses a close-start barrier, then compares rejection without abort against abort before rejection. It restores the real close and awaits fixture teardown: ```js import { createServer } from 'node:http'; import assert from 'node:assert/strict'; import { createRequire } from 'node:module'; import { resolve } from 'node:path'; import { pathToFileURL } from 'node:url'; const require = createRequire(resolve('package.json')); const { WebSocketServer } = require('ws'); const { CdpConnection } = await import(pathToFileURL(resolve('src/browser/CdpConnection.ts')).href); const { V8InspectorProvider } = await import(pathToFileURL(resolve('src/inspector/V8InspectorProvider.ts')).href); const { observeJavaScriptRuntime } = await import(pathToFileURL(resolve('src/application/javascript/JavaScriptRuntimeObservationService.ts')).href); const { observeJavaScriptRuntimeInputSchema } = await import(pathToFileURL(resolve('src/domain/javascript/javascriptRuntimeObservation.ts')).href); const { projectAnalysisError } = await import(pathToFileURL(resolve('src/domain/analysisErrorProjection.ts')).href); for (const abort of [false, true]) { let endpoint = ''; const server = createServer((req, res) => { res.setHeader('content-type', 'application/json'); if (req.url === '/json/version') res.end(JSON.stringify({Browser: 'Node.js/v24.18.0', 'Protocol-Version': '1.3', 'V8-Version': 'fixture'})); else if (req.url === '/json/list') res.end(JSON.stringify([{id: 'fixture', type: 'node', url: `${endpoint}/app.js`, attached: false, webSocketDebuggerUrl: endpoint.replace('http:', 'ws:') + '/fixture'}])); else res.writeHead(404).end(); }); const sockets = new WebSocketServer({server}); sockets.on('connection', socket => socket.on('message', raw => { const {id} = JSON.parse(raw.toString()); socket.send(JSON.stringify({id, result: {}})); })); await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); const addr = server.address(); if (!addr || typeof addr === 'string') throw new Error('no port'); endpoint = `http://127.0.0.1:${addr.port}`; const started = Promise.withResolvers(); const close = Promise.withResolvers(); const originalClose = CdpConnection.prototype.close; let connection; let settled = false; CdpConnection.prototype.close = function() { connection = this; started.resolve(); return close.promise; }; try { const controller = new AbortController(); const input = observeJavaScriptRuntimeInputSchema.parse({inspector_endpoint: endpoint, target_id: 'fixture', observation_ms: 0}); const observing = observeJavaScriptRuntime(new V8InspectorProvider(), input, {signal: controller.signal}); await started.promise; if (abort) controller.abort(new Error('fixture cancellation')); else { settled = true; close.reject(new Error('fixture close failed')); } const result = await observing; assert.equal(result.ok, false); if (result.ok) throw new Error('expected error'); const error = result.error; const before = {abort, closeSettled: settled, code: projectAnalysisError(error).code, cleanupIncomplete: error.cleanupIncomplete, resources: error.cleanupResources, partialEvidence: error.partialObservation !== undefined}; console.log(JSON.stringify(before)); assert.equal(error.cleanupIncomplete, !abort); assert.deepEqual(error.cleanupResources, abort ? [] : ['browser_transport']); assert.ok(error.partialObservation); if (abort) { settled = true; close.reject(new Error('fixture close failed')); await new Promise(resolve => setImmediate(resolve)); assert.equal(error.cleanupIncomplete, false); console.log(JSON.stringify({afterLateRejection: true, code: projectAnalysisError(error).code, cleanupIncomplete: error.cleanupIncomplete, resources: error.cleanupResources})); } } finally { close.resolve(); CdpConnection.prototype.close = originalClose; if (connection) await originalClose.call(connection); for (const client of sockets.clients) client.terminate(); await new Promise(resolve => sockets.close(() => resolve())); await new Promise(resolve => server.close(() => resolve())); } } ``` ## Expected behavior Caller cancellation may stop waiting promptly, but it must not imply confirmed resource release. Preserve unresolved transport cleanup and its owner/recovery information alongside cancellation and completed Evidence, or retain and surface the eventual close outcome. The [lifecycle contract](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/docs/tool-design.md#L116-L133) requires unresolved cleanup to remain attributed to its owner and one-shot cleanup to preserve completed results and remaining resources. ## Impact and scope The controlled boundary run loses a genuine injected cleanup failure and initially omits a pending transport. Callers cannot distinguish that state from cleanup completed successfully using the returned cleanup status. Cancellation and partial Evidence themselves remain correct. NOT_ENOUGH_INFO: frequency with real Inspector peers, persistent socket leakage, and native close-failure conditions. No target mutation or data loss was observed. Production close has its own graceful and forced-close bounds; this report does not claim cancellation prevents that background attempt. ## REA version Package metadata 6.7.0; tested source main `3537e58c1ab29309f06eabf50bc52adba9817a0c`. The relevant provider, connection, and application-service source files have no diff from `rea-agents-6.7.0`, but the published release runtime was not tested. ## Runtime and platform Node.js v26.12.0; Darwin 27.0.0; arm64. Source execution through locked `tsx`; focused Vitest 5.0.3 control also passed both characterization cases. The repository-pinned Node 24.18.0 runtime was not exercised. Fixture-reported Inspector runtime metadata is synthetic. ## Target and artifact context Authored loopback HTTP/WebSocket Inspector protocol fixture with one `node` target and an HTTP script location; zero-duration observation. No real provider app was launched or attached. ## Interface Other The reproduction exercises the shared application workflow used by the public Inspector tool. Source tracing confirms the [MCP registration forwards its signal to this workflow](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/server/registerJavaScriptRuntimeObservationTools.ts#L57-L65). Actual MCP-client cancellation/result delivery was not tested. ## Earliest failure stage Cleanup, cancellation, or lifecycle ## Relevant output or evidence Executed standalone output: ```json {"abort":false,"closeSettled":true,"code":"cleanup_incomplete","cleanupIncomplete":true,"resources":["browser_transport"],"partialEvidence":true} {"abort":true,"closeSettled":false,"code":"cancelled","cleanupIncomplete":false,"resources":[],"partialEvidence":true} {"afterLateRejection":true,"code":"cancelled","cleanupIncomplete":false,"resources":[]} ``` This establishes loss of the controlled close outcome through the application boundary, not a real socket leak. ## Typed outcome or recovery expected Retain typed cancellation and partial Evidence, together with truthful unresolved `browser_transport` cleanup and recovery ownership until release is confirmed. A later close failure must remain observable rather than being consumed without updating the result or its owner. ## Analysis or suspected cause The provider [memoizes the close helper's promise](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/inspector/V8InspectorProvider.ts#L159-L165). The [helper resolves on abort and suppresses later close rejection](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/inspector/V8InspectorProvider.ts#L298-L320), so the [final cleanup classification](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/inspector/V8InspectorProvider.ts#L263-L284) sees no failure. The application correctly classifies late cancellation, but has no retained close outcome to add. SOURCE-ONLY supporting reachability: [the real transport close can reject when closure remains unconfirmed](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/src/browser/CdpConnection.ts#L215-L276). Its graceful-close timer allows one second before termination, followed if needed by another one-second confirmation timer. Neither native failure path was faulted in this reproduction. ## Additional context Open and closed issue bodies were searched for Inspector, provider/helper identifiers, transport cleanup, and cancellation/close combinations. [#4227](https://github.com/morluto/rea/issues/4227) is the umbrella lifecycle audit. [#4261](https://github.com/morluto/rea/issues/4261) addressed retained Evidence and validation after provider completion, [#3553](https://github.com/morluto/rea/issues/3553) addressed setup deadlines, and [#4869](https://github.com/morluto/rea/issues/4869) addressed browser-scenario cleanup markers after successful recovery. Their triggers and closure conditions differ from this Inspector final-close abort race.

bsakaguchi closed this issue

2026-10-31 00:36:51 +00:00

bsakaguchi commented

2026-10-31 00:36:52 +00:00

Author

Owner

@morluto commented on GitHub (Oct 19, 2026):

Fixed on main in 87ab054ea3.

Removed the abort-versus-close success race and superseded close wrappers. Inspector now uses the existing exact CDP transport owner across provider instances and waits for bounded physical close. Failed close retains its lease, completed Evidence, and original cancellation reason; replacement connections remain blocked until that lease releases. Whole-operation admission/draining includes discovery and finalization. CLI one-shot completion and server-owned MCP shutdown join/retry cleanup; caller-owned optional providers remain caller-owned.

Loopback regressions hold final close, abort, confirm observation remains pending, reject close, reject a retry without opening another socket, then recover and confirm the original error clears. They also cover cleanup-only failure and shutdown with active/queued observations. This fixes ownership attribution; it does not claim a natural socket leak reproduction.

Validation on pinned Node 24.18.0: 696 passed, 5 skipped across 67 selected files; full npm run check, npm run docs:check, and test discovery passed (1,632 files discovered exactly once). Controlled filesystem/final-close seams and loopback HTTP/WebSocket peers establish the described boundaries; no real Chrome/Electron target, natural OS close failure, Windows, or Bun qualification is claimed.

@morluto commented on GitHub (Oct 19, 2026): Fixed on main in 87ab054ea384b3fcbae2998b1f94685edcd5f624. Removed the abort-versus-close success race and superseded close wrappers. Inspector now uses the existing exact CDP transport owner across provider instances and waits for bounded physical close. Failed close retains its lease, completed Evidence, and original cancellation reason; replacement connections remain blocked until that lease releases. Whole-operation admission/draining includes discovery and finalization. CLI one-shot completion and server-owned MCP shutdown join/retry cleanup; caller-owned optional providers remain caller-owned. Loopback regressions hold final close, abort, confirm observation remains pending, reject close, reject a retry without opening another socket, then recover and confirm the original error clears. They also cover cleanup-only failure and shutdown with active/queued observations. This fixes ownership attribution; it does not claim a natural socket leak reproduction. Validation on pinned Node 24.18.0: 696 passed, 5 skipped across 67 selected files; full `npm run check`, `npm run docs:check`, and test discovery passed (1,632 files discovered exactly once). Controlled filesystem/final-close seams and loopback HTTP/WebSocket peers establish the described boundaries; no real Chrome/Electron target, natural OS close failure, Windows, or Bun qualification is claimed.

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

No description provided.

Rows
Columns