Skip to content

Follow-ups from the #734 review: exit-hold observability, held-exit simplification, terminal-state predicate consolidation, CI-run JS exitCode test #735

Description

@debugmcpdev

Follow-ups the review of #734 (fixes for #720, #729, #731) surfaced and that were deliberately left out of that PR. None is a user-visible defect today; each is a latent edge or a maintenance trap.

1. isRunning() is false during the exit hold while the session is still RUNNING/PAUSED

ProxyProcessAdapter (src/implementations/process-launcher-impl.ts) now records _exitCode/_signalCode at the raw child exit and holds the wrapper exit until the IPC channel reports EOF (disconnect/close, capped at 1 s). For that window ProxyManager.isRunning() is false and stop()'s hasExited() is true while session.state is still RUNNING/PAUSED, so:

  • a tool call landing in the window gets Cannot <op>: no active proxy for session <id> while list_debug_sessions still reports running/paused and exposure vanishes from the listing (session-store.ts gates it on isRunning());
  • stop() resolves via hasExited() without waiting, runs cleanup() (which resets exitEmitted and nulls proxyProcess but does not detach tracked listeners), and the held exit then re-runs handleProxyExit and emits a second exit on the discarded manager. Benign today only because every stop() caller detaches the session handlers first and managers are never reused.

The window is normally sub-millisecond (the EOF follows the exit within the same poll on both platforms); a fix is either to set the exit fields at delivery or to record the "stop() callers must detach first" invariant next to cleanup().

2. The held-exit machinery could be a one-bit flag delivered from close

The review's probes (win32 and Linux, including a forced exit-first ordering with a 1.2 s stall, and a detached adapter-shaped grandchild alive for 3 s) never saw the 1 s cap fire and never saw close precede the last forwarded message: Node's close accounting counts the IPC slot and the channel EOF path emits disconnect synchronously before its maybeClose. If that holds, "if connected at the raw exit, set a pending flag and emit exit from closeHandler" needs no timer and no disconnect listener bookkeeping. The cap was kept deliberately as a safety net; revisit once the ordering has been stable in CI for a while, and record the invariant that a future worker-side spawn with stdio: 'inherit' would delay the exit report until that grandchild dies.

3. Five spellings of "the session is over"

isTerminalSessionState() now exists in @debugmcp/shared (next to mapLegacyState) and is used by carriesLastStop, the auto-continue erase and the continue() revert guard. execution-tools.ts SESSION_OVER, execution-controller.ts (two inline STOPPED || ERROR sites), launch-readiness.ts and session-manager-core.ts handleExit still spell it themselves; mapLegacyState cannot serve because ERROR maps to lifecycle ACTIVE. Consolidate on the predicate.

4. No CI-run assertion on exitCode for a JavaScript launch

The nested-server exitCode: 7 contract (#731) is asserted only by tests/e2e/mcp-server-self-debug.test.ts, which no CI job runs (#732), and the only CI-run JavaScript integration test asserts launch config only. A js-debug vendor bump that changes its env-overlay semantics would break exit-code reporting with the pure-function unit tests green. Add an integration-project test that launches a script through the JavaScript adapter to completion and asserts the synthesized exitCode, or get the e2e project into CI (#732).

Refs #734, #729, #731, #720, #732, #692.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions