morluto/rea

Fork 0

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

[GH-ISSUE #5989] [Bug] Ownerless CDP discovery and attach cleanup erase the primary failure #3657

New issue

Closed

opened 2026-10-31 00:42:46 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

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

Owner

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

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

Connect-mode scenario policy initialization loses its original discovery or attach error when socket cleanup rejects. The returned failure is exactly the cleanup error, with neither a cause nor a retained primary. With successful cleanup, the same operation correctly reports the original CDP method and protocol rejection. Both endpoint discovery and target attachment reproduced at the production policy-owner boundary using an actual loopback HTTP/WebSocket fixture and controlled cleanup rejection.

Reproduction

Run from the source checkout with its locked dependencies installed. Save the following focused test as tests/boundary/browser/repro.test.ts and run:

node node_modules/vitest/vitest.mjs run tests/boundary/browser/repro.test.ts --maxWorkers 1

The test characterizes the incorrect behavior, so its assertions pass on this revision. The self-contained body below was replayed independently after the combined six-case run; its assertions passed.

import { expect, it, vi } from 'vitest';
import type { Page } from 'playwright-core';
import { CdpConnection } from '../../../src/browser/CdpConnection.js';
import { PlaywrightScenarioServiceWorkers } from '../../../src/browser/PlaywrightScenarioServiceWorkers.js';
import { BrowserObservationError } from '../../../src/domain/browserObservationError.js';
import { projectAnalysisError } from '../../../src/domain/analysisErrorProjection.js';
import { startFakeCdpBrowser } from '../../fixtures/fakeCdpBrowser.js';

it.each(['Target.getTargets', 'Target.attachToTarget'])(
  'ownerless policy initialization loses %s failure only when close rejects', async method => {
    for (const failClose of [false, true]) {
      const browser = await startFakeCdpBrowser({ commandError: command => command.method === method
        ? { code: -32000, message: 'controlled primary failure' } : undefined });
      const cleanup = new BrowserObservationError('capture_browser_scenario', 'cleanup_failed', { detail: 'controlled close failure' });
      const original = CdpConnection.prototype.close;
      const close = vi.spyOn(CdpConnection.prototype, 'close').mockImplementation(async function () {
        await original.call(this);
        if (failClose) throw cleanup;
      });
      const page = { frames: () => [{ evaluate: async () => null, isDetached: () => false }], isClosed: () => true } as unknown as Page;
      const policy = new PlaywrightScenarioServiceWorkers(page, browser.browserWebSocketUrl, 'allowed-page');
      try {
        const failure = await policy.initialize().then(() => undefined, error => error);
        expect(close).toHaveBeenCalledOnce();
        if (failClose) {
          expect(failure).toBe(cleanup);
          expect(failure.cause).toBeUndefined();
          expect(failure.cleanupPrimary).toBeUndefined();
        } else {
          expect(failure).toMatchObject({ reason: 'protocol_error' });
          expect(failure.userMessage).toContain('controlled primary failure');
        }
        console.log('OWNERLESS', method, failClose, JSON.stringify(projectAnalysisError(failure)));
        await policy.close();
      } finally { close.mockRestore(); await browser.close(); }
    }
  });

The Page policy precheck is simulated as returning no worker-policy violation. CDP commands run through the actual socket/parser. The close override first performs real socket release, then injects the rejection; this avoids leaving a test socket open. This establishes error-precedence behavior, rather than a naturally occurring transport stall. No full CLI/MCP scenario or real browser was executed.

Expected behavior

Keep incomplete cleanup visible while retaining the original operation failure and its diagnostic. No specific outer error code is required: a cleanup_incomplete envelope may contain the original primary. The lifecycle contract requires cleanup to preserve the primary diagnostic. The owned branches in the same module already compose both failures.

Impact and scope

P2: a bounded double-failure case discards the operation and reason needed to diagnose the failed scenario setup. Both tested CDP methods reproduced with a rejected close, and their normal-close controls retained the primary message. No data loss, target damage, persistent leak, cancellation case, natural close failure, or real-world frequency was established. Independent CDP logs could recover the original error; that workaround was not tested.

REA version

rea-agents 6.7.0; source revision 52415975942a4b9c53a817915b313ccc404f280e.

Runtime and platform

Node.js v26.12.0 and repository-pinned v24.18.0; macOS 27.0.1 arm64; locked Vitest 5.0.3. Each standalone test was replayed under both Node versions. Source tests with existing dependencies; no build performed.

Target and artifact context

Connect-mode browser scenario with service_workers=block; synthetic allowed page and controlled discovery/attach rejection. Only local fixture transports are opened.

Interface

Other

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Observed characterization results:

Target.getTargets + normal close: protocol_error; message names Target.getTargets and controlled primary failure
Target.getTargets + rejected close: cleanup_failed; message controlled close failure; cause absent; cleanupPrimary absent
Target.attachToTarget + normal close: protocol_error; message names Target.attachToTarget and controlled primary failure
Target.attachToTarget + rejected close: cleanup_failed; message controlled close failure; cause absent; cleanupPrimary absent

Projecting either replacement error returns code: cleanup_incomplete and details.reason: cleanup_failed, with no original CDP error in the returned object. Two focused tests exercised all four interleavings. These observations cover the production policy-owner and shared error projection, not public adapter serialization.

Typed outcome or recovery expected

Retain the exact discovery/attach primary, plus cleanup status and diagnostic, through the caller-visible failure. Keep successful cleanup rethrowing the original failure. Preserve the same behavior at both ownerless catch sites.

Analysis or suspected cause

Source-derived: openCdpEndpoint and attachCdpTargetSession await connection.close() before rethrowing the primary. A rejected await bypasses that throw. Policy installation calls both without an owner. Scenario initialization reaches this code for connect mode with the block policy. The exact public CLI/MCP result beyond this boundary is source-derived.

Additional context

Fetched main 25e1a24a3ed3671a0148757555f18d16c29d936a leaves the relevant producer and recovery behavior unchanged. Later shared cleanup/projection edits only avoid duplicating captured command output; these cases carry no captured command output. No newer runtime was built or executed. Last known good version: NOT_ENOUGH_INFO; none established. Related #4467/PR #4571 concern clearing stale cleanup status after successful retry; #5330 concerns a late stop request; #5756 loses the target-termination reason before composing an execution observation. Those fixes do not repair these ownerless catch blocks.

Originally created by @coygeek on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5989 ### 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 Connect-mode scenario policy initialization loses its original discovery or attach error when socket cleanup rejects. The returned failure is exactly the cleanup error, with neither a cause nor a retained primary. With successful cleanup, the same operation correctly reports the original CDP method and protocol rejection. Both endpoint discovery and target attachment reproduced at the production policy-owner boundary using an actual loopback HTTP/WebSocket fixture and controlled cleanup rejection. ### Reproduction Run from the source checkout with its locked dependencies installed. Save the following focused test as `tests/boundary/browser/repro.test.ts` and run: ```sh node node_modules/vitest/vitest.mjs run tests/boundary/browser/repro.test.ts --maxWorkers 1 ``` The test characterizes the incorrect behavior, so its assertions pass on this revision. The self-contained body below was replayed independently after the combined six-case run; its assertions passed. ```ts import { expect, it, vi } from 'vitest'; import type { Page } from 'playwright-core'; import { CdpConnection } from '../../../src/browser/CdpConnection.js'; import { PlaywrightScenarioServiceWorkers } from '../../../src/browser/PlaywrightScenarioServiceWorkers.js'; import { BrowserObservationError } from '../../../src/domain/browserObservationError.js'; import { projectAnalysisError } from '../../../src/domain/analysisErrorProjection.js'; import { startFakeCdpBrowser } from '../../fixtures/fakeCdpBrowser.js'; it.each(['Target.getTargets', 'Target.attachToTarget'])( 'ownerless policy initialization loses %s failure only when close rejects', async method => { for (const failClose of [false, true]) { const browser = await startFakeCdpBrowser({ commandError: command => command.method === method ? { code: -32000, message: 'controlled primary failure' } : undefined }); const cleanup = new BrowserObservationError('capture_browser_scenario', 'cleanup_failed', { detail: 'controlled close failure' }); const original = CdpConnection.prototype.close; const close = vi.spyOn(CdpConnection.prototype, 'close').mockImplementation(async function () { await original.call(this); if (failClose) throw cleanup; }); const page = { frames: () => [{ evaluate: async () => null, isDetached: () => false }], isClosed: () => true } as unknown as Page; const policy = new PlaywrightScenarioServiceWorkers(page, browser.browserWebSocketUrl, 'allowed-page'); try { const failure = await policy.initialize().then(() => undefined, error => error); expect(close).toHaveBeenCalledOnce(); if (failClose) { expect(failure).toBe(cleanup); expect(failure.cause).toBeUndefined(); expect(failure.cleanupPrimary).toBeUndefined(); } else { expect(failure).toMatchObject({ reason: 'protocol_error' }); expect(failure.userMessage).toContain('controlled primary failure'); } console.log('OWNERLESS', method, failClose, JSON.stringify(projectAnalysisError(failure))); await policy.close(); } finally { close.mockRestore(); await browser.close(); } } }); ``` The Page policy precheck is simulated as returning no worker-policy violation. CDP commands run through the actual socket/parser. The close override first performs real socket release, then injects the rejection; this avoids leaving a test socket open. This establishes error-precedence behavior, rather than a naturally occurring transport stall. No full CLI/MCP scenario or real browser was executed. ### Expected behavior Keep incomplete cleanup visible while retaining the original operation failure and its diagnostic. No specific outer error code is required: a `cleanup_incomplete` envelope may contain the original primary. The [lifecycle contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/tool-design.md#L132-L138) requires cleanup to preserve the primary diagnostic. The [owned branches in the same module](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/browser/CdpTargetSession.ts#L75-L85) already compose both failures. ### Impact and scope P2: a bounded double-failure case discards the operation and reason needed to diagnose the failed scenario setup. Both tested CDP methods reproduced with a rejected close, and their normal-close controls retained the primary message. No data loss, target damage, persistent leak, cancellation case, natural close failure, or real-world frequency was established. Independent CDP logs could recover the original error; that workaround was not tested. ### REA version `rea-agents` 6.7.0; source revision `52415975942a4b9c53a817915b313ccc404f280e`. ### Runtime and platform Node.js v26.12.0 and repository-pinned v24.18.0; macOS 27.0.1 arm64; locked Vitest 5.0.3. Each standalone test was replayed under both Node versions. Source tests with existing dependencies; no build performed. ### Target and artifact context Connect-mode browser scenario with service_workers=block; synthetic allowed page and controlled discovery/attach rejection. Only local fixture transports are opened. ### Interface Other ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Observed characterization results: ```text Target.getTargets + normal close: protocol_error; message names Target.getTargets and controlled primary failure Target.getTargets + rejected close: cleanup_failed; message controlled close failure; cause absent; cleanupPrimary absent Target.attachToTarget + normal close: protocol_error; message names Target.attachToTarget and controlled primary failure Target.attachToTarget + rejected close: cleanup_failed; message controlled close failure; cause absent; cleanupPrimary absent ``` Projecting either replacement error returns `code: cleanup_incomplete` and `details.reason: cleanup_failed`, with no original CDP error in the returned object. Two focused tests exercised all four interleavings. These observations cover the production policy-owner and shared error projection, not public adapter serialization. ### Typed outcome or recovery expected Retain the exact discovery/attach primary, plus cleanup status and diagnostic, through the caller-visible failure. Keep successful cleanup rethrowing the original failure. Preserve the same behavior at both ownerless catch sites. ### Analysis or suspected cause Source-derived: [openCdpEndpoint](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/browser/CdpTargetSession.ts#L75-L85) and [attachCdpTargetSession](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/browser/CdpTargetSession.ts#L148-L171) await `connection.close()` before rethrowing the primary. A rejected await bypasses that throw. [Policy installation](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/browser/PlaywrightScenarioServiceWorkers.ts#L79-L103) calls both without an owner. [Scenario initialization](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/browser/PlaywrightScenarioSession.ts#L282-L294) reaches this code for connect mode with the block policy. The exact public CLI/MCP result beyond this boundary is source-derived. ### Additional context Fetched main `25e1a24a3ed3671a0148757555f18d16c29d936a` leaves the relevant producer and recovery behavior unchanged. Later shared cleanup/projection edits only avoid duplicating captured command output; these cases carry no captured command output. No newer runtime was built or executed. Last known good version: NOT_ENOUGH_INFO; none established. Related #4467/PR #4571 concern clearing stale cleanup status after successful retry; #5330 concerns a late stop request; #5756 loses the target-termination reason before composing an execution observation. Those fixes do not repair these ownerless catch blocks.

bsakaguchi closed this issue

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

bsakaguchi commented

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

Author

Owner

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

Fixed in c86288db1. Ownerless CDP discovery/attach failure cleanup now composes the cleanup failure with the primary protocol error. Owned service-worker policy transports retain their exact retry owner, and successful retry clears cleanup status. Verified by tests/boundary/browser/cdpFailedAdmission.test.ts. Verification used loopback fake CDP and production provider/error projection; no real-browser run is claimed.

@morluto commented on GitHub (Oct 23, 2026): Fixed in [c86288db1](https://github.com/morluto/rea/commit/c86288db1b79cf4edf96c3d1618d5b71ee36c042). Ownerless CDP discovery/attach failure cleanup now composes the cleanup failure with the primary protocol error. Owned service-worker policy transports retain their exact retry owner, and successful retry clears cleanup status. Verified by `tests/boundary/browser/cdpFailedAdmission.test.ts`. Verification used loopback fake CDP and production provider/error projection; no real-browser run is claimed.

bsakaguchi referenced this issue

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

[PR #3657] [CLOSED] docs: add Bangla (বাংলা) README translation #5865

bsakaguchi referenced this issue

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

[GH-ISSUE #3795] [Tracker] Open documentation issues and pull requests (October 2026) #1468

bsakaguchi referenced this issue

2026-11-01 21:11:07 +00:00

[GH-ISSUE #5654] [Tracker] Open PRs that can't merge yet: conflicts and red CI (October 2026) #3425