From cd3bee9e0510a5245752552257a216de16f213b2 Mon Sep 17 00:00:00 2001 From: Alex Alecu Date: Thu, 26 Feb 2026 13:33:22 +0200 Subject: [PATCH] test(memory): remove MCP-specific docs and lifecycle tests --- .../test/fixture/mcp/fake-mcp-server.js | 91 --------- packages/opencode/test/memory/README.md | 78 -------- .../test/memory/mcp-lifecycle.test.ts | 155 --------------- .../test/memory/orphan-process.test.ts | 179 ------------------ 4 files changed, 503 deletions(-) delete mode 100644 packages/opencode/test/fixture/mcp/fake-mcp-server.js delete mode 100644 packages/opencode/test/memory/README.md delete mode 100644 packages/opencode/test/memory/mcp-lifecycle.test.ts delete mode 100644 packages/opencode/test/memory/orphan-process.test.ts diff --git a/packages/opencode/test/fixture/mcp/fake-mcp-server.js b/packages/opencode/test/fixture/mcp/fake-mcp-server.js deleted file mode 100644 index 39bc5a59534..00000000000 --- a/packages/opencode/test/fixture/mcp/fake-mcp-server.js +++ /dev/null @@ -1,91 +0,0 @@ -// Minimal JSON-RPC 2.0 MCP server over stdio for testing -// Uses newline-delimited JSON (NDJSON) format matching the MCP SDK -// Exits on stdin close, SIGTERM, or SIGINT - -let readBuffer = "" - -process.stdin.setEncoding("utf8") -process.stdin.on("data", (chunk) => { - readBuffer += chunk - processBuffer() -}) - -process.stdin.on("end", () => { - process.exit(0) -}) - -process.on("SIGTERM", () => process.exit(0)) -process.on("SIGINT", () => process.exit(0)) - -function processBuffer() { - while (true) { - const index = readBuffer.indexOf("\n") - if (index === -1) break - const line = readBuffer.slice(0, index).replace(/\r$/, "") - readBuffer = readBuffer.slice(index + 1) - if (line.length === 0) continue - handle(line) - } -} - -function send(msg) { - process.stdout.write(JSON.stringify(msg) + "\n") -} - -function handle(raw) { - let data - try { - data = JSON.parse(raw) - } catch { - return - } - - if (data.method === "initialize") { - send({ - jsonrpc: "2.0", - id: data.id, - result: { - protocolVersion: "2024-11-05", - capabilities: { tools: {} }, - serverInfo: { name: "fake-mcp-server", version: "1.0.0" }, - }, - }) - return - } - - if (data.method === "notifications/initialized") { - return - } - - if (data.method === "tools/list") { - send({ - jsonrpc: "2.0", - id: data.id, - result: { - tools: [ - { - name: "fake_tool", - description: "A fake tool for testing", - inputSchema: { type: "object", properties: {}, additionalProperties: false }, - }, - ], - }, - }) - return - } - - if (data.method === "tools/call") { - send({ - jsonrpc: "2.0", - id: data.id, - result: { content: [{ type: "text", text: "ok" }] }, - }) - return - } - - // Respond to any other request to keep transport flowing - if (typeof data.id !== "undefined") { - send({ jsonrpc: "2.0", id: data.id, result: null }) - return - } -} diff --git a/packages/opencode/test/memory/README.md b/packages/opencode/test/memory/README.md deleted file mode 100644 index 52243330e2b..00000000000 --- a/packages/opencode/test/memory/README.md +++ /dev/null @@ -1,78 +0,0 @@ -# Memory Leak & Orphan Process Detection Tests - -## Overview - -8 test files and 1 fixture file in `packages/opencode/test/` to detect memory leaks and orphan processes in Kilo CLI. These tests target the specific leak sources identified in upstream [opencode#3013](https://github.com/anomalyco/opencode/issues/3013). - -## Files Created - -| File | Lines | Purpose | -|------|-------|---------| -| `test/fixture/mcp/fake-mcp-server.js` | ~90 | Minimal JSON-RPC 2.0 MCP server over stdio | -| `test/memory/helper.ts` | ~130 | Shared utilities for all memory/process tests | -| `test/memory/orphan-process.test.ts` | ~175 | Orphan process detection after MCP/LSP lifecycle | -| `test/memory/heap-growth.test.ts` | ~125 | Unbounded heap growth across subsystems | -| `test/memory/disposal.test.ts` | ~145 | Disposal chain correctness verification | -| `test/memory/mcp-lifecycle.test.ts` | ~130 | MCP-specific leak scenarios | -| `test/memory/lsp-lifecycle.test.ts` | ~130 | LSP server process lifecycle | -| `test/memory/state-leak.test.ts` | ~175 | State/Bus/GlobalBus/subscription leaks | -| `test/memory/session-heap-growth.test.ts` | ~145 | Full session DB + event lifecycle | - -## Test Results: 28 tests total - -### 23 PASS — no leaks detected in these areas - -| Suite | Tests | Key Findings | -|-------|-------|-------------| -| `disposal.test.ts` | 5/5 | Disposal chain works: `Instance.dispose` -> `State.dispose`, idempotent `disposeAll`, slow disposal completes | -| `state-leak.test.ts` | 5/5 | `recordsByKey` cleared on dispose, Instance cache cleared on `disposeAll`, GlobalBus listeners bounded, Bus subscriptions per-instance and cleared | -| `lsp-lifecycle.test.ts` | 3/3 | `LSPClient.shutdown()` correctly kills server process, no orphans after 5 cycles, 0.22 MB growth over 10 cycles | -| `heap-growth.test.ts` | 4/4 | Instance provide/dispose: 0.57 MB/20 cycles. Bus subscriptions: 0.50 MB/100 cycles. State entries: -0.40 MB/100 cycles. MCP add/disconnect heap: 0.00 MB/3 cycles | -| `session-heap-growth.test.ts` | 3/3 | Session create/remove: 0.32 MB/10 cycles. Message CRUD: 0.15 MB/50 ops. Full lifecycle with events: 0.27 MB/25 sessions | -| `orphan-process.test.ts` | 1/4 | LSP shutdown: no orphans | -| `mcp-lifecycle.test.ts` | 1/3 | `MCP.tools()` 50x: 0.05 MB growth | - -### 5 FAIL — correctly detecting real MCP orphan process bugs - -| Test | Error | -|------|-------| -| MCP: no orphans after dispose | `Found 1 orphan process: bun fake-mcp-server.js` | -| MCP: no orphans after disconnect | `Found 1 orphan process: bun fake-mcp-server.js` | -| disposeAll cleans up all instances | `Found 1 orphan process: bun fake-mcp-server.js` | -| MCP.add closes existing before overwriting | `Found 1 orphan process: bun fake-mcp-server.js` | -| MCP.connect closes existing before reconnecting | `Found 1 orphan process: bun fake-mcp-server.js` | - -## Root Cause Confirmed - -Every MCP test that spawns a server and then calls `client.close()` (via `StdioClientTransport`) leaves the child process alive. The MCP SDK's `StdioClientTransport.close()` closes the transport pipes but does **not** kill the spawned child process. This is the exact orphan process bug described in upstream #3013. - -In contrast, `LSPClient.shutdown()` explicitly calls `process.kill()` on the server process, which is why all LSP tests pass cleanly. - -## Architecture Decisions - -- **Real processes, mocked nothing for process tests**: MCP/LSP servers are real `child_process.spawn` instances running `fake-mcp-server.js` / `fake-lsp-server.js`. This ensures process lifecycle is tested authentically. -- **`pgrep -P` for process tree snapshots**: Works on macOS and Linux CI. Each test takes a before/after snapshot and asserts no new orphan descendants. -- **`afterEach` safety net**: Force-kills any orphans to prevent cascading test failures. -- **Empty config for MCP tests**: The MCP `state()` init connects to ALL configured servers on first access. Putting servers in config doubles connections and causes timeouts. Tests use `MCP.add()` directly. -- **Session tests use real DB, no LLM mock**: Instead of mocking `streamText`'s complex async iterable, session tests exercise `Session.create/updateMessage/updatePart/remove` with Bus subscriptions — covering the real Database + Bus + State code paths. -- **Generous thresholds**: `< 10 MB` for most growth tests, `< 5 MB` for targeted ones. Multiple GC passes (`Bun.gc(true)` x 3) with 50ms sleeps for stable measurement. - -## Running the Tests - -```bash -cd packages/opencode - -# All memory tests (non-MCP-process tests complete in ~150s) -bun test test/memory/disposal.test.ts test/memory/state-leak.test.ts test/memory/lsp-lifecycle.test.ts test/memory/heap-growth.test.ts test/memory/session-heap-growth.test.ts - -# MCP process tests (detect real orphan bugs, ~300s) -bun test test/memory/orphan-process.test.ts test/memory/mcp-lifecycle.test.ts - -# Individual suites -bun test test/memory/disposal.test.ts -bun test test/memory/session-heap-growth.test.ts -``` - -## What Happens When The Bug Is Fixed - -Once `StdioClientTransport.close()` is patched to kill the child process (or a wrapper is added in `src/mcp/index.ts`), all 5 currently-failing orphan tests will pass — providing regression protection. diff --git a/packages/opencode/test/memory/mcp-lifecycle.test.ts b/packages/opencode/test/memory/mcp-lifecycle.test.ts deleted file mode 100644 index 4de919eabdc..00000000000 --- a/packages/opencode/test/memory/mcp-lifecycle.test.ts +++ /dev/null @@ -1,155 +0,0 @@ -import { describe, test, expect } from "bun:test" -import path from "path" -import { Instance } from "../../src/project/instance" -import { MCP } from "../../src/mcp" -import { tmpdir } from "../fixture/fixture" -import { PROJECT_ROOT, snapshotDescendants, assertNoOrphans, forceKillAll, stableHeapMB, waitForExit } from "./helper" - -const FAKE_MCP_SERVER = path.join(PROJECT_ROOT, "test/fixture/mcp/fake-mcp-server.js") - -describe("memory: MCP lifecycle", () => { - const cleanup = async (baseline: Set, name: string) => { - try { - const afterPids = await snapshotDescendants(process.pid) - const orphans = [...afterPids].filter((pid) => !baseline.has(pid)) - forceKillAll(orphans) - } catch (err) { - console.log(`[${name}] cleanup failed: ${err}`) - } - } - - test("MCP.add closes existing client before overwriting", async () => { - await using tmp = await tmpdir({ git: true }) - const baseline = await snapshotDescendants(process.pid) - - try { - await Instance.provide({ - directory: tmp.path, - fn: async () => { - try { - // Add first server - await MCP.add("lifecycle-test", { - type: "local", - command: ["bun", FAKE_MCP_SERVER], - }) - - const afterFirstPids = await snapshotDescendants(process.pid) - const firstOnlyPids = [...afterFirstPids].filter((p) => !baseline.has(p)) - const firstCount = firstOnlyPids.length - - // Add second server with same key — should close first - await MCP.add("lifecycle-test", { - type: "local", - command: ["bun", FAKE_MCP_SERVER], - }) - - expect(await waitForExit(firstOnlyPids)).toBe(true) - const afterSecondPids = await snapshotDescendants(process.pid) - const secondCount = [...afterSecondPids].filter((p) => !baseline.has(p)).length - - // Process count should stay roughly constant (old killed, new spawned) - // Allow +1 for timing of process teardown - expect(secondCount).toBeLessThanOrEqual(firstCount + 1) - } finally { - await Instance.dispose() - } - }, - }) - - const beforeAssertPids = await snapshotDescendants(process.pid) - expect(await waitForExit([...beforeAssertPids].filter((p) => !baseline.has(p)))).toBe(true) - const afterPids = await snapshotDescendants(process.pid) - await assertNoOrphans(baseline, afterPids) - } finally { - await cleanup(baseline, "MCP.add closes existing client before overwriting") - } - }, 120_000) - - test("MCP.connect closes existing client before reconnecting", async () => { - await using tmp = await tmpdir({ - git: true, - config: { - mcp: { - "connect-test": { - type: "local", - command: ["bun", FAKE_MCP_SERVER], - }, - }, - }, - }) - const baseline = await snapshotDescendants(process.pid) - - try { - await Instance.provide({ - directory: tmp.path, - fn: async () => { - try { - // First connect (state init may already connect from config) - await MCP.connect("connect-test") - - const afterFirstPids = await snapshotDescendants(process.pid) - const firstOnlyPids = [...afterFirstPids].filter((p) => !baseline.has(p)) - const firstCount = firstOnlyPids.length - - // Second connect — should close existing before reconnecting - await MCP.connect("connect-test") - - expect(await waitForExit(firstOnlyPids)).toBe(true) - const afterSecondPids = await snapshotDescendants(process.pid) - const secondCount = [...afterSecondPids].filter((p) => !baseline.has(p)).length - - // Should not accumulate processes - expect(secondCount).toBeLessThanOrEqual(firstCount + 1) - } finally { - await Instance.dispose() - } - }, - }) - - const beforeAssertPids = await snapshotDescendants(process.pid) - expect(await waitForExit([...beforeAssertPids].filter((p) => !baseline.has(p)))).toBe(true) - const afterPids = await snapshotDescendants(process.pid) - await assertNoOrphans(baseline, afterPids) - } finally { - await cleanup(baseline, "MCP.connect closes existing client before reconnecting") - } - }, 120_000) - - test("MCP.tools() does not leak memory", async () => { - await using tmp = await tmpdir({ git: true }) - const baseline = await snapshotDescendants(process.pid) - - try { - await Instance.provide({ - directory: tmp.path, - fn: async () => { - try { - await MCP.add("tools-test", { - type: "local", - command: ["bun", FAKE_MCP_SERVER], - }) - - // Warm up - await MCP.tools() - - const baseline = await stableHeapMB() - - for (let i = 0; i < 50; i++) { - await MCP.tools() - } - - const after = await stableHeapMB() - const growth = after - baseline - - console.log(` MCP.tools() 50x growth: ${growth.toFixed(2)} MB`) - expect(growth).toBeLessThan(5) - } finally { - await Instance.dispose() - } - }, - }) - } finally { - await cleanup(baseline, "MCP.tools() does not leak memory") - } - }, 60_000) -}) diff --git a/packages/opencode/test/memory/orphan-process.test.ts b/packages/opencode/test/memory/orphan-process.test.ts deleted file mode 100644 index 274d13497be..00000000000 --- a/packages/opencode/test/memory/orphan-process.test.ts +++ /dev/null @@ -1,179 +0,0 @@ -import { describe, test, expect } from "bun:test" -import path from "path" -import { Instance } from "../../src/project/instance" -import { MCP } from "../../src/mcp" -import { LSPClient } from "../../src/lsp/client" -import { spawn } from "child_process" -import { tmpdir } from "../fixture/fixture" -import { PROJECT_ROOT, snapshotDescendants, assertNoOrphans, forceKillAll, waitForExit } from "./helper" - -const FAKE_MCP_SERVER = path.join(PROJECT_ROOT, "test/fixture/mcp/fake-mcp-server.js") -const FAKE_LSP_SERVER = path.join(PROJECT_ROOT, "test/fixture/lsp/fake-lsp-server.js") - -// Don't put MCP in config — the MCP state() init function connects to ALL -// configured servers on first access, which doubles connections and causes timeouts. -// Instead, use empty config and add servers via MCP.add(). - -describe("memory: orphan process detection", () => { - const cleanup = async (beforePids: Set) => { - // Safety net: kill any orphaned processes to prevent cascading failures - try { - const afterPids = await snapshotDescendants(process.pid) - const orphans: number[] = [] - for (const pid of afterPids) { - if (!beforePids.has(pid)) orphans.push(pid) - } - forceKillAll(orphans) - } catch (error) { - // Best-effort cleanup - console.warn("orphan cleanup failed", error) - } - } - - const baseline = async (run: (beforePids: Set) => Promise) => { - const beforePids = await snapshotDescendants(process.pid) - try { - await run(beforePids) - } finally { - await cleanup(beforePids) - } - } - - test("MCP local server: no orphans after dispose", async () => { - await using tmp = await tmpdir({ git: true }) - - await baseline(async (beforePids) => { - const spawned = new Set() - - await Instance.provide({ - directory: tmp.path, - fn: async () => { - // MCP.add triggers spawn of the fake server via StdioClientTransport - await MCP.add("test-server", { - type: "local", - command: ["bun", FAKE_MCP_SERVER], - }) - - // Verify server is running (new descendants exist) - const duringPids = await snapshotDescendants(process.pid) - const newProcesses = [...duringPids].filter((p) => !beforePids.has(p)) - expect(newProcesses.length).toBeGreaterThan(0) - for (const pid of newProcesses) { - spawned.add(pid) - } - - // Dispose the instance — should close all MCP clients and kill processes - await Instance.dispose() - }, - }) - - expect(await waitForExit([...spawned])).toBe(true) - const afterPids = await snapshotDescendants(process.pid) - await assertNoOrphans(beforePids, afterPids) - }) - }, 60_000) - - test("MCP local server: no orphans after disconnect", async () => { - await using tmp = await tmpdir({ git: true }) - - await baseline(async (beforePids) => { - await Instance.provide({ - directory: tmp.path, - fn: async () => { - await MCP.add("test-server", { - type: "local", - command: ["bun", FAKE_MCP_SERVER], - }) - - const duringPids = await snapshotDescendants(process.pid) - const newProcesses = [...duringPids].filter((p) => !beforePids.has(p)) - expect(newProcesses.length).toBeGreaterThan(0) - - // Disconnect should close the client (which closes the transport/process) - await MCP.disconnect("test-server") - expect(await waitForExit(newProcesses)).toBe(true) - - const afterPids = await snapshotDescendants(process.pid) - await assertNoOrphans(beforePids, afterPids) - - await Instance.dispose() - }, - }) - }) - }, 60_000) - - test("disposeAll cleans up all instances", async () => { - await using tmp1 = await tmpdir({ git: true }) - await using tmp2 = await tmpdir({ git: true }) - - await baseline(async (beforePids) => { - // Create two instances with MCP servers - await Instance.provide({ - directory: tmp1.path, - fn: async () => { - await MCP.add("test-server", { - type: "local", - command: ["bun", FAKE_MCP_SERVER], - }) - }, - }) - - await Instance.provide({ - directory: tmp2.path, - fn: async () => { - await MCP.add("test-server", { - type: "local", - command: ["bun", FAKE_MCP_SERVER], - }) - }, - }) - - // Verify servers are running - const duringPids = await snapshotDescendants(process.pid) - const newProcesses = [...duringPids].filter((p) => !beforePids.has(p)) - expect(newProcesses.length).toBeGreaterThan(0) - - // Dispose all - await Instance.disposeAll() - expect(await waitForExit(newProcesses)).toBe(true) - - const afterPids = await snapshotDescendants(process.pid) - await assertNoOrphans(beforePids, afterPids) - }) - }, 120_000) - - test("LSP server: no orphans after shutdown", async () => { - await using tmp = await tmpdir({ git: true }) - - await baseline(async (beforePids) => { - await Instance.provide({ - directory: tmp.path, - fn: async () => { - const serverProcess = spawn("bun", [FAKE_LSP_SERVER], { - stdio: ["pipe", "pipe", "pipe"], - cwd: tmp.path, - }) - - const client = await LSPClient.create({ - serverID: "test-lsp", - server: { process: serverProcess as any }, - root: tmp.path, - }) - - expect(client).toBeTruthy() - - // Shutdown should kill the process - await client!.shutdown() - if (serverProcess.pid) { - expect(await waitForExit([serverProcess.pid])).toBe(true) - } - - const afterPids = await snapshotDescendants(process.pid) - await assertNoOrphans(beforePids, afterPids) - - await Instance.dispose() - }, - }) - }) - }, 30_000) -})