From f23d0084b6e023ffb1af9e332d8666affc99d542 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sun, 11 Oct 2026 10:35:54 -0700 Subject: [PATCH] perf(github): resolve credentials once per GraphQL request (#18289) --- .../src/server/GitHubApi.test.ts | 52 ++++++++++++++++--- .../src/server/GitHubApi.ts | 46 ++++++++-------- 2 files changed, 65 insertions(+), 33 deletions(-) diff --git a/packages/source-control-github/src/server/GitHubApi.test.ts b/packages/source-control-github/src/server/GitHubApi.test.ts index e77f10ec9370ab354aad8a739530615cd3ec1e08..44aa7f50874ae67abdc4d24e811bc7d5087ff5f3 100644 --- a/packages/source-control-github/src/server/GitHubApi.test.ts +++ b/packages/source-control-github/src/server/GitHubApi.test.ts @@ -1,6 +1,5 @@ import { describe, expect, it } from "@effect/vitest"; import * as NodeCrypto from "@effect/platform-node/NodeCrypto"; -import * as NodeServices from "@effect/platform-node/NodeServices"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Redacted from "effect/Redacted"; @@ -24,16 +23,20 @@ function harness(respond: (request: HttpClientRequest.HttpClientRequest) => Resp const requests: Array = []; let tokens = ["first", "second"]; let invalidations = 0; + let credentialReads = 0; const credentials = Layer.succeed( GitHubCredentials.GitHubCredentials, GitHubCredentials.GitHubCredentials.of({ get: (host) => - Effect.sync(() => ({ - host, - token: Redacted.make(tokens[0]!), - source: "gh" as const, - fingerprint: `${host}:${tokens[0]}`, - })), + Effect.sync(() => { + credentialReads++; + return { + host, + token: Redacted.make(tokens[0]!), + source: "gh" as const, + fingerprint: `${host}:${tokens[0]}`, + }; + }), invalidate: () => Effect.sync(() => { invalidations++; @@ -53,7 +56,12 @@ function harness(respond: (request: HttpClientRequest.HttpClientRequest) => Resp Layer.provideMerge(GitHubQuota.layer), Layer.provideMerge(SourceControlRateLimit.layer), ); - return { layer, requests, invalidations: () => invalidations }; + return { + layer, + requests, + invalidations: () => invalidations, + credentialReads: () => credentialReads, + }; } const json = (body: unknown, init?: ResponseInit) => @@ -119,6 +127,34 @@ describe("GitHubApi", () => { }).pipe(Effect.provide(layer)); }); + it.effect("resolves GraphQL credentials once per request and refreshes after refusal", () => { + const { layer, requests, invalidations, credentialReads } = harness((request) => + request.headers.authorization === "Bearer first" + ? new Response(null, { status: 401 }) + : json({ data: { viewer: { login: "julius" } } }), + ); + return Effect.gen(function* () { + const api = yield* GitHubApi.GitHubApi; + const input = { + host: "github.com", + operation: "viewer", + query: "query { viewer { login } }", + }; + const refused = yield* Effect.flip(api.graphql(input)); + expect(refused._tag).toBe("GitHubApiAuthenticationError"); + expect(credentialReads()).toBe(1); + expect(invalidations()).toBe(1); + + const body = yield* api.graphql(input); + expect(body).toContain('"login":"julius"'); + expect(credentialReads()).toBe(2); + expect(requests.map((request) => request.headers.authorization)).toEqual([ + "Bearer first", + "Bearer second", + ]); + }).pipe(Effect.provide(layer)); + }); + it.effect( "traces the operation, path, query and quota, never the query string or variables", () => { diff --git a/packages/source-control-github/src/server/GitHubApi.ts b/packages/source-control-github/src/server/GitHubApi.ts index 29d8b2904b1ff6671f55debc70955d2e6ffd3bac..63f8531086d941ef56bda3c85fee869212494fcc 100644 --- a/packages/source-control-github/src/server/GitHubApi.ts +++ b/packages/source-control-github/src/server/GitHubApi.ts @@ -534,8 +534,6 @@ export const make = Effect.gen(function* () { const graphql: GitHubApi["Service"]["graphql"] = Effect.fn("GitHubApi.graphql")( function* (input) { const host = normalizeHost(input.host); - const { fingerprint } = yield* credential(host); - const scope = (yield* SourceControlRateLimit.CredentialScope) || fingerprint; const allowReserve = input.allowReserve ?? (yield* SourceControlRateLimit.Interactive); // The document, never its variables: user text (bodies, search terms) travels as variables. yield* Effect.annotateCurrentSpan({ @@ -546,32 +544,30 @@ export const make = Effect.gen(function* () { ? `${input.query.slice(0, TRACED_QUERY_MAX_CHARS)}…` : input.query, }); - return yield* Effect.gen(function* () { - const response = yield* send({ + const response = yield* send({ + host, + operation: input.operation, + request: HttpClientRequest.post(gitHubApiUrls(host).graphql).pipe( + HttpClientRequest.acceptJson, + HttpClientRequest.bodyJsonUnsafe({ + query: input.query, + variables: input.variables ?? {}, + }), + ), + maxResponseBytes: input.maxResponseBytes ?? DEFAULT_MAX_RESPONSE_BYTES, + allowReserve, + acceptNotModified: false, + graphql: true, + }); + // A body cut at the byte cap hides any `errors` past the cut and cannot be decoded. + if (response.truncated) { + return yield* new GitHubApiResponseError({ host, operation: input.operation, - request: HttpClientRequest.post(gitHubApiUrls(host).graphql).pipe( - HttpClientRequest.acceptJson, - HttpClientRequest.bodyJsonUnsafe({ - query: input.query, - variables: input.variables ?? {}, - }), - ), - maxResponseBytes: input.maxResponseBytes ?? DEFAULT_MAX_RESPONSE_BYTES, - allowReserve, - acceptNotModified: false, - graphql: true, + status: response.status, }); - // A body cut at the byte cap hides any `errors` past the cut and cannot be decoded. - if (response.truncated) { - return yield* new GitHubApiResponseError({ - host, - operation: input.operation, - status: response.status, - }); - } - return response.body; - }).pipe(Effect.provideService(SourceControlRateLimit.CredentialScope, scope)); + } + return response.body; }, ); -- 2.54.0