From 9e7afaf41edacfecdd1a0b36c91c428de9adfd42 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:26:00 -0700 Subject: [PATCH 1/9] Disable product telemetry in CI (#1701) CI reports to the production PostHog project from every e2e shard. Each shard is a fresh container, so it mints a fresh anonymous analytics id and arrives on a fresh runner IP, which inflates machine and user counts. Set DO_NOT_TRACK, EXECUTOR_DISABLE_ANALYTICS and EXECUTOR_DISABLE_INTEGRATIONS_FETCH on the workflows that execute product code. Pin disabled: false in the two registry tests that assert the fetch happens, so they do not inherit the ambient flags. --- .github/workflows/ci.yml | 12 ++++++++++++ .github/workflows/publish-desktop.yml | 12 ++++++++++++ .github/workflows/publish-selfhost-docker.yml | 12 ++++++++++++ .github/workflows/release.yml | 12 ++++++++++++ .../core/integrations-registry/src/registry.test.ts | 8 ++++++++ 5 files changed, 56 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 53065b9d50..49b9c446db 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,6 +10,18 @@ concurrency: group: ci-${{ github.ref }} cancel-in-progress: true +# CI is not a user. Without this, executor's own product telemetry reports to the +# production PostHog project from every e2e shard: the analytics layer +# (integration_added, execution_completed, artifact_*) and the +# integrations-registry fetch, which lands as a `hit` event. Each shard is a +# fresh container, so it mints a fresh anonymous id and arrives on a fresh +# runner IP — inflating machine and user counts by roughly an order of +# magnitude. Both opt-outs are the packages' documented CI/test hooks. +env: + DO_NOT_TRACK: "1" + EXECUTOR_DISABLE_ANALYTICS: "1" + EXECUTOR_DISABLE_INTEGRATIONS_FETCH: "1" + jobs: changes: name: Changed paths diff --git a/.github/workflows/publish-desktop.yml b/.github/workflows/publish-desktop.yml index 090e0d6d40..54f22c7ce7 100644 --- a/.github/workflows/publish-desktop.yml +++ b/.github/workflows/publish-desktop.yml @@ -21,6 +21,18 @@ concurrency: group: publish-desktop-${{ github.ref }} cancel-in-progress: false +# CI is not a user. Without this, executor's own product telemetry reports to the +# production PostHog project from every e2e shard: the analytics layer +# (integration_added, execution_completed, artifact_*) and the +# integrations-registry fetch, which lands as a `hit` event. Each shard is a +# fresh container, so it mints a fresh anonymous id and arrives on a fresh +# runner IP — inflating machine and user counts by roughly an order of +# magnitude. Both opt-outs are the packages' documented CI/test hooks. +env: + DO_NOT_TRACK: "1" + EXECUTOR_DISABLE_ANALYTICS: "1" + EXECUTOR_DISABLE_INTEGRATIONS_FETCH: "1" + jobs: build: permissions: diff --git a/.github/workflows/publish-selfhost-docker.yml b/.github/workflows/publish-selfhost-docker.yml index 1da6470320..a70192b13b 100644 --- a/.github/workflows/publish-selfhost-docker.yml +++ b/.github/workflows/publish-selfhost-docker.yml @@ -16,6 +16,18 @@ concurrency: group: publish-selfhost-docker-${{ inputs.tag }} cancel-in-progress: false +# CI is not a user. Without this, executor's own product telemetry reports to the +# production PostHog project from every e2e shard: the analytics layer +# (integration_added, execution_completed, artifact_*) and the +# integrations-registry fetch, which lands as a `hit` event. Each shard is a +# fresh container, so it mints a fresh anonymous id and arrives on a fresh +# runner IP — inflating machine and user counts by roughly an order of +# magnitude. Both opt-outs are the packages' documented CI/test hooks. +env: + DO_NOT_TRACK: "1" + EXECUTOR_DISABLE_ANALYTICS: "1" + EXECUTOR_DISABLE_INTEGRATIONS_FETCH: "1" + jobs: metadata: runs-on: blacksmith-4vcpu-ubuntu-2404 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 241d4757f8..171b42a14a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -13,6 +13,18 @@ concurrency: group: release-${{ github.ref }} cancel-in-progress: false +# CI is not a user. Without this, executor's own product telemetry reports to the +# production PostHog project from every e2e shard: the analytics layer +# (integration_added, execution_completed, artifact_*) and the +# integrations-registry fetch, which lands as a `hit` event. Each shard is a +# fresh container, so it mints a fresh anonymous id and arrives on a fresh +# runner IP — inflating machine and user counts by roughly an order of +# magnitude. Both opt-outs are the packages' documented CI/test hooks. +env: + DO_NOT_TRACK: "1" + EXECUTOR_DISABLE_ANALYTICS: "1" + EXECUTOR_DISABLE_INTEGRATIONS_FETCH: "1" + jobs: release: runs-on: ubuntu-latest diff --git a/packages/core/integrations-registry/src/registry.test.ts b/packages/core/integrations-registry/src/registry.test.ts index 20da742fbd..589cac482a 100644 --- a/packages/core/integrations-registry/src/registry.test.ts +++ b/packages/core/integrations-registry/src/registry.test.ts @@ -131,6 +131,10 @@ describe("IntegrationsRegistry", () => { Effect.provide( integrationsRegistryLayer({ userAgent: TEST_USER_AGENT, + // Pinned: this case asserts the fetch happens, so it must not + // inherit an ambient DO_NOT_TRACK / EXECUTOR_DISABLE_INTEGRATIONS_FETCH + // (CI sets both). Mirrors the analytics package's test config. + disabled: false, cacheDir, url: "https://integrations.test/api.json", }).pipe(Layer.provide(httpLayer), Layer.provide(NodeFileSystem.layer)), @@ -164,6 +168,10 @@ describe("IntegrationsRegistry", () => { Effect.provide( integrationsRegistryLayer({ userAgent: TEST_USER_AGENT, + // Pinned: this case asserts the fetch happens, so it must not + // inherit an ambient DO_NOT_TRACK / EXECUTOR_DISABLE_INTEGRATIONS_FETCH + // (CI sets both). Mirrors the analytics package's test config. + disabled: false, cacheDir, url: "https://integrations.test/api.json", }).pipe(Layer.provide(httpLayer), Layer.provide(NodeFileSystem.layer)), From b917755751a94e352df2d10d0ae4bcbe98d6e730 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:28:58 -0700 Subject: [PATCH 2/9] Exempt paid organizations from the execution rate-limit backstop (#1695) The per-org hourly cap was sized for free-tier abuse but applied to every org regardless of plan, so a paying customer running a heavy workload was blocked mid-run. Paid orgs now skip the cap. The exemption is resolved only once the counter reports an org over the limit, so the common path is unchanged, and the result is cached per org. An unresolved exemption does not fail open: that would disable the backstop during exactly the billing outage it exists to cover, so a stale positive is reused when available and the cap otherwise applies. Blocks are logged, which nothing did before. --- .../engine/execution-rate-limit.node.test.ts | 171 ++++++++++++++++++ apps/cloud/src/engine/execution-rate-limit.ts | 168 ++++++++++++++--- .../src/engine/execution-stack-metered.ts | 17 +- e2e/cloud/mcp-execution-limits.test.ts | 49 +++++ e2e/src/surfaces/autumn.ts | 36 ++++ 5 files changed, 417 insertions(+), 24 deletions(-) create mode 100644 apps/cloud/src/engine/execution-rate-limit.node.test.ts diff --git a/apps/cloud/src/engine/execution-rate-limit.node.test.ts b/apps/cloud/src/engine/execution-rate-limit.node.test.ts new file mode 100644 index 0000000000..22d570bcec --- /dev/null +++ b/apps/cloud/src/engine/execution-rate-limit.node.test.ts @@ -0,0 +1,171 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Data, Effect } from "effect"; + +import type { ExecutionEngine } from "@executor-js/execution"; + +import { makeExecutionRateLimiter } from "./execution-rate-limit"; +import { RATE_LIMIT_BLOCKED_MESSAGE } from "./execution-limit-messages"; + +const ORG = "org_test"; + +/** Stands in for whatever the real lookups fail with (Autumn down, DO down). */ +class UpstreamDownError extends Data.TaggedError("UpstreamDownError")<{ + readonly which: string; +}> {} + +// A stand-in engine: `execute` resolves to a marker, so a test tells an allowed +// execution (marker) from a blocked one (the gate's error result) by which of +// the two came back. A blocked decision never reaches the engine at all. +const engineStub: ExecutionEngine = { + execute: () => Effect.succeed({ result: "ran" }), + executeWithPause: () => Effect.succeed({ status: "completed", result: { result: "ran" } }), + resume: () => Effect.succeed(null), + getPausedExecution: () => Effect.succeed(null), + pausedExecutionCount: () => Effect.succeed(0), + hasPausedExecutions: () => Effect.succeed(false), + getDescription: Effect.succeed("stub"), +}; + +/** Counter that hands out a caller-controlled sequence of counts. */ +const countingIncrement = (counts: ReadonlyArray) => { + let calls = 0; + return () => Effect.succeed(counts[Math.min(calls++, counts.length - 1)] ?? 0); +}; + +const runExecute = (limiter: ReturnType) => + Effect.runPromise( + limiter.decorate(ORG, engineStub).execute("code", { + // Never invoked: the stub ignores it, and a blocked execution never runs. + onElicitation: () => Effect.die("elicitation is not exercised here"), + }), + ); + +describe("execution rate limiter — paid exemption", () => { + it("allows executions under the cap without consulting the exemption", async () => { + let exemptionCalls = 0; + const limiter = makeExecutionRateLimiter(countingIncrement([1]), { + limit: 10, + isExempt: () => { + exemptionCalls += 1; + return Effect.succeed(false); + }, + }); + + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + // The whole point of resolving lazily: the common path costs no lookup. + expect(exemptionCalls).toBe(0); + }); + + it("blocks a non-exempt org over the cap", async () => { + const limiter = makeExecutionRateLimiter(countingIncrement([11]), { + limit: 10, + isExempt: () => Effect.succeed(false), + }); + + expect(await runExecute(limiter)).toMatchObject({ + result: null, + error: RATE_LIMIT_BLOCKED_MESSAGE, + }); + }); + + it("allows an exempt org over the cap", async () => { + const limiter = makeExecutionRateLimiter(countingIncrement([11]), { + limit: 10, + isExempt: () => Effect.succeed(true), + }); + + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + }); + + it("caches the exemption so a paid org past the cap looks it up once", async () => { + let exemptionCalls = 0; + const limiter = makeExecutionRateLimiter(countingIncrement([11, 12, 13]), { + limit: 10, + exemptionTtlMs: 60_000, + now: () => 1_000, + isExempt: () => { + exemptionCalls += 1; + return Effect.succeed(true); + }, + }); + + for (let i = 0; i < 3; i += 1) + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + expect(exemptionCalls).toBe(1); + }); + + it("re-resolves once the cached exemption expires", async () => { + let exemptionCalls = 0; + let clock = 1_000; + const limiter = makeExecutionRateLimiter(countingIncrement([11, 12]), { + limit: 10, + exemptionTtlMs: 1_000, + now: () => clock, + isExempt: () => { + exemptionCalls += 1; + return Effect.succeed(true); + }, + }); + + await runExecute(limiter); + clock += 5_000; + await runExecute(limiter); + expect(exemptionCalls).toBe(2); + }); + + it("blocks when the exemption cannot be resolved and nothing is cached", async () => { + // Deliberately NOT fail-open: an unresolvable exemption during an Autumn + // outage must not switch the backstop off, which is the one scenario it + // exists to cover. + const limiter = makeExecutionRateLimiter(countingIncrement([11]), { + limit: 10, + isExempt: () => Effect.fail(new UpstreamDownError({ which: "autumn" })), + }); + + expect(await runExecute(limiter)).toMatchObject({ + result: null, + error: RATE_LIMIT_BLOCKED_MESSAGE, + }); + }); + + it("honours a stale exemption when a later lookup fails", async () => { + let clock = 1_000; + let shouldFail = false; + const limiter = makeExecutionRateLimiter(countingIncrement([11, 12]), { + limit: 10, + exemptionTtlMs: 1_000, + now: () => clock, + isExempt: () => + shouldFail ? Effect.fail(new UpstreamDownError({ which: "autumn" })) : Effect.succeed(true), + }); + + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + + // Cache expires and Autumn is now unreachable: the known-paid org keeps + // running rather than getting blocked mid-workload by a blip. + clock += 5_000; + shouldFail = true; + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + }); + + it("fails open when the counter itself is unreachable", async () => { + const limiter = makeExecutionRateLimiter( + () => Effect.fail(new UpstreamDownError({ which: "counter DO" })), + { + limit: 10, + isExempt: () => Effect.succeed(false), + }, + ); + + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + }); + + it("applies the cap when no exemption predicate is wired", async () => { + const limiter = makeExecutionRateLimiter(countingIncrement([11]), { limit: 10 }); + + expect(await runExecute(limiter)).toMatchObject({ + result: null, + error: RATE_LIMIT_BLOCKED_MESSAGE, + }); + }); +}); diff --git a/apps/cloud/src/engine/execution-rate-limit.ts b/apps/cloud/src/engine/execution-rate-limit.ts index b68d4c044e..3b5ea8194c 100644 --- a/apps/cloud/src/engine/execution-rate-limit.ts +++ b/apps/cloud/src/engine/execution-rate-limit.ts @@ -1,5 +1,5 @@ // --------------------------------------------------------------------------- -// Per-org execution rate limit — an abuse backstop independent of billing. +// Per-org execution rate limit — a free-tier abuse backstop. // // The balance gate (execution-gate.ts) depends on Autumn and fails open, so a // billing outage plus runaway automation could still run unbounded executions. @@ -8,9 +8,22 @@ // each MCP session lives in its own DO instance, so an in-memory counter // would be per-session and trivially bypassed by opening more sessions). // -// Like the balance gate it FAILS OPEN: an unreachable counter DO, a missing +// Paid organizations are EXEMPT. The cap was sized for free-tier abuse but +// applied to everyone, and on 2026-08-18 it blocked a paying customer +// mid-workload — their agent gave up on Executor and routed around it. Paid +// usage is what the balance gate and metered overage are for; this backstop +// has no business capping it. +// +// The exemption is resolved ONLY once the counter reports an org over the cap, +// so the common path (under the cap) costs the counter increment and nothing +// else. `isExempt` is an opaque predicate: this module still names no billing +// concept, and the Autumn coupling lives in `execution-stack-metered.ts`, +// which already owns that dependency. +// +// FAIL OPEN applies to the COUNTER: an unreachable counter DO, a missing // binding, or a slow call allows the execution (warn + Sentry). The backstop -// must never take executions down with it. +// must never take executions down with it. An unresolved EXEMPTION is the one +// thing that does not fail open — see `resolveExemption`. // --------------------------------------------------------------------------- import { DurableObject, env } from "cloudflare:workers"; @@ -25,14 +38,26 @@ import { RATE_LIMIT_BLOCKED_MESSAGE } from "./execution-limit-messages"; // Fixed window: all executions in the same clock hour share one counter. export const RATE_LIMIT_WINDOW_MS = 3_600_000; -// Calibration: the heaviest legitimate org runs ~1.1k executions per MONTH, -// so 1000 per HOUR is far above any human-driven usage and only trips on -// runaway automation (the incident this backstops: ~18k in 30 days would -// still pass, which is fine — that class of overrun is the balance gate's -// job; this catches tight loops). +// The cap for organizations WITHOUT a paid subscription. +// +// The original calibration ("the heaviest legitimate org runs ~1.1k executions +// per MONTH, so 1000 per HOUR is far above any human-driven usage") went stale +// inside six weeks: by 2026-08-18 a paying org was sustaining ~5.3k executions +// per DAY and crossed this cap in a single hour. Sizing a shared number +// against the largest customer is a losing game, so the number no longer tries +// to describe them — paid orgs are exempt below, and this now has only +// free-tier abuse to cover, which is what it was picked for. export const EXECUTIONS_PER_ORG_PER_HOUR = 1000; // Counter DO slower than this => fail open rather than stall executions. const RATE_LIMIT_CHECK_TIMEOUT_MS = 2_000; +// Exemption lookup slower than this => treat as unresolved. +const EXEMPTION_CHECK_TIMEOUT_MS = 2_000; +// An org over the cap is checked at most once per TTL rather than once per +// execution, so a paid org running far past it doesn't hammer the lookup. +const EXEMPTION_CACHE_TTL_MS = 60_000; +// Sweep guard, mirroring the balance gate's cache: one long-lived isolate can +// serve many orgs. +const EXEMPTION_CACHE_MAX_ENTRIES = 10_000; // The DO purges its storage this long after the last increment, so idle orgs // cost nothing. Two windows: long enough that an active window never purges. const COUNTER_PURGE_AFTER_MS = 2 * RATE_LIMIT_WINDOW_MS; @@ -51,6 +76,20 @@ class RateLimitCheckTimeoutError extends Data.TaggedError("RateLimitCheckTimeout readonly timeoutMs: number; }> {} +/** Internal sentinel for an exemption lookup that exceeded its time budget. */ +class ExemptionCheckTimeoutError extends Data.TaggedError("ExemptionCheckTimeoutError")<{ + readonly timeoutMs: number; +}> {} + +/** + * Whether an organization is exempt from the cap. Production passes a paid- + * subscription check; keeping it an opaque predicate is what lets this module + * stay free of any billing import. + */ +export type ExecutionRateLimitExemption = ( + organizationId: string, +) => Effect.Effect; + // --------------------------------------------------------------------------- // Counter Durable Object — one instance per organization (idFromName(orgId)). // Stores a single { windowId, count } record: an increment in a new window @@ -119,12 +158,76 @@ export const makeExecutionRateLimiter = ( readonly windowMs?: number; readonly timeoutMs?: number; readonly now?: () => number; + readonly isExempt?: ExecutionRateLimitExemption; + readonly exemptionTtlMs?: number; }, ): ExecutionRateLimiter => { const limit = options?.limit ?? EXECUTIONS_PER_ORG_PER_HOUR; const windowMs = options?.windowMs ?? RATE_LIMIT_WINDOW_MS; const timeoutMs = options?.timeoutMs ?? RATE_LIMIT_CHECK_TIMEOUT_MS; const now = options?.now ?? Date.now; + const isExempt = options?.isExempt; + const exemptionTtlMs = options?.exemptionTtlMs ?? EXEMPTION_CACHE_TTL_MS; + + const exemptionCache = new Map< + string, + { readonly exempt: boolean; readonly expiresAtMs: number } + >([]); + + const writeExemptionCache = (organizationId: string, exempt: boolean, nowMs: number): void => { + if (exemptionCache.size >= EXEMPTION_CACHE_MAX_ENTRIES) { + for (const [key, entry] of exemptionCache) { + if (entry.expiresAtMs <= nowMs) exemptionCache.delete(key); + } + // Still saturated after dropping expired entries: reset rather than grow. + if (exemptionCache.size >= EXEMPTION_CACHE_MAX_ENTRIES) exemptionCache.clear(); + } + exemptionCache.set(organizationId, { exempt, expiresAtMs: nowMs + exemptionTtlMs }); + }; + + /** + * Resolved only for orgs already over the cap, so the lookup never touches + * the common path. + * + * This is the one place that does NOT fail open. The balance gate already + * allows executions when Autumn is unreachable; if the exemption did too, + * an Autumn outage would switch this backstop off entirely — precisely the + * "billing outage plus runaway automation" case it exists to cover. A stale + * positive is honoured ahead of that fallback, so a blip can't flip a + * known-paid org into a block mid-workload. + */ + const resolveExemption = (organizationId: string): Effect.Effect => + Effect.suspend(() => { + if (!isExempt) return Effect.succeed(false); + const nowMs = now(); + const cached = exemptionCache.get(organizationId); + if (cached && cached.expiresAtMs > nowMs) return Effect.succeed(cached.exempt); + return isExempt(organizationId).pipe( + Effect.timeoutOrElse({ + duration: `${EXEMPTION_CHECK_TIMEOUT_MS} millis`, + orElse: () => + Effect.fail(new ExemptionCheckTimeoutError({ timeoutMs: EXEMPTION_CHECK_TIMEOUT_MS })), + }), + Effect.map((exempt) => { + writeExemptionCache(organizationId, exempt, nowMs); + return exempt; + }), + Effect.catch((error: unknown) => + Effect.gen(function* () { + yield* Effect.sync(() => { + console.warn( + `[rate-limit] exemption lookup failed for ${organizationId}; treating as ${ + cached ? "last known" : "not exempt" + }:`, + error, + ); + }); + yield* captureCauseEffect(error); + return cached?.exempt ?? false; + }), + ), + ); + }); const decide = (organizationId: string): Effect.Effect => Effect.suspend(() => { @@ -134,18 +237,34 @@ export const makeExecutionRateLimiter = ( duration: `${timeoutMs} millis`, orElse: () => Effect.fail(new RateLimitCheckTimeoutError({ timeoutMs })), }), - Effect.map( - (count): GateDecision => - count > limit - ? { - blocked: true, - error: new ExecutionRateLimitExceededError({ - organizationId, - message: RATE_LIMIT_BLOCKED_MESSAGE, - }), - } - : { blocked: false }, - ), + Effect.flatMap((count): Effect.Effect => { + // Under the cap: no exemption lookup, no extra I/O. + if (count <= limit) return Effect.succeed({ blocked: false }); + return Effect.gen(function* () { + if (yield* resolveExemption(organizationId)) { + return { blocked: false } as const satisfies GateDecision; + } + // The only record that the backstop fired. A blocked execution is + // never usage-tracked (the gate short-circuits before the tracker) + // and deliberately not sent to Sentry — a backstop stopping + // runaway automation is expected, not exceptional — so without + // this line a blocked org is invisible outside a bug report, which + // is how the 2026-08-18 block went unnoticed until a customer + // sent a screenshot. + yield* Effect.sync(() => { + console.warn( + `[rate-limit] blocked execution for ${organizationId}: ${count} > ${limit} in window ${windowId}`, + ); + }); + return { + blocked: true, + error: new ExecutionRateLimitExceededError({ + organizationId, + message: RATE_LIMIT_BLOCKED_MESSAGE, + }), + } as const satisfies GateDecision; + }); + }), // FAIL OPEN: the backstop must never block executions because its // counter is unreachable or slow. Effect.catch((error: unknown) => @@ -185,8 +304,13 @@ type RateLimiterNamespace = { * Production rate limiter backed by the `EXECUTION_RATE_LIMITER` counter DO. * When the binding is absent (unit-test workers, older local setups) the * limiter is disabled: every check passes, logged once at construction. + * + * `isExempt` decides which orgs the cap skips; production passes a paid- + * subscription check from `execution-stack-metered.ts`. */ -export const makeCloudExecutionRateLimiter = (): ExecutionRateLimiter => { +export const makeCloudExecutionRateLimiter = ( + isExempt: ExecutionRateLimitExemption, +): ExecutionRateLimiter => { const limit = resolveRateLimit(); const namespace = (env as { EXECUTION_RATE_LIMITER?: RateLimiterNamespace }) .EXECUTION_RATE_LIMITER; @@ -204,7 +328,7 @@ export const makeCloudExecutionRateLimiter = (): ExecutionRateLimiter => { ) as ExecutionRateLimiterStub; return stub.increment(windowId); }), - { limit }, + { limit, isExempt }, ); }; diff --git a/apps/cloud/src/engine/execution-stack-metered.ts b/apps/cloud/src/engine/execution-stack-metered.ts index 4ea70b6d43..6063e1456c 100644 --- a/apps/cloud/src/engine/execution-stack-metered.ts +++ b/apps/cloud/src/engine/execution-stack-metered.ts @@ -26,6 +26,7 @@ import { } from "@executor-js/api/server"; import { AutumnService } from "../extensions/billing/service"; +import { hasPaidOrganizationSubscription } from "../extensions/billing/plans"; import type { DbService } from "../db/db"; import { CloudExecutionSeamsLayer } from "../engine/execution-stack"; import { makeExecutionLimitGate } from "./execution-gate"; @@ -34,7 +35,8 @@ import { withExecutionUsageTracking } from "./execution-usage"; // Usage-metering decorator bound to the billing service, plus the two // pre-execution guards this layer owns, ordered cheapest first: -// 1. rate-limit backstop (counter DO, independent of billing) +// 1. rate-limit backstop (counter DO; free-tier abuse only — paid orgs are +// exempt via the subscription lookup below) // 2. execution balance gate (Autumn check, cached 60s, fails open) // 3. usage tracking — fire-and-forget (`Effect.runFork`) so the billing // call can't stall a user-facing execution. @@ -48,7 +50,18 @@ export const CloudMeteringEngineDecorator: Layer.Layer autumn.checkExecutionBalance(organizationId), ); - const rateLimiter = makeCloudExecutionRateLimiter(); + // The limiter's paid-org exemption. This is the billing coupling the + // limiter module deliberately avoids owning, and it reads the same + // `PAID_AUTUMN_PLAN_IDS` config as the org-creation and seat gates so + // "paid" means one thing across the app. The limiter calls this only + // for orgs already over the cap and caches the answer, so the extra + // Autumn round trip stays off the hot path. + const rateLimiter = makeCloudExecutionRateLimiter((organizationId) => + Effect.map( + autumn.use((client) => client.customers.getOrCreate({ customerId: organizationId })), + (customer) => hasPaidOrganizationSubscription(customer.subscriptions), + ), + ); return { decorate: (engine, identity: EngineStackIdentity) => rateLimiter.decorate( diff --git a/e2e/cloud/mcp-execution-limits.test.ts b/e2e/cloud/mcp-execution-limits.test.ts index f80dcaac23..4c3272267c 100644 --- a/e2e/cloud/mcp-execution-limits.test.ts +++ b/e2e/cloud/mcp-execution-limits.test.ts @@ -198,3 +198,52 @@ scenario( ); }), ); + +scenario( + "Billing · a paid org runs past the rate-limit backstop instead of being blocked", + { timeout: 180_000 }, + Effect.gen(function* () { + // The backstop is sized for free-tier abuse, and it used to apply to every + // org regardless of plan: on 2026-08-18 it blocked a paying customer + // mid-workload. This pins the exemption end to end — the same run that + // blocks a free org (the scenario above) must sail past the cap here. + yield* Billing; + const autumn = yield* Autumn; + const target = yield* Target; + const mcp = yield* Mcp; + + const identity = yield* target.newIdentity(); + const bearer = yield* mcp.mintBearer(emailOf(identity)); + const customerId = orgIdOf(bearer); + + // Enterprise, not Team: it carries no price and no card-required trial, so + // the emulator activates it inline rather than answering with a checkout + // URL. Its executions are unlimited, so the BALANCE gate can never be what + // allows or blocks here — the rate-limit backstop is isolated as the only + // guard under test. + yield* autumn.attachPlan(customerId, "enterprise"); + + const session = mcp.session(identity); + + // Deliberately past E2E_EXECUTION_RATE_LIMIT: the count that blocks a free + // org. Every one must run. + const overCap = RATE_LIMIT + 3; + for (let i = 1; i <= overCap; i++) { + const ok = yield* session.call("execute", { code: `return ${i};` }); + expect(ok.ok, `execution ${i} succeeds for a paid org (cap is ${RATE_LIMIT})`).toBe(true); + expect( + ok.text, + `execution ${i} returns its value rather than the backstop message`, + ).not.toContain(RATE_LIMIT_BLOCKED_MESSAGE); + } + + // Nothing was withheld from the meter either: a paid org past the cap is + // billed for every execution it ran. + const metered = yield* autumn.expectUsage({ + customerId, + featureId: "executions", + count: overCap, + }); + expect(metered.length, "every execution past the cap is still metered").toBe(overCap); + }), +); diff --git a/e2e/src/surfaces/autumn.ts b/e2e/src/surfaces/autumn.ts index a831491178..a401cf5486 100644 --- a/e2e/src/surfaces/autumn.ts +++ b/e2e/src/surfaces/autumn.ts @@ -76,6 +76,14 @@ export interface AutumnSurface { * which drives `remaining` to zero. This is the setup a "blocked at cap" * scenario runs before opening the session it expects to be gated. */ readonly exhaustExecutions: (customerId: string) => Effect.Effect; + /** Put an org on a plan directly (`billing.attach`), skipping the hosted + * checkout. Only valid for a plan that needs no payment — Enterprise carries + * no price and no card-required trial, so the emulator activates its + * subscription inline. Team does have both, so it must still go through the + * browser checkout + `settleCheckout`. This is the setup a scenario runs when + * it needs a PAID org and the upgrade journey itself is not what's under + * test (e.g. the rate-limit backstop's paid exemption). */ + readonly attachPlan: (customerId: string, planId: string) => Effect.Effect; /** Arm a fault against a matching Autumn request (`POST /_emulate/faults`). */ readonly armFault: (input: FaultInput) => Effect.Effect; /** Clear every armed fault (`DELETE /_emulate/faults`) — the finalizer that @@ -155,6 +163,33 @@ export const makeAutumnSurface = (autumnUrl: string): AutumnSurface => { } }); + const attachPlan = (customerId: string, planId: string) => + Effect.gen(function* () { + const response = yield* Effect.promise(() => + fetch(`${autumnUrl}/v1/billing.attach`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ customer_id: customerId, plan_id: planId }), + }), + ); + if (!response.ok) { + return yield* Effect.fail( + `autumn billing.attach responded ${response.status}: ${yield* Effect.promise(() => response.text())}`, + ); + } + // A plan that needs payment answers with a checkout URL instead of + // activating; that silently leaves the org on Free, and a scenario that + // asked for a paid org would then assert against the wrong plan. + const body = (yield* Effect.promise(() => response.json())) as { + readonly payment_url?: string | null; + }; + if (body.payment_url) { + return yield* Effect.fail( + `autumn billing.attach returned a checkout URL for '${planId}': it requires payment, so it cannot be attached directly`, + ); + } + }); + const armFault = (input: FaultInput) => Effect.gen(function* () { const response = yield* Effect.promise(() => @@ -213,6 +248,7 @@ export const makeAutumnSurface = (autumnUrl: string): AutumnSurface => { usageEvents, settleCheckout, exhaustExecutions, + attachPlan, armFault, clearFaults, ledgerFor, From 9a38ab4610666c8726b8fc6e76ca4338cb48f4eb Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:35:20 -0700 Subject: [PATCH 3/9] Restore targeted placement to aws:us-east-1 (#1704) Reverts the wrangler config change from #1693. Since that deploy user_store.getOrganization went from ~12ms to ~300ms p50 and mcp.auth.authorize_organization from ~70ms to ~400-700ms, which is the Worker no longer sharing a region with Hyperdrive. --- apps/cloud/wrangler.jsonc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/apps/cloud/wrangler.jsonc b/apps/cloud/wrangler.jsonc index 3fa69306e0..3578de6d25 100644 --- a/apps/cloud/wrangler.jsonc +++ b/apps/cloud/wrangler.jsonc @@ -94,6 +94,14 @@ "bucket_name": "executor-cloud-blobs", }, ], + // Pins the Worker to the Hyperdrive/Postgres region. Added in #420 to fix + // MCP Hyperdrive connection pressure; dropped in #1693 as a placement + // experiment, which took `user_store.*` from ~12ms to ~300ms because every + // query started crossing regions. Restored to measure that against the + // JWKS verify time the experiment also moved. + "placement": { + "region": "aws:us-east-1", + }, "worker_loaders": [ { "binding": "LOADER", From 2bdbedf257f54d7c209e8c856c618174c10d6bb3 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:09:51 -0700 Subject: [PATCH 4/9] Reserve the OAuth sign-in window on the click, not after discovery (#1703) The transparent DCR/CIMD connects and OAuth reconnect opened the popup only after their setup round trips answered. window.open needs transient user activation, which expires a few seconds after the click, so a slow API meant the browser refused the window and the connect ended silently. Claim the window on the click and navigate it when the authorization URL arrives; close it on the paths that end without signing in, and on cancel and unmount. Report a refused window instead of swallowing it, and render the sign-in error above the footer where the automatic flows can show it. --- .changeset/oauth-popup-reserve-on-click.md | 9 + e2e/selfhost/mcp-oauth-slow-connect.test.ts | 132 +++++++++ .../react/src/components/accounts-section.tsx | 8 + .../src/components/add-account-modal.test.ts | 279 +++++++++++++++++- .../src/components/add-account-modal.tsx | 106 +++++-- packages/react/src/plugins/oauth-sign-in.tsx | 110 +++++-- 6 files changed, 593 insertions(+), 51 deletions(-) create mode 100644 .changeset/oauth-popup-reserve-on-click.md create mode 100644 e2e/selfhost/mcp-oauth-slow-connect.test.ts diff --git a/.changeset/oauth-popup-reserve-on-click.md b/.changeset/oauth-popup-reserve-on-click.md new file mode 100644 index 0000000000..cf8d9430a5 --- /dev/null +++ b/.changeset/oauth-popup-reserve-on-click.md @@ -0,0 +1,9 @@ +--- +"@executor-js/react": patch +--- + +**A slow OAuth discovery no longer kills the connect with no popup and no error** + +The transparent connect flows opened the sign-in window only after their setup round trips had answered: DCR after probe and dynamic registration, CIMD after minting the client, reconnect after starting the session. `window.open` needs transient user activation, which browsers expire a few seconds after the click, so once the API was slow enough the browser refused the window and the connect ended with nothing on screen but the button returning to "Connect". Every MCP integration takes that path. + +The window is now claimed on the click itself and navigated when the authorization URL arrives, however long that takes, and it is closed again on the paths that end without signing in (failed probe, no registration endpoint, rejected registration, failed client mint) as well as on cancel and unmount. A window the browser does refuse is now reported instead of swallowed: the flows stop before their round trips, and the sign-in error renders above the dialog footer, where the automatic flows can actually show it, rather than inside a method tab panel they never render. diff --git a/e2e/selfhost/mcp-oauth-slow-connect.test.ts b/e2e/selfhost/mcp-oauth-slow-connect.test.ts new file mode 100644 index 0000000000..6a7b7fd9d5 --- /dev/null +++ b/e2e/selfhost/mcp-oauth-slow-connect.test.ts @@ -0,0 +1,132 @@ +// Selfhost browser coverage for the transparent DCR connect when OAuth +// discovery is slow, the shape behind the report: "When I hit connect, it loads +// for a second, but then theres no user feedback and nothing happens." +// +// One click runs probe -> register -> start. The first two are network round +// trips, and `window.open` needs transient user activation, which a real +// browser expires a few seconds after the click. The shipped code opened the +// sign-in window AFTER both round trips, so once the API was slow the browser +// refused it and the connect died silently. The window is now claimed on the +// click and navigated later, so discovery latency no longer decides whether the +// user can connect. +// +// What this scenario guards: the whole slow path still works end to end, with +// the reservation threaded from the click through registration to a window that +// really lands on the discovered authorization server. +// +// What it CANNOT guard, and why: Playwright drives Chromium in automation mode, +// which never enforces the activation rule for `window.open` (verified headed +// and headless, and with `--disable-popup-blocking` removed via +// ignoreDefaultArgs). So a browser here opens the window no matter how stale +// the click is, and this scenario passes against the pre-fix ordering too. The +// ordering itself is pinned by unit tests over `runDcrConnect` in +// packages/react/src/components/add-account-modal.test.ts, which do fail when +// the reservation moves back after the round trips. See LEARNINGS.md. +import { randomBytes } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { deriveMcpNamespace } from "@executor-js/plugin-mcp"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { makeGreetingMcpServer, serveMcpServerWithOAuth } from "@executor-js/plugin-mcp/testing"; +import { IntegrationSlug } from "@executor-js/sdk/shared"; +import { OAuthTestServer } from "@executor-js/sdk/testing"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Target } from "../src/services"; + +const api = composePluginApi([mcpHttpPlugin()] as const); + +// Comfortably past Chromium's ~5s transient user activation once both land on +// the same click, and well under the step timeouts below. +const STALL_MS = 3_500; + +const isDiscoveryCall = (url: string): boolean => + url.includes("/api/oauth/probe") || url.includes("/api/oauth/clients/register-dynamic"); + +scenario( + "MCP OAuth · a slow discovery round trip still opens the sign-in window", + { timeout: 240_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeApiClient } = yield* Api; + const oauth = yield* OAuthTestServer; + const server = yield* serveMcpServerWithOAuth( + () => makeGreetingMcpServer({ name: "slow-connect-mcp" }), + { path: "/mcp" }, + ); + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + const displayName = `Slow MCP ${randomBytes(3).toString("hex")}`; + const slug = IntegrationSlug.make(deriveMcpNamespace({ name: displayName })); + + yield* Effect.gen(function* () { + yield* browser.session(identity, async ({ page, step }) => { + await step("Add an OAuth-protected MCP integration", async () => { + const addUrl = new URL("/integrations/add/mcp", target.baseUrl); + addUrl.searchParams.set("url", server.endpoint); + await page.goto(addUrl.toString(), { waitUntil: "networkidle" }); + await page.getByText("How does this server authenticate?").waitFor({ timeout: 30_000 }); + await page.getByPlaceholder("e.g. Linear").fill(displayName); + await page.getByRole("button", { name: "Add integration" }).click(); + await page.waitForURL(/\/integrations\/(?!add\b)[^/?]+$/, { timeout: 30_000 }); + await page.getByText("Connections").first().waitFor(); + }); + + await step("Make OAuth discovery slow, the way a degraded API is", async () => { + // Delay the responses rather than the requests, so the app sees a + // genuinely slow API and not a stalled network stack. + await page.route( + (url) => isDiscoveryCall(url.href), + async (route) => { + await new Promise((resolve) => setTimeout(resolve, STALL_MS)); + await route.continue(); + }, + ); + }); + + await step("Connect, and wait out the slow discovery", async () => { + await page.getByRole("button", { name: "Add connection" }).first().click(); + await page.getByRole("heading", { name: /Add connection/ }).waitFor(); + await page.getByRole("tab", { name: "OAuth" }).waitFor(); + + const popupPromise = page.waitForEvent("popup", { timeout: 60_000 }); + await page.getByRole("button", { name: "Connect", exact: true }).click(); + + // In a real browser the two stalls outlast the click's user + // activation, so this popup only exists because it was reserved on + // the click. Automation-mode Chromium would open it either way; + // the ordering is pinned by the unit tests named above. + const popup = await popupPromise; + await popup.waitForURL((url) => url.origin === new URL(oauth.issuerUrl).origin, { + timeout: 60_000, + }); + await popup.waitForLoadState("domcontentloaded", { timeout: 30_000 }); + expect( + new URL(popup.url()).origin, + "the reserved window reached the discovered authorization host", + ).toBe(new URL(oauth.authorizationEndpoint).origin); + await popup.close(); + }); + }); + + const oauthRequests = yield* oauth.requests; + expect( + oauthRequests.some( + (request) => request.method === "POST" && request.path === "/register", + ), + "the slow connect still dynamically registered its OAuth client", + ).toBe(true); + expect( + oauthRequests.some( + (request) => request.method === "GET" && request.path === "/authorize", + ), + "the slow connect still reached the authorize endpoint", + ).toBe(true); + }).pipe(Effect.ensuring(client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore))); + }), + ).pipe(Effect.provide(OAuthTestServer.layer())), +); diff --git a/packages/react/src/components/accounts-section.tsx b/packages/react/src/components/accounts-section.tsx index dddbfbe805..f0ec361dab 100644 --- a/packages/react/src/components/accounts-section.tsx +++ b/packages/react/src/components/accounts-section.tsx @@ -279,6 +279,11 @@ function OwnerAccounts(props: { } const payload = oauthReconnectPayload(connection); if (payload === null) return; + // Claim the sign-in window on the click: `oauth.start` below is a network + // round trip, and the browser's user activation can expire before it + // answers, which would leave Reconnect silently doing nothing. + const reservation = oauthPopup.reserve(); + if (reservation.kind === "blocked") return; // `oauth.start` discriminates the grant: client_credentials mints inline // (`status: "connected"`, no authorization URL) while authorization_code // returns a redirect the popup must complete. The popup hook only handles @@ -291,6 +296,7 @@ function OwnerAccounts(props: { reactivityKeys: connectionWriteKeys, }); if (Exit.isFailure(startExit)) { + oauthPopup.releaseReservation(); toast.error(messageFromExit(startExit, "Failed to reconnect")); trackEvent("connection_reconnected", { integration_slug: String(connection.integration), @@ -301,6 +307,7 @@ function OwnerAccounts(props: { } const started = startExit.value; if (started.status === "connected") { + oauthPopup.releaseReservation(); toast.success("Reconnected"); trackEvent("connection_reconnected", { integration_slug: String(connection.integration), @@ -311,6 +318,7 @@ function OwnerAccounts(props: { } void oauthPopup.openAuthorization({ owner: payload.owner, + reservation, run: () => Promise.resolve({ state: started.state, diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index acc5952691..b95a3b2aa4 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -9,6 +9,7 @@ import { } from "@executor-js/sdk/shared"; import type { AuthMethod } from "../lib/auth-placements"; +import type { OAuthPopupReservation } from "../plugins/oauth-sign-in"; import { connectionNameFrom, connectionLabel, @@ -57,7 +58,35 @@ type RegisterArgs = { readonly originIntegration?: IntegrationSlug; }; -type StartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner }; +type StartArgs = { + readonly client: OAuthClientSlug; + readonly owner: Owner; + readonly reservation: OAuthPopupReservation; +}; + +// A reservation the browser honoured. The desktop shape is the one that carries +// no `Window`, so the orchestrators' ordering is testable without a DOM. +const RESERVED: OAuthPopupReservation = { + kind: "desktop", + bridge: { openExternal: (): Promise => Promise.resolve() }, +}; + +/** Records the reserve/release calls alongside the network steps, so a test can + * assert the window is claimed BEFORE the first round trip and closed again + * whenever the flow ends without signing in. */ +const popupSpy = (reservation: OAuthPopupReservation = RESERVED) => { + const calls: string[] = []; + return { + calls, + reserve: (): OAuthPopupReservation => { + calls.push("reserve"); + return reservation; + }, + release: (): void => { + calls.push("release"); + }, + }; +}; type CimdCreateArgs = { readonly owner: Owner; @@ -70,7 +99,11 @@ type CimdCreateArgs = { readonly clientSecret: ""; }; -type CimdStartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner }; +type CimdStartArgs = { + readonly client: OAuthClientSlug; + readonly owner: Owner; + readonly reservation: OAuthPopupReservation; +}; const TEST_INTEGRATION = IntegrationSlug.make("linear_mcp"); @@ -310,6 +343,8 @@ describe("runCimdConnect", () => { const outcome = await runCimdConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, createClient: (args: CimdCreateArgs): Promise => { createArgs = args; return Promise.resolve(args.slug); @@ -352,6 +387,8 @@ describe("runCimdConnect", () => { const outcome = await runCimdConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, createClient: (): Promise => { created = true; return Promise.resolve(OAuthClientSlug.make("new-client")); @@ -383,7 +420,221 @@ describe("runCimdConnect", () => { expect(outcome).toEqual({ kind: "started", client: existingSlug, reused: true }); expect(created).toBe(false); - expect(startArgs).toEqual({ client: existingSlug, owner: "user" }); + expect(startArgs).toEqual({ client: existingSlug, owner: "user", reservation: RESERVED }); + }); +}); + +describe("runDcrConnect popup reservation", () => { + const probeOk = (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + registrationEndpoint: "https://auth.example.com/register", + }); + const dcrInput = { + discoveryUrl: "https://mcp.example.com/mcp", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + }; + + // The regression: probe and register are two round trips, and a browser stops + // honouring `window.open` about five seconds after the click. Opening the + // window after them is refused, so the connect ends with no popup, no error, + // and the button back on "Connect". The window has to be claimed up front. + it("claims the sign-in window BEFORE the first network call", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => { + popup.calls.push("probe"); + return probeOk(); + }, + register: (): Promise => { + popup.calls.push("register"); + return Promise.resolve(OAuthClientSlug.make("mcp-app")); + }, + start: (): void => { + popup.calls.push("start"); + }, + }, + dcrInput, + ); + + expect(outcome).toEqual({ kind: "started" }); + expect(popup.calls).toEqual(["reserve", "probe", "register", "start"]); + }); + + it("hands the reserved window to start, so the flow navigates it rather than opening a new one", async () => { + let startArgs: StartArgs | null = null; + const outcome = await runDcrConnect( + { + ...popupSpy(), + probe: probeOk, + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (args: StartArgs): void => { + startArgs = args; + }, + }, + dcrInput, + ); + + expect(outcome).toEqual({ kind: "started" }); + expect(startArgs!.reservation).toBe(RESERVED); + }); + + it("gives up before any network call when the browser refuses the window", async () => { + const popup = popupSpy({ kind: "blocked" }); + let probed = false; + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => { + probed = true; + return probeOk(); + }, + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + // No BYO fallback: registering an app by hand cannot open a window either. + expect(outcome).toEqual({ kind: "popup-blocked" }); + expect(probed).toBe(false); + expect(popup.calls).toEqual(["reserve"]); + }); + + // Reserving early means a flow that ends without signing in is holding an + // about:blank window the user never asked for. + it("closes the reserved window when the probe fails", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => Promise.resolve(null), + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + expect(outcome).toEqual({ kind: "fallback", reason: "probe-failed" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("closes the reserved window when the server advertises no registration endpoint", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + }), + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + expect(outcome.kind).toBe("fallback"); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("closes the reserved window when registration is rejected", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: probeOk, + register: (): Promise<{ readonly error: string }> => + Promise.resolve({ error: "redirect_uri not allowed" }), + start: (): void => {}, + }, + dcrInput, + ); + + expect(outcome).toMatchObject({ reason: "registration-failed" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("keeps the reserved window open on the path that signs in", async () => { + const popup = popupSpy(); + await runDcrConnect( + { + ...popup, + probe: probeOk, + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + expect(popup.calls).not.toContain("release"); + }); +}); + +describe("runCimdConnect popup reservation", () => { + const cimdInput = { + owner: "user" as Owner, + integrationName: "PostHog API", + authorizationUrl: "https://us.posthog.com/oauth/authorize/", + tokenUrl: "https://us.posthog.com/oauth/token/", + resource: null, + clientIdMetadataDocumentUrl: "https://executor.example/api/oauth/client-id-metadata.json", + existingClients: [], + }; + + it("claims the sign-in window before minting the client", async () => { + const popup = popupSpy(); + const outcome = await runCimdConnect( + { + ...popup, + createClient: (args: CimdCreateArgs): Promise => { + popup.calls.push("createClient"); + return Promise.resolve(args.slug); + }, + start: (): void => { + popup.calls.push("start"); + }, + }, + cimdInput, + ); + + expect(outcome.kind).toBe("started"); + expect(popup.calls).toEqual(["reserve", "createClient", "start"]); + }); + + it("closes the reserved window when minting the client fails", async () => { + const popup = popupSpy(); + const outcome = await runCimdConnect( + { + ...popup, + createClient: (): Promise => Promise.resolve(null), + start: (): void => {}, + }, + cimdInput, + ); + + expect(outcome).toEqual({ kind: "failed", reason: "create-failed" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("never claims a window when the method is missing its endpoints", async () => { + const popup = popupSpy(); + const outcome = await runCimdConnect( + { + ...popup, + createClient: (): Promise => Promise.resolve(null), + start: (): void => {}, + }, + { ...cimdInput, tokenUrl: " " }, + ); + + expect(outcome).toEqual({ kind: "failed", reason: "missing-endpoints" }); + expect(popup.calls).toEqual([]); }); }); @@ -416,7 +667,13 @@ describe("runDcrConnect", () => { }; const outcome = await runDcrConnect( - { probe, register, start }, + { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, + probe, + register, + start, + }, { discoveryUrl: "https://mcp.example.com/mcp", owner: "user", @@ -454,6 +711,8 @@ describe("runDcrConnect", () => { let registerArgs: RegisterArgs | null = null; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -490,6 +749,8 @@ describe("runDcrConnect", () => { let registerArgs: RegisterArgs | null = null; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -517,6 +778,8 @@ describe("runDcrConnect", () => { const calls: string[] = []; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => { calls.push("probe"); return Promise.resolve({ @@ -554,6 +817,8 @@ describe("runDcrConnect", () => { const calls: string[] = []; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => { calls.push("probe"); return Promise.resolve(null); @@ -580,6 +845,8 @@ describe("runDcrConnect", () => { const calls: string[] = []; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -620,6 +887,8 @@ describe("runDcrConnect", () => { let started = false; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -659,6 +928,8 @@ describe("runDcrConnect", () => { let registerArgs: RegisterArgs | null = null; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ issuer: "https://auth.example.com", diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 7376a39890..c2a7a5ac98 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -62,6 +62,7 @@ import { oauthClientIdMetadataDocumentUrl, useOAuthPopupFlow, type OAuthCompletionPayload, + type OAuthPopupReservation, } from "../plugins/oauth-sign-in"; import { clientDisplayName, @@ -638,7 +639,17 @@ type CimdCreateClientArgs = { type RunCimdConnectDeps = { readonly createClient: (args: CimdCreateClientArgs) => Promise; - readonly start: (args: { readonly client: OAuthClientSlug; readonly owner: Owner }) => void; + readonly start: (args: CimdStartArgs) => void; + /** Claim the sign-in window before any await. See `useOAuthPopupFlow.reserve`. */ + readonly reserve: () => OAuthPopupReservation; + /** Close a claimed window this flow turned out not to need. */ + readonly release: () => void; +}; + +type CimdStartArgs = { + readonly client: OAuthClientSlug; + readonly owner: Owner; + readonly reservation: OAuthPopupReservation; }; type RunCimdConnectInput = { @@ -653,6 +664,7 @@ type RunCimdConnectInput = { type CimdOutcome = | { readonly kind: "started"; readonly client: OAuthClientSlug; readonly reused: boolean } + | { readonly kind: "popup-blocked" } | { readonly kind: "failed"; readonly reason: "missing-endpoints" | "create-failed" }; export async function runCimdConnect( @@ -663,6 +675,12 @@ export async function runCimdConnect( return { kind: "failed", reason: "missing-endpoints" }; } + // Claim the window while the click still counts. Minting the client is a + // round trip, and the browser stops honouring `window.open` once the + // activation from the click expires. + const reservation = deps.reserve(); + if (reservation.kind === "blocked") return { kind: "popup-blocked" }; + const resource = input.resource ?? null; const existing = input.existingClients.find( (client) => @@ -675,7 +693,7 @@ export async function runCimdConnect( ); if (existing) { - deps.start({ client: existing.slug, owner: input.owner }); + deps.start({ client: existing.slug, owner: input.owner, reservation }); return { kind: "started", client: existing.slug, reused: true }; } @@ -693,8 +711,11 @@ export async function runCimdConnect( clientId: input.clientIdMetadataDocumentUrl, clientSecret: "", }); - if (created === null) return { kind: "failed", reason: "create-failed" }; - deps.start({ client: created, owner: input.owner }); + if (created === null) { + deps.release(); + return { kind: "failed", reason: "create-failed" }; + } + deps.start({ client: created, owner: input.owner, reservation }); return { kind: "started", client: created, reused: false }; } @@ -741,16 +762,20 @@ type DcrRegisterArgs = { type DcrStartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner; + readonly reservation: OAuthPopupReservation; }; /** Outcome of the DCR orchestration. `"started"` means the OAuth flow handed - * off (the popup/inline start ran); `"fallback"` means we could not auto-set-up - * — a failed probe, no registration endpoint, or a failed registration — and - * the caller should fall back to the bring-your-own-app picker. A failed probe + * off (the popup/inline start ran); `"popup-blocked"` means the browser refused + * the sign-in window, which no app picker can fix, so the caller reports it + * rather than falling back; `"fallback"` means we could not auto-set-up (a + * failed probe, no registration endpoint, or a failed registration) and the + * caller should fall back to the bring-your-own-app picker. A failed probe * carries no probe result; the other two reasons always carry the probe that * seeds the picker. */ type DcrOutcome = | { readonly kind: "started" } + | { readonly kind: "popup-blocked" } | { readonly kind: "fallback"; readonly reason: "probe-failed" } | { readonly kind: "fallback"; @@ -774,6 +799,10 @@ type RunDcrConnectDeps = { ) => Promise; /** Start the OAuth flow with the minted client (popup / inline). */ readonly start: (args: DcrStartArgs) => void; + /** Claim the sign-in window before any await. See `useOAuthPopupFlow.reserve`. */ + readonly reserve: () => OAuthPopupReservation; + /** Close a claimed window this flow turned out not to need. */ + readonly release: () => void; }; type RunDcrConnectInput = { @@ -804,8 +833,9 @@ type RunDcrConnectInput = { const DCR_CLIENT_NAME = "Executor"; /** - * Run the transparent DCR connect sequence: probe → register → start. + * Run the transparent DCR connect sequence: reserve → probe → register → start. * + * - Popup refused → `{ kind: "popup-blocked" }` before any network call. * - Probe failure → `{ kind: "fallback", reason: "probe-failed" }` (caller shows BYO). * - No registration endpoint → `{ kind: "fallback", reason: "no-registration-endpoint", probe }`. * - Register rejected with a message → `{ kind: "fallback", reason: "registration-failed", probe, message }` @@ -817,10 +847,23 @@ export async function runDcrConnect( deps: RunDcrConnectDeps, input: RunDcrConnectInput, ): Promise { + // Claim the sign-in window FIRST, on the click that got us here. Probe and + // register are two network round trips; the browser's user activation expires + // well before they finish, so a window opened after them is refused and the + // connect dies with nothing on screen. + const reservation = deps.reserve(); + if (reservation.kind === "blocked") return { kind: "popup-blocked" }; + const probe = await deps.probe(input.discoveryUrl); - if (probe === null) return { kind: "fallback", reason: "probe-failed" }; + if (probe === null) { + deps.release(); + return { kind: "fallback", reason: "probe-failed" }; + } const registrationEndpoint = probe.registrationEndpoint; - if (!registrationEndpoint) return { kind: "fallback", reason: "no-registration-endpoint", probe }; + if (!registrationEndpoint) { + deps.release(); + return { kind: "fallback", reason: "no-registration-endpoint", probe }; + } const slug = optimisticDcrClientSlug(probe.issuer ?? registrationEndpoint); const scopes = registrationScopes(input.declaredScopes ?? [], probe.scopesSupported ?? []); @@ -838,13 +881,17 @@ export async function runDcrConnect( redirectUri: input.redirectUri, originIntegration: input.integration, }); - if (minted === null) return { kind: "fallback", reason: "registration-failed", probe }; + if (minted === null) { + deps.release(); + return { kind: "fallback", reason: "registration-failed", probe }; + } // OAuthClientSlug is a branded string; an object return carries the failure // message (e.g. the server rejected the redirect URI) for the BYO fallback. if (typeof minted === "object") { + deps.release(); return { kind: "fallback", reason: "registration-failed", probe, message: minted.error }; } - deps.start({ client: minted, owner: input.owner }); + deps.start({ client: minted, owner: input.owner, reservation }); return { kind: "started" }; } @@ -2083,6 +2130,8 @@ function AddAccountModalView(props: AddAccountModalProps) { setCimdBusy(true); const outcome = await runCimdConnect( { + reserve: oauthPopup.reserve, + release: oauthPopup.releaseReservation, createClient: async (args: CimdCreateClientArgs): Promise => { const exit = await doCreateOAuthClient({ payload: { @@ -2100,8 +2149,9 @@ function AddAccountModalView(props: AddAccountModalProps) { if (Exit.isFailure(exit)) return null; return exit.value.client; }, - start: (args: { readonly client: OAuthClientSlug; readonly owner: Owner }): void => { + start: (args: CimdStartArgs): void => { void oauthPopup.start({ + reservation: args.reservation, payload: { client: args.client, // CIMD creates the public client under the connection owner, so @@ -2160,6 +2210,8 @@ function AddAccountModalView(props: AddAccountModalProps) { setDcrBusy(true); const outcome = await runDcrConnect( { + reserve: oauthPopup.reserve, + release: oauthPopup.releaseReservation, probe: async (url: string): Promise => { const exit = await doProbe({ payload: { url }, reactivityKeys: [] }); if (Exit.isFailure(exit)) return null; @@ -2194,6 +2246,7 @@ function AddAccountModalView(props: AddAccountModalProps) { }, start: (args: DcrStartArgs): void => { void oauthPopup.start({ + reservation: args.reservation, payload: { client: args.client, // DCR registers the client under the connection owner, so the app @@ -2236,6 +2289,10 @@ function AddAccountModalView(props: AddAccountModalProps) { success: outcome.kind === "started", ...(outcome.kind === "fallback" ? { dcr_fallback: true } : {}), }); + // Deliberately absent: a "popup-blocked" branch. Registering an app by hand + // does not make the browser open a window, so dropping to the BYO picker + // would send the user down a path that cannot succeed either. `reserve` + // already put the reason in `oauthPopup.error`, which the footer renders. if (outcome.kind === "fallback") { setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null); setDcrFailed(true); @@ -2497,17 +2554,6 @@ function AddAccountModalView(props: AddAccountModalProps) { : `${integrationName} supports Client ID Metadata Document OAuth. We'll use this Executor host's public client metadata document and sign you in.`}

- ) : dcrActive ? ( - // Transparent DCR: no picker. We register an app for you and run - // the OAuth flow with a single Connect click. -
-

No app to choose

-

- {dcrConnecting - ? `Connecting to ${integrationName}…` - : `${integrationName} supports automatic setup. We register an app for you and sign you in — no client ID or app to pick.`} -

-
) : oauthLoading ? (

Loading OAuth apps…

) : ( @@ -2739,9 +2785,6 @@ function AddAccountModalView(props: AddAccountModalProps) { /> )} - {isOAuth && oauthPopup.error ? ( -

{oauthPopup.error}

- ) : null} )} @@ -2909,6 +2952,15 @@ function AddAccountModalView(props: AddAccountModalProps) { {continueError}

) : null} + {/* Above the footer, not inside the method tab: the automatic + (CIMD/DCR) flows render no tab panel at all, and putting the + sign-in error in there left a blocked popup with nothing on + screen but the button returning to "Connect". */} + {isOAuth && oauthPopup.error ? ( +

+ {oauthPopup.error} +

+ ) : null}