morluto/rea

Fork 0

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

[PR #3101] [CLOSED] fix(browser): settle retained cleanup after cancelled opening #5617

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3101
Author: @ShitK
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/browser-cancelled-opening-cleanup

📝 Commits (4)

fc104e0 fix(browser): settle retained cleanup after cancelled opening
dd13dc5 Merge remote-tracking branch 'origin/main' into HEAD
49324e9 Merge upstream MCP identity fix into browser cleanup branch
ac63f6c Merge remote-tracking branch 'origin/main' into HEAD

📊 Changes

1 file changed (+216 additions, -0 deletions)

View changed files

➕ src/browser/PlaywrightScenarioCancellation.test.ts (+216 -0)

📄 Description

Summary

Fixes #2597.

Allow a browser-scenario provider to accept later captures and finish shutdown after a cancelled opening releases its resources.

Problem and expected behavior

Cancelling while openPlaywrightScenarioBrowser is pending retains a cleanup callback that captures the cancelled request signal. Each retry rejects with cleanup_failed, even after the browser and private profile have been released. Cleanup must remain owned until release succeeds, then allow the provider to recover.

Change and scope

Remove the request signal from the late-opening cleanup callback and from the cleanup owner's internal API. Resource release has its own confirmation deadline; request cancellation must not permanently prevent confirmation. Keep the one-second retained-cleanup retry boundary and the owner's 30-second deadline.

Replace the narrower cancellable-cleanup wait test with production Session → CleanupOwner → Provider coverage. Only browser opening/resource actions are controlled in these regressions; real release failures still block new sessions and remain retryable.

Contract and boundary impact

Semantic owner: PlaywrightScenarioBrowserCleanupOwner, with its caller in PlaywrightScenarioSession.open.
Scope decision: not applicable; routine cleanup fix within the existing browser capability.
CLI/MCP, tool catalog, Evidence, provenance, generated metadata, package and installation contracts: unchanged.
Compatibility: internal cleanup API no longer accepts a request signal. Its only production caller passing one was the affected late-opening path.
Target authority: existing caller-selected launch/attach rules remain in place. Browser/profile cleanup still requires confirmed release, with failed ownership retained.
Existing scenario effects include process launch/attachment, page navigation/scripts, and owned profile creation/removal. No new execution, injection, network, or mutation capability is added.

Evidence and regression coverage

Five regressions exercise successful recovery, browser-close failure, profile-removal failure, shutdown while opening completes late, and ownership across pending-opening/cleanup deadlines. The request still returns cancellation promptly, cleanup retries do not open another session early, and released resources are not closed repeatedly.

An isolated snapshot of base b3c5672ce67564e9b7c7baa6ce21f1674e27b287 with the final tests fails all five; applying the two production changes makes all five pass.

Current validation (`49324e9b7`)

Focused tests: 11 files, 96 passed, 1 platform-conditional skip. Includes all four files that failed in CI, production stdio MCP doctor, server identity, JSON validation, and the four browser cancellation/cleanup files.
npm run check: passed.
npm run verify:package: passed on macOS arm64, including an isolated packed installation, MCP doctor/tools/evidence, and verified cleanup.
Normal pre-push check:fast: passed.
Updated GitHub CI is running; the local package check is not a claim of Linux verification.

CI baseline diagnosis

The remaining shard and Linux package failures at dd13dc587 share a binary_session output rejection: the JSON validator rejects the lazy accessor in server_identity.catalog. On the exact unmodified main parent 131b2d85f, the same four files reproduce all 10 failures, and production mcp doctor fails with the same missing identity/request-flow checks.

Applying only the upstream serverIdentity.ts fix from ee9bf58ab to that baseline yields 17 passed / 1 skipped, and mcp doctor reports healthy. The upstream commit is now merged into this branch, preserving its authorship. The PR diff against main remains the original four browser files.

Original implementation validation (`fc104e012`)

npm run test:focused -- src/browser/PlaywrightScenarioCancellation.test.ts src/browser/PlaywrightScenarioBrowser.test.ts src/browser/PlaywrightScenarioSession.test.ts src/browser/PlaywrightBrowserScenarioProvider.test.ts: 45 passed.
vitest run src/browser: 33 files, 404 passed.
npm run check: passed, including build, typecheck, lint, formatting, Knip, metadata and architecture/module-boundary checks.
npm run verify:test-discovery: all 1,243 test files discovered exactly once.
Supplemental real-provider probe on macOS arm64, Chrome 154.0.8037.98: delayed only the return of a real browser opening, cancelled the request, confirmed removal of its real profile, completed a second real scenario with the expected DOM marker, confirmed both profiles were removed, and closed the same provider successfully. Uses compiled production Session/Owner/Provider; this is not CLI/MCP cancellation coverage.
npm run verify:browser: blocked before scenario verification by assertFormDestinations (destinations: []). Reproduced the same failure on unmodified base with the same Chrome; owned Chrome and fixture site were cleaned up.
npm test: completed, 13,744 passed / 32 failed / 314 skipped (1,243 files; 29m09s). All 32 failures across 20 files also reproduce on an isolated unmodified base, with matching per-file counts. They concern existing JavaScript/source-map contracts, other provider fixtures, catalog metadata, comparison-schema validation and process liveness; this PR does not claim a green full suite.

The original CI failures were reproduced on the original base. The project collaborator subsequently integrated #3080 and confirmed that those failures were unrelated to this PR. Current validation and the later MCP baseline repair are recorded above.

Compatibility, safety, and release

No public breaking change or migration. Local real-browser coverage is macOS/Chrome. The previous merged-head GitHub real-browser lane passed; current-head Linux CI is still running. Actual cleanup failures and incomplete observations remain visible. No release metadata changes are needed.

Before merge

Concrete problem and related issue identified.
Ordinary fix within existing scope and authority.
One focused outcome with a Conventional Commit title.
Observable behavior, genuine cleanup failures and bounded ownership covered.
Public contracts/docs/generated metadata reviewed; no changes needed.
Final diff checked for secrets, unrelated changes 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/3101 **Author:** [@ShitK](https://github.com/ShitK) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/browser-cancelled-opening-cleanup` --- ### 📝 Commits (4) - [`fc104e0`](https://github.com/morluto/rea/commit/fc104e0125de6789a0391c474041ef20e11574f6) fix(browser): settle retained cleanup after cancelled opening - [`dd13dc5`](https://github.com/morluto/rea/commit/dd13dc5872d0bd247eb5b7209c0076ba26d15f61) Merge remote-tracking branch 'origin/main' into HEAD - [`49324e9`](https://github.com/morluto/rea/commit/49324e9b7fefaaf40ee050960797a5eb85407701) Merge upstream MCP identity fix into browser cleanup branch - [`ac63f6c`](https://github.com/morluto/rea/commit/ac63f6c98f3f8c951dbfe22b6f10b57119f5d816) Merge remote-tracking branch 'origin/main' into HEAD ### 📊 Changes **1 file changed** (+216 additions, -0 deletions) <details> <summary>View changed files</summary> ➕ `src/browser/PlaywrightScenarioCancellation.test.ts` (+216 -0) </details> ### 📄 Description ## Summary Fixes #2597. Allow a browser-scenario provider to accept later captures and finish shutdown after a cancelled opening releases its resources. ## Problem and expected behavior Cancelling while `openPlaywrightScenarioBrowser` is pending retains a cleanup callback that captures the cancelled request signal. Each retry rejects with `cleanup_failed`, even after the browser and private profile have been released. Cleanup must remain owned until release succeeds, then allow the provider to recover. ## Change and scope Remove the request signal from the late-opening cleanup callback and from the cleanup owner's internal API. Resource release has its own confirmation deadline; request cancellation must not permanently prevent confirmation. Keep the one-second retained-cleanup retry boundary and the owner's 30-second deadline. Replace the narrower cancellable-cleanup wait test with production Session → CleanupOwner → Provider coverage. Only browser opening/resource actions are controlled in these regressions; real release failures still block new sessions and remain retryable. ## Contract and boundary impact - Semantic owner: `PlaywrightScenarioBrowserCleanupOwner`, with its caller in `PlaywrightScenarioSession.open`. - Scope decision: not applicable; routine cleanup fix within the existing browser capability. - CLI/MCP, tool catalog, Evidence, provenance, generated metadata, package and installation contracts: unchanged. - Compatibility: internal cleanup API no longer accepts a request signal. Its only production caller passing one was the affected late-opening path. - Target authority: existing caller-selected launch/attach rules remain in place. Browser/profile cleanup still requires confirmed release, with failed ownership retained. - Existing scenario effects include process launch/attachment, page navigation/scripts, and owned profile creation/removal. No new execution, injection, network, or mutation capability is added. ## Evidence and regression coverage Five regressions exercise successful recovery, browser-close failure, profile-removal failure, shutdown while opening completes late, and ownership across pending-opening/cleanup deadlines. The request still returns cancellation promptly, cleanup retries do not open another session early, and released resources are not closed repeatedly. An isolated snapshot of base `b3c5672ce67564e9b7c7baa6ce21f1674e27b287` with the final tests fails all five; applying the two production changes makes all five pass. ## Current validation (49324e9b7) - Focused tests: **11 files, 96 passed, 1 platform-conditional skip**. Includes all four files that failed in CI, production stdio MCP doctor, server identity, JSON validation, and the four browser cancellation/cleanup files. - `npm run check`: passed. - `npm run verify:package`: passed on macOS arm64, including an isolated packed installation, MCP doctor/tools/evidence, and verified cleanup. - Normal pre-push `check:fast`: passed. - Updated [GitHub CI](https://github.com/morluto/rea/actions/runs/38521271558) is running; the local package check is not a claim of Linux verification. ### CI baseline diagnosis The remaining shard and Linux package failures at `dd13dc587` share a `binary_session` output rejection: the JSON validator rejects the lazy accessor in `server_identity.catalog`. On the exact unmodified main parent `131b2d85f`, the same four files reproduce all 10 failures, and production `mcp doctor` fails with the same missing identity/request-flow checks. Applying only the upstream `serverIdentity.ts` fix from [ee9bf58ab](https://github.com/morluto/rea/commit/ee9bf58abc2e0e6450b3dcdae95b51d222412ebe) to that baseline yields **17 passed / 1 skipped**, and `mcp doctor` reports healthy. The upstream commit is now merged into this branch, preserving its authorship. The PR diff against main remains the original four browser files. ## Original implementation validation (fc104e012) - `npm run test:focused -- src/browser/PlaywrightScenarioCancellation.test.ts src/browser/PlaywrightScenarioBrowser.test.ts src/browser/PlaywrightScenarioSession.test.ts src/browser/PlaywrightBrowserScenarioProvider.test.ts`: 45 passed. - `vitest run src/browser`: 33 files, 404 passed. - `npm run check`: passed, including build, typecheck, lint, formatting, Knip, metadata and architecture/module-boundary checks. - `npm run verify:test-discovery`: all 1,243 test files discovered exactly once. - Supplemental real-provider probe on macOS arm64, Chrome 154.0.8037.98: delayed only the return of a real browser opening, cancelled the request, confirmed removal of its real profile, completed a second real scenario with the expected DOM marker, confirmed both profiles were removed, and closed the same provider successfully. Uses compiled production Session/Owner/Provider; this is not CLI/MCP cancellation coverage. - `npm run verify:browser`: blocked before scenario verification by `assertFormDestinations` (`destinations: []`). Reproduced the same failure on unmodified base with the same Chrome; owned Chrome and fixture site were cleaned up. - `npm test`: completed, **13,744 passed / 32 failed / 314 skipped** (1,243 files; 29m09s). All **32 failures across 20 files also reproduce on an isolated unmodified base**, with matching per-file counts. They concern existing JavaScript/source-map contracts, other provider fixtures, catalog metadata, comparison-schema validation and process liveness; this PR does not claim a green full suite. The original CI failures were reproduced on the original base. The project collaborator subsequently integrated #3080 and confirmed that those failures were unrelated to this PR. Current validation and the later MCP baseline repair are recorded above. ## Compatibility, safety, and release No public breaking change or migration. Local real-browser coverage is macOS/Chrome. The previous merged-head GitHub real-browser lane passed; current-head Linux CI is still running. Actual cleanup failures and incomplete observations remain visible. No release metadata changes are needed. ## Before merge - [x] Concrete problem and related issue identified. - [x] Ordinary fix within existing scope and authority. - [x] One focused outcome with a Conventional Commit title. - [x] Observable behavior, genuine cleanup failures and bounded ownership covered. - [x] Public contracts/docs/generated metadata reviewed; no changes needed. - [x] Final diff checked for secrets, unrelated changes 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:08:01 +00:00

closed this issue
added the
pull-request
label

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

No description provided.

Rows
Columns