morluto/rea

Fork 0

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

[PR #4358] [CLOSED] fix(mcp): retain shared observation provider ownership #6200

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:12:37 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4358
Author: @geekwolf
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: fix/mcp-observation-provider-ownership

📝 Commits (1)

12d6676 fix(mcp): retain shared observation provider ownership

📊 Changes

3 files changed (+180 additions, -37 deletions)

View changed files

📝 src/main/transport.ts (+34 -2)
📝 src/server/createServer.ts (+9 -3)
📝 tests/boundary/process/browserScenarioProbeLifecycle.test.ts (+137 -32)

📄 Description

Upstream coverage update — 2026-10-16

Main now independently covers this defect through #4274, commit 1b97b0f370. Its transport owns all three shared observation ports, deduplicates provider identity, retains failed cleanup for retry and preserves default direct-server ownership. Relevant main workflows were rechecked on 8ab5024397: 53 tests passed, with two existing Windows-gated POSIX signal cases skipped. This draft remains open, not merged; its superseded option naming and implementation are not being replayed. Historical proposal and validation follow.

Summary

Keep process-shared observation providers alive when the MCP SDK closes a discovery probe or replaces a server. This fixes a regression introduced by 93c620c3 in the lifecycle originally addressed by #2195; no separate issue was opened.

Problem and change

The transport loads optional observation providers once and shares them across SDK-created servers. Each server also began closing the supplied providers, so discarding a probe permanently closed the shared browser scenario provider before the replacement could use it. Browser and passive Electron observation ports had the same duplicate ownership, while transport shutdown only closed the scenario port.

Add an explicit ownership option for these three observation ports. Directly constructed servers keep their existing default ownership; the process transport borrows the ports into each server and owns final cleanup. A pending-owner set and a shared close attempt retain failed owners for retry without closing successful peers again. Startup failure also releases all loaded observation peers.

No tool schema or new provider capability. Scope decision: not applicable to this correction within existing resource authority. No extra execution, installation, target mutation, package metadata or performance claim.

Validation

Pinned main 42fc6c53ae188978deb3bedfe88287b6b316ba22: the strengthened probe/startup/retry regressions failed before production changes. Probe/replacement closure called shared closes twice; startup left two peers unclosed.
After the fix, all 13 tests in browserScenarioProbeLifecycle.test.ts and serverLifecycleMcp.test.ts passed. They exercise actual server construction/closure, all three borrowed ports, startup failure, concurrent cleanup with a denied peer, retry of only the failed peer, direct-server default ownership and the existing direct-server cleanup-failure retry workflow.
npm run check:fast, full npm run check, scoped formatting and git diff --check passed on Windows x64, Node 24.18.0 / npm 11.16.0.
Controlled providers and in-memory MCP clients do not establish real Chromium/Electron or external-client behavior. No full-suite or hosted CI success is claimed; current-head CI remains required. Other main cancellation/CLI cleanup regressions are separate focused PRs.
Related independent baseline fixes are #4356 and #4357. Combining all three locally passed 65 focused tests (two Windows-gated POSIX CLI cases skipped). The current hosted TShark/sigrok lanes fail the successful-retry CLI cases corrected by #4356; they are not attributed to this ownership fix or reported as passing.

Author readiness

Human owner: @geekwolf

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.

_{🔄 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/4358 **Author:** [@geekwolf](https://github.com/geekwolf) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/mcp-observation-provider-ownership` --- ### 📝 Commits (1) - [`12d6676`](https://github.com/morluto/rea/commit/12d6676975dd1902b40ba7be3e8b71c9a40feefa) fix(mcp): retain shared observation provider ownership ### 📊 Changes **3 files changed** (+180 additions, -37 deletions) <details> <summary>View changed files</summary> 📝 `src/main/transport.ts` (+34 -2) 📝 `src/server/createServer.ts` (+9 -3) 📝 `tests/boundary/process/browserScenarioProbeLifecycle.test.ts` (+137 -32) </details> ### 📄 Description ## Upstream coverage update — 2026-10-16 Main now independently covers this defect through #4274, commit 1b97b0f3707b1cf78ac98bd4ae81638b33cc286f. Its transport owns all three shared observation ports, deduplicates provider identity, retains failed cleanup for retry and preserves default direct-server ownership. Relevant main workflows were rechecked on 8ab5024397ddbd3b37ab68a13cabedc2eb7ce7dd: 53 tests passed, with two existing Windows-gated POSIX signal cases skipped. This draft remains open, not merged; its superseded option naming and implementation are not being replayed. Historical proposal and validation follow. --- ## Summary Keep process-shared observation providers alive when the MCP SDK closes a discovery probe or replaces a server. This fixes a regression introduced by 93c620c3 in the lifecycle originally addressed by #2195; no separate issue was opened. ## Problem and change The transport loads optional observation providers once and shares them across SDK-created servers. Each server also began closing the supplied providers, so discarding a probe permanently closed the shared browser scenario provider before the replacement could use it. Browser and passive Electron observation ports had the same duplicate ownership, while transport shutdown only closed the scenario port. Add an explicit ownership option for these three observation ports. Directly constructed servers keep their existing default ownership; the process transport borrows the ports into each server and owns final cleanup. A pending-owner set and a shared close attempt retain failed owners for retry without closing successful peers again. Startup failure also releases all loaded observation peers. No tool schema or new provider capability. Scope decision: not applicable to this correction within existing resource authority. No extra execution, installation, target mutation, package metadata or performance claim. ## Validation - Pinned main `42fc6c53ae188978deb3bedfe88287b6b316ba22`: the strengthened probe/startup/retry regressions failed before production changes. Probe/replacement closure called shared closes twice; startup left two peers unclosed. - After the fix, all 13 tests in `browserScenarioProbeLifecycle.test.ts` and `serverLifecycleMcp.test.ts` passed. They exercise actual server construction/closure, all three borrowed ports, startup failure, concurrent cleanup with a denied peer, retry of only the failed peer, direct-server default ownership and the existing direct-server cleanup-failure retry workflow. - `npm run check:fast`, full `npm run check`, scoped formatting and `git diff --check` passed on Windows x64, Node 24.18.0 / npm 11.16.0. - Controlled providers and in-memory MCP clients do not establish real Chromium/Electron or external-client behavior. No full-suite or hosted CI success is claimed; current-head CI remains required. Other main cancellation/CLI cleanup regressions are separate focused PRs. - Related independent baseline fixes are #4356 and #4357. Combining all three locally passed 65 focused tests (two Windows-gated POSIX CLI cases skipped). The current hosted TShark/sigrok lanes fail the successful-retry CLI cases corrected by #4356; they are not attributed to this ownership fix or reported as passing. ## Author readiness Human owner: @geekwolf - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:12:37 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:46:30 +00:00

[PR #6282] [MERGED] fix(ghidra): skip the Java probe without an installation #7254

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

No description provided.

Rows
Columns