From c0a58ad1c1bf519746cd4040b177ab23bef04f1b Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Sat, 13 Jun 2026 19:25:23 -0500 Subject: [PATCH 1/3] fix(mcp): stop idle OAuth callback server --- packages/opencode/src/mcp/index.ts | 108 ++++++++-------- packages/opencode/src/mcp/oauth-callback.ts | 70 ++++++++--- .../opencode/test/mcp/oauth-callback.test.ts | 115 ++++++++++++++++++ 3 files changed, 229 insertions(+), 64 deletions(-) diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 08d58118c992..e323ccc8417a 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -745,7 +745,7 @@ export const layer = Layer.effect( return mcpConfig }) - const startAuth = Effect.fn("MCP.startAuth")(function* (mcpName: string) { + const startAuth = Effect.fn("MCP.startAuth")(function* (mcpName: string, pendingState?: string) { const mcpConfig = yield* requireMcpConfig(mcpName) if (mcpConfig.type !== "remote") throw new Error(`MCP server ${mcpName} is not a remote server`) if (mcpConfig.oauth === false) throw new Error(`MCP server ${mcpName} has OAuth explicitly disabled`) @@ -763,9 +763,11 @@ export const layer = Layer.effect( // Start the callback server with custom redirectUri if configured yield* Effect.promise(() => McpOAuthCallback.ensureRunning(effectiveRedirectUri)) - const oauthState = Array.from(crypto.getRandomValues(new Uint8Array(32))) - .map((b) => b.toString(16).padStart(2, "0")) - .join("") + const oauthState = + pendingState ?? + Array.from(crypto.getRandomValues(new Uint8Array(32))) + .map((b) => b.toString(16).padStart(2, "0")) + .join("") yield* auth.updateOAuthState(mcpName, oauthState) let capturedUrl: URL | undefined const authProvider = new McpOAuthProvider( @@ -811,60 +813,66 @@ export const layer = Layer.effect( }) const authenticate = Effect.fn("MCP.authenticate")(function* (mcpName: string) { - const result = yield* startAuth(mcpName) - if (!result.authorizationUrl) { - const client = "client" in result ? result.client : undefined - const mcpConfig = yield* requireMcpConfig(mcpName).pipe( - Effect.tapError(() => Effect.tryPromise(() => client?.close() ?? Promise.resolve()).pipe(Effect.ignore)), - ) + const oauthState = Array.from(crypto.getRandomValues(new Uint8Array(32))) + .map((b) => b.toString(16).padStart(2, "0")) + .join("") + const callbackPromise = McpOAuthCallback.waitForCallback(oauthState, mcpName) + void callbackPromise.catch(() => {}) + + return yield* Effect.gen(function* () { + const result = yield* startAuth(mcpName, oauthState) + if (!result.authorizationUrl) { + const client = "client" in result ? result.client : undefined + const mcpConfig = yield* requireMcpConfig(mcpName).pipe( + Effect.tapError(() => Effect.tryPromise(() => client?.close() ?? Promise.resolve()).pipe(Effect.ignore)), + ) + + const listed = client + ? client.getServerCapabilities()?.tools + ? yield* McpCatalog.defs(client, mcpConfig.timeout) + : [] + : undefined + if (!client || !listed) { + yield* Effect.tryPromise(() => client?.close() ?? Promise.resolve()).pipe(Effect.ignore) + return { status: "failed", error: "Failed to get tools" } satisfies Status + } - const listed = client - ? client.getServerCapabilities()?.tools - ? yield* McpCatalog.defs(client, mcpConfig.timeout) - : [] - : undefined - if (!client || !listed) { - yield* Effect.tryPromise(() => client?.close() ?? Promise.resolve()).pipe(Effect.ignore) - return { status: "failed", error: "Failed to get tools" } satisfies Status + const s = yield* InstanceState.get(state) + yield* auth.clearOAuthState(mcpName) + return yield* storeClient(s, mcpName, client, listed, mcpConfig.timeout) } - const s = yield* InstanceState.get(state) - yield* auth.clearOAuthState(mcpName) - return yield* storeClient(s, mcpName, client, listed, mcpConfig.timeout) - } - - const callbackPromise = McpOAuthCallback.waitForCallback(result.oauthState, mcpName) - - yield* Effect.tryPromise(() => open(result.authorizationUrl)).pipe( - Effect.flatMap((subprocess) => - Effect.callback((resume) => { - const timer = setTimeout(() => resume(Effect.void), 500) - subprocess.on("error", (err) => { - clearTimeout(timer) - resume(Effect.fail(err)) - }) - subprocess.on("exit", (code) => { - if (code !== null && code !== 0) { + yield* Effect.tryPromise(() => open(result.authorizationUrl)).pipe( + Effect.flatMap((subprocess) => + Effect.callback((resume) => { + const timer = setTimeout(() => resume(Effect.void), 500) + subprocess.on("error", (err) => { clearTimeout(timer) - resume(Effect.fail(new Error(`Browser open failed with exit code ${code}`))) - } - }) + resume(Effect.fail(err)) + }) + subprocess.on("exit", (code) => { + if (code !== null && code !== 0) { + clearTimeout(timer) + resume(Effect.fail(new Error(`Browser open failed with exit code ${code}`))) + } + }) + }), + ), + Effect.catch(() => { + return events.publish(BrowserOpenFailed, { mcpName, url: result.authorizationUrl }).pipe(Effect.ignore) }), - ), - Effect.catch(() => { - return events.publish(BrowserOpenFailed, { mcpName, url: result.authorizationUrl }).pipe(Effect.ignore) - }), - ) + ) - const code = yield* Effect.promise(() => callbackPromise) + const code = yield* Effect.promise(() => callbackPromise) - const storedState = yield* auth.getOAuthState(mcpName) - if (storedState !== result.oauthState) { + const storedState = yield* auth.getOAuthState(mcpName) + if (storedState !== result.oauthState) { + yield* auth.clearOAuthState(mcpName) + throw new Error("OAuth state mismatch - potential CSRF attack") + } yield* auth.clearOAuthState(mcpName) - throw new Error("OAuth state mismatch - potential CSRF attack") - } - yield* auth.clearOAuthState(mcpName) - return yield* finishAuth(mcpName, code) + return yield* finishAuth(mcpName, code) + }).pipe(Effect.ensuring(Effect.sync(() => McpOAuthCallback.cancelPending(mcpName)))) }) const finishAuth = Effect.fn("MCP.finishAuth")(function* (mcpName: string, authorizationCode: string) { diff --git a/packages/opencode/src/mcp/oauth-callback.ts b/packages/opencode/src/mcp/oauth-callback.ts index 8d3161849ae3..90aa3d4ba9e7 100644 --- a/packages/opencode/src/mcp/oauth-callback.ts +++ b/packages/opencode/src/mcp/oauth-callback.ts @@ -54,6 +54,9 @@ interface PendingAuth { } let server: ReturnType | undefined +let starting: Promise | undefined +let stopping: Promise | undefined +let idleStopRequested = false const pendingAuths = new Map() // Reverse index: mcpName → oauthState, so cancelPending(mcpName) can // find the right entry in pendingAuths (which is keyed by oauthState). @@ -70,6 +73,23 @@ function cleanupStateIndex(oauthState: string) { } } +function stopIfIdle() { + if (pendingAuths.size > 0) return + if (starting || stopping) { + idleStopRequested = true + return + } + if (!server) return + + const current = server + server = undefined + idleStopRequested = false + stopping = new Promise((resolve) => current.close(() => resolve())).finally(() => { + stopping = undefined + if (idleStopRequested) stopIfIdle() + }) +} + function handleRequest(req: import("http").IncomingMessage, res: import("http").ServerResponse) { const url = new URL(req.url || "/", `http://localhost:${currentPort}`) @@ -103,6 +123,7 @@ function handleRequest(req: import("http").IncomingMessage, res: import("http"). } res.writeHead(200, { "Content-Type": "text/html" }) res.end(HTML_ERROR(errorMsg)) + stopIfIdle() return } @@ -129,12 +150,19 @@ function handleRequest(req: import("http").IncomingMessage, res: import("http"). res.writeHead(200, { "Content-Type": "text/html" }) res.end(HTML_SUCCESS) + stopIfIdle() } export async function ensureRunning(redirectUri?: string): Promise { // Parse the redirect URI to get port and path (uses defaults if not provided) const { port, path } = parseRedirectUri(redirectUri) + if (stopping) await stopping + if (starting) { + await starting + return ensureRunning(redirectUri) + } + // If server is running on a different port/path, stop it first if (server && (currentPort !== port || currentPath !== path)) { await stop() @@ -142,24 +170,31 @@ export async function ensureRunning(redirectUri?: string): Promise { if (server) return - const running = await isPortInUse(port) - if (running) { - return - } + starting = (async () => { + if (await isPortInUse(port)) return - currentPort = port - currentPath = path + currentPort = port + currentPath = path - server = createServer(handleRequest) - await new Promise((resolve, reject) => { - server!.listen(currentPort, () => { - resolve() + const next = createServer(handleRequest) + await new Promise((resolve, reject) => { + next.listen(currentPort, () => resolve()) + next.on("error", reject) }) - server!.on("error", reject) + server = next + })() + await starting.finally(() => { + starting = undefined }) + if (idleStopRequested) stopIfIdle() } -export function waitForCallback(oauthState: string, mcpName?: string): Promise { +export function waitForCallback( + oauthState: string, + mcpName?: string, + timeoutMs: number = CALLBACK_TIMEOUT_MS, +): Promise { + idleStopRequested = false if (mcpName) mcpNameToState.set(mcpName, oauthState) return new Promise((resolve, reject) => { const timeout = setTimeout(() => { @@ -167,8 +202,9 @@ export function waitForCallback(oauthState: string, mcpName?: string): Promise { + if (starting) await starting + if (stopping) await stopping + if (server) { - await new Promise((resolve) => server!.close(() => resolve())) + const current = server server = undefined + await new Promise((resolve) => current.close(() => resolve())) } for (const [_name, pending] of pendingAuths) { @@ -212,6 +253,7 @@ export async function stop(): Promise { } pendingAuths.clear() mcpNameToState.clear() + idleStopRequested = false } export function isRunning(): boolean { diff --git a/packages/opencode/test/mcp/oauth-callback.test.ts b/packages/opencode/test/mcp/oauth-callback.test.ts index 58a4fa8c86cc..8fadeb59753c 100644 --- a/packages/opencode/test/mcp/oauth-callback.test.ts +++ b/packages/opencode/test/mcp/oauth-callback.test.ts @@ -1,4 +1,5 @@ import { test, expect, describe, afterEach } from "bun:test" +import { createServer } from "http" import { McpOAuthCallback } from "../../src/mcp/oauth-callback" import { parseRedirectUri } from "../../src/mcp/oauth-provider" @@ -32,3 +33,117 @@ describe("McpOAuthCallback.ensureRunning", () => { expect(McpOAuthCallback.isRunning()).toBe(true) }) }) + +describe("McpOAuthCallback lifecycle", () => { + afterEach(async () => { + await McpOAuthCallback.stop() + }) + + test("stops after a successful callback response completes", async () => { + const port = await availablePort() + await McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) + const callback = McpOAuthCallback.waitForCallback("success") + + const response = await fetch(`http://127.0.0.1:${port}/callback?code=code&state=success`) + + expect(response.status).toBe(200) + expect(await callback).toBe("code") + await waitForStop() + }) + + test("stops after a provider error", async () => { + const port = await availablePort() + await McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) + const callback = McpOAuthCallback.waitForCallback("provider-error") + const error = callback.catch((cause) => cause) + + const response = await fetch( + `http://127.0.0.1:${port}/callback?error=access_denied&error_description=Denied&state=provider-error`, + ) + + expect(response.status).toBe(200) + await response.text() + expect(await error).toEqual(new Error("Denied")) + await waitForStop() + }) + + test("stops after cancellation", async () => { + const port = await availablePort() + await McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) + const callback = McpOAuthCallback.waitForCallback("cancelled", "server") + + McpOAuthCallback.cancelPending("server") + + await expect(callback).rejects.toThrow("Authorization cancelled") + await waitForStop() + }) + + test("stops when cancellation races listener startup", async () => { + const port = await availablePort() + const callback = McpOAuthCallback.waitForCallback("cancelled", "starting-server") + const starting = McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) + + McpOAuthCallback.cancelPending("starting-server") + + await expect(callback).rejects.toThrow("Authorization cancelled") + await starting + await waitForStop() + }) + + test("stops after timeout", async () => { + const port = await availablePort() + await McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) + const callback = McpOAuthCallback.waitForCallback("timeout", undefined, 10) + + await expect(callback).rejects.toThrow("OAuth callback timeout") + await waitForStop() + }) + + test("stays running until all simultaneous callbacks settle", async () => { + const port = await availablePort() + const redirectUri = `http://127.0.0.1:${port}/callback` + await Promise.all([McpOAuthCallback.ensureRunning(redirectUri), McpOAuthCallback.ensureRunning(redirectUri)]) + const first = McpOAuthCallback.waitForCallback("first") + const second = McpOAuthCallback.waitForCallback("second") + + await fetch(`${redirectUri}?code=one&state=first`) + expect(await first).toBe("one") + expect(McpOAuthCallback.isRunning()).toBe(true) + + await fetch(`${redirectUri}?code=two&state=second`) + expect(await second).toBe("two") + await waitForStop() + }) + + test("restarts when a callback is registered during shutdown", async () => { + const port = await availablePort() + const redirectUri = `http://127.0.0.1:${port}/callback` + await McpOAuthCallback.ensureRunning(redirectUri) + const first = McpOAuthCallback.waitForCallback("first") + + await fetch(`${redirectUri}?code=one&state=first`) + expect(await first).toBe("one") + + const second = McpOAuthCallback.waitForCallback("second") + await McpOAuthCallback.ensureRunning(redirectUri) + const response = await fetch(`${redirectUri}?code=two&state=second`) + + expect(response.status).toBe(200) + expect(await second).toBe("two") + await waitForStop() + }) +}) + +async function availablePort() { + const probe = createServer() + await new Promise((resolve) => probe.listen(0, "127.0.0.1", resolve)) + const address = probe.address() + if (!address || typeof address === "string") throw new Error("Failed to allocate callback test port") + await new Promise((resolve) => probe.close(() => resolve())) + return address.port +} + +async function waitForStop() { + for (let attempt = 0; attempt < 50 && McpOAuthCallback.isRunning(); attempt++) await Bun.sleep(10) + expect(McpOAuthCallback.isRunning()).toBe(false) +} From 25cf9c443977c58bd08edef3af6a787742b177ac Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Sun, 14 Jun 2026 22:23:46 -0400 Subject: [PATCH 2/3] refactor(mcp): simplify OAuth callback cleanup --- packages/opencode/src/mcp/index.ts | 108 ++++++++---------- packages/opencode/src/mcp/oauth-callback.ts | 63 +++------- .../opencode/test/mcp/oauth-callback.test.ts | 97 +--------------- 3 files changed, 68 insertions(+), 200 deletions(-) diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index e323ccc8417a..08d58118c992 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -745,7 +745,7 @@ export const layer = Layer.effect( return mcpConfig }) - const startAuth = Effect.fn("MCP.startAuth")(function* (mcpName: string, pendingState?: string) { + const startAuth = Effect.fn("MCP.startAuth")(function* (mcpName: string) { const mcpConfig = yield* requireMcpConfig(mcpName) if (mcpConfig.type !== "remote") throw new Error(`MCP server ${mcpName} is not a remote server`) if (mcpConfig.oauth === false) throw new Error(`MCP server ${mcpName} has OAuth explicitly disabled`) @@ -763,11 +763,9 @@ export const layer = Layer.effect( // Start the callback server with custom redirectUri if configured yield* Effect.promise(() => McpOAuthCallback.ensureRunning(effectiveRedirectUri)) - const oauthState = - pendingState ?? - Array.from(crypto.getRandomValues(new Uint8Array(32))) - .map((b) => b.toString(16).padStart(2, "0")) - .join("") + const oauthState = Array.from(crypto.getRandomValues(new Uint8Array(32))) + .map((b) => b.toString(16).padStart(2, "0")) + .join("") yield* auth.updateOAuthState(mcpName, oauthState) let capturedUrl: URL | undefined const authProvider = new McpOAuthProvider( @@ -813,66 +811,60 @@ export const layer = Layer.effect( }) const authenticate = Effect.fn("MCP.authenticate")(function* (mcpName: string) { - const oauthState = Array.from(crypto.getRandomValues(new Uint8Array(32))) - .map((b) => b.toString(16).padStart(2, "0")) - .join("") - const callbackPromise = McpOAuthCallback.waitForCallback(oauthState, mcpName) - void callbackPromise.catch(() => {}) - - return yield* Effect.gen(function* () { - const result = yield* startAuth(mcpName, oauthState) - if (!result.authorizationUrl) { - const client = "client" in result ? result.client : undefined - const mcpConfig = yield* requireMcpConfig(mcpName).pipe( - Effect.tapError(() => Effect.tryPromise(() => client?.close() ?? Promise.resolve()).pipe(Effect.ignore)), - ) - - const listed = client - ? client.getServerCapabilities()?.tools - ? yield* McpCatalog.defs(client, mcpConfig.timeout) - : [] - : undefined - if (!client || !listed) { - yield* Effect.tryPromise(() => client?.close() ?? Promise.resolve()).pipe(Effect.ignore) - return { status: "failed", error: "Failed to get tools" } satisfies Status - } + const result = yield* startAuth(mcpName) + if (!result.authorizationUrl) { + const client = "client" in result ? result.client : undefined + const mcpConfig = yield* requireMcpConfig(mcpName).pipe( + Effect.tapError(() => Effect.tryPromise(() => client?.close() ?? Promise.resolve()).pipe(Effect.ignore)), + ) - const s = yield* InstanceState.get(state) - yield* auth.clearOAuthState(mcpName) - return yield* storeClient(s, mcpName, client, listed, mcpConfig.timeout) + const listed = client + ? client.getServerCapabilities()?.tools + ? yield* McpCatalog.defs(client, mcpConfig.timeout) + : [] + : undefined + if (!client || !listed) { + yield* Effect.tryPromise(() => client?.close() ?? Promise.resolve()).pipe(Effect.ignore) + return { status: "failed", error: "Failed to get tools" } satisfies Status } - yield* Effect.tryPromise(() => open(result.authorizationUrl)).pipe( - Effect.flatMap((subprocess) => - Effect.callback((resume) => { - const timer = setTimeout(() => resume(Effect.void), 500) - subprocess.on("error", (err) => { + const s = yield* InstanceState.get(state) + yield* auth.clearOAuthState(mcpName) + return yield* storeClient(s, mcpName, client, listed, mcpConfig.timeout) + } + + const callbackPromise = McpOAuthCallback.waitForCallback(result.oauthState, mcpName) + + yield* Effect.tryPromise(() => open(result.authorizationUrl)).pipe( + Effect.flatMap((subprocess) => + Effect.callback((resume) => { + const timer = setTimeout(() => resume(Effect.void), 500) + subprocess.on("error", (err) => { + clearTimeout(timer) + resume(Effect.fail(err)) + }) + subprocess.on("exit", (code) => { + if (code !== null && code !== 0) { clearTimeout(timer) - resume(Effect.fail(err)) - }) - subprocess.on("exit", (code) => { - if (code !== null && code !== 0) { - clearTimeout(timer) - resume(Effect.fail(new Error(`Browser open failed with exit code ${code}`))) - } - }) - }), - ), - Effect.catch(() => { - return events.publish(BrowserOpenFailed, { mcpName, url: result.authorizationUrl }).pipe(Effect.ignore) + resume(Effect.fail(new Error(`Browser open failed with exit code ${code}`))) + } + }) }), - ) + ), + Effect.catch(() => { + return events.publish(BrowserOpenFailed, { mcpName, url: result.authorizationUrl }).pipe(Effect.ignore) + }), + ) - const code = yield* Effect.promise(() => callbackPromise) + const code = yield* Effect.promise(() => callbackPromise) - const storedState = yield* auth.getOAuthState(mcpName) - if (storedState !== result.oauthState) { - yield* auth.clearOAuthState(mcpName) - throw new Error("OAuth state mismatch - potential CSRF attack") - } + const storedState = yield* auth.getOAuthState(mcpName) + if (storedState !== result.oauthState) { yield* auth.clearOAuthState(mcpName) - return yield* finishAuth(mcpName, code) - }).pipe(Effect.ensuring(Effect.sync(() => McpOAuthCallback.cancelPending(mcpName)))) + throw new Error("OAuth state mismatch - potential CSRF attack") + } + yield* auth.clearOAuthState(mcpName) + return yield* finishAuth(mcpName, code) }) const finishAuth = Effect.fn("MCP.finishAuth")(function* (mcpName: string, authorizationCode: string) { diff --git a/packages/opencode/src/mcp/oauth-callback.ts b/packages/opencode/src/mcp/oauth-callback.ts index 90aa3d4ba9e7..d91598a42f09 100644 --- a/packages/opencode/src/mcp/oauth-callback.ts +++ b/packages/opencode/src/mcp/oauth-callback.ts @@ -54,9 +54,6 @@ interface PendingAuth { } let server: ReturnType | undefined -let starting: Promise | undefined -let stopping: Promise | undefined -let idleStopRequested = false const pendingAuths = new Map() // Reverse index: mcpName → oauthState, so cancelPending(mcpName) can // find the right entry in pendingAuths (which is keyed by oauthState). @@ -74,20 +71,10 @@ function cleanupStateIndex(oauthState: string) { } function stopIfIdle() { - if (pendingAuths.size > 0) return - if (starting || stopping) { - idleStopRequested = true - return - } - if (!server) return + if (pendingAuths.size > 0 || !server) return - const current = server + server.close() server = undefined - idleStopRequested = false - stopping = new Promise((resolve) => current.close(() => resolve())).finally(() => { - stopping = undefined - if (idleStopRequested) stopIfIdle() - }) } function handleRequest(req: import("http").IncomingMessage, res: import("http").ServerResponse) { @@ -157,12 +144,6 @@ export async function ensureRunning(redirectUri?: string): Promise { // Parse the redirect URI to get port and path (uses defaults if not provided) const { port, path } = parseRedirectUri(redirectUri) - if (stopping) await stopping - if (starting) { - await starting - return ensureRunning(redirectUri) - } - // If server is running on a different port/path, stop it first if (server && (currentPort !== port || currentPath !== path)) { await stop() @@ -170,31 +151,24 @@ export async function ensureRunning(redirectUri?: string): Promise { if (server) return - starting = (async () => { - if (await isPortInUse(port)) return + const running = await isPortInUse(port) + if (running) { + return + } - currentPort = port - currentPath = path + currentPort = port + currentPath = path - const next = createServer(handleRequest) - await new Promise((resolve, reject) => { - next.listen(currentPort, () => resolve()) - next.on("error", reject) + server = createServer(handleRequest) + await new Promise((resolve, reject) => { + server!.listen(currentPort, () => { + resolve() }) - server = next - })() - await starting.finally(() => { - starting = undefined + server!.on("error", reject) }) - if (idleStopRequested) stopIfIdle() } -export function waitForCallback( - oauthState: string, - mcpName?: string, - timeoutMs: number = CALLBACK_TIMEOUT_MS, -): Promise { - idleStopRequested = false +export function waitForCallback(oauthState: string, mcpName?: string): Promise { if (mcpName) mcpNameToState.set(mcpName, oauthState) return new Promise((resolve, reject) => { const timeout = setTimeout(() => { @@ -204,7 +178,7 @@ export function waitForCallback( reject(new Error("OAuth callback timeout - authorization took too long")) stopIfIdle() } - }, timeoutMs) + }, CALLBACK_TIMEOUT_MS) pendingAuths.set(oauthState, { resolve, reject, timeout }) }) @@ -238,13 +212,9 @@ export async function isPortInUse(port: number = OAUTH_CALLBACK_PORT): Promise { - if (starting) await starting - if (stopping) await stopping - if (server) { - const current = server + await new Promise((resolve) => server!.close(() => resolve())) server = undefined - await new Promise((resolve) => current.close(() => resolve())) } for (const [_name, pending] of pendingAuths) { @@ -253,7 +223,6 @@ export async function stop(): Promise { } pendingAuths.clear() mcpNameToState.clear() - idleStopRequested = false } export function isRunning(): boolean { diff --git a/packages/opencode/test/mcp/oauth-callback.test.ts b/packages/opencode/test/mcp/oauth-callback.test.ts index 8fadeb59753c..9048ee3bf942 100644 --- a/packages/opencode/test/mcp/oauth-callback.test.ts +++ b/packages/opencode/test/mcp/oauth-callback.test.ts @@ -32,14 +32,8 @@ describe("McpOAuthCallback.ensureRunning", () => { await McpOAuthCallback.ensureRunning("http://127.0.0.1:18000/custom/callback") expect(McpOAuthCallback.isRunning()).toBe(true) }) -}) - -describe("McpOAuthCallback lifecycle", () => { - afterEach(async () => { - await McpOAuthCallback.stop() - }) - test("stops after a successful callback response completes", async () => { + test("stops after the callback completes", async () => { const port = await availablePort() await McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) const callback = McpOAuthCallback.waitForCallback("success") @@ -48,89 +42,7 @@ describe("McpOAuthCallback lifecycle", () => { expect(response.status).toBe(200) expect(await callback).toBe("code") - await waitForStop() - }) - - test("stops after a provider error", async () => { - const port = await availablePort() - await McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) - const callback = McpOAuthCallback.waitForCallback("provider-error") - const error = callback.catch((cause) => cause) - - const response = await fetch( - `http://127.0.0.1:${port}/callback?error=access_denied&error_description=Denied&state=provider-error`, - ) - - expect(response.status).toBe(200) - await response.text() - expect(await error).toEqual(new Error("Denied")) - await waitForStop() - }) - - test("stops after cancellation", async () => { - const port = await availablePort() - await McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) - const callback = McpOAuthCallback.waitForCallback("cancelled", "server") - - McpOAuthCallback.cancelPending("server") - - await expect(callback).rejects.toThrow("Authorization cancelled") - await waitForStop() - }) - - test("stops when cancellation races listener startup", async () => { - const port = await availablePort() - const callback = McpOAuthCallback.waitForCallback("cancelled", "starting-server") - const starting = McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) - - McpOAuthCallback.cancelPending("starting-server") - - await expect(callback).rejects.toThrow("Authorization cancelled") - await starting - await waitForStop() - }) - - test("stops after timeout", async () => { - const port = await availablePort() - await McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) - const callback = McpOAuthCallback.waitForCallback("timeout", undefined, 10) - - await expect(callback).rejects.toThrow("OAuth callback timeout") - await waitForStop() - }) - - test("stays running until all simultaneous callbacks settle", async () => { - const port = await availablePort() - const redirectUri = `http://127.0.0.1:${port}/callback` - await Promise.all([McpOAuthCallback.ensureRunning(redirectUri), McpOAuthCallback.ensureRunning(redirectUri)]) - const first = McpOAuthCallback.waitForCallback("first") - const second = McpOAuthCallback.waitForCallback("second") - - await fetch(`${redirectUri}?code=one&state=first`) - expect(await first).toBe("one") - expect(McpOAuthCallback.isRunning()).toBe(true) - - await fetch(`${redirectUri}?code=two&state=second`) - expect(await second).toBe("two") - await waitForStop() - }) - - test("restarts when a callback is registered during shutdown", async () => { - const port = await availablePort() - const redirectUri = `http://127.0.0.1:${port}/callback` - await McpOAuthCallback.ensureRunning(redirectUri) - const first = McpOAuthCallback.waitForCallback("first") - - await fetch(`${redirectUri}?code=one&state=first`) - expect(await first).toBe("one") - - const second = McpOAuthCallback.waitForCallback("second") - await McpOAuthCallback.ensureRunning(redirectUri) - const response = await fetch(`${redirectUri}?code=two&state=second`) - - expect(response.status).toBe(200) - expect(await second).toBe("two") - await waitForStop() + expect(McpOAuthCallback.isRunning()).toBe(false) }) }) @@ -142,8 +54,3 @@ async function availablePort() { await new Promise((resolve) => probe.close(() => resolve())) return address.port } - -async function waitForStop() { - for (let attempt = 0; attempt < 50 && McpOAuthCallback.isRunning(); attempt++) await Bun.sleep(10) - expect(McpOAuthCallback.isRunning()).toBe(false) -} From a3851c4d50914bcb91616768f04790379b8963ac Mon Sep 17 00:00:00 2001 From: Aiden Cline Date: Sun, 14 Jun 2026 22:24:40 -0400 Subject: [PATCH 3/3] test(mcp): simplify callback cleanup coverage --- .../opencode/test/mcp/oauth-callback.test.ts | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/packages/opencode/test/mcp/oauth-callback.test.ts b/packages/opencode/test/mcp/oauth-callback.test.ts index 512d470f79d2..cac7146580de 100644 --- a/packages/opencode/test/mcp/oauth-callback.test.ts +++ b/packages/opencode/test/mcp/oauth-callback.test.ts @@ -1,5 +1,4 @@ import { test, expect, describe, afterEach } from "bun:test" -import { createServer } from "http" import { McpOAuthCallback } from "../../src/mcp/oauth-callback" import { parseRedirectUri } from "../../src/mcp/oauth-provider" @@ -34,11 +33,11 @@ describe("McpOAuthCallback.ensureRunning", () => { }) test("stops after the callback completes", async () => { - const port = await availablePort() - await McpOAuthCallback.ensureRunning(`http://127.0.0.1:${port}/callback`) + const redirectUri = "http://127.0.0.1:18003/custom/callback" + await McpOAuthCallback.ensureRunning(redirectUri) const callback = McpOAuthCallback.waitForCallback("success") - const response = await fetch(`http://127.0.0.1:${port}/callback?code=code&state=success`) + const response = await fetch(`${redirectUri}?code=code&state=success`) expect(response.status).toBe(200) expect(await callback).toBe("code") @@ -71,12 +70,3 @@ describe("McpOAuthCallback.ensureRunning", () => { expect(await response.text()).toContain('
The user denied access
') }) }) - -async function availablePort() { - const probe = createServer() - await new Promise((resolve) => probe.listen(0, "127.0.0.1", resolve)) - const address = probe.address() - if (!address || typeof address === "string") throw new Error("Failed to allocate callback test port") - await new Promise((resolve) => probe.close(() => resolve())) - return address.port -}