morluto/rea

Fork 0

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

[PR #3555] [MERGED] fix(browser): bound CDP handshakes and command responses #5820

New issue

Closed

opened 2026-10-31 01:09:40 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3555
Author: @dakjdakd
Created: 10/14/2026
Status: ✅ Merged
Merged: 10/15/2026
Merged by: @baron

Base: main ← Head: fix/passive-target-setup-deadline

📝 Commits (3)

fcf1de1 fix(browser): bound CDP handshakes and command responses
09e1fc7 test(browser): expire stalled deadlines after fixture readiness
17404c9 docs(browser): explain handshake deadline ownership

📊 Changes

5 files changed (+335 additions, -27 deletions)

View changed files

📝 docs/browser-observation.md (+4 -2)
📝 docs/javascript-runtime-observation.md (+5 -0)
📝 src/browser/CdpConnection.ts (+47 -25)
➕ tests/boundary/browser/cdpTargetDeadlines.test.ts (+277 -0)
📝 tests/fixtures/inspector/fakeV8Inspector.ts (+2 -0)

📄 Description

Summary

Fixes #3553.

Passive browser and Inspector calls can finish HTTP discovery, then wait for a peer that never completes its WebSocket handshake or replies to a setup command. This change bounds each of those waits at the shared CdpConnection boundary using the existing 20-second command budget. The caller receives provider_timeout instead of waiting for an interrupt or disconnect. The requested observation duration remains a separate phase.

Problem and expected behavior

A local peer can advertise a valid debugging target and accept a connection without answering Target.attachToTarget, Page.enable, Runtime.enable, or Inspector's Debugger.enable. Discovery is already bounded, but that deadline does not continue into the later handshake and setup commands. At the affected revision, WebSocket construction disables the library handshake timeout, and the shared command map has no internal response timer.

With --observation-ms 0, controlled loopback peers left inspect-web-page and observe-javascript-runtime pending for 12,012 ms and 12,010 ms after their first setup command. This original CLI reproduction used c99c6a89d, Windows x64, Node.js 24.11.1 / npm 11.6.2. Closing the fixture produced a disconnect error. The transport, target-session and Inspector source is identical at fix baseline ca1b2f860. The measured wait is observed; the missing internal bound is source-derived.

Each handshake and command reply needs a finite wait independent of observation duration. Caller abort must retain cancellation; timeout must preserve partial Inspector facts and release owned resources.

Change and scope

src/browser/CdpConnection.ts owns the correction. A handshake timer rejects with BrowserObservationError reason timeout and terminates the unopened socket. Every pending command gets a response timer using WEB_RUNTIME_LIMITS.commandTimeoutMs, currently 20,000 ms. This is a per-wait limit: each handshake or RPC starts its own timer. It is not a single 20-second absolute deadline for all setup, capture, and cleanup combined.

The pending entry now exposes dispose, replacing the narrower abort-listener cleanup name. Its one cleanup path clears both the response timer and caller abort listener. Success, protocol rejection, cancellation, disconnect, and expiry remove pending ownership through #complete. A response arriving after its timer fired has no pending entry to resolve and is ignored; later commands retain their own correlation IDs. Command serialization happens before pending resources are allocated so a serialization exception cannot strand an entry, listener, or timer.

The browser and Inspector guides distinguish transport deadlines from observation duration. The Inspector fixture adds a withheld-method option; boundary tests exercise both providers over loopback connections. No dependency, configuration option, provider, or generated artifact is added. Active-runtime wrappers and provider cleanup retain their existing phase budgets.

This reliability fix adds no protocol methods, explicit pause/resume, or target-execution capability. A new-capability maintainer decision is not applicable under the contribution scope.

Contract and boundary impact

Semantic owner and earliest changed stage: CdpConnection, during handshake or a pending protocol response after endpoint admission/discovery.
Contribution scope fit and linked maintainer decision: existing passive-observation reliability; additional capability approval not applicable.
CLI and MCP/tool-catalog contract: no input or schema change. The existing timeout projection supplies code: provider_timeout, category: timeout, and details.reason: timeout; interruption remains cancellation.
Provider, bridge, target-format, or platform compatibility: shared CDP/Inspector transport; no native bridge or target-format change. Real-provider checks ran on Windows x64.
Evidence, artifact, provenance, or reconstruction contract: Inspector retains observations collected before failure. No successful Evidence shape, identity, source provenance, or reconstruction contract changes.
Target selection, authority, upstream behavior, and cleanup: endpoint and exact-target checks remain owned by their providers. The timeout closes REA's connection; it does not send target-close commands, terminate caller-owned targets, evaluate code, or acquire extra authority.
Generated metadata, package, or installation impact: none. Authored guides are updated; ignored build outputs and generated catalogs are not committed.

Expected effects of the affected observation workflows:

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

The attach item means connecting to an existing debugging target; no launch operation is added.

Evidence and regression coverage

Tests added or updated: tests/boundary/browser/cdpTargetDeadlines.test.ts adds 12 cases covering handshake; browser attach/arming; Inspector Runtime/Debugger enable with partial observations; late replies; independent observation duration; and timer/listener disposal on success, rejection, abort, and disconnect. tests/fixtures/inspector/fakeV8Inspector.ts supplies withheld responses.
Base reproduction: before production changes, seven of the initial eight regression cases failed because the fallback caller cancellation arrived instead of an internal timeout. The independent observation-window control passed. The final boundary tests retain the actual 20-second transport timers and real socket scheduling. They wait until the fixture accepts the handshake or receives the withheld command, then invoke only the single live deadline callback. Healthy handshakes and replies retain their full production allowance. A deliberately delayed healthy reply exercises a delay longer than the former 100 ms test budget; the independent observation-window case requests 20,250 ms, confirms that no command deadline remains active, then completes only the observation timer.
User-visible result: actual-duration compiled CLI runs without a caller timeout produced the following results. The fixture's emergency process guard did not fire, and all four runs had zero remaining connections before fixture teardown.

Withheld response	Public CLI	Wait after command/upgrade	Outcome
`Target.attachToTarget` reply	`inspect-web-page`	20,077 ms	`provider_timeout`
`Runtime.enable` reply	`observe-javascript-runtime`	20,080 ms	`provider_timeout`
WebSocket upgrade	`inspect-web-page`	20,077 ms	`provider_timeout`
WebSocket upgrade	`observe-javascript-runtime`	20,086 ms	`provider_timeout`

Representative error:

{
  "code": "provider_timeout",
  "category": "timeout",
  "message": "CDP command Target.attachToTarget exceeded its 20000 ms response deadline.",
  "retryable": true,
  "details": { "operation": "inspect_web_page", "reason": "timeout" }
}

Remaining proof gaps: the original stalled-peer measurements used public CLI, while real healthy-provider verification covered both CLI and MCP. The scope of the broader browser scenario verifier is stated below.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Fix validation: Windows x64, pinned Node.js 24.18.0 / npm 11.16.0:

Review follow-up: npm run test:focused -- tests/boundary/browser/cdpTargetDeadlines.test.ts tests/boundary/browser/cdpConnection.test.ts tests/boundary/browser/cdpCleanupCancellation.test.ts src/inspector/V8InspectorProvider.partial.test.ts src/inspector/V8InspectorProvider.cleanup.test.ts — 40 tests passed across 5 files, including all 12 deadline cases, delayed healthy replies, timer/listener disposal, cancellation, late-response correlation, and partial Inspector observations. Initial validation across the broader related provider, browser-socket, termination, final-collection, and world-ownership files covered 79 distinct tests across 10 files; the follow-up overlaps that coverage.
npm run check — passed typecheck, lint with zero errors, formatting, knip, metadata checks, and architecture guards.
npm run docs:check — passed generated/documentation consistency and five README conformance tests.
npm run verify:test-discovery — all 1,285 test files discovered exactly once.
npm run verify:inspector — passed seven real Node Inspector filename fixtures through public CLI and stdio MCP, including spaces, underscores, hash and percent spellings.
npm run verify:browser:dom with installed Chrome 155.0.8059.40 — passed real passive inspection through public CLI and stdio MCP, including four form-destination checks against the native browser oracle.
Compiled inspect-web-page and observe-javascript-runtime commands against controlled loopback peers — all four actual-duration timeout cases above passed, with exit code 1 and no stderr. The issue's reduced standalone fixture was additionally executed through the compiled browser CLI.
git diff --check — clean.

Validation scope: npm run verify:browser did not pass; full browser scenario coverage remains unverified. Scoped passive-browser CLI/MCP and Inspector lanes passed. Controlled peers establish stalled-transport behavior; real-provider checks cover the exercised healthy workflows, not every platform or scenario.

Compatibility, safety, and release

Breaking changes or migration steps: no schema or configuration migration. A command that previously could wait indefinitely now fails after its own 20-second response deadline. Callers receive the already-supported timeout outcome and can review target responsiveness and retained observations before retrying.
Real Hopper/Ghidra, browser, or OS coverage: Windows real Chrome and Node Inspector coverage as above; no Hopper/Ghidra behavior changed or claimed.
Package or release metadata impact: none; no dependency or installation change.
Security, privacy, process, or containment review: preserves validated loopback endpoints and target authority. Deadlines bound REA-owned waits and connections without closing caller-owned applications or adding evaluation, instrumentation, or mutation.

Review follow-up

The WebSocket options now explain why handshakeTimeout: 0 is intentional: waitForOpen owns the handshake deadline, caller cancellation, cleanup, and typed errors. Keeping the library timer disabled gives the handshake failure one owner and preserves the timeout projection already used by both providers.

The deadline remains per handshake or command, as described above. A cumulative setup budget would require a workflow-level policy for the complete arming sequence and is separate from bounding an individual stalled transport wait. This follow-up adds the requested ownership comment and retains the existing cancellation and per-wait behavior.

Follow-up validation on Windows x64 with Node 24.18.0 / npm 11.16.0: the CDP connection and target-deadline boundary files passed 29/29 tests, including all 12 deadline cases. npm run check:fast, formatting for CdpConnection.ts, and git diff --check passed. No transport behavior changed in this follow-up.

Author readiness

Human owner: @dakjdakd

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3555 **Author:** [@dakjdakd](https://github.com/dakjdakd) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/15/2026 **Merged by:** [@baron](https://github.com/baron) **Base:** `main` ← **Head:** `fix/passive-target-setup-deadline` --- ### 📝 Commits (3) - [`fcf1de1`](https://github.com/morluto/rea/commit/fcf1de1c71a49f9dbab103ab498e9833ddc9df65) fix(browser): bound CDP handshakes and command responses - [`09e1fc7`](https://github.com/morluto/rea/commit/09e1fc78883cc09402dd1f7575286d4ec64f4e07) test(browser): expire stalled deadlines after fixture readiness - [`17404c9`](https://github.com/morluto/rea/commit/17404c9737dd1b6aec61de75a58c03bede92083f) docs(browser): explain handshake deadline ownership ### 📊 Changes **5 files changed** (+335 additions, -27 deletions) <details> <summary>View changed files</summary> 📝 `docs/browser-observation.md` (+4 -2) 📝 `docs/javascript-runtime-observation.md` (+5 -0) 📝 `src/browser/CdpConnection.ts` (+47 -25) ➕ `tests/boundary/browser/cdpTargetDeadlines.test.ts` (+277 -0) 📝 `tests/fixtures/inspector/fakeV8Inspector.ts` (+2 -0) </details> ### 📄 Description ## Summary Fixes #3553. Passive browser and Inspector calls can finish HTTP discovery, then wait for a peer that never completes its WebSocket handshake or replies to a setup command. This change bounds each of those waits at the shared `CdpConnection` boundary using the existing 20-second command budget. The caller receives `provider_timeout` instead of waiting for an interrupt or disconnect. The requested observation duration remains a separate phase. ## Problem and expected behavior A local peer can advertise a valid debugging target and accept a connection without answering `Target.attachToTarget`, `Page.enable`, `Runtime.enable`, or Inspector's `Debugger.enable`. Discovery is already bounded, but that deadline does not continue into the later handshake and setup commands. At the affected revision, WebSocket construction disables the library handshake timeout, and the shared command map has no internal response timer. With `--observation-ms 0`, controlled loopback peers left `inspect-web-page` and `observe-javascript-runtime` pending for 12,012 ms and 12,010 ms after their first setup command. This original CLI reproduction used `c99c6a89d`, Windows x64, Node.js 24.11.1 / npm 11.6.2. Closing the fixture produced a disconnect error. The transport, target-session and Inspector source is identical at fix baseline `ca1b2f860`. The measured wait is observed; the missing internal bound is source-derived. Each handshake and command reply needs a finite wait independent of observation duration. Caller abort must retain cancellation; timeout must preserve partial Inspector facts and release owned resources. ## Change and scope `src/browser/CdpConnection.ts` owns the correction. A handshake timer rejects with `BrowserObservationError` reason `timeout` and terminates the unopened socket. Every pending command gets a response timer using `WEB_RUNTIME_LIMITS.commandTimeoutMs`, currently 20,000 ms. This is a per-wait limit: each handshake or RPC starts its own timer. It is not a single 20-second absolute deadline for all setup, capture, and cleanup combined. The pending entry now exposes `dispose`, replacing the narrower abort-listener cleanup name. Its one cleanup path clears both the response timer and caller abort listener. Success, protocol rejection, cancellation, disconnect, and expiry remove pending ownership through `#complete`. A response arriving after its timer fired has no pending entry to resolve and is ignored; later commands retain their own correlation IDs. Command serialization happens before pending resources are allocated so a serialization exception cannot strand an entry, listener, or timer. The browser and Inspector guides distinguish transport deadlines from observation duration. The Inspector fixture adds a withheld-method option; boundary tests exercise both providers over loopback connections. No dependency, configuration option, provider, or generated artifact is added. Active-runtime wrappers and provider cleanup retain their existing phase budgets. This reliability fix adds no protocol methods, explicit pause/resume, or target-execution capability. A new-capability maintainer decision is not applicable under the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). ## Contract and boundary impact - **Semantic owner and earliest changed stage:** `CdpConnection`, during handshake or a pending protocol response after endpoint admission/discovery. - **Contribution scope fit and linked maintainer decision:** existing passive-observation reliability; additional capability approval not applicable. - **CLI and MCP/tool-catalog contract:** no input or schema change. The existing timeout projection supplies `code: provider_timeout`, `category: timeout`, and `details.reason: timeout`; interruption remains cancellation. - **Provider, bridge, target-format, or platform compatibility:** shared CDP/Inspector transport; no native bridge or target-format change. Real-provider checks ran on Windows x64. - **Evidence, artifact, provenance, or reconstruction contract:** Inspector retains observations collected before failure. No successful Evidence shape, identity, source provenance, or reconstruction contract changes. - **Target selection, authority, upstream behavior, and cleanup:** endpoint and exact-target checks remain owned by their providers. The timeout closes REA's connection; it does not send target-close commands, terminate caller-owned targets, evaluate code, or acquire extra authority. - **Generated metadata, package, or installation impact:** none. Authored guides are updated; ignored build outputs and generated catalogs are not committed. Expected effects of the affected observation workflows: - [x] Read existing files or captured data. - [x] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [x] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. The attach item means connecting to an existing debugging target; no launch operation is added. ## Evidence and regression coverage - **Tests added or updated:** `tests/boundary/browser/cdpTargetDeadlines.test.ts` adds 12 cases covering handshake; browser attach/arming; Inspector Runtime/Debugger enable with partial observations; late replies; independent observation duration; and timer/listener disposal on success, rejection, abort, and disconnect. `tests/fixtures/inspector/fakeV8Inspector.ts` supplies withheld responses. - **Base reproduction:** before production changes, seven of the initial eight regression cases failed because the fallback caller cancellation arrived instead of an internal timeout. The independent observation-window control passed. The final boundary tests retain the actual 20-second transport timers and real socket scheduling. They wait until the fixture accepts the handshake or receives the withheld command, then invoke only the single live deadline callback. Healthy handshakes and replies retain their full production allowance. A deliberately delayed healthy reply exercises a delay longer than the former 100 ms test budget; the independent observation-window case requests 20,250 ms, confirms that no command deadline remains active, then completes only the observation timer. - **User-visible result:** actual-duration compiled CLI runs without a caller timeout produced the following results. The fixture's emergency process guard did not fire, and all four runs had zero remaining connections before fixture teardown. | Withheld response | Public CLI | Wait after command/upgrade | Outcome | |---|---|---:|---| | `Target.attachToTarget` reply | `inspect-web-page` | 20,077 ms | `provider_timeout` | | `Runtime.enable` reply | `observe-javascript-runtime` | 20,080 ms | `provider_timeout` | | WebSocket upgrade | `inspect-web-page` | 20,077 ms | `provider_timeout` | | WebSocket upgrade | `observe-javascript-runtime` | 20,086 ms | `provider_timeout` | Representative error: ```json { "code": "provider_timeout", "category": "timeout", "message": "CDP command Target.attachToTarget exceeded its 20000 ms response deadline.", "retryable": true, "details": { "operation": "inspect_web_page", "reason": "timeout" } } ``` - **Remaining proof gaps:** the original stalled-peer measurements used public CLI, while real healthy-provider verification covered both CLI and MCP. The scope of the broader browser scenario verifier is stated below. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed Fix validation: Windows x64, pinned Node.js 24.18.0 / npm 11.16.0: - Review follow-up: `npm run test:focused -- tests/boundary/browser/cdpTargetDeadlines.test.ts tests/boundary/browser/cdpConnection.test.ts tests/boundary/browser/cdpCleanupCancellation.test.ts src/inspector/V8InspectorProvider.partial.test.ts src/inspector/V8InspectorProvider.cleanup.test.ts` — **40 tests passed across 5 files**, including all 12 deadline cases, delayed healthy replies, timer/listener disposal, cancellation, late-response correlation, and partial Inspector observations. Initial validation across the broader related provider, browser-socket, termination, final-collection, and world-ownership files covered 79 distinct tests across 10 files; the follow-up overlaps that coverage. - `npm run check` — passed typecheck, lint with zero errors, formatting, knip, metadata checks, and architecture guards. - `npm run docs:check` — passed generated/documentation consistency and five README conformance tests. - `npm run verify:test-discovery` — all 1,285 test files discovered exactly once. - `npm run verify:inspector` — passed seven real Node Inspector filename fixtures through public CLI and stdio MCP, including spaces, underscores, hash and percent spellings. - `npm run verify:browser:dom` with installed Chrome 155.0.8059.40 — passed real passive inspection through public CLI and stdio MCP, including four form-destination checks against the native browser oracle. - Compiled `inspect-web-page` and `observe-javascript-runtime` commands against controlled loopback peers — all four actual-duration timeout cases above passed, with exit code 1 and no stderr. The issue's reduced standalone fixture was additionally executed through the compiled browser CLI. - `git diff --check` — clean. **Validation scope:** `npm run verify:browser` did not pass; full browser scenario coverage remains unverified. Scoped passive-browser CLI/MCP and Inspector lanes passed. Controlled peers establish stalled-transport behavior; real-provider checks cover the exercised healthy workflows, not every platform or scenario. ## Compatibility, safety, and release - **Breaking changes or migration steps:** no schema or configuration migration. A command that previously could wait indefinitely now fails after its own 20-second response deadline. Callers receive the already-supported timeout outcome and can review target responsiveness and retained observations before retrying. - **Real Hopper/Ghidra, browser, or OS coverage:** Windows real Chrome and Node Inspector coverage as above; no Hopper/Ghidra behavior changed or claimed. - **Package or release metadata impact:** none; no dependency or installation change. - **Security, privacy, process, or containment review:** preserves validated loopback endpoints and target authority. Deadlines bound REA-owned waits and connections without closing caller-owned applications or adding evaluation, instrumentation, or mutation. ## Review follow-up The WebSocket options now explain why `handshakeTimeout: 0` is intentional: `waitForOpen` owns the handshake deadline, caller cancellation, cleanup, and typed errors. Keeping the library timer disabled gives the handshake failure one owner and preserves the timeout projection already used by both providers. The deadline remains per handshake or command, as described above. A cumulative setup budget would require a workflow-level policy for the complete arming sequence and is separate from bounding an individual stalled transport wait. This follow-up adds the requested ownership comment and retains the existing cancellation and per-wait behavior. Follow-up validation on Windows x64 with Node 24.18.0 / npm 11.16.0: the CDP connection and target-deadline boundary files passed **29/29 tests**, including all 12 deadline cases. `npm run check:fast`, formatting for `CdpConnection.ts`, and `git diff --check` passed. No transport behavior changed in this follow-up. ## Author readiness Human owner: @dakjdakd - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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