morluto/rea

Fork 0

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

[GH-ISSUE #5222] [Bug] MCP shutdown skips the JavaScript runtime provider close hook #3201

New issue

Closed

opened 2026-10-31 00:37:46 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

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

Owner

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

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

MCP or agent integration

Observed behavior

MCP shutdown completes without calling the loaded JavaScript runtime observation provider's close() hook. In the recorded composition-boundary run below, the real runner constructs one real server, the transport and browser peer close once, shutdown unregisters its handler, and runtime close is never called. A separate server-owned control calls the same real runtime provider's close once.

Reproduction

Use a source checkout at the tested revision below, with the pinned runtime on PATH and locked dependencies installed (npm ci for a fresh checkout). Save this complete test as tests/composition/verification-runtime-shutdown.test.ts:

import { afterAll, expect, it, vi } from "vitest";
import { mkdirSync, mkdtempSync, rmSync } from "node:fs";
import { resolve } from "node:path";
import { run } from "../../src/main.js";
import { createServer } from "../../src/server/createServer.js";
import { createJavaScriptRuntimeObservationProvider } from "../../src/composition/javascriptRuntimeObservation.js";
import { createBrowserObservationProvider } from "../../src/composition/browserObservation.js";
import { createTestBinarySession } from "../fixtures/binarySession.js";

mkdirSync(".cache", { recursive: true });
const root = mkdtempSync(resolve(".cache/verification-runtime-"));
const env = { HOME: `${root}/home`, XDG_CONFIG_HOME: `${root}/config`,
  XDG_CACHE_HOME: `${root}/cache`, REA_LOG_LEVEL: "silent" };
for (const path of [env.HOME, env.XDG_CONFIG_HOME, env.XDG_CACHE_HOME]) mkdirSync(path);
afterAll(() => rmSync(root, { recursive: true, force: true }));

it("closes a server-owned runtime provider (control)", async () => {
  const runtime = createJavaScriptRuntimeObservationProvider();
  const close = vi.spyOn(runtime, "close");
  const session = createTestBinarySession(() => { throw new Error("No target allowed"); });
  const server = createServer({ kind: "session", session }, {
    environment: env, javascriptRuntimeObservation: runtime,
  });
  try {
    await server.close();
    expect(close).toHaveBeenCalledTimes(1);
  } finally {
    await runtime.close?.();
    await session.close();
    close.mockRestore();
  }
});

it("closes a caller-owned runtime provider at MCP shutdown", async () => {
  const runtime = createJavaScriptRuntimeObservationProvider();
  const browser = createBrowserObservationProvider();
  const runtimeClose = vi.spyOn(runtime, "close");
  const browserClose = vi.spyOn(browser, "close");
  const completed = Promise.withResolvers<void>();
  let shutdown!: () => void;
  let servers = 0, transportCloses = 0;
  const stderr: string[] = [], exitCodes: number[] = [];
  try {
    const result = await run({
      env,
      loadOptionalProviders: async () => ({ javascriptRuntimeObservation: runtime, browserObservation: browser }),
      createServer: (source, options) => {
        expect(options?.optionalObservationProviderOwner).toBe("caller");
        expect(options?.javascriptRuntimeObservation).toBe(runtime);
        servers++;
        return createServer(source, options);
      },
      serve: (factory) => {
        const server = factory({ era: "legacy" });
        return { close: async () => { transportCloses++; await server.close(); } };
      },
      registerShutdown: (handler) => { shutdown = handler; return () => completed.resolve(); },
      writeStderr: (text) => { stderr.push(text); },
      setExitCode: (code) => { exitCodes.push(code); },
    });
    expect(result).toBe(0);
    expect(shutdown).toBeTypeOf("function");
    shutdown();
    await completed.promise;
    console.log(JSON.stringify({ result, servers, transportCloses,
      browserCloses: browserClose.mock.calls.length, runtimeCloses: runtimeClose.mock.calls.length,
      stderr, exitCodes }));
    expect(servers).toBe(1);
    expect(transportCloses).toBe(1);
    expect(browserClose).toHaveBeenCalledTimes(1);
    expect(stderr).toEqual([]);
    expect(exitCodes).toEqual([]);
    expect(runtimeClose).toHaveBeenCalledTimes(1);
  } finally {
    await runtime.close?.();
    await browser.close?.();
    runtimeClose.mockRestore();
    browserClose.mockRestore();
  }
});

From the repository root, run:

node node_modules/vitest/vitest.mjs run --project composition tests/composition/verification-runtime-shutdown.test.ts --maxWorkers=1

The reduced test ran once: the server-owned control passed and the MCP-shutdown test failed at its final runtime-close assertion. Spies call through to real close methods. The injected transport constructs a real server, and the unregister callback marks production shutdown completion. The fixture supplies isolated configuration directories and opens no target or provider client. Finalizers close the idle providers and remove the fixture directory even on failure. Remove the temporary test after collecting the result:

rm tests/composition/verification-runtime-shutdown.test.ts

Expected behavior

Invoke and await the runtime provider's shutdown hook before process-owned cleanup completes. The runtime guide promises retained-cleanup retry at MCP shutdown; the runtime port assigns that retry to close(). Closing an individual caller-owned server should continue to leave the shared provider open until process shutdown.

Impact and scope

The documented shutdown retry is omitted on current main. This reproduction establishes the missing invocation, not a retained live socket, leftover file, naturally occurring cleanup failure, exit delay, or post-exit persistence. NOT_ENOUGH_INFO: practical frequency and live-resource consequences. No supported MCP-client workaround was tested; manual fixture disposal is not a user-facing workaround.

REA version

Tested source checkout: 39275420ea18ad803eccfcc2702865b98ed36e44. In a pre-filing default-branch comparison, the affected shutdown/runtime code and contract documentation were byte-identical to the tested baseline despite an unrelated branch-head advance. Published release source compared: rea-agents-6.7.0 at 27dc42d10249f0410904a75c0997842eb03b931c was compared from source only: it has the same transport cleanup enumeration but no runtime-provider close hook or retained-cleanup shutdown promise. This report does not attribute the specific new-hook regression to that release, and no release runtime was executed.

Runtime and platform

Node.js 24.18.0, npm 11.16.0, locked Vitest 5.0.3; Darwin 27.0.0 arm64.

Target and artifact context

Idle production runtime and browser providers, explicit isolated environment, controlled stdio handle and shutdown callback. No analysis target, live Inspector endpoint, external API, or real Inspector cleanup failure was used.

Interface

MCP

The production runner/server/shutdown composition was executed; an end-to-end stdio-client session was not run.

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

The runner returned zero, while the Vitest command exited 1 with one passing and one failing test:

{"result":0,"servers":1,"transportCloses":1,"browserCloses":1,"runtimeCloses":0,"stderr":[],"exitCodes":[]}

AssertionError: expected "close" to be called 1 times, but got 0 times

All assertions before expect(runtimeClose).toHaveBeenCalledTimes(1) passed. The passing server-owned control rules out a non-callable hook or a spy that cannot observe it.

Typed outcome or recovery expected

Proposed restoration check: the same runner case must observe one runtime close before shutdown completion, alongside one browser close, one transport close, and the passing server-owned control. A controlled retained-cleanup failure should additionally verify that shutdown waits for retry and reports unresolved cleanup; that additional case has not been run.

Analysis or suspected cause

Source comparison shows the transport cleanup enumeration and ownership handoff omit javascriptRuntimeObservation from process-owned cleanup while injecting it into servers as caller-owned. The server ownership guard then correctly excludes it from per-server close. The runtime hook retries retained Inspector and Electron-file cleanup, but neither owner invokes it in this MCP process path.

Additional context

Related #5083 addressed cancellation during Inspector final close. Its resolution explicitly preserves caller-owned optional providers; this reproduction checks the process owner's later shutdown invocation, which is a distinct closure signal. No Android failure is needed.

Triage assessment

Impact level: P2
Assessment status: provisional
Impact basis: Process shutdown omits the documented retained-resource cleanup retry and completes without invoking the loaded owner. Consequential live-resource impact and frequency remain unmeasured.
Workaround status: unverified
Workaround basis: No supported user-facing way to request the omitted process-owned close was tested; server-owned control and manual fixture disposal do not establish a practical MCP-client workaround.

Originally created by @coygeek on GitHub (Oct 19, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5222 ## Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ## Area MCP or agent integration ## Observed behavior MCP shutdown completes without calling the loaded JavaScript runtime observation provider's `close()` hook. In the recorded composition-boundary run below, the real runner constructs one real server, the transport and browser peer close once, shutdown unregisters its handler, and runtime close is never called. A separate server-owned control calls the same real runtime provider's close once. ## Reproduction Use a source checkout at the tested revision below, with the pinned runtime on PATH and locked dependencies installed (`npm ci` for a fresh checkout). Save this complete test as `tests/composition/verification-runtime-shutdown.test.ts`: ```ts import { afterAll, expect, it, vi } from "vitest"; import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; import { resolve } from "node:path"; import { run } from "../../src/main.js"; import { createServer } from "../../src/server/createServer.js"; import { createJavaScriptRuntimeObservationProvider } from "../../src/composition/javascriptRuntimeObservation.js"; import { createBrowserObservationProvider } from "../../src/composition/browserObservation.js"; import { createTestBinarySession } from "../fixtures/binarySession.js"; mkdirSync(".cache", { recursive: true }); const root = mkdtempSync(resolve(".cache/verification-runtime-")); const env = { HOME: `${root}/home`, XDG_CONFIG_HOME: `${root}/config`, XDG_CACHE_HOME: `${root}/cache`, REA_LOG_LEVEL: "silent" }; for (const path of [env.HOME, env.XDG_CONFIG_HOME, env.XDG_CACHE_HOME]) mkdirSync(path); afterAll(() => rmSync(root, { recursive: true, force: true })); it("closes a server-owned runtime provider (control)", async () => { const runtime = createJavaScriptRuntimeObservationProvider(); const close = vi.spyOn(runtime, "close"); const session = createTestBinarySession(() => { throw new Error("No target allowed"); }); const server = createServer({ kind: "session", session }, { environment: env, javascriptRuntimeObservation: runtime, }); try { await server.close(); expect(close).toHaveBeenCalledTimes(1); } finally { await runtime.close?.(); await session.close(); close.mockRestore(); } }); it("closes a caller-owned runtime provider at MCP shutdown", async () => { const runtime = createJavaScriptRuntimeObservationProvider(); const browser = createBrowserObservationProvider(); const runtimeClose = vi.spyOn(runtime, "close"); const browserClose = vi.spyOn(browser, "close"); const completed = Promise.withResolvers<void>(); let shutdown!: () => void; let servers = 0, transportCloses = 0; const stderr: string[] = [], exitCodes: number[] = []; try { const result = await run({ env, loadOptionalProviders: async () => ({ javascriptRuntimeObservation: runtime, browserObservation: browser }), createServer: (source, options) => { expect(options?.optionalObservationProviderOwner).toBe("caller"); expect(options?.javascriptRuntimeObservation).toBe(runtime); servers++; return createServer(source, options); }, serve: (factory) => { const server = factory({ era: "legacy" }); return { close: async () => { transportCloses++; await server.close(); } }; }, registerShutdown: (handler) => { shutdown = handler; return () => completed.resolve(); }, writeStderr: (text) => { stderr.push(text); }, setExitCode: (code) => { exitCodes.push(code); }, }); expect(result).toBe(0); expect(shutdown).toBeTypeOf("function"); shutdown(); await completed.promise; console.log(JSON.stringify({ result, servers, transportCloses, browserCloses: browserClose.mock.calls.length, runtimeCloses: runtimeClose.mock.calls.length, stderr, exitCodes })); expect(servers).toBe(1); expect(transportCloses).toBe(1); expect(browserClose).toHaveBeenCalledTimes(1); expect(stderr).toEqual([]); expect(exitCodes).toEqual([]); expect(runtimeClose).toHaveBeenCalledTimes(1); } finally { await runtime.close?.(); await browser.close?.(); runtimeClose.mockRestore(); browserClose.mockRestore(); } }); ``` From the repository root, run: ```sh node node_modules/vitest/vitest.mjs run --project composition tests/composition/verification-runtime-shutdown.test.ts --maxWorkers=1 ``` The reduced test ran once: the server-owned control passed and the MCP-shutdown test failed at its final runtime-close assertion. Spies call through to real close methods. The injected transport constructs a real server, and the unregister callback marks production shutdown completion. The fixture supplies isolated configuration directories and opens no target or provider client. Finalizers close the idle providers and remove the fixture directory even on failure. Remove the temporary test after collecting the result: ```sh rm tests/composition/verification-runtime-shutdown.test.ts ``` ## Expected behavior Invoke and await the runtime provider's shutdown hook before process-owned cleanup completes. The [runtime guide](https://github.com/morluto/rea/blob/39275420ea18ad803eccfcc2702865b98ed36e44/docs/javascript-runtime-observation.md#L155-L162) promises retained-cleanup retry at MCP shutdown; the [runtime port](https://github.com/morluto/rea/blob/39275420ea18ad803eccfcc2702865b98ed36e44/src/application/javascript/JavaScriptRuntimeObservationPort.ts#L15-L18) assigns that retry to `close()`. Closing an individual caller-owned server should continue to leave the shared provider open until process shutdown. ## Impact and scope The documented shutdown retry is omitted on current main. This reproduction establishes the missing invocation, not a retained live socket, leftover file, naturally occurring cleanup failure, exit delay, or post-exit persistence. NOT_ENOUGH_INFO: practical frequency and live-resource consequences. No supported MCP-client workaround was tested; manual fixture disposal is not a user-facing workaround. ## REA version Tested source checkout: `39275420ea18ad803eccfcc2702865b98ed36e44`. In a pre-filing default-branch comparison, the affected shutdown/runtime code and contract documentation were byte-identical to the tested baseline despite an unrelated branch-head advance. Published release source compared: `rea-agents-6.7.0` at `27dc42d10249f0410904a75c0997842eb03b931c` was compared from source only: it has the same transport cleanup enumeration but no runtime-provider close hook or retained-cleanup shutdown promise. This report does not attribute the specific new-hook regression to that release, and no release runtime was executed. ## Runtime and platform Node.js 24.18.0, npm 11.16.0, locked Vitest 5.0.3; Darwin 27.0.0 arm64. ## Target and artifact context Idle production runtime and browser providers, explicit isolated environment, controlled stdio handle and shutdown callback. No analysis target, live Inspector endpoint, external API, or real Inspector cleanup failure was used. ## Interface MCP The production runner/server/shutdown composition was executed; an end-to-end stdio-client session was not run. ## Earliest failure stage Cleanup, cancellation, or lifecycle ## Relevant output or evidence The runner returned zero, while the Vitest command exited 1 with one passing and one failing test: ```json {"result":0,"servers":1,"transportCloses":1,"browserCloses":1,"runtimeCloses":0,"stderr":[],"exitCodes":[]} ``` ```text AssertionError: expected "close" to be called 1 times, but got 0 times ``` All assertions before `expect(runtimeClose).toHaveBeenCalledTimes(1)` passed. The passing server-owned control rules out a non-callable hook or a spy that cannot observe it. ## Typed outcome or recovery expected Proposed restoration check: the same runner case must observe one runtime close before shutdown completion, alongside one browser close, one transport close, and the passing server-owned control. A controlled retained-cleanup failure should additionally verify that shutdown waits for retry and reports unresolved cleanup; that additional case has not been run. ## Analysis or suspected cause Source comparison shows the [transport cleanup enumeration and ownership handoff](https://github.com/morluto/rea/blob/39275420ea18ad803eccfcc2702865b98ed36e44/src/main/transport.ts#L81-L128) omit `javascriptRuntimeObservation` from process-owned cleanup while injecting it into servers as caller-owned. The [server ownership guard](https://github.com/morluto/rea/blob/39275420ea18ad803eccfcc2702865b98ed36e44/src/server/createServer.ts#L401-L410) then correctly excludes it from per-server close. The [runtime hook](https://github.com/morluto/rea/blob/39275420ea18ad803eccfcc2702865b98ed36e44/src/inspector/V8InspectorProvider.ts#L91-L105) retries retained Inspector and Electron-file cleanup, but neither owner invokes it in this MCP process path. ## Additional context Related [#5083](https://github.com/morluto/rea/issues/5083) addressed cancellation during Inspector final close. Its resolution explicitly preserves caller-owned optional providers; this reproduction checks the process owner's later shutdown invocation, which is a distinct closure signal. No Android failure is needed. ## Triage assessment - Impact level: P2 - Assessment status: provisional - Impact basis: Process shutdown omits the documented retained-resource cleanup retry and completes without invoking the loaded owner. Consequential live-resource impact and frequency remain unmeasured. - Workaround status: unverified - Workaround basis: No supported user-facing way to request the omitted process-owned close was tested; server-owned control and manual fixture disposal do not establish a practical MCP-client workaround.

bsakaguchi closed this issue

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

bsakaguchi commented

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

Author

Owner

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

Fixed on main in 00dd959b8.

Root cause: the process owner maintained a separate four-provider cleanup list while the canonical optional-provider result contained five ports. The server correctly treated them as caller-owned, leaving no owner to invoke the JavaScript runtime hook at shutdown. Cleanup now derives its set from the typed ports after separating load-failure metadata; the original result, including failure reasons, still reaches the server. Successful peers are removed only after close completes, while failed peers remain available for retry.

Validation on Node24.18.0/npm11.16.0:

The new runner/server regression failed on baseline (runtime.close:0 calls), then passed. It covers caller-owned server closure, a controlled runtime close rejection, successful browser/transport cleanup, retry of only the unfinished runtime, shutdown completion, and exact forwarding of a failed peer's metadata.
19 focused tests passed across five files, including all five optional-provider isolation cases and the existing discovery-probe/replacement-server and startup-failure cleanup controls. The new regression was rerun after correcting its SDK factory typing/test lane.
npm run check, npm run docs:check and exact test discovery passed (1,641 files). Independent source review found no blocker.

A compiled runner probe with real idle browser/runtime providers and an injected SDK handle changed runtime close calls0→1. Three fresh-process medians before→after were0.516→0.526s elapsed,0.734→0.747s CPU,373.5→375.9MB sampled process-family RSS. Internal probe output stayed54bytes/18 estimated cl100k tokens; this is not MCP wire or agent context. Runs were nonalternating with OS caches retained; brief peaks may be missed. No performance improvement is claimed.

Coverage establishes invocation and retry through production composition. It does not establish a naturally occurring retained Inspector socket/file, active Inspector cleanup behavior, post-exit persistence, Linux/Windows behavior, or a full stdio-client shutdown. Adjacent Android retries were reviewed: its provider memoizes successful close, so no separate duplicate-cleanup effect was established.

Local evidence: .cache/codebase-audit/shutdown5222-{baseline-red,focused-final,final-regression,check-publish,docs,discovery}.log, shutdown5222-provenance.json, and shutdown5222-costs.json.

@morluto commented on GitHub (Oct 19, 2026): Fixed on main in [`00dd959b8`](https://github.com/morluto/rea/commit/00dd959b82bb6675e01f54ade1180e345c826fee). Root cause: the process owner maintained a separate four-provider cleanup list while the canonical optional-provider result contained five ports. The server correctly treated them as caller-owned, leaving no owner to invoke the JavaScript runtime hook at shutdown. Cleanup now derives its set from the typed ports after separating load-failure metadata; the original result, including failure reasons, still reaches the server. Successful peers are removed only after close completes, while failed peers remain available for retry. Validation on Node24.18.0/npm11.16.0: - The new runner/server regression failed on baseline (`runtime.close`:0 calls), then passed. It covers caller-owned server closure, a controlled runtime close rejection, successful browser/transport cleanup, retry of only the unfinished runtime, shutdown completion, and exact forwarding of a failed peer's metadata. - **19 focused tests passed across five files**, including all five optional-provider isolation cases and the existing discovery-probe/replacement-server and startup-failure cleanup controls. The new regression was rerun after correcting its SDK factory typing/test lane. - `npm run check`, `npm run docs:check` and exact test discovery passed (1,641 files). Independent source review found no blocker. A compiled runner probe with real idle browser/runtime providers and an injected SDK handle changed runtime close calls0→1. Three fresh-process medians before→after were0.516→0.526s elapsed,0.734→0.747s CPU,373.5→375.9MB sampled process-family RSS. Internal probe output stayed54bytes/18 estimated cl100k tokens; this is not MCP wire or agent context. Runs were nonalternating with OS caches retained; brief peaks may be missed. No performance improvement is claimed. Coverage establishes invocation and retry through production composition. It does **not** establish a naturally occurring retained Inspector socket/file, active Inspector cleanup behavior, post-exit persistence, Linux/Windows behavior, or a full stdio-client shutdown. Adjacent Android retries were reviewed: its provider memoizes successful close, so no separate duplicate-cleanup effect was established. Local evidence: `.cache/codebase-audit/shutdown5222-{baseline-red,focused-final,final-regression,check-publish,docs,discovery}.log`, `shutdown5222-provenance.json`, and `shutdown5222-costs.json`.

bsakaguchi referenced this issue

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

[GH-ISSUE #5467] [Bug] Managed/native verification treats an empty normalized module identity as known #3322

bsakaguchi referenced this issue

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

[GH-ISSUE #5468] [Bug] Managed/native verification rejects inspection Evidence with an empty P/Invoke import name #3325