morluto/rea

Fork 0

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

[PR #4884] [CLOSED] fix(mcp): close the session when a signal interrupts startup #6455

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:14:34 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4884
Author: @Adolanium
Created: 10/17/2026
Status: ❌ Closed

Base: main ← Head: fix/startup-signal-cleanup

📝 Commits (10+)

c84d11e fix(mcp): close the session when a signal interrupts startup
f6fa42a fix(mcp): keep startup failures and failed shutdown owners when a request races startup
16ea307 Merge remote-tracking branch 'upstream/main' into fix/startup-signal-cleanup
5f1c9ff Merge remote-tracking branch 'upstream/main' into fix/startup-signal-cleanup
4b7b302 fix(mcp): keep provider documents when a shutdown signal cancels the initial open
8075ad4 fix(mcp): start no transport after a shutdown request during provider loading
10c87a6 fix(mcp): keep a reported shutdown failure as the startup outcome
f91dffe Merge remote-tracking branch 'upstream/main' into fix/startup-signal-cleanup
cf5a537 fix(mcp): recognize a cancelled startup open that reports its own cleanup
28cb1d3 fix(mcp): retain startup cleanup retry ownership

📊 Changes

12 files changed (+1084 additions, -140 deletions)

View changed files

📝 src/application/binary/BinarySession.ts (+32 -19)
📝 src/application/binary/BinarySessionOpen.ts (+6 -0)
📝 src/domain/analysisErrorProjection.ts (+11 -0)
📝 src/main/eagerRuntime.ts (+52 -28)
📝 src/main/shutdown.ts (+205 -54)
📝 src/main/startup.ts (+29 -5)
📝 src/main/transport.ts (+11 -0)
📝 src/main/types.ts (+1 -0)
📝 src/server/shutdownRegistration.test.ts (+22 -17)
📝 tests/boundary/mcp/mainErrors.test.ts (+6 -1)
📝 tests/boundary/mcp/shutdown.test.ts (+19 -16)
➕ tests/boundary/mcp/startupShutdown.test.ts (+690 -0)

📄 Description

Summary

Refs #4885 (startup signal handling only; startup EOF remains open).

REA installed its SIGINT, SIGTERM, and stdin EOF handlers only after the initial target opened and the MCP transport started. A signal during a slow HOPPER_TARGET_PATH open used Node's default handler and ended REA without session.close(). REA now handles SIGINT and SIGTERM from session creation, aborts the initial open, and closes the session even when the transport never started. Stdin EOF during startup is still not observed (see Not included).

Problem and expected behavior

run() in src/main.ts documents that EOF and process signals close bridge resources. It called openInitialTarget (session.open(HOPPER_TARGET_PATH) with no signal) and startMcpTransport before createShutdown, which registers the handlers through registerProcessShutdown. Provider children are spawned with detached: platform !== "win32" in src/process/ProviderProcess.ts, so a default signal exit can leave them running after REA is gone.

Change and scope

createShutdown in src/main/shutdown.ts now takes only the session and registers the handlers at once. A shutdown request aborts its signal and closes the owners attached so far. attachTransport adds the stdio handle, Android, and optional-provider owners after the transport starts. finish waits for an in-flight attempt and closes any owner attached after the request. release unregisters the handlers when startup fails and has already closed its own resources, unless a shutdown attempt that raced the failure failed; then the handlers stay registered so a repeated request retries the retained owners.
The handlers stay registered until startup settles. A successful attempt during startup does not unregister them, so a repeated request while finish closes a late-attached transport joins that cleanup instead of reaching Node's default signal handling.
finish does not retry an attempt that already failed and was reported when no owner was attached after it. It returns 1 and keeps the handlers registered, so a repeated signal retries the retained owners, as after transport start.
A settled attempt now frees the attempt slot after success as well as failure. Closed owners are never closed again, so a repeated request after success is a no-op, and an owner attached later gets its own attempt.
openInitialTarget in src/main/startup.ts passes the shutdown signal to session.open. When the open fails because shutdown aborted it, it returns cancelled and leaves cleanup and reporting to the shutdown. It decides with isCancelledAnalysisError from src/domain/analysisErrorProjection.ts, which compares the primary error's own code, so a cancellation that also reports incomplete cleanup still counts, not from signal.aborted alone, so a failure that preceded the request is still reported as the startup failure. Its logger, stderr writer, and signal are now one options object.
runEager (in src/main/eagerRuntime.ts since #4424; run() before that) creates the shutdown owner right after the session. It returns the shutdown outcome (0, or 1 when cleanup failed and was reported) when a request arrives during the open or the transport start. If startup throws unexpectedly, run() closes the session through finish before it rethrows, so the handlers do not outlive a failed startup.
RuntimeDependencies.createSession lets tests supply the session. It defaults to createBinarySession.
A cancelled startup open keeps provider documents, as process shutdown does. BinarySessionOpenOptions.retainProviderDocumentsOnCancel makes #failedOpen retire the new client with retainDocument: true when the open's signal aborted and the open failed because it was cancelled, the same test openInitialTarget uses to classify a cancelled open. A provider failure that races the signal still closes what it launched. openInitialTarget sets it. Without it, an external Hopper application would get shutdown_document and close its GUI document, which normal MCP shutdown deliberately retains. A cancelled open_binary tool call and non-cancellation open failures keep their existing cleanup.
When a failed initial open leaves cleanup pending (28cb1d31a), startup finishes through finishFailedOpen. It retries with the failed open's own non-retaining close, passed explicitly on every attempt so a policy saved by a raced shutdown request cannot persist, so an externally launched Hopper document still closes, and keeps the handlers registered for later retries under the same policy. A shutdown request during the initial open aborts it but does not close the session yet: whileOpening leaves the session owner out of shutdown attempts until the open settles, since a close queued behind the open could otherwise replace a failed open's non-retaining policy and succeed before startup sees the failure. Startup then closes the session through finish, finishFailedOpen, or release. Each shutdown attempt first calls session.beginShutdown() (from #6411), so a request the server admitted before shutdown cannot reopen the session after its close: the late open fails as cancelled and starts no provider.
A reported shutdown failure stays the outcome. If an attempt fails and is reported during startup, and a later attempt that also closes a late-attached owner succeeds, finish still returns 1, so the entrypoint does not overwrite the failure with 0.
startMcpTransport takes the shutdown signal. If shutdown was requested while optional providers loaded, it does not call serve(). It returns the loaded providers' close as a shutdown owner, and finish closes them. McpTransportOwners.handle is optional for this case. A request during serve() itself still closes the transport that starts.
After merging #4955, the attached handle is McpServerHandle, and its prepareShutdown barrier runs before any owner closes. A failed barrier closes nothing, and the next request retries it. For rea mcp --http, the startup signal latch is released when the shutdown owner registers at session creation, so a signal during the initial open aborts it there too.

session.open already accepted a signal and checks it between lifecycle steps and passes it to the provider health call; the only change below BinarySession is the retention choice above.

Not included: stdin EOF during startup. Nothing reads process.stdin until the MCP transport starts, so the end and close listeners registered early do not fire for a client that disconnects during the initial open. Observing it would mean reading stdin before the transport exists and handing the buffered messages to the SDK, which changes stdin ownership and is left for a separate change. The optional-provider loaders take no signal, so a request during loading waits for that load to finish. Whether each real provider stops its startup promptly on abort was not checked.

Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the readiness and merge checklists
unchecked. Maintainers can help resolve open questions.

Contribution scope: not applicable. This is a fix within the existing lifecycle boundary.

Contract and boundary impact

Semantic owner and earliest changed stage: process shutdown owner createShutdown, now from session creation instead of after transport start.
Contribution scope fit and linked maintainer decision: not applicable; ordinary fix within existing boundaries.
CLI and MCP/tool-catalog contract: none.
Provider, bridge, target-format, or platform compatibility: none. Providers receive an abort signal during the initial open, as they already do for open_binary.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details (if applicable): SIGINT and SIGTERM during startup now abort the initial open and close the session with retainProviderDocuments: true. Stdin EOF is handled only after the transport starts, as before. A cancelled initial open no longer prints an "Initial target failed to open" error. REA exits 0 after a clean interrupted startup and 1 when cleanup fails.
Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

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/mcp/startupShutdown.test.ts drives run(). The first test blocks the provider health call of the initial open, sends a shutdown request, and checks that the open's signal is aborted. It sends a second request while client cleanup is blocked, then checks one client close, no transport start, one unregistration, no stderr output, exit code 0, and a closed session. The second test sends a request while optional providers load and blocks the close of the transport handle that starts afterwards. It checks that the handlers are still registered during that close, sends a second request there, and then checks one handle close, one unregistration, and that a later request does not close the handle again. The third test makes client cleanup fail during an interrupted open. It checks one reported failure, exit code 1, two client close attempts (the aborted open and the shutdown), and retained handlers; a repeated request then closes the client and unregisters. A parameterized test makes the provider health call fail with a timeout and sends a request while the failed open's client close is blocked. It checks exit code 1 and that the timeout message is reported. When the shutdown cleanup succeeds, the handlers unregister. When it fails, they stay registered, and a repeated request closes the client and unregisters. Reverting the cause check fails both cases; reverting the release retention fails the second. Two tests cover a failed open that races the shutdown request: in one the provider reports a timeout after the signal aborted, and in the other the failed open's first close fails and the shutdown close would then succeed. Both check exit code 1, no pending cleanup, and that no close used retainDocument: true. Reverting the cancellation check in #failedOpen fails the first; letting shutdown close the session during the open fails the second. Another test opens the session from inside serve after a raced request, and checks that the open is refused as cancelled, no client is created or closed, and the handle closes once; without the seal it fails. Main's two #6411 tests in tests/boundary/mcp/shutdown.test.ts now attach their handle through attachTransport and also fail without the seal. The last test makes session.open throw and checks that run() rethrows, closes the session once, and unregisters. tests/boundary/mcp/mainErrors.test.ts now also checks that a transport start failure unregisters the handlers. tests/boundary/mcp/shutdown.test.ts and src/server/shutdownRegistration.test.ts now attach the transport through attachTransport, with their retry and idempotence assertions unchanged.
Base reproduction or other evidence: executed the final tests with the upstream/main source. All four startupShutdown tests failed: three timed out because no handler was registered during startup (on that base createSession does not exist, so the real session was used), and the throw test saw no unregistration. The mainErrors transport start failure test also failed, because no handler was registered before the transport started. The default-signal exit and the orphaned provider children were not reproduced at runtime; that part is source-derived.
User-visible CLI/MCP output (if applicable): an interrupted startup with clean cleanup prints nothing. A failed cleanup prints REA could not close cleanly. End the REA process before starting it again. as before.
Remaining proof gaps: no real provider was opened or signalled. Real OS signals were not sent and no real stdin pipe was closed; the tests call the registered handler. Stdin EOF during startup is not handled. startMcpTransport is not cancellable.

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

At e49b50bb8 (upstream/main 4c547008c merged, integrating #6411's shutdown seal) on Linux (WSL Ubuntu, Node 24.18.0), after npm run build:cached: npx tsc -p . --noEmit passed and oxfmt --check was clean; oxlint reported the same count as at c04f289c6. The lifecycle, deferred-runtime, session and BinarySession tests plus browserCli.test.ts passed 34 files and 372 tests in two runs. With the session.beginShutdown() call removed, the converted startup test and main's two seal tests failed.
At c04f289c6 (upstream/main ed069bb9d merged) on Linux (WSL Ubuntu, Node 24.18.0), after npm run build:cached: npx tsc -p . --noEmit passed and oxfmt --check was clean. oxlint reported the same warnings as main on the shared changed files, and none in startupShutdown.test.ts. The lifecycle, deferred-runtime, session and BinarySession tests, plus tests/boundary/cli/browserCli.test.ts (which failed in CI on fe1275626 against older main), passed 34 files and 370 tests in two runs.
At fe1275626 on Linux (WSL Ubuntu, Node 24.18.0): npx tsc -p . --noEmit passed, oxfmt --check was clean, and oxlint reported the same 11 warnings as 83586cade. The same test set passed 33 files and 356 tests in two runs. The negative check above failed the new test.
At 83586cade on Linux (WSL Ubuntu, Node 24.18.0): npx tsc -p . --noEmit passed, oxfmt --check was clean, and oxlint reported the same 11 warnings as a95b470b7. The same test set passed 33 files and 355 tests in two runs. Removing the re-arm in attachTransport failed the new reopened-session test (0 client closes instead of 1).
At a95b470b7 on Linux (WSL Ubuntu, Node 24.18.0), after npm run build:cached: npx tsc -p . --noEmit passed. oxfmt --check was clean. oxlint on the four touched files reported the same 11 warnings as d929ac01e (the BinarySession.ts file-length warning is now 1327 lines). The lifecycle, deferred-runtime, session, and BinarySession tests passed 33 files and 354 tests in two runs. Each of the two reversions above failed its regression test.
At 9249971b1 on Linux (WSL Ubuntu, Node 24.18.0): npx tsc -p . --noEmit passed, and oxfmt and oxlint were clean. The lifecycle, deferred-runtime, session, and BinarySession tests passed 30 files and 322 tests in two runs. With the session close option omitted again for the non-retaining policy, the raced-shutdown failed-open test failed.
After merging main again (316180b4e; #4424 moved startup into src/main/eagerRuntime.ts, which now holds this PR's lifecycle and hands lifecycle.shutdown to captureShutdown) on Linux (WSL Ubuntu, Node 24.18.0): npx tsc -p . --noEmit passed, oxfmt was clean, and oxlint reported the same single warning as main. tests/boundary/mcp, src/main, src/server/http, src/server/shutdownRegistration.test.ts, tests/composition/analysis-sessions, and src/application/binary passed 1420 tests in two runs. After npm run mcp-catalog:generate, the catalog-dependent contractPresentation, deferredRuntime, and toolSchemaValidity, together with mcpStartupPolicy, startupShutdown, and shutdown, passed 68 tests in two runs.
At e09a158ee on Linux (WSL Ubuntu, Node 24.18.0): npx tsc -p . --noEmit passed. oxfmt and oxlint were clean on the changed files. The lifecycle tests, tests/composition/analysis-sessions, and src/application/binary passed 28 files and 279 tests in two runs. With run() calling finish() instead of finishFailedOpen(), the failed-open retry test failed on its no-retainDocument assertion.
At cf5a53729 on Linux (WSL Ubuntu, Node 24.18.0): npx tsc -p . --noEmit passed. oxfmt was clean. oxlint reported the same 4 warnings as the parent commit. The lifecycle tests, src/domain, src/application/binary, and tests/composition/analysis-sessions passed 297 files and 5677 tests in two runs. With the cancellation check reverted to the projected code, the new test for a cancellation that carries its own cleanup observation failed.
After merging main again (f91dffede; the #failedOpen conflict is described in the PR comments) on Linux (WSL Ubuntu, Node 24.18.0), after npm ci and npm run build:cached: npx tsc -p . --noEmit passed and oxfmt was clean. oxlint on the five touched files reported the same 12 warnings as main. The lifecycle tests, tests/composition/analysis-sessions, src/application/binary, the Hopper cleanupRetry and externalExit client tests, and the HTTP serve tests passed 32 files and 361 tests in two runs.
At 10c87a6d7 on Linux (WSL Ubuntu, Node 24.18.0): npx tsc -p . --noEmit passed, and oxfmt and oxlint were clean on the changed files. The same 9 lifecycle and HTTP test files passed 112 tests in two runs. With the sticky failure flag removed, the new late-owner retry test failed.
At 8075ad4fc on Linux (WSL Ubuntu, Node 24.18.0): npx tsc -p . --noEmit passed. oxfmt --check was clean. oxlint reported only the existing max-lines-per-function warning on startMcpTransport. shutdownRegistration, mainErrors, shutdown, startupShutdown, serverLifecycleMcp, optionalProviderShutdown, mcpHttpOptions, serveStreamableHttp, and streamableHttpMcp passed 9 files and 111 tests in two runs. With the post-load signal check removed from startMcpTransport, the provider-loading startup test failed.
At 4b7b30284 on Linux (WSL Ubuntu, Node 24.18.0), after npm run build:cached: npx tsc -p . --noEmit passed. oxfmt --check was clean. oxlint on the four touched files reported only the 4 warnings already present on the parent commit. npx vitest run on shutdownRegistration, mainErrors, shutdown, startupShutdown, serverLifecycleMcp, tests/composition/analysis-sessions, the Hopper cleanupRetry and externalExit client tests, and src/application/binary passed 27 files and 269 tests. With retainProviderDocumentsOnCancel removed from openInitialTarget, the startup signal test failed: close options [{}] instead of [{ retainDocument: true }].
After merging upstream/main 60bd64647 (streamable HTTP transport, #4955) into the branch (5f1c9ffd9), on Linux (WSL Ubuntu, Node 24.18.0): npm run build:cached, npx tsc -p . --noEmit passed, and npx vitest run on shutdownRegistration, mainErrors, shutdown, startupShutdown, streamableHttpMcp, mcpHttpOptions, optionalProviderShutdown, serverLifecycleMcp, and serveStreamableHttp passed 110 of 110 tests in two runs. In the same loaded run on 60bd64647 alone, the streamableHttpMcp round-trip test failed twice out of two, so that failure predates this branch. It passes when run alone.
npx vitest run tests/boundary/mcp/startupShutdown.test.ts tests/boundary/mcp/shutdown.test.ts src/server/shutdownRegistration.test.ts tests/boundary/mcp/mainErrors.test.ts — 21 passed at f6fa42a83 (with dist/ built by npx tsc -p tsconfig.build.json). The new startupShutdown cases were also run with each fix reverted, failing as described above.
The same startupShutdown and mainErrors test files with the upstream/main source — 5 failed (4 startupShutdown tests and the mainErrors transport start failure test), as expected. A sixth failure, the compiled-runtime import test, was only because dist/ was not built at that point.
npx vitest run tests/boundary/mcp/serverLifecycleMcp.test.ts tests/boundary/mcp/optionalProviderIsolation.test.ts tests/process-global/mcpDeliveryEnvironment.test.ts tests/boundary/process/browserScenarioProbeLifecycle.test.ts — 13 passed, 4 failed, 2 skipped. The 2 SQLite stdio shutdown tests failed with missing packaged Windows native artifacts (native/windows/build/manifest.json). The 2 Electron isolation tests failed with an analyze_javascript_application error. The same 4 tests fail the same way on upstream/main on this host.
npm run check:fast (typecheck and lint) — passed.
npx tsc --noEmit --pretty false — no errors (before the review fixes).
npx oxlint on the changed source and test files — 0 warnings, 0 errors.
npx oxfmt --check on the changed source and test files — all formatted.

Compatibility, safety, and release

Breaking changes or migration steps: none for users. createShutdown and openInitialTarget are internal and their call sites are updated.
Real Hopper/Ghidra, browser, or OS coverage: not run. Windows 11 only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: a signal or EOF during startup now goes through the owned session cleanup instead of a default exit that can leave detached provider children running.

Author readiness

Human owner: @Adolanium

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.

Maintainer blocker verification

A failed initial open that retains cleanup ownership now calls finishFailedOpen(), which switches the session owner to non-retaining cleanup before both the current retry and later signal retries. Failed cleanup keeps the exact owner and registered signal handlers until retry succeeds. Direct review of e09a158 confirmed this call is wired in main.ts; the current regression records underlying close options and asserts that no failed-open retry uses retainDocument: true.

The focused startup-shutdown boundary suite passed all 10 tests and npm run check:fast passed on Node 24.18.0/npm 11.16.0. These are lifecycle fixtures, not a real-provider run.

Startup stdin EOF remains unimplemented: a failed initial open returns before MCP transport begins reading stdin. This PR therefore covers startup signals and their cleanup, and does not fully close #4885's EOF criterion.

_{🔄 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/4884 **Author:** [@Adolanium](https://github.com/Adolanium) **Created:** 10/17/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/startup-signal-cleanup` --- ### 📝 Commits (10+) - [`c84d11e`](https://github.com/morluto/rea/commit/c84d11e52fd78694eb22788a5b364e02bdd0f2e6) fix(mcp): close the session when a signal interrupts startup - [`f6fa42a`](https://github.com/morluto/rea/commit/f6fa42a83bccd7b29d176ba9be9983eccf57fda9) fix(mcp): keep startup failures and failed shutdown owners when a request races startup - [`16ea307`](https://github.com/morluto/rea/commit/16ea307f014b258cf95397d62b178ded03143a66) Merge remote-tracking branch 'upstream/main' into fix/startup-signal-cleanup - [`5f1c9ff`](https://github.com/morluto/rea/commit/5f1c9ffd9c86d99f5379b2d2544362022543509b) Merge remote-tracking branch 'upstream/main' into fix/startup-signal-cleanup - [`4b7b302`](https://github.com/morluto/rea/commit/4b7b30284a8d1988f47bdb015e94def0643769ca) fix(mcp): keep provider documents when a shutdown signal cancels the initial open - [`8075ad4`](https://github.com/morluto/rea/commit/8075ad4fc65bce0e66ca12d2025a942573dc70dd) fix(mcp): start no transport after a shutdown request during provider loading - [`10c87a6`](https://github.com/morluto/rea/commit/10c87a6d78413ba31bdcd53f864f6daf4d449925) fix(mcp): keep a reported shutdown failure as the startup outcome - [`f91dffe`](https://github.com/morluto/rea/commit/f91dffedef92b7f2ff0f51f400af1244a0d4fb0a) Merge remote-tracking branch 'upstream/main' into fix/startup-signal-cleanup - [`cf5a537`](https://github.com/morluto/rea/commit/cf5a5372980b5a2f59baf53b553c1e2c96409b21) fix(mcp): recognize a cancelled startup open that reports its own cleanup - [`28cb1d3`](https://github.com/morluto/rea/commit/28cb1d31a21f7f6f2d4a8b6687d158578b4f42b6) fix(mcp): retain startup cleanup retry ownership ### 📊 Changes **12 files changed** (+1084 additions, -140 deletions) <details> <summary>View changed files</summary> 📝 `src/application/binary/BinarySession.ts` (+32 -19) 📝 `src/application/binary/BinarySessionOpen.ts` (+6 -0) 📝 `src/domain/analysisErrorProjection.ts` (+11 -0) 📝 `src/main/eagerRuntime.ts` (+52 -28) 📝 `src/main/shutdown.ts` (+205 -54) 📝 `src/main/startup.ts` (+29 -5) 📝 `src/main/transport.ts` (+11 -0) 📝 `src/main/types.ts` (+1 -0) 📝 `src/server/shutdownRegistration.test.ts` (+22 -17) 📝 `tests/boundary/mcp/mainErrors.test.ts` (+6 -1) 📝 `tests/boundary/mcp/shutdown.test.ts` (+19 -16) ➕ `tests/boundary/mcp/startupShutdown.test.ts` (+690 -0) </details> ### 📄 Description ## Summary Refs #4885 (startup signal handling only; startup EOF remains open). REA installed its SIGINT, SIGTERM, and stdin EOF handlers only after the initial target opened and the MCP transport started. A signal during a slow `HOPPER_TARGET_PATH` open used Node's default handler and ended REA without `session.close()`. REA now handles SIGINT and SIGTERM from session creation, aborts the initial open, and closes the session even when the transport never started. Stdin EOF during startup is still not observed (see Not included). ## Problem and expected behavior `run()` in `src/main.ts` documents that EOF and process signals close bridge resources. It called `openInitialTarget` (`session.open(HOPPER_TARGET_PATH)` with no signal) and `startMcpTransport` before `createShutdown`, which registers the handlers through `registerProcessShutdown`. Provider children are spawned with `detached: platform !== "win32"` in `src/process/ProviderProcess.ts`, so a default signal exit can leave them running after REA is gone. ## Change and scope - `createShutdown` in `src/main/shutdown.ts` now takes only the session and registers the handlers at once. A shutdown request aborts its `signal` and closes the owners attached so far. `attachTransport` adds the stdio handle, Android, and optional-provider owners after the transport starts. `finish` waits for an in-flight attempt and closes any owner attached after the request. `release` unregisters the handlers when startup fails and has already closed its own resources, unless a shutdown attempt that raced the failure failed; then the handlers stay registered so a repeated request retries the retained owners. - The handlers stay registered until startup settles. A successful attempt during startup does not unregister them, so a repeated request while `finish` closes a late-attached transport joins that cleanup instead of reaching Node's default signal handling. - `finish` does not retry an attempt that already failed and was reported when no owner was attached after it. It returns 1 and keeps the handlers registered, so a repeated signal retries the retained owners, as after transport start. - A settled attempt now frees the attempt slot after success as well as failure. Closed owners are never closed again, so a repeated request after success is a no-op, and an owner attached later gets its own attempt. - `openInitialTarget` in `src/main/startup.ts` passes the shutdown signal to `session.open`. When the open fails because shutdown aborted it, it returns `cancelled` and leaves cleanup and reporting to the shutdown. It decides with `isCancelledAnalysisError` from `src/domain/analysisErrorProjection.ts`, which compares the primary error's own code, so a cancellation that also reports incomplete cleanup still counts, not from `signal.aborted` alone, so a failure that preceded the request is still reported as the startup failure. Its logger, stderr writer, and signal are now one options object. - `runEager` (in `src/main/eagerRuntime.ts` since #4424; `run()` before that) creates the shutdown owner right after the session. It returns the shutdown outcome (0, or 1 when cleanup failed and was reported) when a request arrives during the open or the transport start. If startup throws unexpectedly, `run()` closes the session through `finish` before it rethrows, so the handlers do not outlive a failed startup. - `RuntimeDependencies.createSession` lets tests supply the session. It defaults to `createBinarySession`. - A cancelled startup open keeps provider documents, as process shutdown does. `BinarySessionOpenOptions.retainProviderDocumentsOnCancel` makes `#failedOpen` retire the new client with `retainDocument: true` when the open's signal aborted and the open failed because it was cancelled, the same test `openInitialTarget` uses to classify a cancelled open. A provider failure that races the signal still closes what it launched. `openInitialTarget` sets it. Without it, an external Hopper application would get `shutdown_document` and close its GUI document, which normal MCP shutdown deliberately retains. A cancelled `open_binary` tool call and non-cancellation open failures keep their existing cleanup. - When a failed initial open leaves cleanup pending (28cb1d31a), startup finishes through `finishFailedOpen`. It retries with the failed open's own non-retaining close, passed explicitly on every attempt so a policy saved by a raced shutdown request cannot persist, so an externally launched Hopper document still closes, and keeps the handlers registered for later retries under the same policy. A shutdown request during the initial open aborts it but does not close the session yet: `whileOpening` leaves the session owner out of shutdown attempts until the open settles, since a close queued behind the open could otherwise replace a failed open's non-retaining policy and succeed before startup sees the failure. Startup then closes the session through `finish`, `finishFailedOpen`, or `release`. Each shutdown attempt first calls `session.beginShutdown()` (from #6411), so a request the server admitted before shutdown cannot reopen the session after its close: the late open fails as cancelled and starts no provider. - A reported shutdown failure stays the outcome. If an attempt fails and is reported during startup, and a later attempt that also closes a late-attached owner succeeds, `finish` still returns 1, so the entrypoint does not overwrite the failure with 0. - `startMcpTransport` takes the shutdown signal. If shutdown was requested while optional providers loaded, it does not call `serve()`. It returns the loaded providers' close as a shutdown owner, and `finish` closes them. `McpTransportOwners.handle` is optional for this case. A request during `serve()` itself still closes the transport that starts. - After merging #4955, the attached handle is `McpServerHandle`, and its `prepareShutdown` barrier runs before any owner closes. A failed barrier closes nothing, and the next request retries it. For `rea mcp --http`, the startup signal latch is released when the shutdown owner registers at session creation, so a signal during the initial open aborts it there too. `session.open` already accepted a signal and checks it between lifecycle steps and passes it to the provider `health` call; the only change below `BinarySession` is the retention choice above. Not included: stdin EOF during startup. Nothing reads `process.stdin` until the MCP transport starts, so the `end` and `close` listeners registered early do not fire for a client that disconnects during the initial open. Observing it would mean reading stdin before the transport exists and handing the buffered messages to the SDK, which changes stdin ownership and is left for a separate change. The optional-provider loaders take no signal, so a request during loading waits for that load to finish. Whether each real provider stops its startup promptly on abort was not checked. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". Draft PRs may leave the scope decision pending and the readiness and merge checklists unchecked. Maintainers can help resolve open questions. Contribution scope: not applicable. This is a fix within the existing lifecycle boundary. ## Contract and boundary impact - Semantic owner and earliest changed stage: process shutdown owner `createShutdown`, now from session creation instead of after transport start. - Contribution scope fit and linked maintainer decision: not applicable; ordinary fix within existing boundaries. - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: none. Providers receive an abort signal during the initial open, as they already do for `open_binary`. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details (if applicable): SIGINT and SIGTERM during startup now abort the initial open and close the session with `retainProviderDocuments: true`. Stdin EOF is handled only after the transport starts, as before. A cancelled initial open no longer prints an "Initial target failed to open" error. REA exits 0 after a clean interrupted startup and 1 when cleanup fails. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [ ] Read existing files or captured data. - [x] 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/mcp/startupShutdown.test.ts` drives `run()`. The first test blocks the provider `health` call of the initial open, sends a shutdown request, and checks that the open's signal is aborted. It sends a second request while client cleanup is blocked, then checks one client close, no transport start, one unregistration, no stderr output, exit code 0, and a closed session. The second test sends a request while optional providers load and blocks the close of the transport handle that starts afterwards. It checks that the handlers are still registered during that close, sends a second request there, and then checks one handle close, one unregistration, and that a later request does not close the handle again. The third test makes client cleanup fail during an interrupted open. It checks one reported failure, exit code 1, two client close attempts (the aborted open and the shutdown), and retained handlers; a repeated request then closes the client and unregisters. A parameterized test makes the provider `health` call fail with a timeout and sends a request while the failed open's client close is blocked. It checks exit code 1 and that the timeout message is reported. When the shutdown cleanup succeeds, the handlers unregister. When it fails, they stay registered, and a repeated request closes the client and unregisters. Reverting the cause check fails both cases; reverting the `release` retention fails the second. Two tests cover a failed open that races the shutdown request: in one the provider reports a timeout after the signal aborted, and in the other the failed open's first close fails and the shutdown close would then succeed. Both check exit code 1, no pending cleanup, and that no close used `retainDocument: true`. Reverting the cancellation check in `#failedOpen` fails the first; letting shutdown close the session during the open fails the second. Another test opens the session from inside `serve` after a raced request, and checks that the open is refused as cancelled, no client is created or closed, and the handle closes once; without the seal it fails. Main's two #6411 tests in `tests/boundary/mcp/shutdown.test.ts` now attach their handle through `attachTransport` and also fail without the seal. The last test makes `session.open` throw and checks that `run()` rethrows, closes the session once, and unregisters. `tests/boundary/mcp/mainErrors.test.ts` now also checks that a transport start failure unregisters the handlers. `tests/boundary/mcp/shutdown.test.ts` and `src/server/shutdownRegistration.test.ts` now attach the transport through `attachTransport`, with their retry and idempotence assertions unchanged. - Base reproduction or other evidence: executed the final tests with the upstream/main source. All four `startupShutdown` tests failed: three timed out because no handler was registered during startup (on that base `createSession` does not exist, so the real session was used), and the throw test saw no unregistration. The `mainErrors` transport start failure test also failed, because no handler was registered before the transport started. The default-signal exit and the orphaned provider children were not reproduced at runtime; that part is source-derived. - User-visible CLI/MCP output (if applicable): an interrupted startup with clean cleanup prints nothing. A failed cleanup prints `REA could not close cleanly. End the REA process before starting it again.` as before. - Remaining proof gaps: no real provider was opened or signalled. Real OS signals were not sent and no real stdin pipe was closed; the tests call the registered handler. Stdin EOF during startup is not handled. `startMcpTransport` is not cancellable. 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 - At e49b50bb8 (upstream/main 4c547008c merged, integrating #6411's shutdown seal) on Linux (WSL Ubuntu, Node 24.18.0), after `npm run build:cached`: `npx tsc -p . --noEmit` passed and `oxfmt --check` was clean; `oxlint` reported the same count as at c04f289c6. The lifecycle, deferred-runtime, session and BinarySession tests plus `browserCli.test.ts` passed 34 files and 372 tests in two runs. With the `session.beginShutdown()` call removed, the converted startup test and main's two seal tests failed. - At c04f289c6 (upstream/main ed069bb9d merged) on Linux (WSL Ubuntu, Node 24.18.0), after `npm run build:cached`: `npx tsc -p . --noEmit` passed and `oxfmt --check` was clean. `oxlint` reported the same warnings as main on the shared changed files, and none in `startupShutdown.test.ts`. The lifecycle, deferred-runtime, session and BinarySession tests, plus `tests/boundary/cli/browserCli.test.ts` (which failed in CI on fe1275626 against older main), passed 34 files and 370 tests in two runs. - At fe1275626 on Linux (WSL Ubuntu, Node 24.18.0): `npx tsc -p . --noEmit` passed, `oxfmt --check` was clean, and `oxlint` reported the same 11 warnings as 83586cade. The same test set passed 33 files and 356 tests in two runs. The negative check above failed the new test. - At 83586cade on Linux (WSL Ubuntu, Node 24.18.0): `npx tsc -p . --noEmit` passed, `oxfmt --check` was clean, and `oxlint` reported the same 11 warnings as a95b470b7. The same test set passed 33 files and 355 tests in two runs. Removing the re-arm in `attachTransport` failed the new reopened-session test (0 client closes instead of 1). - At a95b470b7 on Linux (WSL Ubuntu, Node 24.18.0), after `npm run build:cached`: `npx tsc -p . --noEmit` passed. `oxfmt --check` was clean. `oxlint` on the four touched files reported the same 11 warnings as d929ac01e (the `BinarySession.ts` file-length warning is now 1327 lines). The lifecycle, deferred-runtime, session, and BinarySession tests passed 33 files and 354 tests in two runs. Each of the two reversions above failed its regression test. - At 9249971b1 on Linux (WSL Ubuntu, Node 24.18.0): `npx tsc -p . --noEmit` passed, and `oxfmt` and `oxlint` were clean. The lifecycle, deferred-runtime, session, and BinarySession tests passed 30 files and 322 tests in two runs. With the session close option omitted again for the non-retaining policy, the raced-shutdown failed-open test failed. - After merging main again (316180b4e; #4424 moved startup into `src/main/eagerRuntime.ts`, which now holds this PR's lifecycle and hands `lifecycle.shutdown` to `captureShutdown`) on Linux (WSL Ubuntu, Node 24.18.0): `npx tsc -p . --noEmit` passed, `oxfmt` was clean, and `oxlint` reported the same single warning as main. `tests/boundary/mcp`, `src/main`, `src/server/http`, `src/server/shutdownRegistration.test.ts`, `tests/composition/analysis-sessions`, and `src/application/binary` passed 1420 tests in two runs. After `npm run mcp-catalog:generate`, the catalog-dependent `contractPresentation`, `deferredRuntime`, and `toolSchemaValidity`, together with `mcpStartupPolicy`, `startupShutdown`, and `shutdown`, passed 68 tests in two runs. - At e09a158ee on Linux (WSL Ubuntu, Node 24.18.0): `npx tsc -p . --noEmit` passed. `oxfmt` and `oxlint` were clean on the changed files. The lifecycle tests, `tests/composition/analysis-sessions`, and `src/application/binary` passed 28 files and 279 tests in two runs. With `run()` calling `finish()` instead of `finishFailedOpen()`, the failed-open retry test failed on its no-`retainDocument` assertion. - At cf5a53729 on Linux (WSL Ubuntu, Node 24.18.0): `npx tsc -p . --noEmit` passed. `oxfmt` was clean. `oxlint` reported the same 4 warnings as the parent commit. The lifecycle tests, `src/domain`, `src/application/binary`, and `tests/composition/analysis-sessions` passed 297 files and 5677 tests in two runs. With the cancellation check reverted to the projected code, the new test for a cancellation that carries its own cleanup observation failed. - After merging main again (f91dffede; the `#failedOpen` conflict is described in the PR comments) on Linux (WSL Ubuntu, Node 24.18.0), after `npm ci` and `npm run build:cached`: `npx tsc -p . --noEmit` passed and `oxfmt` was clean. `oxlint` on the five touched files reported the same 12 warnings as main. The lifecycle tests, `tests/composition/analysis-sessions`, `src/application/binary`, the Hopper `cleanupRetry` and `externalExit` client tests, and the HTTP serve tests passed 32 files and 361 tests in two runs. - At 10c87a6d7 on Linux (WSL Ubuntu, Node 24.18.0): `npx tsc -p . --noEmit` passed, and `oxfmt` and `oxlint` were clean on the changed files. The same 9 lifecycle and HTTP test files passed 112 tests in two runs. With the sticky failure flag removed, the new late-owner retry test failed. - At 8075ad4fc on Linux (WSL Ubuntu, Node 24.18.0): `npx tsc -p . --noEmit` passed. `oxfmt --check` was clean. `oxlint` reported only the existing `max-lines-per-function` warning on `startMcpTransport`. `shutdownRegistration`, `mainErrors`, `shutdown`, `startupShutdown`, `serverLifecycleMcp`, `optionalProviderShutdown`, `mcpHttpOptions`, `serveStreamableHttp`, and `streamableHttpMcp` passed 9 files and 111 tests in two runs. With the post-load signal check removed from `startMcpTransport`, the provider-loading startup test failed. - At 4b7b30284 on Linux (WSL Ubuntu, Node 24.18.0), after `npm run build:cached`: `npx tsc -p . --noEmit` passed. `oxfmt --check` was clean. `oxlint` on the four touched files reported only the 4 warnings already present on the parent commit. `npx vitest run` on `shutdownRegistration`, `mainErrors`, `shutdown`, `startupShutdown`, `serverLifecycleMcp`, `tests/composition/analysis-sessions`, the Hopper `cleanupRetry` and `externalExit` client tests, and `src/application/binary` passed 27 files and 269 tests. With `retainProviderDocumentsOnCancel` removed from `openInitialTarget`, the startup signal test failed: close options `[{}]` instead of `[{ retainDocument: true }]`. - After merging upstream/main 60bd64647 (streamable HTTP transport, #4955) into the branch (5f1c9ffd9), on Linux (WSL Ubuntu, Node 24.18.0): `npm run build:cached`, `npx tsc -p . --noEmit` passed, and `npx vitest run` on `shutdownRegistration`, `mainErrors`, `shutdown`, `startupShutdown`, `streamableHttpMcp`, `mcpHttpOptions`, `optionalProviderShutdown`, `serverLifecycleMcp`, and `serveStreamableHttp` passed 110 of 110 tests in two runs. In the same loaded run on 60bd64647 alone, the `streamableHttpMcp` round-trip test failed twice out of two, so that failure predates this branch. It passes when run alone. - `npx vitest run tests/boundary/mcp/startupShutdown.test.ts tests/boundary/mcp/shutdown.test.ts src/server/shutdownRegistration.test.ts tests/boundary/mcp/mainErrors.test.ts` — 21 passed at f6fa42a83 (with `dist/` built by `npx tsc -p tsconfig.build.json`). The new `startupShutdown` cases were also run with each fix reverted, failing as described above. - The same `startupShutdown` and `mainErrors` test files with the upstream/main source — 5 failed (4 `startupShutdown` tests and the `mainErrors` transport start failure test), as expected. A sixth failure, the compiled-runtime import test, was only because `dist/` was not built at that point. - `npx vitest run tests/boundary/mcp/serverLifecycleMcp.test.ts tests/boundary/mcp/optionalProviderIsolation.test.ts tests/process-global/mcpDeliveryEnvironment.test.ts tests/boundary/process/browserScenarioProbeLifecycle.test.ts` — 13 passed, 4 failed, 2 skipped. The 2 SQLite stdio shutdown tests failed with missing packaged Windows native artifacts (`native/windows/build/manifest.json`). The 2 Electron isolation tests failed with an `analyze_javascript_application` error. The same 4 tests fail the same way on upstream/main on this host. - `npm run check:fast` (typecheck and lint) — passed. - `npx tsc --noEmit --pretty false` — no errors (before the review fixes). - `npx oxlint` on the changed source and test files — 0 warnings, 0 errors. - `npx oxfmt --check` on the changed source and test files — all formatted. ## Compatibility, safety, and release - Breaking changes or migration steps: none for users. `createShutdown` and `openInitialTarget` are internal and their call sites are updated. - Real Hopper/Ghidra, browser, or OS coverage: not run. Windows 11 only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: a signal or EOF during startup now goes through the owned session cleanup instead of a default exit that can leave detached provider children running. ## Author readiness  Human owner: @Adolanium - [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  - [ ] 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.  ### Maintainer blocker verification A failed initial open that retains cleanup ownership now calls `finishFailedOpen()`, which switches the session owner to non-retaining cleanup before both the current retry and later signal retries. Failed cleanup keeps the exact owner and registered signal handlers until retry succeeds. Direct review of `e09a158` confirmed this call is wired in `main.ts`; the current regression records underlying close options and asserts that no failed-open retry uses `retainDocument: true`. The focused startup-shutdown boundary suite passed all 10 tests and `npm run check:fast` passed on Node 24.18.0/npm 11.16.0. These are lifecycle fixtures, not a real-provider run. Startup stdin EOF remains unimplemented: a failed initial open returns before MCP transport begins reading stdin. This PR therefore covers startup signals and their cleanup, and does not fully close #4885's EOF criterion. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:14:34 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:47:21 +00:00

[PR #6489] [MERGED] fix(doctor): report a missing --format value as MISSING_OUTPUT_VALUE #7367

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

No description provided.

Rows
Columns