[PR #4616] [CLOSED] fix(mcp): leave shared observation providers to the transport that loads them #6334

Closed
opened 2026-10-31 01:13:39 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4616
Author: @starterio-dotcom
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: claude/elegant-bartik-298d88


📝 Commits (1)

  • ae966ee fix(mcp): leave shared observation providers to the transport that loads them

📊 Changes

7 files changed (+302 additions, -104 deletions)

View changed files

📝 src/application/OptionalObservationProviders.ts (+4 -1)
📝 src/main/transport.ts (+0 -1)
📝 src/server/createServer.ts (+4 -9)
📝 tests/boundary/mcp/browserScenarioMcp.test.ts (+9 -88)
📝 tests/boundary/mcp/serverLifecycleMcp.test.ts (+7 -4)
📝 tests/boundary/process/browserScenarioProbeLifecycle.test.ts (+188 -1)
➕ tests/fixtures/browserScenarioCapture.ts (+90 -0)

📄 Description

Summary

capture_browser_scenario over MCP could fail for the rest of the server process with cleanup_incomplete / "Browser scenario provider is closing." even though no caller had closed anything. The process-shared Playwright scenario provider was being closed by a server instance that did not own it: when the MCP SDK's stdio entry discards its server/discover probe instance for a legacy initialize on the same connection, that instance's provider cleanup closed the shared port, and the pinned replacement instance then answered every scenario call with the closing error. #2205 fixed this for 6.5.0 and #4274 reintroduced a server-side close behind optionalObservationProviderOwner, whose default still closed injected ports. Refs #2195, #4274.

Problem and expected behavior

Trigger: one stdio connection that opens with a modern server/discover and continues with a legacy initialize, then calls capture_browser_scenario. Violated invariant: a resource is released only by its owner. startMcpTransport loads the optional observation ports once per process and hands the same instances to every server instance the SDK constructs, so a server instance must never close them. Expected: scenario calls on the pinned instance succeed, and the shared providers close exactly once at process shutdown.

Change and scope

  • createServer no longer closes the injected browser, browser-scenario, or Electron observation ports; the optionalObservationProviderOwner option is removed because the server never constructs those ports and therefore never owns them.
  • startMcpTransport remains the single owner and closes them once at process shutdown (unchanged behavior, no longer dependent on a flag).
  • The ownership rule is stated on OptionalObservationProviders.
  • The CLI path (capture-browser-scenario) is unchanged: it still creates and closes its own provider per command.

Not included: the Windows run-exclusive wrapper's dependency on the packaged native addon, which surfaced while validating this change, is handled separately.

Contribution scope: ordinary fix within existing lifecycle boundaries; maintainer scope decision not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: startMcpTransport (src/main/transport.ts) owns the loaded optional observation ports; createServer (src/server/createServer.ts) drops its close branch for them.
  • Contribution scope fit and linked maintainer decision: ordinary fix; not applicable.
  • CLI and MCP/tool-catalog contract: unchanged. CreateServerOptions.optionalObservationProviderOwner is removed; it was an internal composition option, not a CLI or MCP contract.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details: shared observation providers close only at MCP process shutdown; a discarded probe instance closes only the services it bound itself.
  • Generated metadata, package, or installation impact: none.

Expected effects: lifecycle ownership only; this change adds no target interaction.

  • 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.

Evidence and regression coverage

  • Tests added or updated:
    • tests/boundary/process/browserScenarioProbeLifecycle.test.ts: new test that drives the production serveStdio entry over an in-memory wire through a modern server/discover probe, a legacy initialize, and a capture_browser_scenario call on the pinned instance, then asserts the provider closes exactly once at process shutdown.
    • tests/boundary/mcp/serverLifecycleMcp.test.ts: the bound-service shutdown test now asserts that an injected scenario port is never closed by server.close().
    • tests/fixtures/browserScenarioCapture.ts: the valid fake capture builder, shared by the scenario MCP tests.
  • Base reproduction or other evidence (executed): against a 6.4.0 build (main at eaf88aca), a raw stdio JSON-RPC script sending server/discover → legacy initialize → tools/call capture_browser_scenario returned:
    {"error":{"code":"cleanup_incomplete","category":"execution_failure","message":"Browser scenario provider is closing.",...,"details":{"operation":"capture_browser_scenario","reason":"cleanup_failed","cleanup":"incomplete","resources":[],"cleanup_reason":"Provider is closing","execution_failure":"unreadable_output"}}}
    With the legacy initialize only, the same call reached the provider. On this branch, temporarily restoring the server-side close of options.browserScenarioCapture made both probe tests fail (expected "vi.fn()" to not be called at all, but actually been called 1 times); with the change they pass.
  • User-visible CLI/MCP output: none changed.
  • Remaining proof gaps: the exact opening exchange Claude Code sent in the originally reported session was not captured; a direct tool call through the same 6.4.0 build in another session did not trip the failure, so the trigger depends on the client's handshake. The new test uses a fake scenario port, not a real browser.

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

  • npm run check:fast — typecheck and lint, 2 tasks successful.
  • npm run test:focused -- tests/boundary/process/browserScenarioProbeLifecycle.test.ts tests/boundary/mcp/serverLifecycleMcp.test.ts tests/boundary/mcp/browserScenarioMcp.test.ts tests/boundary/mcp/shutdown.test.ts tests/boundary/mcp/optionalProviderIsolation.test.ts src/server/shutdownRegistration.test.ts — 6 files, 29 tests passed (Windows 11, Node 24.21.0; the packaged Windows native artifact was restored from the rea-agents@6.5.0 tarball so the build step and SQLite lifecycle cases could run).
  • Runtime reproduction script against the 6.4.0 build — results quoted above.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. CreateServerOptions.optionalObservationProviderOwner is removed; no caller outside src/main/transport.ts passed it.
  • Real Hopper/Ghidra, browser, or OS coverage: no real browser run; the Playwright provider's own close semantics are unchanged.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none required; shared provider cleanup still happens once at process shutdown.

Author readiness

Human owner: @starterio-dotcom

  • 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.

🤖 Generated with Claude Code


🔄 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/4616 **Author:** [@starterio-dotcom](https://github.com/starterio-dotcom) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `claude/elegant-bartik-298d88` --- ### 📝 Commits (1) - [`ae966ee`](https://github.com/morluto/rea/commit/ae966ee238c6eb55c9b0519bf616ae49b960e75a) fix(mcp): leave shared observation providers to the transport that loads them ### 📊 Changes **7 files changed** (+302 additions, -104 deletions) <details> <summary>View changed files</summary> 📝 `src/application/OptionalObservationProviders.ts` (+4 -1) 📝 `src/main/transport.ts` (+0 -1) 📝 `src/server/createServer.ts` (+4 -9) 📝 `tests/boundary/mcp/browserScenarioMcp.test.ts` (+9 -88) 📝 `tests/boundary/mcp/serverLifecycleMcp.test.ts` (+7 -4) 📝 `tests/boundary/process/browserScenarioProbeLifecycle.test.ts` (+188 -1) ➕ `tests/fixtures/browserScenarioCapture.ts` (+90 -0) </details> ### 📄 Description ## Summary `capture_browser_scenario` over MCP could fail for the rest of the server process with `cleanup_incomplete` / "Browser scenario provider is closing." even though no caller had closed anything. The process-shared Playwright scenario provider was being closed by a server instance that did not own it: when the MCP SDK's stdio entry discards its `server/discover` probe instance for a legacy `initialize` on the same connection, that instance's provider cleanup closed the shared port, and the pinned replacement instance then answered every scenario call with the closing error. #2205 fixed this for 6.5.0 and #4274 reintroduced a server-side close behind `optionalObservationProviderOwner`, whose default still closed injected ports. Refs #2195, #4274. ## Problem and expected behavior Trigger: one stdio connection that opens with a modern `server/discover` and continues with a legacy `initialize`, then calls `capture_browser_scenario`. Violated invariant: a resource is released only by its owner. `startMcpTransport` loads the optional observation ports once per process and hands the same instances to every server instance the SDK constructs, so a server instance must never close them. Expected: scenario calls on the pinned instance succeed, and the shared providers close exactly once at process shutdown. ## Change and scope - `createServer` no longer closes the injected browser, browser-scenario, or Electron observation ports; the `optionalObservationProviderOwner` option is removed because the server never constructs those ports and therefore never owns them. - `startMcpTransport` remains the single owner and closes them once at process shutdown (unchanged behavior, no longer dependent on a flag). - The ownership rule is stated on `OptionalObservationProviders`. - The CLI path (`capture-browser-scenario`) is unchanged: it still creates and closes its own provider per command. Not included: the Windows `run-exclusive` wrapper's dependency on the packaged native addon, which surfaced while validating this change, is handled separately. Contribution scope: ordinary fix within existing lifecycle boundaries; maintainer scope decision not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: `startMcpTransport` (src/main/transport.ts) owns the loaded optional observation ports; `createServer` (src/server/createServer.ts) drops its close branch for them. - Contribution scope fit and linked maintainer decision: ordinary fix; not applicable. - CLI and MCP/tool-catalog contract: unchanged. `CreateServerOptions.optionalObservationProviderOwner` is removed; it was an internal composition option, not a CLI or MCP contract. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: shared observation providers close only at MCP process shutdown; a discarded probe instance closes only the services it bound itself. - Generated metadata, package, or installation impact: none. Expected effects: lifecycle ownership only; this change adds no target interaction. - [ ] 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. ## Evidence and regression coverage - Tests added or updated: - `tests/boundary/process/browserScenarioProbeLifecycle.test.ts`: new test that drives the production `serveStdio` entry over an in-memory wire through a modern `server/discover` probe, a legacy `initialize`, and a `capture_browser_scenario` call on the pinned instance, then asserts the provider closes exactly once at process shutdown. - `tests/boundary/mcp/serverLifecycleMcp.test.ts`: the bound-service shutdown test now asserts that an injected scenario port is never closed by `server.close()`. - `tests/fixtures/browserScenarioCapture.ts`: the valid fake capture builder, shared by the scenario MCP tests. - Base reproduction or other evidence (executed): against a 6.4.0 build (main at eaf88aca), a raw stdio JSON-RPC script sending `server/discover` → legacy `initialize` → `tools/call capture_browser_scenario` returned: `{"error":{"code":"cleanup_incomplete","category":"execution_failure","message":"Browser scenario provider is closing.",...,"details":{"operation":"capture_browser_scenario","reason":"cleanup_failed","cleanup":"incomplete","resources":[],"cleanup_reason":"Provider is closing","execution_failure":"unreadable_output"}}}` With the legacy `initialize` only, the same call reached the provider. On this branch, temporarily restoring the server-side close of `options.browserScenarioCapture` made both probe tests fail (`expected "vi.fn()" to not be called at all, but actually been called 1 times`); with the change they pass. - User-visible CLI/MCP output: none changed. - Remaining proof gaps: the exact opening exchange Claude Code sent in the originally reported session was not captured; a direct tool call through the same 6.4.0 build in another session did not trip the failure, so the trigger depends on the client's handshake. The new test uses a fake scenario port, not a real browser. 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 - `npm run check:fast` — typecheck and lint, 2 tasks successful. - `npm run test:focused -- tests/boundary/process/browserScenarioProbeLifecycle.test.ts tests/boundary/mcp/serverLifecycleMcp.test.ts tests/boundary/mcp/browserScenarioMcp.test.ts tests/boundary/mcp/shutdown.test.ts tests/boundary/mcp/optionalProviderIsolation.test.ts src/server/shutdownRegistration.test.ts` — 6 files, 29 tests passed (Windows 11, Node 24.21.0; the packaged Windows native artifact was restored from the rea-agents@6.5.0 tarball so the build step and SQLite lifecycle cases could run). - Runtime reproduction script against the 6.4.0 build — results quoted above. ## Compatibility, safety, and release - Breaking changes or migration steps: none. `CreateServerOptions.optionalObservationProviderOwner` is removed; no caller outside `src/main/transport.ts` passed it. - Real Hopper/Ghidra, browser, or OS coverage: no real browser run; the Playwright provider's own close semantics are unchanged. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none required; shared provider cleanup still happens once at process shutdown. ## Author readiness Human owner: @starterio-dotcom - [ ] 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:13:39 +00:00
Sign in to join this conversation.
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#6334
No description provided.