From 1ea1e30f6344985927cf68657bd583dbf7f928cf Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 13:17:08 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20=E2=80=94?= =?UTF-8?q?=20real=20latch=20coverage,=20consistent=20docker=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The `retriedDynamic` latch test never made the retry fail: a real `listen(0)` always succeeds, so it produced one EADDRINUSE and passed with the latch deleted. Mock `node:http` so every listen fails, asserting exactly two listen attempts and a settled result. Verified by mutation: removing the latch now hangs the test instead of passing it. - The migration guide's Docker notes still described a dynamic sandbox port and told readers to pin a custom one, contradicting the new fixed 6275 two sections later. - Narrowed "or any container" in the troubleshooting entry, which overstated the scope and contradicted the PR's own scope note. It now names the IPv4-forwarder case and explicitly excludes the official image and a plain loopback -p. Signed-off-by: cliffhall --- .../server/sandbox-controller.test.ts | 58 ++++++++++++++++--- docs/v1-to-v2-migration.md | 9 +-- 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/clients/web/src/test/integration/server/sandbox-controller.test.ts b/clients/web/src/test/integration/server/sandbox-controller.test.ts index a1f73f7e..6614c06e 100644 --- a/clients/web/src/test/integration/server/sandbox-controller.test.ts +++ b/clients/web/src/test/integration/server/sandbox-controller.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { createServer, type Server } from "node:http"; +import { EventEmitter } from "node:events"; import { createSandboxController, DEFAULT_SANDBOX_PORT, @@ -408,27 +409,66 @@ describe("createSandboxController", () => { }); it("gives up after one retry when the dynamic fallback also fails", async () => { - // Guards the `retriedDynamic` latch: without it an EADDRINUSE loop could - // re-listen forever instead of settling. - const { port, release } = await claimPort(); + // Guards the `retriedDynamic` latch. A real `listen(0)` effectively always + // succeeds, so claiming a port only ever produces ONE EADDRINUSE — under + // which this test would pass with the latch deleted. Mock `node:http` so + // every listen fails with EADDRINUSE: the first trips the retry, the second + // must be caught by the latch and settle. Without the latch this recurses + // until the test times out rather than resolving. + vi.resetModules(); + const listenCalls: number[] = []; + vi.doMock("node:http", async () => { + const actual = + await vi.importActual("node:http"); + return { + ...actual, + createServer: () => { + const emitter = new EventEmitter(); + return Object.assign(emitter, { + listen: (p: number) => { + listenCalls.push(p); + setImmediate(() => + emitter.emit( + "error", + Object.assign(new Error("in use"), { code: "EADDRINUSE" }), + ), + ); + }, + address: () => null, + close: (cb?: () => void) => cb?.(), + }); + }, + }; + }); const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - const controller = createSandboxController({ port, host: "127.0.0.1" }); try { + const mod = await import("../../../../server/sandbox-controller.js"); + const controller = mod.createSandboxController({ + port: 6275, + host: "127.0.0.1", + }); + // Settles rather than looping — the contract the Vite plugin depends on. const result = await controller.start(); - // The retry succeeds here (a dynamic port is always available), so assert - // the latch is what bounds it: exactly one retry warning was emitted. + expect(result).toEqual({ port: 0, url: "" }); + expect(controller.getUrl()).toBeNull(); + // Exactly two listens: the fixed port, then one dynamic retry. A third + // would mean the latch failed to bound the loop. + expect(listenCalls).toEqual([6275, 0]); expect( warnSpy.mock.calls.filter((c) => String(c[0]).includes("falling back to an OS-assigned port"), ), ).toHaveLength(1); - expect(result.port).toBeGreaterThan(0); + // The second failure degrades loudly instead of retrying again. + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining("Sandbox: port 6275 in use"), + ); } finally { warnSpy.mockRestore(); errorSpy.mockRestore(); - await controller.close(); - await release(); + vi.doUnmock("node:http"); + vi.resetModules(); } }); diff --git a/docs/v1-to-v2-migration.md b/docs/v1-to-v2-migration.md index 0d1b2fe3..c241e9df 100644 --- a/docs/v1-to-v2-migration.md +++ b/docs/v1-to-v2-migration.md @@ -330,14 +330,15 @@ docker run --rm -p 127.0.0.1:6274:6274 ghcr.io/modelcontextprotocol/inspector:la Notes: - **Keep the `127.0.0.1:` prefix on the published port**, as the v1 recipe did. `-p 6274:6274` publishes on every host interface, and the container's `HOST=0.0.0.0` is about the _container's_ interfaces, not the host's — the two are independent. That matters here because the backend spawns processes, `/` embeds the API token, and requests arriving with **no** `Origin` header (i.e. anything that isn't a browser) skip the origin allow-list, leaving the token as the only guard. Bind to loopback and opt into wider exposure deliberately. -- **The Apps tab needs one more published port.** The sandbox server is dynamic by default and the `Dockerfile` `EXPOSE`s only `6274`, so the recipe above covers everything _except_ MCP Apps. To use them, pin and publish the sandbox port as well: +- **The Apps tab needs one more published port.** The sandbox server listens on `6275` by default, so the recipe above covers everything _except_ MCP Apps. To use them, publish that port as well: ```bash - docker run --rm -p 127.0.0.1:6274:6274 -p 127.0.0.1:6280:6280 \ - -e MCP_SANDBOX_PORT=6280 \ + docker run --rm -p 127.0.0.1:6274:6274 -p 127.0.0.1:6275:6275 \ ghcr.io/modelcontextprotocol/inspector:latest ``` + Publish it on the **same port number** inside and out: the sandbox URL is handed to the browser via `/api/config` as `http://localhost:/sandbox`, so remapping it advertises a port the browser can't reach. To use a different number, move both ends — `-e MCP_SANDBOX_PORT=6280 -p 127.0.0.1:6280:6280`. + - The v2 image sets `DANGEROUSLY_BIND_ALL_INTERFACES=true` internally (a container must bind `0.0.0.0` to be reachable through `-p`). Setting a bare `HOST=0.0.0.0` **outside** a container now exits with an error. - **If you remap the published port** (`-p 8080:6274`), the browser's origin no longer matches the in-container port, so set `ALLOWED_ORIGINS=http://localhost:8080,http://127.0.0.1:8080` (or run `-e CLIENT_PORT=8080 -p 8080:8080`) or connects will 403. - The image runs as non-root and has a `HEALTHCHECK` that assumes `--web`; add `--no-healthcheck` when running `--cli` / `--tui`. @@ -357,7 +358,7 @@ Notes: **"The Apps tab is blank in Docker / over SSH."** The MCP Apps sandbox is a second port (`6275` by default). Publish/forward it too — see [MCP Apps caveats](../clients/web/README.md#hosting-on-a-network). -**"The page never loads inside a VS Code dev container (or any container)."** Fixed in the release carrying #1951 — upgrade. Before that fix the default `HOST=localhost` bound IPv6 loopback only on Linux, so the container's port forwarder (which connects over IPv4 `127.0.0.1`) could never reach it; the terminal printed a normal banner while the browser spun forever. On an older version, work around it with `HOST=127.0.0.1`. Note the failure is a **hang**, not a connection error, because the forwarder accepts on the host side before failing to connect inward. +**"The page never loads inside a VS Code dev container."** Fixed in the release carrying #1951 — upgrade; on an older version, work around it with `HOST=127.0.0.1`. Before that fix the default `HOST=localhost` bound IPv6 loopback **only** on Linux, so anything connecting over IPv4 `127.0.0.1` was refused — which is what a dev container's port forwarder does. The terminal printed a normal banner while the browser spun forever; the failure is a **hang** rather than a connection error because the forwarder accepts on the host side before failing to connect inward. The same fix covers an `ssh -L 6274:127.0.0.1:6274` tunnel and a container healthcheck. It does **not** describe the official Docker image (it binds `0.0.0.0`, so it was never affected) or a plain `docker run -p` against a loopback bind, which is unreachable across the network namespace for an unrelated reason. **"`HOST=0.0.0.0` exits with an error."** That's the wildcard-bind guard. Bind a specific address instead, or set `DANGEROUSLY_BIND_ALL_INTERFACES=true` if you genuinely need all interfaces. See [Host binding & the origin allow-list](../clients/web/README.md#host-binding--the-origin-allow-list).