diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 7875028ac8..a57ce9b5ab 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -63,7 +63,7 @@ import type { RepositorySettings, } from "../types"; import { errorMessage, nowIso, repoParts, strippedErrorMessage } from "../utils/json"; -import { createInstallationToken, getAppInstallation } from "./app"; +import { createInstallationToken, getAppInstallation, withInstallationTokenRetry } from "./app"; import { GITTENSORY_LEGACY_CONTEXT_CHECK_NAME, GITTENSORY_LEGACY_GATE_CHECK_NAME, @@ -3456,8 +3456,17 @@ export async function fetchLiveIssueState( issueNumber: number, token: string | undefined, admissionKey?: GitHubRateLimitAdmissionKey, + // When supplied, the read self-heals a stale cached installation token by re-minting once on a 401, matching + // the write-path convention (#6191) — the token argument is then owned by withInstallationTokenRetry. Omit it + // (public-token / no-installation reads) to fetch with the passed token exactly as before. + installationId?: number, ): Promise { - const result = await githubJsonWithHeaders<{ state?: string | null }>(env, repoFullName, `/issues/${issueNumber}`, token, githubRateLimitOptions(admissionKey)).catch(() => undefined); + const run = (accessToken: string | undefined) => + githubJsonWithHeaders<{ state?: string | null }>(env, repoFullName, `/issues/${issueNumber}`, accessToken, githubRateLimitOptions(admissionKey)); + const result = await (installationId === undefined + ? run(token) + : withInstallationTokenRetry(env, installationId, run) + ).catch(() => undefined); return result?.data.state ?? undefined; } @@ -3471,8 +3480,15 @@ export async function fetchLivePullRequestHeadSha( prNumber: number, token: string | undefined, admissionKey?: GitHubRateLimitAdmissionKey, + // See fetchLiveIssueState: when supplied, a stale cached installation token self-heals via one re-mint (#6191). + installationId?: number, ): Promise { - const result = await githubJsonWithHeaders<{ head?: { sha?: string | null } | null }>(env, repoFullName, `/pulls/${prNumber}`, token, githubRateLimitOptions(admissionKey)).catch(() => undefined); + const run = (accessToken: string | undefined) => + githubJsonWithHeaders<{ head?: { sha?: string | null } | null }>(env, repoFullName, `/pulls/${prNumber}`, accessToken, githubRateLimitOptions(admissionKey)); + const result = await (installationId === undefined + ? run(token) + : withInstallationTokenRetry(env, installationId, run) + ).catch(() => undefined); return result?.data.head?.sha ?? undefined; } @@ -3486,9 +3502,16 @@ export async function fetchLivePullRequestResult( prNumber: number, token: string | undefined, admissionKey?: GitHubRateLimitAdmissionKey, + // See fetchLiveIssueState: when supplied, a stale cached installation token self-heals via one re-mint (#6191), + // so a transient 401 is retried rather than surfaced as an "error" the freshness check fails closed on. + installationId?: number, ): Promise { + const run = (accessToken: string | undefined) => + githubJsonWithHeaders(env, repoFullName, `/pulls/${prNumber}`, accessToken, githubRateLimitOptions(admissionKey)); try { - const result = await githubJsonWithHeaders(env, repoFullName, `/pulls/${prNumber}`, token, githubRateLimitOptions(admissionKey)); + const result = installationId === undefined + ? await run(token) + : await withInstallationTokenRetry(env, installationId, run); return { status: "ok", data: result.data }; } catch (error) { return { status: "error", error: strippedErrorMessage(error, "GitHub live PR fetch failed").slice(0, 240) }; diff --git a/src/github/pr-freshness.ts b/src/github/pr-freshness.ts index fc4a2f9ed4..3829c812a1 100644 --- a/src/github/pr-freshness.ts +++ b/src/github/pr-freshness.ts @@ -108,11 +108,11 @@ export async function fetchPullRequestFreshness( const options: PullRequestFreshnessOptions = args.requireDraft !== undefined ? { requireDraft: args.requireDraft } : {}; let tokenError: unknown; - const token = - (await createInstallationToken(env, args.installationId).catch((error) => { - tokenError = error; - return undefined; - })) ?? env.GITHUB_PUBLIC_TOKEN; + const installationToken = await createInstallationToken(env, args.installationId).catch((error) => { + tokenError = error; + return undefined; + }); + const token = installationToken ?? env.GITHUB_PUBLIC_TOKEN; if (!token) { return classifyPullRequestFreshness(undefined, args.expectedHeadSha, { ...options, @@ -121,7 +121,17 @@ export async function fetchPullRequestFreshness( }); } const admissionKey = githubRateLimitAdmissionKeyForToken(env, token, args.installationId); - const live = await fetchLivePullRequestResult(env, args.repoFullName, args.pullNumber, token, admissionKey); + // Route through the read helper's self-heal ONLY when we hold an installation token: a stale cached one then + // re-mints once on a 401 (#6191) instead of failing the freshness check closed. A public-token fallback has no + // installation to re-mint, so it fetches with the token as before. + const live = await fetchLivePullRequestResult( + env, + args.repoFullName, + args.pullNumber, + token, + admissionKey, + installationToken !== undefined ? args.installationId : undefined, + ); if (live.status === "error") { return classifyPullRequestFreshness(undefined, args.expectedHeadSha, { ...options, diff --git a/test/unit/live-read-token-self-heal.test.ts b/test/unit/live-read-token-self-heal.test.ts new file mode 100644 index 0000000000..af74d0c019 --- /dev/null +++ b/test/unit/live-read-token-self-heal.test.ts @@ -0,0 +1,89 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + fetchLiveIssueState, + fetchLivePullRequestHeadSha, + fetchLivePullRequestResult, +} from "../../src/github/backfill"; +import { + clearInstallationTokenCacheForTest, + setInstallationTokenStore, +} from "../../src/github/app"; +import { createTestEnv } from "../helpers/d1"; + +// These read helpers historically fed a (possibly-stale) cached installation token straight into a raw GitHub +// GET with no retry, so a single 401 was surfaced as unavailable/undefined -- failing the reopen-guard and +// gate-override re-checks closed on a transient token issue. Passing an installationId now routes the read +// through withInstallationTokenRetry (#6191): the stale token is evicted, a fresh one is minted once, and the +// read succeeds -- matching the write-path self-heal convention. + +function authToken(init: RequestInit | undefined): string { + return (new Headers(init?.headers).get("authorization") ?? "").replace(/^Bearer\s+/i, ""); +} + +// A cache store whose get() hands out `stale-token` until the first rejection, then `fresh-token`; the fetch +// stub rejects the stale token with a 401 exactly once and serves `body` to the freshly-minted token. Returns +// the ordered list of tokens the network actually saw so a test can prove the retry re-minted. +function seedSelfHealingToken(body: unknown): { seenTokens: string[] } { + let rejected = false; + setInstallationTokenStore({ + get: async () => ({ token: rejected ? "fresh-token" : "stale-token", expiresAtMs: Date.now() + 60 * 60_000 }), + set: async () => {}, + }); + const seenTokens: string[] = []; + vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => { + const token = authToken(init); + seenTokens.push(token); + if (token === "stale-token") { + rejected = true; + return new Response(JSON.stringify({ message: "Bad credentials" }), { status: 401 }); + } + return Response.json(body); + }); + return { seenTokens }; +} + +describe("live read helpers self-heal a stale cached installation token (#6191)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + setInstallationTokenStore(null); + clearInstallationTokenCacheForTest(); + }); + + it("fetchLivePullRequestResult retries a 401 with a freshly-minted token", async () => { + const env = createTestEnv(); + const { seenTokens } = seedSelfHealingToken({ state: "open", head: { sha: "sha7" } }); + const result = await fetchLivePullRequestResult(env, "owner/repo", 7, "stale-token", undefined, 123); + expect(result).toEqual({ status: "ok", data: { state: "open", head: { sha: "sha7" } } }); + expect(seenTokens).toEqual(["stale-token", "fresh-token"]); + }); + + it("fetchLiveIssueState retries a 401 with a freshly-minted token", async () => { + const env = createTestEnv(); + const { seenTokens } = seedSelfHealingToken({ state: "open" }); + expect(await fetchLiveIssueState(env, "owner/repo", 42, "stale-token", undefined, 123)).toBe("open"); + expect(seenTokens).toEqual(["stale-token", "fresh-token"]); + }); + + it("fetchLivePullRequestHeadSha retries a 401 with a freshly-minted token", async () => { + const env = createTestEnv(); + const { seenTokens } = seedSelfHealingToken({ head: { sha: "live-sha" } }); + expect(await fetchLivePullRequestHeadSha(env, "owner/repo", 90, "stale-token", undefined, 123)).toBe("live-sha"); + expect(seenTokens).toEqual(["stale-token", "fresh-token"]); + }); + + // Without an installationId the helpers keep their prior behavior exactly: the passed token is used as-is and a + // 401 is NOT retried (surfaced as error/undefined), since a public-token read has no installation to re-mint. + it("does not retry when no installationId is supplied (public-token read path unchanged)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + const seenTokens: string[] = []; + vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => { + seenTokens.push(authToken(init)); + return new Response(JSON.stringify({ message: "Bad credentials" }), { status: 401 }); + }); + const result = await fetchLivePullRequestResult(env, "owner/repo", 7, "public-token"); + expect(result.status).toBe("error"); + expect(await fetchLiveIssueState(env, "owner/repo", 42, "public-token")).toBeUndefined(); + expect(await fetchLivePullRequestHeadSha(env, "owner/repo", 90, "public-token")).toBeUndefined(); + expect(seenTokens).toEqual(["public-token", "public-token", "public-token"]); + }); +}); diff --git a/test/unit/pr-freshness.test.ts b/test/unit/pr-freshness.test.ts index 5e85309a10..989a0f9993 100644 --- a/test/unit/pr-freshness.test.ts +++ b/test/unit/pr-freshness.test.ts @@ -5,11 +5,17 @@ import { pullRequestFreshnessDetail, reviewedPullRequestHeadSha, } from "../../src/github/pr-freshness"; +import { + clearInstallationTokenCacheForTest, + setInstallationTokenStore, +} from "../../src/github/app"; import { createTestEnv } from "../helpers/d1"; describe("PR freshness guards", () => { afterEach(() => { vi.unstubAllGlobals(); + setInstallationTokenStore(null); + clearInstallationTokenCacheForTest(); }); it("classifies a matching open head as current", () => { @@ -220,6 +226,37 @@ describe("PR freshness guards", () => { }); }); + it("self-heals a stale cached installation token on the live PR-freshness read (401 -> re-mint -> current)", async () => { + // Regression for the read-path fail-closed bug: fetchPullRequestFreshness fed a stale cached installation + // token straight into the live PR fetch with no retry, so a single 401 was classified `status: "stale", + // reason: "unavailable"` -- failing the reopen-guard/gate-override re-check closed for what the write path + // would have transparently retried. It now routes through withInstallationTokenRetry (#6191). + const env = createTestEnv(); + let rejected = false; + setInstallationTokenStore({ + get: async () => ({ token: rejected ? "fresh-token" : "stale-token", expiresAtMs: Date.now() + 60 * 60_000 }), + set: async () => {}, + }); + const seenTokens: string[] = []; + vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => { + const token = (new Headers(init?.headers).get("authorization") ?? "").replace(/^Bearer\s+/i, ""); + seenTokens.push(token); + if (token === "stale-token") { + rejected = true; + return new Response(JSON.stringify({ message: "Bad credentials" }), { status: 401 }); + } + return Response.json({ state: "open", head: { sha: "sha7" } }); + }); + const result = await fetchPullRequestFreshness(env, { + installationId: 123, + repoFullName: "owner/repo", + pullNumber: 7, + expectedHeadSha: "sha7", + }); + expect(result).toMatchObject({ status: "current", liveHeadSha: "sha7", liveState: "open" }); + expect(seenTokens).toEqual(["stale-token", "fresh-token"]); + }); + it("fails closed when no token can verify live PR state", async () => { const env = createTestEnv(); const result = await fetchPullRequestFreshness(env, {