[PR #5605] [MERGED] fix(browser): honor long passive observation windows #6993

New issue

Closed

opened 2026-11-01 21:44:33 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:44:33 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5605
Author: @gensokyo-dev
Created: 10/20/2026
Status: ✅ Merged
Merged: 10/20/2026
Merged by: @gensokyo-dev

Base: main ← Head: fix/3929-passive-window-clock

📝 Commits (2)

571c2ef fix(browser): honor long passive observation windows
b5d3f08 Merge remote-tracking branch 'origin/main' into fix/3929-passive-window-clock

📊 Changes

3 files changed (+148 additions, -18 deletions)

View changed files

📝 docs/browser-observation.md (+5 -0)
📝 src/browser/CdpObservationSession.ts (+24 -17)
📝 tests/boundary/browser/cdpBrowserProvider.timelineEvents.test.ts (+119 -1)

📄 Description

Summary

Fixes #3929. Passive browser observation accepts long windows, but its single Node timer turns 2_147_483_648 / 3_000_000_000 ms into a 1-ms timer. Real Chromium on pinned main 8443692e2 reports window_elapsed after 5 ms, before cancellation or scope exit can occur.

The observation owner now uses the existing cancellable, segmented wait. Caller cancellation, target/scope termination and elapsed time share one settlement/cleanup owner. Registering caller cancellation before target-end notification also removes an existing listener leak when the target already ended. Abort during the armed handshake starts no window timer; opaque cancellation causes remain intact.

This takes the narrow timer approach proposed in #4040 without adopting that PR's broader browser Evidence changes. #4040 remains open for its separate work.

Contract and scope

Shared browser provider implementation serves both CLI and MCP. Caller-selected durations, schema, target identity, Evidence provenance, partial coverage and full export access remain unchanged. No new cache or result reuse: repeated observation reads current browser state. Each wait retains one representable timer, one abort listener and one target-end subscription, all released on settlement.

Existing read-only CDP attachment and network authority are preserved; no target execution, injection, mutation, GUI control, new provider or dependency is introduced. Additional maintainer scope decision: not applicable to this lifecycle bug fix. Generated catalogs/build outputs are uncommitted.

Executed validation

Linux x64, Node 24.18.0, Chrome for Testing 156.0.8078.4, actual MCP SDK protocol 2026-07-28:

New production-boundary cases fail on exact base: four failures covering two oversized physical timers, an already-ended target listener leak, and a timer allocated after armed-handshake cancellation. Candidate: 18 tests pass across the wait helper, timeline/resource-type provider boundaries and MCP browser workflow. Full npm run check passes; docs:check passes with 5 consistency tests.
Integration with newer main 90e433df0, final head b5d3f0871: full check, the same 18 tests, and docs checks pass again. Real Chromium MCP cancellation, recovery and scope exit also pass (22.081 s, 18.949 s family CPU, 1,297.5 MiB peak), cleanup verified and no OOM. A reversed command-argument harness invocation failed before loading the runtime; it is retained and excluded.
Actual public CLI on owned Chromium, four cases per revision: ordinary 1-second observation with navigation, both oversized durations with cancellation, and oversized duration with scope exit. Main falsely completes all three long windows in 5 ms; candidate ordinary window takes 1,008 ms, both long windows cancel, and scope exit returns target_left_scope after 397 ms. No overflow warning; browser remains alive after each operation.
Actual public MCP: long-window cancellation, a succeeding observation on the same server, then long-window scope exit all pass. Healthy main/candidate controls each complete two new 1-second windows with external navigation, distinct current Evidence and full Evidence export. Observer remains passive; the test driver navigates its own loopback fixture page.
Multi-day completion is not claimed. Existing helper fake-clock coverage proves crossing the timer segment boundary; real checks prove bounded waiting/cancellation/scope exit. Original macOS/Node 26 environment is unverified. An initial probe used ping, unsupported by the pinned protocol; retained as an excluded harness attempt, then replaced by a real recovery observation.

Local logs, profiler receipts and captures are retained under /tmp/rea-3929-proof; they are not committed.

Performance and context

One CPU, 8-GiB hard / 7-GiB early process-family memory bounds, no swap. Equivalent completed MCP tasks include catalog discovery, two fresh observation windows, full Evidence export and close:

Revision	Elapsed	Family CPU	Peak family memory	First / repeat call
Main	20.866 s	18.074 s	1,295.9 MiB	9.373 / 1.064 s
Candidate	20.665 s	18.185 s	1,278.0 MiB	9.486 / 1.214 s

The repeat warms server imports, not observation results. OS caches/shared-host load are uncontrolled; these samples establish no general speedup. Completed runs verify cleanup and no OOM using counters sampled at payload exit while the observer remains alive, not final whole-scope counters.

cl100k_base estimates, main/candidate: compact input-catalog proxy 44,631 / 44,631 tokens, selected input schema 104 / 104, first response content 2,046 / 2,054, repeat content 2,010 / 2,022, and complete captured post-connect protocol 722,569 / 722,610. Local exported Evidence costs 3,961 / 3,981 tokens and remains separately accessible. Catalog proxies include name/description/inputSchema; protocol captures include output schemas. Actual model usage, initialization outside capture hooks and downstream prompts are unmeasured. Normal event coverage and detail remain preserved.

CI/review assessment and authorized bypass

Latest head b5d3f0871: Codex review completes with no findings. Greptile also completes with 5/5 and no unresolved review threads. Classification/readiness/title jobs remain queued without a runner. No queued CI is claimed as passing.

The task owner explicitly authorized bypass once independently qualified. The merge decision uses completed full checks, precise base-failing lifecycle regressions, native CLI/MCP workflows and final-tree newer-main integration. Pending results remain visible and will be assessed when available; unrelated main failures will not be mixed into this fix.

Author readiness

Human owner: @gensokyo-dev

The task owner explicitly delegated validation, readiness declarations and merge to this agent. These declarations are supplied under that authorization; no separate new manual human review by the original author is asserted.

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

Focused lifecycle fix; contribution scope and existing authority preserved.
Root cause, related cleanup, regressions and real public workflows checked; meaningful verification gaps recorded.
Available latest-head CI/review state assessed; queued work and bypass authorization recorded above.

Post-merge CI attribution: shard 1 failed only jsonOutputOwnership.test.ts / preserves cancellation when staging cleanup also fails at writer.close() with unexpected staging entries (1 failed / 1,817 passed tests). This is the same pre-existing failure independently reproduced on pre-change main cf5194f68 and the earlier browser candidate; the test, OwnedStagedOutput and ArtifactResourceScope blobs are byte-identical on that baseline and this PR head b5d3f0871. No observation-window case failed. Other shards were cancelled after this unrelated failure; portable checks remain queued. No full CI pass is claimed, and no unrelated staging-cleanup repair is included.

_{🔄 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/5605 **Author:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Created:** 10/20/2026 **Status:** ✅ Merged **Merged:** 10/20/2026 **Merged by:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Base:** `main` ← **Head:** `fix/3929-passive-window-clock` --- ### 📝 Commits (2) - [`571c2ef`](https://github.com/morluto/rea/commit/571c2efc97763dcbc995be1e22c6f5cbfd4e18ba) fix(browser): honor long passive observation windows - [`b5d3f08`](https://github.com/morluto/rea/commit/b5d3f087106bc738fe4df1b1c94a4ec714b89571) Merge remote-tracking branch 'origin/main' into fix/3929-passive-window-clock ### 📊 Changes **3 files changed** (+148 additions, -18 deletions) <details> <summary>View changed files</summary> 📝 `docs/browser-observation.md` (+5 -0) 📝 `src/browser/CdpObservationSession.ts` (+24 -17) 📝 `tests/boundary/browser/cdpBrowserProvider.timelineEvents.test.ts` (+119 -1) </details> ### 📄 Description ## Summary Fixes #3929. Passive browser observation accepts long windows, but its single Node timer turns `2_147_483_648` / `3_000_000_000` ms into a 1-ms timer. Real Chromium on pinned main `8443692e2` reports `window_elapsed` after **5 ms**, before cancellation or scope exit can occur. The observation owner now uses the existing cancellable, segmented wait. Caller cancellation, target/scope termination and elapsed time share one settlement/cleanup owner. Registering caller cancellation before target-end notification also removes an existing listener leak when the target already ended. Abort during the armed handshake starts no window timer; opaque cancellation causes remain intact. This takes the narrow timer approach proposed in #4040 without adopting that PR's broader browser Evidence changes. #4040 remains open for its separate work. ## Contract and scope Shared browser provider implementation serves both CLI and MCP. Caller-selected durations, schema, target identity, Evidence provenance, partial coverage and full export access remain unchanged. No new cache or result reuse: repeated observation reads current browser state. Each wait retains one representable timer, one abort listener and one target-end subscription, all released on settlement. Existing read-only CDP attachment and network authority are preserved; no target execution, injection, mutation, GUI control, new provider or dependency is introduced. Additional maintainer scope decision: not applicable to this lifecycle bug fix. Generated catalogs/build outputs are uncommitted. ## Executed validation Linux x64, Node **24.18.0**, Chrome for Testing **156.0.8078.4**, actual MCP SDK protocol **2026-07-28**: - New production-boundary cases fail on exact base: four failures covering two oversized physical timers, an already-ended target listener leak, and a timer allocated after armed-handshake cancellation. Candidate: **18 tests pass** across the wait helper, timeline/resource-type provider boundaries and MCP browser workflow. Full `npm run check` passes; `docs:check` passes with **5 consistency tests**. - Integration with newer main **90e433df0**, final head **b5d3f0871**: full check, the same **18 tests**, and docs checks pass again. Real Chromium MCP cancellation, recovery and scope exit also pass (**22.081 s**, **18.949 s** family CPU, **1,297.5 MiB** peak), cleanup verified and no OOM. A reversed command-argument harness invocation failed before loading the runtime; it is retained and excluded. - Actual public CLI on owned Chromium, four cases per revision: ordinary 1-second observation with navigation, both oversized durations with cancellation, and oversized duration with scope exit. Main falsely completes all three long windows in 5 ms; candidate ordinary window takes 1,008 ms, both long windows cancel, and scope exit returns `target_left_scope` after 397 ms. No overflow warning; browser remains alive after each operation. - Actual public MCP: long-window cancellation, a succeeding observation on the same server, then long-window scope exit all pass. Healthy main/candidate controls each complete two new 1-second windows with external navigation, distinct current Evidence and full Evidence export. Observer remains passive; the test driver navigates its own loopback fixture page. - Multi-day completion is not claimed. Existing helper fake-clock coverage proves crossing the timer segment boundary; real checks prove bounded waiting/cancellation/scope exit. Original macOS/Node 26 environment is unverified. An initial probe used `ping`, unsupported by the pinned protocol; retained as an excluded harness attempt, then replaced by a real recovery observation. Local logs, profiler receipts and captures are retained under `/tmp/rea-3929-proof`; they are not committed. ## Performance and context One CPU, 8-GiB hard / 7-GiB early process-family memory bounds, no swap. Equivalent completed MCP tasks include catalog discovery, two fresh observation windows, full Evidence export and close: | Revision | Elapsed | Family CPU | Peak family memory | First / repeat call | | --- | ---: | ---: | ---: | ---: | | Main | 20.866 s | 18.074 s | 1,295.9 MiB | 9.373 / 1.064 s | | Candidate | 20.665 s | 18.185 s | 1,278.0 MiB | 9.486 / 1.214 s | The repeat warms server imports, not observation results. OS caches/shared-host load are uncontrolled; these samples establish no general speedup. Completed runs verify cleanup and no OOM using counters sampled at payload exit while the observer remains alive, not final whole-scope counters. `cl100k_base` estimates, main/candidate: compact input-catalog proxy **44,631 / 44,631 tokens**, selected input schema **104 / 104**, first response content **2,046 / 2,054**, repeat content **2,010 / 2,022**, and complete captured post-connect protocol **722,569 / 722,610**. Local exported Evidence costs **3,961 / 3,981 tokens** and remains separately accessible. Catalog proxies include name/description/inputSchema; protocol captures include output schemas. Actual model usage, initialization outside capture hooks and downstream prompts are unmeasured. Normal event coverage and detail remain preserved. ## CI/review assessment and authorized bypass Latest head `b5d3f0871`: Codex review completes with **no findings**. Greptile also completes with **5/5** and no unresolved review threads. Classification/readiness/title jobs remain queued without a runner. No queued CI is claimed as passing. The task owner explicitly authorized bypass once independently qualified. The merge decision uses completed full checks, precise base-failing lifecycle regressions, native CLI/MCP workflows and final-tree newer-main integration. Pending results remain visible and will be assessed when available; unrelated main failures will not be mixed into this fix. ## Author readiness Human owner: @gensokyo-dev The task owner explicitly delegated validation, readiness declarations and merge to this agent. These declarations are supplied under that authorization; no separate new manual human review by the original author is asserted. - [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 - [x] Focused lifecycle fix; contribution scope and existing authority preserved. - [x] Root cause, related cleanup, regressions and real public workflows checked; meaningful verification gaps recorded. - [x] Available latest-head CI/review state assessed; queued work and bypass authorization recorded above. Post-merge CI attribution: shard 1 failed only `jsonOutputOwnership.test.ts` / `preserves cancellation when staging cleanup also fails` at `writer.close()` with unexpected staging entries (1 failed / 1,817 passed tests). This is the same pre-existing failure independently reproduced on pre-change main `cf5194f68` and the earlier browser candidate; the test, OwnedStagedOutput and ArtifactResourceScope blobs are byte-identical on that baseline and this PR head `b5d3f0871`. No observation-window case failed. Other shards were cancelled after this unrelated failure; portable checks remain queued. No full CI pass is claimed, and no unrelated staging-cleanup repair is included. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:44:33 +00:00