diff --git a/.changeset/oauth-await-longpoll.md b/.changeset/oauth-await-longpoll.md new file mode 100644 index 000000000..b402e78ab --- /dev/null +++ b/.changeset/oauth-await-longpoll.md @@ -0,0 +1,10 @@ +--- +"@executor-js/local": patch +"@executor-js/react": patch +--- + +**Desktop OAuth connects finish the moment the provider redirects** + +When the desktop app runs an OAuth flow in the system browser, the app learned about completion by polling the local server once a second. The completed result sat in memory while the user watched the "Connecting…" spinner for up to a second more — about half a second wasted on average, on every connect. + +The await endpoint now long-polls: the server holds the request open (up to 25 seconds per hold) and answers the instant the flow completes. The client polls one request at a time and reconnects after each answer, so requests never stack. Mixed versions stay compatible in both directions: an old client still gets its answer within one poll of a new server, and a new client against an old server behaves exactly as before. diff --git a/apps/local/src/oauth-result-store.test.ts b/apps/local/src/oauth-result-store.test.ts new file mode 100644 index 000000000..97d3c63e7 --- /dev/null +++ b/apps/local/src/oauth-result-store.test.ts @@ -0,0 +1,222 @@ +import { afterEach, describe, expect, it } from "@effect/vitest"; +// oxlint-disable-next-line executor/no-vitest-import -- boundary: deterministic timer control comes from vitest itself +import { vi } from "vitest"; +import { OAUTH_POPUP_MESSAGE_TYPE, type OAuthPopupResult } from "@executor-js/sdk"; + +import { + __oauthAwaitHeldWaiterTotalForTests, + __oauthAwaitWaiterCountForTests, + __resetOAuthResultStoreForTests, + consumeOAuthResult, + publishOAuthResult, + waitForOAuthResult, +} from "./oauth-result-store"; + +const sampleResult = (sessionId: string): OAuthPopupResult => ({ + type: OAUTH_POPUP_MESSAGE_TYPE, + ok: false, + sessionId, + error: "access denied", +}); + +afterEach(() => { + __resetOAuthResultStoreForTests(); + vi.useRealTimers(); +}); + +// Waiter registration, publish wake-ups, aborts, and the over-cap instant +// answer are all synchronous, so every ordering below is exact — no sleeps. + +describe("waitForOAuthResult", () => { + it("resolves immediately and consumes when the result is already published", async () => { + publishOAuthResult(sampleResult("s-ready")); + + const result = await waitForOAuthResult("s-ready", { timeoutMs: 5000 }); + + expect(result).toMatchObject({ sessionId: "s-ready" }); + // One-shot: the wait consumed the entry. + expect(consumeOAuthResult("s-ready")).toBeNull(); + expect(__oauthAwaitWaiterCountForTests("s-ready")).toBe(0); + }); + + it("resolves a held wait the moment the result is published", async () => { + const pending = waitForOAuthResult("s-mid", { timeoutMs: 5000 }); + expect(__oauthAwaitWaiterCountForTests("s-mid")).toBe(1); + + const publishedAt = Date.now(); + publishOAuthResult(sampleResult("s-mid")); + const result = await pending; + + // Resolved by the publish, not the 5s deadline. + expect(Date.now() - publishedAt).toBeLessThan(1000); + expect(result).toMatchObject({ sessionId: "s-mid" }); + expect(consumeOAuthResult("s-mid")).toBeNull(); + expect(__oauthAwaitWaiterCountForTests("s-mid")).toBe(0); + }); + + it("returns null at the deadline and removes the waiter", async () => { + vi.useFakeTimers(); + const pending = waitForOAuthResult("s-deadline", { timeoutMs: 25_000 }); + expect(__oauthAwaitWaiterCountForTests("s-deadline")).toBe(1); + + vi.advanceTimersByTime(25_000); + + expect(await pending).toBeNull(); + expect(__oauthAwaitWaiterCountForTests("s-deadline")).toBe(0); + }); + + it("resolves immediately when the signal is already aborted", async () => { + const controller = new AbortController(); + controller.abort(); + + const result = await waitForOAuthResult("s-pre-aborted", { + timeoutMs: 5000, + signal: controller.signal, + }); + + expect(result).toBeNull(); + expect(__oauthAwaitWaiterCountForTests("s-pre-aborted")).toBe(0); + }); +}); + +describe("waitForOAuthResult publish/abort races", () => { + it("abort settles first; a publish arriving after leaves the result for the next consumer", async () => { + const controller = new AbortController(); + const pending = waitForOAuthResult("s-abort-first", { + timeoutMs: 5000, + signal: controller.signal, + }); + expect(__oauthAwaitWaiterCountForTests("s-abort-first")).toBe(1); + + controller.abort(); + expect(await pending).toBeNull(); + expect(__oauthAwaitWaiterCountForTests("s-abort-first")).toBe(0); + + // The publish lands after the abort settled: the dead waiter must not + // consume it — it stays in the store for the client's next poll. + publishOAuthResult(sampleResult("s-abort-first")); + expect(consumeOAuthResult("s-abort-first")).toMatchObject({ sessionId: "s-abort-first" }); + }); + + it("publish resolves the waiter; an abort firing immediately after does not double-consume", async () => { + const controller = new AbortController(); + const pending = waitForOAuthResult("s-pub-first", { + timeoutMs: 5000, + signal: controller.signal, + }); + expect(__oauthAwaitWaiterCountForTests("s-pub-first")).toBe(1); + + publishOAuthResult(sampleResult("s-pub-first")); + controller.abort(); + + expect(await pending).toMatchObject({ sessionId: "s-pub-first" }); + expect(__oauthAwaitWaiterCountForTests("s-pub-first")).toBe(0); + + // The late abort must not have consumed or dropped anything: a second + // publish for the session is still delivered intact. + publishOAuthResult(sampleResult("s-pub-first")); + expect(consumeOAuthResult("s-pub-first")).toMatchObject({ sessionId: "s-pub-first" }); + }); +}); + +describe("waitForOAuthResult publish/timeout races", () => { + it("deadline fires first; a publish just after leaves the result consumable", async () => { + vi.useFakeTimers(); + const pending = waitForOAuthResult("s-late-pub", { timeoutMs: 25_000 }); + expect(__oauthAwaitWaiterCountForTests("s-late-pub")).toBe(1); + + vi.advanceTimersByTime(25_000); + expect(await pending).toBeNull(); + expect(__oauthAwaitWaiterCountForTests("s-late-pub")).toBe(0); + + publishOAuthResult(sampleResult("s-late-pub")); + expect(consumeOAuthResult("s-late-pub")).toMatchObject({ sessionId: "s-late-pub" }); + }); + + it("publish resolves the waiter; the stale deadline timer is inert afterwards", async () => { + vi.useFakeTimers(); + const pending = waitForOAuthResult("s-early-pub", { timeoutMs: 25_000 }); + expect(__oauthAwaitWaiterCountForTests("s-early-pub")).toBe(1); + + publishOAuthResult(sampleResult("s-early-pub")); + expect(await pending).toMatchObject({ sessionId: "s-early-pub" }); + expect(__oauthAwaitWaiterCountForTests("s-early-pub")).toBe(0); + + // Run the clock past the original deadline: the settled waiter's timer + // was cleared, so nothing re-fires, re-registers, or consumes again. + vi.advanceTimersByTime(25_000); + expect(__oauthAwaitWaiterCountForTests("s-early-pub")).toBe(0); + publishOAuthResult(sampleResult("s-early-pub")); + expect(consumeOAuthResult("s-early-pub")).toMatchObject({ sessionId: "s-early-pub" }); + }); +}); + +describe("waitForOAuthResult held-waiter caps", () => { + it("holds one waiter per session; stacked requests answer null immediately (old-client behavior)", async () => { + // An unpatched desktop client polls every second and would stack ~25 + // concurrent requests per flow against a holding server. Only the first + // may hold; the rest get the pre-long-poll instant "still pending". + const held = waitForOAuthResult("s-stack", { timeoutMs: 5000 }); + expect(__oauthAwaitWaiterCountForTests("s-stack")).toBe(1); + + const stacked = [ + waitForOAuthResult("s-stack", { timeoutMs: 5000 }), + waitForOAuthResult("s-stack", { timeoutMs: 5000 }), + waitForOAuthResult("s-stack", { timeoutMs: 5000 }), + ]; + // The stacked requests settle null BEFORE any publish — instant answers. + for (const request of stacked) expect(await request).toBeNull(); + expect(__oauthAwaitWaiterCountForTests("s-stack")).toBe(1); + expect(__oauthAwaitHeldWaiterTotalForTests()).toBe(1); + + publishOAuthResult(sampleResult("s-stack")); + // Exactly one consumer receives the one-shot result: the held waiter. + expect(await held).toMatchObject({ sessionId: "s-stack" }); + expect(consumeOAuthResult("s-stack")).toBeNull(); + // No leaked waiters after the flow. + expect(__oauthAwaitWaiterCountForTests("s-stack")).toBe(0); + expect(__oauthAwaitHeldWaiterTotalForTests()).toBe(0); + }); + + it("re-arms the per-session hold after the held waiter settles", async () => { + const controller = new AbortController(); + const first = waitForOAuthResult("s-rearm", { timeoutMs: 5000, signal: controller.signal }); + // Over the per-session cap while the first is held. + expect(await waitForOAuthResult("s-rearm", { timeoutMs: 5000 })).toBeNull(); + + controller.abort(); + expect(await first).toBeNull(); + expect(__oauthAwaitWaiterCountForTests("s-rearm")).toBe(0); + + // The slot is free again: the next request holds and gets the result. + const second = waitForOAuthResult("s-rearm", { timeoutMs: 5000 }); + expect(__oauthAwaitWaiterCountForTests("s-rearm")).toBe(1); + publishOAuthResult(sampleResult("s-rearm")); + expect(await second).toMatchObject({ sessionId: "s-rearm" }); + expect(__oauthAwaitWaiterCountForTests("s-rearm")).toBe(0); + }); + + it("caps total held waiters globally; over-cap sessions answer instantly but stored results still deliver", async () => { + const held = Array.from({ length: 64 }, (_, index) => + waitForOAuthResult(`s-global-${index}`, { timeoutMs: 5000 }), + ); + expect(__oauthAwaitHeldWaiterTotalForTests()).toBe(64); + + // A distinct pending session over the global cap answers null instantly + // instead of holding. + expect(await waitForOAuthResult("s-global-over", { timeoutMs: 5000 })).toBeNull(); + expect(__oauthAwaitHeldWaiterTotalForTests()).toBe(64); + + // Over-cap behavior matches the pre-long-poll server exactly: a result + // already in the store is still consumed and answered immediately. + publishOAuthResult(sampleResult("s-global-stored")); + expect(await waitForOAuthResult("s-global-stored", { timeoutMs: 5000 })).toMatchObject({ + sessionId: "s-global-stored", + }); + + // Settle every held waiter and confirm the registry drains completely. + __resetOAuthResultStoreForTests(); + for (const request of held) expect(await request).toBeNull(); + expect(__oauthAwaitHeldWaiterTotalForTests()).toBe(0); + }); +}); diff --git a/apps/local/src/oauth-result-store.ts b/apps/local/src/oauth-result-store.ts index c9023368d..89698d4ca 100644 --- a/apps/local/src/oauth-result-store.ts +++ b/apps/local/src/oauth-result-store.ts @@ -28,6 +28,37 @@ const RESULT_TTL_MS = 10 * 60 * 1000; // 10 minutes — long enough for slow MFA const store = new Map(); +/** + * Long-poll waiters, keyed by sessionId. Each entry is the wake callback for + * the single held `/api/oauth/await/:sessionId` request for that session. + * `publishOAuthResult` wakes the waiter, which runs `consumeOAuthResult` and + * answers with the result. + * + * Held waiters are bounded — each pins a connection, a registry entry, and a + * deadline timer, and the route only needs a bearer, so unbounded holds are a + * DoS surface. At most one waiter is held per session (the patched client + * polls sequentially; an unpatched client polling every second would + * otherwise stack ~25 holds per flow against a 25s deadline), and at most + * `MAX_HELD_WAITERS_TOTAL` across all sessions. Over either bound, + * `waitForOAuthResult` degrades to the pre-long-poll behavior: it answers the + * current store value immediately (null = still pending) instead of holding, + * so over-cap callers see exactly what an old server would have sent. + */ +const MAX_HELD_WAITERS_TOTAL = 64; + +const waiters = new Map void>(); + +const removeWaiter = (sessionId: string, wake: () => void): void => { + if (waiters.get(sessionId) === wake) waiters.delete(sessionId); +}; + +const wakeWaiter = (sessionId: string): void => { + const wake = waiters.get(sessionId); + if (!wake) return; + waiters.delete(sessionId); + wake(); +}; + const cleanupExpired = (now: number) => { for (const [sessionId, entry] of store) { if (entry.expiresAt < now) store.delete(sessionId); @@ -44,6 +75,7 @@ export const publishOAuthResult = (result: AnyResult): void => { const now = Date.now(); cleanupExpired(now); store.set(sessionId, { result, expiresAt: now + RESULT_TTL_MS }); + wakeWaiter(sessionId); }; /** @@ -59,7 +91,57 @@ export const consumeOAuthResult = (sessionId: string): AnyResult | null => { return entry.result; }; -/** Test-only — clears the entire store between tests. */ +/** + * Long-poll for a result. Consumes and resolves immediately when a result + * is already stored; otherwise holds until `publishOAuthResult` fires for + * the sessionId, the deadline elapses, or `signal` aborts (client gone). + * The latter two resolve `null` — the same "still pending" answer an + * immediate poll gives — so the caller's retry loop keeps working. A + * waiter that times out or aborts is always removed from the registry. + * + * Holding is bounded (see the waiter registry above): when the session + * already has a held waiter, or the global held-waiter ceiling is reached, + * this answers `null` immediately instead of holding. + */ +export const waitForOAuthResult = ( + sessionId: string, + opts: { readonly timeoutMs: number; readonly signal?: AbortSignal }, +): Promise => { + const immediate = consumeOAuthResult(sessionId); + if (immediate !== null) return Promise.resolve(immediate); + if (opts.timeoutMs <= 0 || opts.signal?.aborted === true) return Promise.resolve(null); + if (waiters.has(sessionId) || waiters.size >= MAX_HELD_WAITERS_TOTAL) { + return Promise.resolve(null); + } + + return new Promise((resolve) => { + let done = false; + const finish = (result: AnyResult | null) => { + if (done) return; + done = true; + clearTimeout(timer); + opts.signal?.removeEventListener("abort", onAbort); + removeWaiter(sessionId, wake); + resolve(result); + }; + const wake = () => finish(consumeOAuthResult(sessionId)); + const onAbort = () => finish(null); + const timer = setTimeout(() => finish(null), opts.timeoutMs); + + waiters.set(sessionId, wake); + opts.signal?.addEventListener("abort", onAbort, { once: true }); + }); +}; + +/** Test-only — clears the store and resolves any held waiters as pending. */ export const __resetOAuthResultStoreForTests = (): void => { store.clear(); + for (const sessionId of [...waiters.keys()]) wakeWaiter(sessionId); }; + +/** Test-only — number of held long-poll waiters for a sessionId (0 or 1). */ +export const __oauthAwaitWaiterCountForTests = (sessionId: string): number => + waiters.has(sessionId) ? 1 : 0; + +/** Test-only — total held long-poll waiters across all sessions. */ +export const __oauthAwaitHeldWaiterTotalForTests = (): number => waiters.size; diff --git a/apps/local/src/serve.test.ts b/apps/local/src/serve.test.ts index b7c7e1fc9..9492c792d 100644 --- a/apps/local/src/serve.test.ts +++ b/apps/local/src/serve.test.ts @@ -3,7 +3,16 @@ import { existsSync, mkdirSync, mkdtempSync, rmSync, statSync, writeFileSync } f import { tmpdir } from "node:os"; import { join } from "node:path"; +import { OAUTH_POPUP_MESSAGE_TYPE, type OAuthPopupResult } from "@executor-js/sdk"; + import { acquireDataDirOwnership } from "./db/data-dir-ownership"; +import { + __oauthAwaitHeldWaiterTotalForTests, + __oauthAwaitWaiterCountForTests, + __resetOAuthResultStoreForTests, + consumeOAuthResult, + publishOAuthResult, +} from "./oauth-result-store"; import { startServer, type ServerInstance } from "./serve"; let clientDir: string; @@ -219,6 +228,121 @@ describe("startServer bearer auth", () => { }); }); +describe("startServer OAuth await long-poll", () => { + afterEach(() => { + __resetOAuthResultStoreForTests(); + }); + + const untilWaiterCount = async (sessionId: string, count: number): Promise => { + const deadline = Date.now() + 2000; + while (__oauthAwaitWaiterCountForTests(sessionId) !== count && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 5)); + } + expect(__oauthAwaitWaiterCountForTests(sessionId)).toBe(count); + }; + + const awaitResult = (sessionId: string): OAuthPopupResult => ({ + type: OAUTH_POPUP_MESSAGE_TYPE, + ok: false, + sessionId, + error: "access denied", + }); + + it("holds the await request open and answers the moment the result publishes", async () => { + const baseUrl = await startTestServer(); + + const pending = fetch(`${baseUrl}/api/oauth/await/session-hold`, { + headers: { authorization: `Bearer ${TOKEN}` }, + }); + // The request is being held, not answered null immediately. + await untilWaiterCount("session-hold", 1); + + const publishedAt = Date.now(); + publishOAuthResult(awaitResult("session-hold")); + const response = await pending; + const body = (await response.json()) as unknown; + + // Resolved by the publish, not a poll tick or the 25s deadline. + expect(Date.now() - publishedAt).toBeLessThan(1000); + expect(body).toMatchObject({ sessionId: "session-hold", ok: false }); + expect(__oauthAwaitWaiterCountForTests("session-hold")).toBe(0); + }); + + it("answers a pre-published result without waiting", async () => { + const baseUrl = await startTestServer(); + publishOAuthResult(awaitResult("session-ready")); + + const response = await fetch(`${baseUrl}/api/oauth/await/session-ready`, { + headers: { authorization: `Bearer ${TOKEN}` }, + }); + const body = (await response.json()) as unknown; + + expect(body).toMatchObject({ sessionId: "session-ready" }); + // One-shot: a second poll for the same session finds nothing. + expect(consumeOAuthResult("session-ready")).toBeNull(); + }); + + it("drops the waiter when the client disconnects", async () => { + const baseUrl = await startTestServer(); + + const controller = new AbortController(); + const pending = fetch(`${baseUrl}/api/oauth/await/session-gone`, { + headers: { authorization: `Bearer ${TOKEN}` }, + signal: controller.signal, + }); + await untilWaiterCount("session-gone", 1); + + controller.abort(); + await expect(pending).rejects.toThrow(); + await untilWaiterCount("session-gone", 0); + + // The dead waiter must not consume a result published afterwards. + publishOAuthResult(awaitResult("session-gone")); + expect(consumeOAuthResult("session-gone")).toMatchObject({ sessionId: "session-gone" }); + }); + + it("holds one request per session; stacked requests answer null instantly and one consumer wins", async () => { + // The mixed-version rollout hazard: an unpatched desktop client polls + // every second and would stack ~25 concurrent requests per flow against + // a holding server. Only the first request may hold — the rest must get + // the pre-long-poll instant "still pending" answer. + const baseUrl = await startTestServer(); + + const held = fetch(`${baseUrl}/api/oauth/await/session-stacked`, { + headers: { authorization: `Bearer ${TOKEN}` }, + }); + // Deterministic ordering: the first request is registered before any + // stacked request is issued. + await untilWaiterCount("session-stacked", 1); + + const stacked = await Promise.all( + [1, 2, 3].map(() => + fetch(`${baseUrl}/api/oauth/await/session-stacked`, { + headers: { authorization: `Bearer ${TOKEN}` }, + }), + ), + ); + // The stacked requests settled BEFORE any publish — instant null answers, + // not held connections. + for (const response of stacked) { + expect(response.status).toBe(200); + expect(await response.json()).toBeNull(); + } + expect(__oauthAwaitWaiterCountForTests("session-stacked")).toBe(1); + expect(__oauthAwaitHeldWaiterTotalForTests()).toBe(1); + + publishOAuthResult(awaitResult("session-stacked")); + const response = await held; + // Exactly one consumer overall receives the one-shot result: the held + // request. Nothing is left behind for a later poll. + expect(await response.json()).toMatchObject({ sessionId: "session-stacked" }); + expect(consumeOAuthResult("session-stacked")).toBeNull(); + // The registry drains completely — no leaked waiters. + expect(__oauthAwaitWaiterCountForTests("session-stacked")).toBe(0); + expect(__oauthAwaitHeldWaiterTotalForTests()).toBe(0); + }); +}); + describe("startServer CORS hardening", () => { it("reflects credentialed CORS only for allowed loopback origins", async () => { const baseUrl = await startTestServer(); diff --git a/apps/local/src/serve.ts b/apps/local/src/serve.ts index 95e8b7705..71725b09c 100644 --- a/apps/local/src/serve.ts +++ b/apps/local/src/serve.ts @@ -13,7 +13,7 @@ import type { Subprocess } from "bun"; import { setOAuthCompletionListener } from "@executor-js/api"; import { oauthClientIdMetadataDocumentFromRequest } from "@executor-js/api/server"; import { loadOrMintLocalAuthToken } from "./auth"; -import { consumeOAuthResult, publishOAuthResult } from "./oauth-result-store"; +import { publishOAuthResult, waitForOAuthResult } from "./oauth-result-store"; import { disposeAnalytics } from "./analytics"; import { startIntegrationsRefresh } from "./integrations"; import { disposeServerHandlers, getServerHandlers } from "./main"; @@ -27,6 +27,11 @@ import { normalizeCredential, } from "./serve-shared"; +// How long one /api/oauth/await request is held open waiting for the OAuth +// completion to be published. Shorter than the client's overall timeout; the +// client reconnects after each deadline, so this only bounds a single hold. +const OAUTH_AWAIT_LONG_POLL_DEADLINE_MS = 25_000; + // --------------------------------------------------------------------------- // Static files // --------------------------------------------------------------------------- @@ -457,9 +462,20 @@ export async function startServer(opts: StartServerOptions = {}): Promise + HttpApiClient.make(api, { + baseUrl: new URL("/api", server.origin).toString(), + transformClient: HttpClient.mapRequest((request) => + HttpClientRequest.setHeader(request, "authorization", `Bearer ${server.token}`), + ), + }).pipe(Effect.provide(FetchHttpClient.layer)); + +interface AwaitAnswer { + /** When the server answered (response headers received). */ + readonly settledAt: number; + readonly status: number; + readonly body: unknown; +} + +/** One `/api/oauth/await/:sessionId` request, exactly as the desktop renderer + * issues it (bearer-gated GET). The promise settles only when the server + * answers — which is the behavior under test. */ +const issueAwait = (server: ServerHandle, sessionId: string): Promise => + fetch(`${server.origin}/api/oauth/await/${encodeURIComponent(sessionId)}`, { + headers: { authorization: `Bearer ${server.token}` }, + }).then(async (response) => { + const settledAt = Date.now(); + return { settledAt, status: response.status, body: (await response.json()) as unknown }; + }); + +/** Drive the test AS's consent (authorize → login) and return the app + * callback URL the provider redirects to — carrying code + state. Unlike the + * durability scenario this does NOT redeem the code by hand: landing the real + * callback is the exact path that publishes the result and wakes waiters. */ +const resolveProviderConsent = (authorizationUrl: string) => + Effect.promise(async () => { + const authorize = await fetch(authorizationUrl, { redirect: "manual" }); + const loginUrl = authorize.headers.get("location"); + if (!loginUrl) throw new Error(`authorize did not redirect: ${authorize.status}`); + const login = await fetch(loginUrl, { + method: "POST", + headers: { authorization: `Basic ${Buffer.from("alice:password").toString("base64")}` }, + redirect: "manual", + }); + const callbackUrl = login.headers.get("location"); + if (!callbackUrl) throw new Error(`login did not redirect: ${login.status}`); + return callbackUrl; + }); + +scenario( + "Local · a held OAuth await answers the moment the provider callback lands", + {}, + Effect.scoped( + Effect.gen(function* () { + const cli = yield* Cli; + const runDir = yield* RunDir; + + const oauth = yield* serveOAuthTestServer({ scopes: ["mcp.read"] }); + // A real MCP upstream that accepts the AS's bearers, so the connect is a + // genuine end-to-end flow, not a stub handshake. + const mcp = yield* serveMcpServer(() => makeEchoMcpServer({ name: "longpoll-mcp" }), { + auth: { + validateAuthorization: (authorization) => oauth.acceptsAuthorizationHeader(authorization), + }, + }); + + const suffix = randomBytes(4).toString("hex"); + const slug = IntegrationSlug.make(`oauth-longpoll-${suffix}`); + const clientSlug = OAuthClientSlug.make(`oauth-longpoll-client-${suffix}`); + + yield* withLocalServer(cli, runDir, (server) => + Effect.gen(function* () { + const client = yield* apiClient(server); + + yield* client.mcp.addServer({ + payload: { + transport: "remote", + name: "Longpoll MCP", + endpoint: mcp.url, + slug: String(slug), + authenticationTemplate: [{ kind: "oauth2" }], + }, + }); + yield* client.oauth.createClient({ + payload: { + owner: "org", + slug: clientSlug, + grant: "authorization_code", + authorizationUrl: oauth.authorizationEndpoint, + tokenUrl: oauth.tokenEndpoint, + clientId: "test-client", + clientSecret: "test-secret", + resource: oauth.mcpResourceUrl, + originIntegration: slug, + }, + }); + + const started = yield* client.oauth.start({ + payload: { + client: clientSlug, + clientOwner: "org", + owner: "org", + name, + integration: slug, + template, + }, + }); + expect(started.status, "oauth.start redirects to the authorization server").toBe( + "redirect", + ); + if (started.status !== "redirect") return yield* Effect.die("no redirect"); + // The await sessionId is the start state — exactly what the desktop + // renderer passes to openOAuthSystemBrowser. + const sessionId = started.state; + + // -- 2. Await BEFORE the callback: the request must be held. ------- + const heldAwait = issueAwait(server, sessionId); + const observed = yield* Effect.promise(() => + Promise.race([ + heldAwait.then(() => "answered" as const), + new Promise<"held">((resolve) => + setTimeout(() => resolve("held"), HELD_OBSERVATION_MS), + ), + ]), + ); + expect( + observed, + "an await issued mid-flow is held open, not answered null immediately (the pre-long-poll behavior)", + ).toBe("held"); + + // A second await while one is already held: over the one-per-session + // cap, so it answers null instantly — the pre-long-poll behavior an + // unpatched 1s-interval client relies on — instead of stacking a + // held connection. + const overCap = yield* Effect.promise(() => + Promise.race([ + issueAwait(server, sessionId), + new Promise<"held">((resolve) => + setTimeout(() => resolve("held"), HELD_OBSERVATION_MS), + ), + ]), + ); + expect(overCap, "an over-cap await answers immediately instead of holding").not.toBe( + "held", + ); + if (overCap === "held") return yield* Effect.die("over-cap await was held"); + expect(overCap.status, "an over-cap await answers 200 like an immediate poll").toBe(200); + expect( + overCap.body, + "an over-cap await answers null — still pending — exactly like the old server", + ).toBeNull(); + + // -- 3. Complete consent and land the real callback redirect. ------ + const redirected = yield* resolveProviderConsent(started.authorizationUrl); + // The harness boots `executor web --port 0` (OS-assigned), and the + // server builds the redirect URI from the CONFIGURED port — so the + // redirect reads `127.0.0.1:0`. Land the same callback path + query + // (code + state, the cryptographic gate) on the server's real bound + // origin; it is the same server and the same handler. + const callbackUrl = new URL(redirected); + expect( + callbackUrl.pathname, + "the provider redirects back to this server's OAuth callback", + ).toBe("/api/oauth/callback"); + const callbackStartedAt = Date.now(); + const callback = yield* Effect.promise(() => + fetch(`${server.origin}${callbackUrl.pathname}${callbackUrl.search}`), + ); + const callbackHtml = yield* Effect.promise(() => callback.text()); + const callbackCompletedAt = Date.now(); + expect(callback.status, "the OAuth callback succeeds").toBe(200); + expect(callbackHtml, "the callback page reports the connect succeeded").toContain( + "Connected", + ); + + // -- 4. The held request answers promptly with the one-shot result. + const answer = yield* Effect.promise(() => heldAwait); + expect(answer.status, "a held await answers 200 like an immediate poll").toBe(200); + expect( + answer.body, + "the held await receives the one-shot result the callback published", + ).toMatchObject({ ok: true, sessionId }); + + const sinceCallback = answer.settledAt - callbackCompletedAt; + // Console output is swallowed by the runner, so park the measured + // timings in the run dir as reviewable evidence. + writeFileSync( + join(runDir, "await-timing.json"), + JSON.stringify( + { + heldObservationMs: HELD_OBSERVATION_MS, + callbackHandlingMs: callbackCompletedAt - callbackStartedAt, + awaitsSettledAfterCallbackMs: sinceCallback, + budgetMs: PROMPT_RESOLUTION_BUDGET_MS, + }, + null, + 2, + ), + ); + expect( + sinceCallback, + "held awaits resolve promptly after the callback — well under the old 1s poll tick", + ).toBeLessThan(PROMPT_RESOLUTION_BUDGET_MS); + }), + ); + }), + ), +); diff --git a/packages/react/src/api/oauth-popup.test.ts b/packages/react/src/api/oauth-popup.test.ts index d27ec973c..04c0a77c8 100644 --- a/packages/react/src/api/oauth-popup.test.ts +++ b/packages/react/src/api/oauth-popup.test.ts @@ -1,6 +1,12 @@ import { describe, expect, it } from "@effect/vitest"; -import { openOAuthPopup, reserveOAuthPopup } from "./oauth-popup"; +import { + OAUTH_POPUP_MESSAGE_TYPE, + openOAuthPopup, + openOAuthSystemBrowser, + reserveOAuthPopup, + type OAuthPopupResult, +} from "./oauth-popup"; type OAuthPopupTestWindow = { readonly screenX: number; @@ -228,3 +234,125 @@ describe("openOAuthPopup", () => { expect(closedCalled).toBe(false); }); }); + +describe("openOAuthSystemBrowser", () => { + const sessionResult = (sessionId: string): OAuthPopupResult => ({ + type: OAUTH_POPUP_MESSAGE_TYPE, + ok: false, + sessionId, + error: "access denied", + }); + + const waitFor = async (predicate: () => boolean): Promise => { + const deadline = Date.now() + 2000; + while (!predicate() && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 5)); + } + expect(predicate()).toBe(true); + }; + + const withFakeFetch = async (fake: typeof fetch, run: () => Promise): Promise => { + const previousFetch = globalThis.fetch; + globalThis.fetch = fake; + // oxlint-disable-next-line executor/no-try-catch-or-throw -- test boundary: always restore the global fetch + try { + await run(); + } finally { + globalThis.fetch = previousFetch; + } + }; + + it("never stacks overlapping await requests while the server holds one open", async () => { + let inFlight = 0; + let maxInFlight = 0; + let calls = 0; + let received: unknown = null; + + // Simulates a long-polling server: the request is held ~40ms, then + // answers with the completed result. + const fake: typeof fetch = async () => { + calls += 1; + inFlight += 1; + maxInFlight = Math.max(maxInFlight, inFlight); + await new Promise((resolve) => setTimeout(resolve, 40)); + inFlight -= 1; + return new Response(JSON.stringify(sessionResult("s-hold"))); + }; + + await withFakeFetch(fake, async () => { + const teardown = openOAuthSystemBrowser({ + url: "https://auth.example/authorize", + sessionId: "s-hold", + openExternal: async () => {}, + onResult: (result) => { + received = result; + }, + // Far shorter than the held request: the old setInterval loop would + // stack many concurrent requests here. + pollMs: 1, + }); + await waitFor(() => received !== null); + teardown(); + }); + + expect(calls).toBe(1); + expect(maxInFlight).toBe(1); + expect(received).toMatchObject({ sessionId: "s-hold" }); + }); + + it("reconnects after a pending (null) answer and delivers the next result", async () => { + let calls = 0; + let received: unknown = null; + + // First request answers "still pending" (an old server, or a long-poll + // deadline); the second delivers the result. + const fake: typeof fetch = async () => { + calls += 1; + const body = calls === 1 ? null : sessionResult("s-retry"); + return new Response(JSON.stringify(body)); + }; + + await withFakeFetch(fake, async () => { + const teardown = openOAuthSystemBrowser({ + url: "https://auth.example/authorize", + sessionId: "s-retry", + openExternal: async () => {}, + onResult: (result) => { + received = result; + }, + pollMs: 1, + }); + await waitFor(() => received !== null); + teardown(); + }); + + expect(calls).toBe(2); + expect(received).toMatchObject({ sessionId: "s-retry" }); + }); + + it("aborts the in-flight await request on teardown", async () => { + const captured: { signal: AbortSignal | null } = { signal: null }; + + // Held open indefinitely; on abort it answers "still pending" the way a + // real aborted fetch stops mattering — the loop is already settled. + const fake: typeof fetch = (_input, init) => + new Promise((resolve) => { + captured.signal = init?.signal ?? null; + init?.signal?.addEventListener("abort", () => resolve(new Response("null"))); + }); + + await withFakeFetch(fake, async () => { + const teardown = openOAuthSystemBrowser({ + url: "https://auth.example/authorize", + sessionId: "s-abort", + openExternal: async () => {}, + onResult: () => {}, + }); + await waitFor(() => captured.signal !== null); + teardown(); + await waitFor(() => captured.signal?.aborted === true); + }); + + expect(captured.signal?.aborted).toBe(true); + }); +}); diff --git a/packages/react/src/api/oauth-popup.ts b/packages/react/src/api/oauth-popup.ts index 3c9bb7834..8a8e71604 100644 --- a/packages/react/src/api/oauth-popup.ts +++ b/packages/react/src/api/oauth-popup.ts @@ -238,7 +238,13 @@ export type OpenOAuthSystemBrowserInput = { readonly onResult: (data: OAuthPopupResult) => void; /** Called once if the external open itself fails (URL rejected, IPC error). */ readonly onOpenFailed?: (cause: unknown) => void; - /** Poll cadence. Default 1000ms. */ + /** + * Delay between poll attempts, applied only after the previous request + * settles (requests never overlap). New local servers long-poll the await + * route and answer the moment the flow completes, so this is a reconnect + * cadence; old servers answer immediately, making this a plain poll + * interval as before. Default 1000ms. + */ readonly pollMs?: number; /** Stop polling after this many ms with no result. Default 10 minutes. */ readonly timeoutMs?: number; @@ -252,14 +258,14 @@ export const openOAuthSystemBrowser = ( input: OpenOAuthSystemBrowserInput, ): (() => void) => { let settled = false; - let pollHandle: ReturnType | null = null; + let pollTimer: ReturnType | null = null; let timeoutHandle: ReturnType | null = null; const controller = new AbortController(); const settle = () => { if (settled) return; settled = true; - if (pollHandle !== null) clearInterval(pollHandle); + if (pollTimer !== null) clearTimeout(pollTimer); if (timeoutHandle !== null) clearTimeout(timeoutHandle); controller.abort(); }; @@ -299,8 +305,23 @@ export const openOAuthSystemBrowser = ( } })(); - pollHandle = setInterval(() => void poll(), input.pollMs ?? OAUTH_AWAIT_DEFAULT_POLL_MS); - void poll(); + // Sequential poll loop: one request at a time, reconnecting `pollMs` after + // the previous one settles. A long-polling server may hold each request for + // many seconds, so a fixed interval would stack overlapping requests. + const scheduleReconnect = () => { + if (settled) return; + pollTimer = setTimeout(() => { + pollTimer = null; + void runPoll(); + }, input.pollMs ?? OAUTH_AWAIT_DEFAULT_POLL_MS); + }; + const runPoll = async () => { + if (settled) return; + await poll(); + scheduleReconnect(); + }; + + void runPoll(); timeoutHandle = setTimeout(() => { if (settled) return; settle();