Skip to content

Add watcher preopen controls and harden server shutdown#120

Open
isham703 wants to merge 1 commit into
isaacphi:mainfrom
isham703:codex/swift-mcp-memory-fix
Open

Add watcher preopen controls and harden server shutdown#120
isham703 wants to merge 1 commit into
isaacphi:mainfrom
isham703:codex/swift-mcp-memory-fix

Conversation

@isham703

@isham703 isham703 commented Mar 2, 2026

Copy link
Copy Markdown

Summary

This PR addresses Swift MCP memory/process buildup observed in large workspaces by adding opt-in runtime controls and hardening shutdown behavior, while preserving existing defaults.

Changes

  • Add runtime flags/env controls (default-preserving):
    • --watcher-preopen-on-register (default true)
    • --watcher-preopen-max-files (default 0 = unlimited)
    • --idle-timeout (default 0s = disabled)
  • Add watcher config fields:
    • PreopenOnRegistration bool
    • PreopenMaxFiles int
  • Gate registration-time preopen scans behind PreopenOnRegistration.
  • Enforce deterministic preopen cap via PreopenMaxFiles (counts successful opens only).
  • Add idle watchdog and activity tracking using MCP BeforeAny hook.
  • Make server cleanup idempotent (sync.Once) and ensure cleanup always runs when stdio loop exits.
  • Make Client.Close() idempotent in LSP client to avoid repeated wait/close races.
  • Update README with new flags/env and Codex-specific example.

Why

In practice, duplicated MCP stacks plus registration-triggered preopen scans can create high memory pressure and process accumulation. This change gives clients an opt-out/cap for preopen and adds reliable idle/process cleanup.

Compatibility

Defaults preserve current behavior globally, so existing clients are unaffected unless they opt in.

Tests

  • Added/updated unit coverage for:
    • preopen disabled => no registration scan opens
    • preopen enabled => matching files open
    • preopen max cap => bounded opens
    • repeated close calls => idempotent/no-op-safe
  • Ran package tests (non-integration) successfully.

Files

  • main.go
  • internal/watcher/interfaces.go
  • internal/watcher/watcher.go
  • internal/lsp/client.go
  • internal/lsp/client_close_test.go
  • internal/watcher/testing/mock_client.go
  • internal/watcher/testing/watcher_test.go
  • README.md

STRd6 pushed a commit to STRd6/mcp-language-server that referenced this pull request May 14, 2026
When the LSP registers workspace/didChangeWatchedFiles patterns, the
old behavior spawned a goroutine that walked the entire workspace and
sent didOpen for every matching file. The TODO said this was for
typescript-language-server, but empirically TS does not register
watchers at all (neither do gopls, clangd, civet-lsp). Only
rust-analyzer registers them today, and it indexes via cargo metadata
independently of didOpen state.

For each LSP in our integration-test matrix, all features (definition,
references, hover, rename, document symbols, diagnostics) work with the
scan removed. Tools that need a specific file already call OpenFile on
demand, so the LSP gets a didOpen for the files actually being queried.

Eliminates the symptom behind issue isaacphi#83 ("too many open files") and
the corresponding memory blowup on large rust workspaces, without
needing PR isaacphi#120's runtime flags.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
STRd6 pushed a commit to STRd6/mcp-language-server that referenced this pull request May 14, 2026
cleanup() in main.go can fire from three independent goroutines:
the signal handler (SIGINT/SIGTERM), the parent-death watcher, and
the main goroutine's error-exit path. Today each call independently
drives CloseAllFiles -> Shutdown -> Exit -> Close on the LSP client,
which produces two parallel teardowns when triggers fire close in
time -- duplicate didClose notifications, racing access to
s.lspClient, and occasional shutdown-after-exit log noise.

Wrap the body of cleanup in a package-level sync.Once and lift
lsp.Client.Close into a sync.Once.Do that caches its return value
in c.closeErr. The first caller runs the teardown; subsequent
callers (and the forthcoming idle watchdog) block on the Once
until it completes, then return the cached error without doing
the work twice.

Adapted from upstream PR isaacphi#120 (isham703).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
STRd6 pushed a commit to STRd6/mcp-language-server that referenced this pull request May 14, 2026
Catches the case where the parent editor (Claude Desktop, Cursor,
etc.) goes away without killing its MCP children -- the existing
parentDeath watcher only notices when the parent reparents to
PID 1, not when the parent stays alive but stops talking. With
--idle-timeout=10m the server arms a timer that resets on every
incoming MCP request via mcp-go's BeforeAny hook; if it fires the
server runs the same cleanup path as SIGTERM.

Default 0 keeps existing behavior (no timeout) so CI and
interactive sessions are unaffected. Combined with the sync.Once
cleanup guard, the idle path is just a third trigger of the same
teardown -- not a new shutdown sequence.

Adapted from upstream PR isaacphi#120 (isham703).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
judggernaut added a commit to judggernaut/mcp-language-server that referenced this pull request Jul 3, 2026
… bug

Implements the one genuinely useful idea from upstream PR isaacphi#120 (idle
auto-shutdown), written fresh against this fork's main.go rather than
merging that PR's broader rewrite, which conflicts with commits already
in this fork (the stdin write-race fix, cleanupOnce, telemetry hooks).

Add --idle-timeout / MCP_IDLE_TIMEOUT (duration, 0 = disabled). When
set, a monitor goroutine shuts the server down after that long with no
MCP request (tracked via a new AddBeforeAny hook alongside the existing
telemetry hooks, so any request counts as activity, not just tool
calls). Useful now that a single machine can end up running several
persistent per-language MCP servers: idle ones can free their LSP
process and file watchers instead of sitting around indefinitely.

Moved the "done" channel from a local variable in main() onto the
mcpServer struct so background goroutines (the idle monitor, and the
pre-existing parent-death detector) can trigger the same cleanup path
without main() passing it around.

While wiring this up, found and fixed a real, latent bug shared by the
idle-timeout path and the pre-existing parent-death detector (the "Claude
desktop does not properly kill child processes" handling): both called
cleanup() from a background goroutine and expected the main goroutine's
blocking server.start() call to then return on its own so the process
could exit. That only ever worked for OS signals, because mcp-go's own
ServeStdio independently catches SIGINT/SIGTERM itself and cancels its
internal context to unblock its stdio read loop; nothing does that for
parent-death or idle-timeout. Confirmed empirically (minimal repro) that
closing os.Stdin from another goroutine does not reliably unblock an
in-flight blocking Read() on the same file descriptor on this platform,
so that fix (also present in PR isaacphi#120) isn't sufficient on its own either.
Both paths now call os.Exit() directly once cleanup finishes, so an
orphaned or idle server actually terminates instead of hanging forever
consuming resources, which is the entire point of both mechanisms.

Verified with a live smoke test: process self-exits at the configured
idle-timeout, and a synthetic parent-death scenario (killing an
intermediate launcher process) confirms the reparent-to-PID-1 path now
also terminates correctly, where it previously would have hung.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant