morluto/rea

Fork 0

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

[GH-ISSUE #4467] [Bug] CDP errors retain cleanup_incomplete after confirmed socket release #1768

New issue

Closed

opened 2026-10-16 21:10:03 +00:00 by bsakaguchi · 3 comments

bsakaguchi commented

2026-10-16 21:10:03 +00:00

Owner

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

Before submitting

I searched open and closed issues and open PRs for related CDP cleanup reports.
No credentials or sensitive target data are included.

Area

Process capture or runtime observation

Observed behavior

A completed CdpBrowserProvider.inspectPage whose socket close rejects returns cleanupIncomplete: true, as expected. A later provider close retries and confirms the real local WebSocket closed, but both the inspection error and the intervening close error still report cleanupIncomplete: true.

Executed once against source fe2a398dd9ab290ba6cb3411403d319cec8d07a2 plus unrelated local artifact/setup fixes. The CDP owner/provider files were unchanged. Node24.18.0, macOS arm64. This uses a real loopback HTTP/WebSocket fixture and a controlled CdpConnection.close rejection; no Chrome/Electron executable or natural transport failure was run.

Reproduction

Added the following case to existing tests/boundary/browser/cdpTargetSessionOwner.test.ts and ran:
npm run test:focused -- tests/boundary/browser/cdpTargetSessionOwner.test.ts

Three existing cases passed; the new case failed at expect(result.error.cleanupIncomplete).toBe(false) with actual true, after observed active sockets reached zero.

Regression case (uses existing browser fixture)

Add imports for CdpBrowserProvider, inspectWebPageInputSchema, AnalysisError and runtime CdpConnection from their corresponding src modules, then:

it("clears returned inspection and shutdown cleanup status only after socket release", async () => {
  const browser = await startFakeCdpBrowser();
  const provider = new CdpBrowserProvider();
  const originalClose = CdpConnection.prototype.close;
  let denied = true;
  const closing = vi
    .spyOn(CdpConnection.prototype, "close")
    .mockImplementation(function (this: CdpConnection) {
      return denied
        ? Promise.reject(new Error("controlled transport close failure"))
        : originalClose.call(this);
    });
  onTestFinished(async () => {
    closing.mockRestore();
    await provider.close();
    await browser.close();
  });
  const result = await provider.inspectPage(
    inspectWebPageInputSchema.parse({
      cdp_endpoint: browser.endpoint,
      allowed_origins: [browser.allowedOrigin],
      target_id: "allowed-page",
      observation_ms: 0,
    }),
  );
  if (result.ok) throw new Error("Expected retained socket cleanup failure");
  expect(result.error.cleanupIncomplete).toBe(true);
  expect(result.error.partialObservation).toBeDefined();
  const observation = result.error.partialObservation;
  const retry = await provider.close().catch((cause: unknown) => cause);
  expect(retry).toBeInstanceOf(AnalysisError);
  if (!(retry instanceof AnalysisError))
    throw new Error("Missing retry failure");
  expect(retry.cleanupIncomplete).toBe(true);
  expect(browser.activeConnectionCount()).toBe(1);
  denied = false;
  await provider.close();
  await vi.waitFor(() => expect(browser.activeConnectionCount()).toBe(0));
  expect(result.error.cleanupIncomplete).toBe(false);
  expect(retry.cleanupIncomplete).toBe(false);
  expect(result.error.partialObservation).toBe(observation);
  expect(browser.commands.map(({ method }) => method)).not.toContain(
    "Browser.close",
  );
  expect(browser.commands.map(({ method }) => method)).not.toContain(
    "Target.closeTarget",
  );
});

Expected behavior

docs/tool-design.md#own-the-lifecycle requires cleanup failures and aggregates to bind to exact owners and resolve current cleanup uncertainty after confirmed release. Retain historical diagnostics and completed observations, while clearing current cleanup status only when all sockets represented by that error close.

Impact and scope

Observed stale status on the public provider workflow and its close retry, using one real local socket. No leaked socket remained after successful retry. Source-derived: owner finish, retryPending and closeAll create cleanup errors and later delete records after successful close without resolving those errors; browser/Electron adapters additionally wrap those failures without retaining an explicit cleanup binding. Mixed-owner aggregates and Electron runtime behavior were not executed in the baseline reproduction. Related lifecycle tracker #4227; distinct from target mutation and worker network issues.

REA version

6.5.0 source fe2a398dd9, local unrelated artifact/setup edits.

Runtime and platform

Node24.18.0; macOS arm64; real local ws fixture, no real Chrome/Electron.

Interface

Production provider boundary used by CLI/MCP; CLI/MCP serialization not exercised in this reproduction.

Typed outcome or recovery expected

Keep each error linked to its exact socket owner. Retry success must update originating, retry and aggregate cleanup status, with mixed outcomes retaining uncertainty for unresolved sockets and preserving observations/primary failures.

Originally created by @morluto on GitHub (Oct 16, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/4467 ### Before submitting - [x] I searched open and closed issues and open PRs for related CDP cleanup reports. - [x] No credentials or sensitive target data are included. ### Area Process capture or runtime observation ### Observed behavior A completed `CdpBrowserProvider.inspectPage` whose socket close rejects returns `cleanupIncomplete: true`, as expected. A later provider close retries and confirms the real local WebSocket closed, but both the inspection error and the intervening close error still report `cleanupIncomplete: true`. Executed once against source `fe2a398dd9ab290ba6cb3411403d319cec8d07a2` plus unrelated local artifact/setup fixes. The CDP owner/provider files were unchanged. Node24.18.0, macOS arm64. This uses a real loopback HTTP/WebSocket fixture and a controlled `CdpConnection.close` rejection; no Chrome/Electron executable or natural transport failure was run. ### Reproduction Added the following case to existing `tests/boundary/browser/cdpTargetSessionOwner.test.ts` and ran: `npm run test:focused -- tests/boundary/browser/cdpTargetSessionOwner.test.ts` Three existing cases passed; the new case failed at `expect(result.error.cleanupIncomplete).toBe(false)` with actual `true`, after observed active sockets reached zero. <details><summary>Regression case (uses existing browser fixture)</summary> Add imports for CdpBrowserProvider, inspectWebPageInputSchema, AnalysisError and runtime CdpConnection from their corresponding src modules, then: ```typescript it("clears returned inspection and shutdown cleanup status only after socket release", async () => { const browser = await startFakeCdpBrowser(); const provider = new CdpBrowserProvider(); const originalClose = CdpConnection.prototype.close; let denied = true; const closing = vi .spyOn(CdpConnection.prototype, "close") .mockImplementation(function (this: CdpConnection) { return denied ? Promise.reject(new Error("controlled transport close failure")) : originalClose.call(this); }); onTestFinished(async () => { closing.mockRestore(); await provider.close(); await browser.close(); }); const result = await provider.inspectPage( inspectWebPageInputSchema.parse({ cdp_endpoint: browser.endpoint, allowed_origins: [browser.allowedOrigin], target_id: "allowed-page", observation_ms: 0, }), ); if (result.ok) throw new Error("Expected retained socket cleanup failure"); expect(result.error.cleanupIncomplete).toBe(true); expect(result.error.partialObservation).toBeDefined(); const observation = result.error.partialObservation; const retry = await provider.close().catch((cause: unknown) => cause); expect(retry).toBeInstanceOf(AnalysisError); if (!(retry instanceof AnalysisError)) throw new Error("Missing retry failure"); expect(retry.cleanupIncomplete).toBe(true); expect(browser.activeConnectionCount()).toBe(1); denied = false; await provider.close(); await vi.waitFor(() => expect(browser.activeConnectionCount()).toBe(0)); expect(result.error.cleanupIncomplete).toBe(false); expect(retry.cleanupIncomplete).toBe(false); expect(result.error.partialObservation).toBe(observation); expect(browser.commands.map(({ method }) => method)).not.toContain( "Browser.close", ); expect(browser.commands.map(({ method }) => method)).not.toContain( "Target.closeTarget", ); }); ``` </details> ### Expected behavior `docs/tool-design.md#own-the-lifecycle` requires cleanup failures and aggregates to bind to exact owners and resolve current cleanup uncertainty after confirmed release. Retain historical diagnostics and completed observations, while clearing current cleanup status only when all sockets represented by that error close. ### Impact and scope Observed stale status on the public provider workflow and its close retry, using one real local socket. No leaked socket remained after successful retry. Source-derived: owner `finish`, `retryPending` and `closeAll` create cleanup errors and later delete records after successful close without resolving those errors; browser/Electron adapters additionally wrap those failures without retaining an explicit cleanup binding. Mixed-owner aggregates and Electron runtime behavior were not executed in the baseline reproduction. Related lifecycle tracker #4227; distinct from target mutation and worker network issues. ### REA version 6.5.0 source fe2a398dd9ab290ba6cb3411403d319cec8d07a2, local unrelated artifact/setup edits. ### Runtime and platform Node24.18.0; macOS arm64; real local ws fixture, no real Chrome/Electron. ### Interface Production provider boundary used by CLI/MCP; CLI/MCP serialization not exercised in this reproduction. ### Typed outcome or recovery expected Keep each error linked to its exact socket owner. Retry success must update originating, retry and aggregate cleanup status, with mixed outcomes retaining uncertainty for unresolved sockets and preserving observations/primary failures.

bsakaguchi added the

triage

bug

labels

2026-10-16 21:10:03 +00:00

bsakaguchi commented

2026-10-16 21:10:04 +00:00

Author

Owner

@qorvr commented on GitHub (Oct 16, 2026):

I investigated this issue and verified the root causes across CdpTargetSessionOwner and the provider adapters (CdpBrowserProvider, CdpElectronProvider, CdpWebRuntimeProvider).

Root Causes

CdpTargetSessionOwner connection-to-error disconnect:
In CdpTargetSessionOwner.finish, retryPending, and #closeAll, connection teardown created cleanupError instances, but connection records were deleted upon eventual successful close without linking those errors to the corresponding connection or AnalysisCleanupLease. Because of this, when retried sockets later successfully closed, resolveCleanup() was never triggered on either the initial inspection error or intermediate close retry errors.
Provider adapter error wrapping:
CdpBrowserProvider.withTargetSession, CdpElectronProvider, and CdpWebRuntimeProvider wrapped cleanup failures or combined them with primary analysis failures into new ProviderCleanupError or analysisErrorWithCleanupFailure instances without binding the wrapped error to the underlying connection lease.

Implemented Fix

A fix is staged and pushed on branch fix/cdp-socket-release-cleanup-status (commit 59adc1fd5):

CdpTargetSessionOwner:
- Attached an AnalysisCleanupLease per tracked OwnedConnection.
- Stored error-to-lease associations in an #errorLeases registry using AnalysisCleanupLeaseGroup for multi-socket aggregate errors.
- On transport close, invoked await record.lease.release(), which automatically updates cleanupIncomplete to false via resolveCleanup() on all bound errors once their constituent sockets terminate.
- Exposed bindCleanupError(error, source) so calling providers can register wrapped errors against the same socket leases.
Provider Adapters (CdpBrowserProvider, CdpElectronProvider, CdpWebRuntimeProvider):
- Bound wrapped failures (analysisErrorWithCleanupFailure, finalError, combined) to the underlying connection leases via owner.bindCleanupError(...).
- Normalized non-AnalysisError primary outcomes through providerError in withTargetSession so structured diagnostics and leases are preserved.
Test Suite (tests/boundary/browser/cdpTargetSessionOwner.test.ts):
- Added the exact reproduction case provided by @morluto, verifying that cleanupIncomplete transitions to false on both the returned inspection result and the close retry error once active sockets reach 0.
- Added mixed-outcome tests verifying that if one socket releases while another socket remains unresolved, the aggregate and unreleased errors retain cleanupIncomplete: true until all sockets close.
- Added tests verifying that primary analysis failure payload, error codes, and partial observations are retained.

All 6 tests in cdpTargetSessionOwner.test.ts pass, and npm run check:fast passes cleanly. Ready for PR once open contributor PR quota allows.

@qorvr commented on GitHub (Oct 16, 2026): I investigated this issue and verified the root causes across `CdpTargetSessionOwner` and the provider adapters (`CdpBrowserProvider`, `CdpElectronProvider`, `CdpWebRuntimeProvider`). ### Root Causes 1. **`CdpTargetSessionOwner` connection-to-error disconnect**: In `CdpTargetSessionOwner.finish`, `retryPending`, and `#closeAll`, connection teardown created `cleanupError` instances, but connection records were deleted upon eventual successful close without linking those errors to the corresponding connection or `AnalysisCleanupLease`. Because of this, when retried sockets later successfully closed, `resolveCleanup()` was never triggered on either the initial inspection error or intermediate close retry errors. 2. **Provider adapter error wrapping**: `CdpBrowserProvider.withTargetSession`, `CdpElectronProvider`, and `CdpWebRuntimeProvider` wrapped cleanup failures or combined them with primary analysis failures into new `ProviderCleanupError` or `analysisErrorWithCleanupFailure` instances without binding the wrapped error to the underlying connection lease. ### Implemented Fix A fix is staged and pushed on branch [`fix/cdp-socket-release-cleanup-status`](https://github.com/qorvr/rea/tree/fix/cdp-socket-release-cleanup-status) (commit `59adc1fd5`): - **`CdpTargetSessionOwner`**: - Attached an `AnalysisCleanupLease` per tracked `OwnedConnection`. - Stored error-to-lease associations in an `#errorLeases` registry using `AnalysisCleanupLeaseGroup` for multi-socket aggregate errors. - On transport close, invoked `await record.lease.release()`, which automatically updates `cleanupIncomplete` to `false` via `resolveCleanup()` on all bound errors once their constituent sockets terminate. - Exposed `bindCleanupError(error, source)` so calling providers can register wrapped errors against the same socket leases. - **Provider Adapters (`CdpBrowserProvider`, `CdpElectronProvider`, `CdpWebRuntimeProvider`)**: - Bound wrapped failures (`analysisErrorWithCleanupFailure`, `finalError`, `combined`) to the underlying connection leases via `owner.bindCleanupError(...)`. - Normalized non-`AnalysisError` primary outcomes through `providerError` in `withTargetSession` so structured diagnostics and leases are preserved. - **Test Suite (`tests/boundary/browser/cdpTargetSessionOwner.test.ts`)**: - Added the exact reproduction case provided by @morluto, verifying that `cleanupIncomplete` transitions to `false` on both the returned inspection result and the close retry error once active sockets reach 0. - Added mixed-outcome tests verifying that if one socket releases while another socket remains unresolved, the aggregate and unreleased errors retain `cleanupIncomplete: true` until all sockets close. - Added tests verifying that primary analysis failure payload, error codes, and partial observations are retained. All 6 tests in `cdpTargetSessionOwner.test.ts` pass, and `npm run check:fast` passes cleanly. Ready for PR once open contributor PR quota allows.

bsakaguchi referenced this issue

2026-10-17 21:01:31 +00:00

[GH-ISSUE #2383] [Tracker] Audit analysis and debugging coverage for the TIOBE top 100 languages (October 2026) #794

bsakaguchi closed this issue

2026-10-31 00:32:41 +00:00

bsakaguchi commented

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

Author

Owner

@CassieFox23 commented on GitHub (Oct 18, 2026):

TL;DR: The test case in this issue now passes on main. Commit ce0d6790e added that exact case and the fix. I did not test the runtime provider part, which draft PR #4571 covers.

Results

Check	Node 22.23.1	Node 24.18.0	Node 26.11.1
Issue's case, main (`8f7d12e30`)	pass	pass	pass
Same file, parent of `ce0d6790e` (`effb71f59`)	fail	fail	fail

The case in the issue matches the case now on main, line for line. On main, all 5 tests in cdpTargetSessionOwner.test.ts pass.

The parent has no new cases, so I copied in the test file from ce0d6790e. The issue's case then failed at expect(result.error.cleanupIncomplete).toBe(false). The value was true. That is the failure this issue reports. The second new case also failed on the parent: keeps retry aggregates incomplete until every reported socket is released.

cdpCleanupCancellation.test.ts passes on main (12 of 12, Node 24). On the parent, 6 of 12 fail at the same cleanupIncomplete check.

Not run

The runtime provider path. #4571 is the open follow-up, and I did not test that PR.
A real Chrome or Electron. The fixture is the repo's fake local CDP browser.
CLI and MCP output.
Node 22 and 26 for cdpCleanupCancellation.test.ts.

Thanks, morluto, for the regression case and the fix.

Test setup

Box: Nobara 44 (Fedora 44 base), Linux 7.2.9
CPU: AMD Ryzen 9 5900X, 12 cores / 24 threads
RAM: 32 GB DDR4-3600
Node: 22.23.1, 24.18.0, 26.11.1
Head / base: 8f7d12e30 / effb71f59
Other: other test agents may have shared the box. I report no timings.

Commands

npm ci --ignore-scripts
npm run build:cached
npm run test:focused -- tests/boundary/browser/cdpTargetSessionOwner.test.ts
npm run test:focused -- tests/boundary/browser/cdpCleanupCancellation.test.ts

For the parent tree, I first ran git show ce0d6790e:tests/boundary/browser/cdpTargetSessionOwner.test.ts and git show ce0d6790e:tests/boundary/browser/cdpCleanupCancellation.test.ts into it. Each run used timeout 60.

🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊

@CassieFox23 commented on GitHub (Oct 18, 2026): **TL;DR:** The test case in this issue now passes on main. Commit ce0d6790e added that exact case and the fix. I did not test the runtime provider part, which draft PR #4571 covers. ### Results | Check | Node 22.23.1 | Node 24.18.0 | Node 26.11.1 | |---|---|---|---| | Issue's case, main (8f7d12e30) | pass | pass | pass | | Same file, parent of ce0d6790e (effb71f59) | fail | fail | fail | The case in the issue matches the case now on main, line for line. On main, all 5 tests in `cdpTargetSessionOwner.test.ts` pass. The parent has no new cases, so I copied in the test file from ce0d6790e. The issue's case then failed at `expect(result.error.cleanupIncomplete).toBe(false)`. The value was `true`. That is the failure this issue reports. The second new case also failed on the parent: `keeps retry aggregates incomplete until every reported socket is released`. `cdpCleanupCancellation.test.ts` passes on main (12 of 12, Node 24). On the parent, 6 of 12 fail at the same `cleanupIncomplete` check. ### Not run - The runtime provider path. #4571 is the open follow-up, and I did not test that PR. - A real Chrome or Electron. The fixture is the repo's fake local CDP browser. - CLI and MCP output. - Node 22 and 26 for `cdpCleanupCancellation.test.ts`. Thanks, morluto, for the regression case and the fix. <details> <summary>Test setup</summary> - Box: Nobara 44 (Fedora 44 base), Linux 7.2.9 - CPU: AMD Ryzen 9 5900X, 12 cores / 24 threads - RAM: 32 GB DDR4-3600 - Node: 22.23.1, 24.18.0, 26.11.1 - Head / base: 8f7d12e30 / effb71f59 - Other: other test agents may have shared the box. I report no timings. </details> <details> <summary>Commands</summary> ```bash npm ci --ignore-scripts npm run build:cached npm run test:focused -- tests/boundary/browser/cdpTargetSessionOwner.test.ts npm run test:focused -- tests/boundary/browser/cdpCleanupCancellation.test.ts ``` For the parent tree, I first ran `git show ce0d6790e:tests/boundary/browser/cdpTargetSessionOwner.test.ts` and `git show ce0d6790e:tests/boundary/browser/cdpCleanupCancellation.test.ts` into it. Each run used `timeout 60`. </details> 🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊

bsakaguchi commented

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

Author

Owner

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

Closing as fixed on main after reconciling this report with the landed owner/consumer changes.

ce0d6790e: fix(browser): bind CDP cleanup failures to exact transport leases. Retain failed socket owners across translated failures and resolve aggregate cleanup only after every required transport closes. Preserve concurrently active peer sessions.

Evidence scope: this closure uses the landed source, regression coverage and validation already recorded in the linked commits/thread. No new local tests or native-provider reproduction were run during this backlog pass; previously documented platform and real-provider gaps still apply.

@morluto commented on GitHub (Oct 18, 2026):  Closing as fixed on `main` after reconciling this report with the landed owner/consumer changes. - [ce0d6790e](https://github.com/morluto/rea/commit/ce0d6790ed2ac594dadba77819727c92037c5927): fix(browser): bind CDP cleanup failures to exact transport leases. Retain failed socket owners across translated failures and resolve aggregate cleanup only after every required transport closes. Preserve concurrently active peer sessions. Evidence scope: this closure uses the landed source, regression coverage and validation already recorded in the linked commits/thread. No new local tests or native-provider reproduction were run during this backlog pass; previously documented platform and real-provider gaps still apply.

bsakaguchi referenced this issue

2026-10-31 00:37:23 +00:00

[GH-ISSUE #5146] [Bug] inspect-managed-members returns unavailable on a 2042-type .NET app and offers no way to list its types #3164

bsakaguchi referenced this issue

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

[PR #1768] [MERGED] fix(managed): charge retained member facts instead of decoding bounds #4804

bsakaguchi referenced this issue

2026-10-31 00:55:41 +00:00

[PR #1822] [MERGED] fix(managed): bound transient CIL by decoded instruction count #4837

bsakaguchi referenced this issue

2026-10-31 01:09:20 +00:00

[PR #3487] [MERGED] fix(managed): charge admitted member facts once in the final projection #5775