[GH-ISSUE #4255] [Bug] Electron page inspection returns success after cancellation during detach #1680

New issue

Closed

opened 2026-10-16 21:08:58 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-16 21:08:58 +00:00

Owner

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

[Bug] Electron page inspection publishes completed Evidence after cancellation during detach

Observed result

Reproduced through the source application/provider boundary with a disposable fake CDP server and local fixture file. The fake target aborts the request signal when Target.detachFromTarget is received. inspectElectronPage(...) then returns ok: true while signal.aborted === true. This establishes the service result classification; it does not claim what an MCP SDK client receives after cancelling its promise.

Baseline: repository 0e1f0bf2d, macOS, Node v24.18.0.

Evidence

src/browser/CdpElectronProvider.ts:125-148 builds a successful inspection result, then awaits closeCdpTargetSession in finally, with no post-cleanup cancellation check.
src/application/javascript/ElectronObservationService.ts:35-53 converts any provider success into Evidence without checking whether the request signal was aborted during provider finalization.
Electron MCP registration passes the MCP request signal to the service (src/server/registerElectronTools.ts:84-97). Its adapter then projects the service result; this audit did not make claims about the MCP client promise's behavior after cancellation.
Reproduce with the retained ignored fixture: PATH=/opt/homebrew/opt/node@24/bin:$PATH ./node_modules/.bin/vitest run --config .cache/codebase-audit/browser/vitest-audit.config.mts .cache/codebase-audit/browser/electron-late-cancellation.repro.test.ts. Output confirms trigger=Target.detachFromTarget, signal_aborted=true, and service_result=success.
Existing provider coverage checks cancellation during discovery and cleanup failures, but no final-detach cancellation interleaving was found in tests/boundary/browser/cdpElectronProvider.test.ts or the Electron MCP tests.
All-state GitHub issue searches found no exact report. #4227 is a broad lifecycle audit design; #4027 is the browser/Electron/JavaScript tracker.

Impact

A cancellation that arrives during final CDP teardown does not prevent an already-collected Electron page observation from being converted into successful Evidence. This violates the repository's cancellation cutoff rule after the last awaited finalization step. It also gives callers no typed cancellation result or retained partial-observation recovery path for that interleaving.

Proposed structural fix

Keep the completed inspection in an owner-local outcome, perform cleanup, then check the signal after the last awaited finalization step. If cancellation wins, return typed cancellation carrying the completed inspection as partial_observation. If cleanup fails, preserve that classification and include the completed facts plus the unresolved transport resource; do not let cancellation erase cleanup failure or vice versa. Reuse the repository's canonical provider-evidence finalization helper if it fits this provider contract.

Acceptance check

Use a controllable CDP detach barrier. Verify the non-aborted control returns Evidence; abort after inspection completion but before detach settles and require cancellation with the completed observation retained; inject detach failure with and without a primary inspection failure and verify cleanup classification, primary-failure precedence, and the reported remaining transport resource. Confirm the external target remains alive and usable.

Originally created by @morluto on GitHub (Oct 15, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/4255 # [Bug] Electron page inspection publishes completed Evidence after cancellation during detach ## Observed result Reproduced through the source application/provider boundary with a disposable fake CDP server and local fixture file. The fake target aborts the request signal when `Target.detachFromTarget` is received. `inspectElectronPage(...)` then returns `ok: true` while `signal.aborted === true`. This establishes the service result classification; it does not claim what an MCP SDK client receives after cancelling its promise. Baseline: repository `0e1f0bf2d`, macOS, Node `v24.18.0`. ## Evidence - `src/browser/CdpElectronProvider.ts:125-148` builds a successful inspection result, then awaits `closeCdpTargetSession` in `finally`, with no post-cleanup cancellation check. - `src/application/javascript/ElectronObservationService.ts:35-53` converts any provider success into Evidence without checking whether the request signal was aborted during provider finalization. - Electron MCP registration passes the MCP request signal to the service (`src/server/registerElectronTools.ts:84-97`). Its adapter then projects the service result; this audit did not make claims about the MCP client promise's behavior after cancellation. - Reproduce with the retained ignored fixture: `PATH=/opt/homebrew/opt/node@24/bin:$PATH ./node_modules/.bin/vitest run --config .cache/codebase-audit/browser/vitest-audit.config.mts .cache/codebase-audit/browser/electron-late-cancellation.repro.test.ts`. Output confirms `trigger=Target.detachFromTarget`, `signal_aborted=true`, and `service_result=success`. - Existing provider coverage checks cancellation during discovery and cleanup failures, but no final-detach cancellation interleaving was found in `tests/boundary/browser/cdpElectronProvider.test.ts` or the Electron MCP tests. - All-state GitHub issue searches found no exact report. #4227 is a broad lifecycle audit design; #4027 is the browser/Electron/JavaScript tracker. ## Impact A cancellation that arrives during final CDP teardown does not prevent an already-collected Electron page observation from being converted into successful Evidence. This violates the repository's cancellation cutoff rule after the last awaited finalization step. It also gives callers no typed cancellation result or retained partial-observation recovery path for that interleaving. ## Proposed structural fix Keep the completed inspection in an owner-local outcome, perform cleanup, then check the signal after the last awaited finalization step. If cancellation wins, return typed cancellation carrying the completed inspection as `partial_observation`. If cleanup fails, preserve that classification and include the completed facts plus the unresolved transport resource; do not let cancellation erase cleanup failure or vice versa. Reuse the repository's canonical provider-evidence finalization helper if it fits this provider contract. ## Acceptance check Use a controllable CDP detach barrier. Verify the non-aborted control returns Evidence; abort after inspection completion but before detach settles and require cancellation with the completed observation retained; inject detach failure with and without a primary inspection failure and verify cleanup classification, primary-failure precedence, and the reported remaining transport resource. Confirm the external target remains alive and usable.

bsakaguchi

2026-10-16 21:08:58 +00:00

closed this issue
added the
triage

bug
labels

bsakaguchi commented

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

Author

Owner

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

Fixed on main in 93c620c3e. Electron page inspection now owns final detach and classifies late cancellation while retaining the completed page observation and cleanup facts.

Controlled CDP/file regressions cover cancellation during detach, partial DOM facts and external-target survival; no real Electron execution is claimed.

Hosted main CI is still pending; this closure is based on the implemented fix and the scoped regression evidence.

@morluto commented on GitHub (Oct 16, 2026): Fixed on `main` in [93c620c3e](https://github.com/morluto/rea/commit/93c620c3e7c401be40b71c480518147c76ade4dc). Electron page inspection now owns final detach and classifies late cancellation while retaining the completed page observation and cleanup facts. Controlled CDP/file regressions cover cancellation during detach, partial DOM facts and external-target survival; no real Electron execution is claimed. Hosted main CI is still pending; this closure is based on the implemented fix and the scoped regression evidence.

bsakaguchi referenced this issue

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

[GH-ISSUE #5968] [Bug] Snapshot-read cancellation attributes open_binary to read_evidence_file #3639

bsakaguchi referenced this issue

2026-10-31 00:54:39 +00:00

[PR #1682] [MERGED] feat(evidence): discover retained records without full payloads #4750

bsakaguchi referenced this issue

2026-10-31 00:54:45 +00:00

[PR #1694] [MERGED] perf(evidence): fuse streamed JSON token handling #4760

bsakaguchi referenced this issue

2026-10-31 00:54:54 +00:00

[PR #1716] [MERGED] perf(snapshot): restore large legacy caches within live memory bounds #4775