From d67ba99edd52f67f8ae29afc26842ae1da0a4d37 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:02:08 +0900 Subject: [PATCH] fix(github): make githubBypassResponseCache force an own network read on both legs githubBypassResponseCache is documented as an absolute force-fresh guarantee, but two legs broke it for its one production caller (fetchLiveBaseBranchAdvancedAt, the force-fresh-rebase gate's live base-tip read): 1. timeoutFetch routed the read INTO the volatile single-flight coalescer. The flag forces cls=null, which is exactly the condition that admits a URL to the coalescer, so a bypass read joined any concurrent identical in-flight read and was answered by its response -- a transient failure included -- instead of issuing its own request. Gate the volatile branch on the flag too, the same way the cache branch already is, so a bypass read goes straight to the network and never publishes itself for replay. 2. githubJsonWithHeaders' 404 unauthenticated retry dropped the flag, so on that path the read became an ordinary cacheable commit-class GET answerable from the persistent response cache (up to GITHUB_COMMIT_CACHE_TTL_SECONDS stale) for a call whose whole purpose is liveness. Propagate the flag on the retry exactly as the first request spreads it; the deliberate rateLimitAdmission omission on that retry is preserved. The volatile path is unchanged for every non-bypass read: the same URLs coalesce, the same exclusion list applies, and the coalesced/bypassed metrics are emitted as before. Closes #10032 --- src/github/backfill.ts | 9 ++- src/github/client.ts | 7 ++- test/unit/backfill.test.ts | 52 ++++++++++++++++ test/unit/github-client.test.ts | 101 ++++++++++++++++++++++++++++++++ 4 files changed, 167 insertions(+), 2 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index ff5c7b2a19..30031edb3b 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -4883,7 +4883,14 @@ async function githubJsonWithHeaders( } if (response.status === 304 && options?.allowNotModified) return notModifiedResponse(response); if (response.status === 404 && token && token === env.GITHUB_PUBLIC_TOKEN) { - response = await timeoutFetch(url, { headers: githubRestHeaders(undefined, options?.validators) }); + // The bypass is a liveness guarantee that must survive the fallback: a read whose whole purpose is a live + // base tip must not silently become a cacheable `commit`-class GET answered from the persistent response + // cache on this retry (#10032). Propagate the flag exactly as the first request spreads it above. The + // rateLimitAdmission omission below is separate and deliberate -- it is NOT propagated here on purpose. + response = await timeoutFetch(url, { + headers: githubRestHeaders(undefined, options?.validators), + ...(options?.bypassResponseCache ? { githubBypassResponseCache: true } : {}), + }); // Do not persist unauthenticated fallback rate-limit headers into the shared REST backoff state. // GitHub's unauthenticated REST bucket is capped below LOW_REST_RATE_LIMIT_REMAINING, so recording // successful fallback responses can incorrectly stall later token-backed segment jobs. diff --git a/src/github/client.ts b/src/github/client.ts index 13af732d1c..22a3bf1924 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -633,7 +633,12 @@ export async function timeoutFetch(input: RequestInfo | URL, init?: GitHubTimeou const headers = requestHeaders(input, init); const conditional = hasConditionalRequestHeader(headers); const cls = method === "GET" && !conditional && !init?.githubBypassResponseCache ? githubCacheClassForUrl(url) : null; - if (method === "GET" && !conditional && cls === null && isVolatileSingleFlightEligibleGithubUrl(url, headers)) { + // A bypass read forces `cls = null` (line above), which is exactly the condition that admits a URL to the + // volatile coalescer -- so without this guard the flag would ROUTE the read INTO single-flight, letting one + // caller's transient failure become every concurrent caller's answer (#10032). Gate the volatile branch on + // the flag too, the same way the cache branch already is, so a bypass read goes straight to the network and + // never publishes itself into `inFlightVolatileGets` for another caller to replay. + if (method === "GET" && !conditional && !init?.githubBypassResponseCache && cls === null && isVolatileSingleFlightEligibleGithubUrl(url, headers)) { return fetchWithVolatileSingleFlight(input, init, volatileSingleFlightScope(url, headers)); } const useCache = responseCache !== null && cls !== null; diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 8b2fff89b5..5abc43bcb2 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -50,6 +50,7 @@ import { clearGitHubResponseCacheForTest, githubRateLimitAdmissionKeyForInstallation, githubRateLimitAdmissionKeyForPublicToken, + GITHUB_RESPONSE_CACHE_REPLAY_HEADER, setGitHubResponseCache, type CachedGitHubResponse, } from "../../src/github/client"; @@ -291,6 +292,57 @@ describe("GitHub backfill", () => { expect(await listLatestGitHubRateLimitObservations(env)).toEqual([]); }); + it("#10032: the 404 unauthenticated retry still bypasses the response cache, not a cache replay", async () => { + // The bypass is a liveness guarantee that must survive the public-token 404 fallback. A response cache is + // installed and would replay a stale commit tip on a cacheable read -- the retry must NOT let it. + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + const cacheGet = vi.fn(async () => ({ + status: 200, + body: JSON.stringify({ commit: { committer: { date: "2000-01-01T00:00:00Z" } } }), + contentType: "application/json", + })); + setGitHubResponseCache({ get: cacheGet, set: async () => undefined }); + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + getFetches += 1; + if (getFetches === 1) return new Response("not found", { status: 404 }); + return Response.json({ commit: { committer: { date: "2026-07-02T23:32:36.181Z" } } }); + }); + + // token === GITHUB_PUBLIC_TOKEN, so the first request 404s and the unauthenticated retry fires. + await expect( + fetchLiveBaseBranchAdvancedAt(env, "JSONbored/gittensory", "main", "public-token", githubRateLimitAdmissionKeyForPublicToken()), + ).resolves.toBe("2026-07-02T23:32:36.181Z"); + + expect(getFetches).toBe(2); + // Neither leg was answered from (or wrote to) the persistent cache -- had the retry dropped the flag, this + // cacheable commit read would have replayed the stale 2000 date instead of issuing its own request. + expect(cacheGet).not.toHaveBeenCalled(); + }); + + it("#10032: the 404 unauthenticated retry sends no rate-limit-admission headers (deliberate omission preserved)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + const admissionFlags: Array = []; + const replayHeaders: Array = []; + let getFetches = 0; + vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => { + getFetches += 1; + // GitHubTimeoutFetchInit is not a plain RequestInit; read the admission flag off the passed init object. + admissionFlags.push(Boolean((init as { githubRateLimitAdmission?: boolean } | undefined)?.githubRateLimitAdmission)); + replayHeaders.push(new Headers(init?.headers).get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)); + if (getFetches === 1) return new Response("not found", { status: 404 }); + return Response.json({ commit: { committer: { date: "2026-07-02T23:32:36.181Z" } } }); + }); + + await expect( + fetchLiveBaseBranchAdvancedAt(env, "JSONbored/gittensory", "main", "public-token", githubRateLimitAdmissionKeyForPublicToken()), + ).resolves.toBe("2026-07-02T23:32:36.181Z"); + + expect(getFetches).toBe(2); + // The retry (second call) carries no admission flag -- that omission is documented and must be preserved. + expect(admissionFlags[1]).toBe(false); + }); + it("fetches how far the default branch has advanced beyond this PR's base commit via the compare API", async () => { const env = createTestEnv(); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { diff --git a/test/unit/github-client.test.ts b/test/unit/github-client.test.ts index f22b374043..cc25c73de6 100644 --- a/test/unit/github-client.test.ts +++ b/test/unit/github-client.test.ts @@ -1443,3 +1443,104 @@ describe("isRateLimitedResponse", () => { await expect(isRateLimitedResponse(response)).resolves.toBe(false); }); }); + +describe("timeoutFetch — githubBypassResponseCache vs the volatile single-flight coalescer (#10032)", () => { + // A sensitive-class GitHub read (cls===null but volatile-eligible) is exactly the shape the coalescer serves: + // two concurrent NON-bypass reads share one in-flight promise. A bypass read forces cls=null too, so without + // the fix it would be ADMITTED to that same coalescer and answered by another caller's in-flight response + // (its transient failure included) instead of performing its own live network read. + const VOLATILE_URL = "https://api.github.com/repos/o/r/issues/7/events?per_page=100&page=1"; + + it("REGRESSION: githubBypassResponseCache must not be answered by the volatile single-flight coalescer", async () => { + let markFetchStarted!: () => void; + const fetchStarted = new Promise((resolve) => { + markFetchStarted = resolve; + }); + let releaseFetch!: () => void; + const fetchGate = new Promise((resolve) => { + releaseFetch = resolve; + }); + let fetches = 0; + vi.stubGlobal("fetch", async () => { + const mine = ++fetches; + if (mine === 1) { + markFetchStarted(); + await fetchGate; + } + return Response.json({ body: mine }); + }); + + // Leader bypass read is in flight (had it published into the coalescer, a follower would replay it); the + // second bypass read must still issue its OWN request, not join the leader. + const first = timeoutFetch(VOLATILE_URL, { githubBypassResponseCache: true }); + await fetchStarted; + const second = timeoutFetch(VOLATILE_URL, { githubBypassResponseCache: true }); + releaseFetch(); + const [a, b] = await Promise.all([first, second]); + + expect(fetches).toBe(2); + expect(a.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBeNull(); + expect(b.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBeNull(); + }); + + it("a bypass GET concurrent with a NON-bypass leader gets its own body, not the leader's replay", async () => { + let markFetchStarted!: () => void; + const fetchStarted = new Promise((resolve) => { + markFetchStarted = resolve; + }); + let releaseFetch!: () => void; + const fetchGate = new Promise((resolve) => { + releaseFetch = resolve; + }); + let fetches = 0; + vi.stubGlobal("fetch", async () => { + const mine = ++fetches; + if (mine === 1) { + markFetchStarted(); + await fetchGate; + return Response.json({ caller: "leader" }); + } + return Response.json({ caller: "bypass-own" }); + }); + + // The non-bypass leader registers its shared promise in the coalescer first; the bypass read must not join + // it -- it fetches for itself and gets its own body. + const leader = timeoutFetch(VOLATILE_URL); + await fetchStarted; + const bypass = timeoutFetch(VOLATILE_URL, { githubBypassResponseCache: true }); + releaseFetch(); + const bypassResponse = await bypass; + + expect(bypassResponse.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBeNull(); + expect(await bypassResponse.json()).toEqual({ caller: "bypass-own" }); + await leader; + expect(fetches).toBe(2); + }); + + it("leaves the volatile path unchanged for NON-bypass reads: two concurrent GETs still coalesce to one fetch", async () => { + let markFetchStarted!: () => void; + const fetchStarted = new Promise((resolve) => { + markFetchStarted = resolve; + }); + let releaseFetch!: () => void; + const fetchGate = new Promise((resolve) => { + releaseFetch = resolve; + }); + let fetches = 0; + vi.stubGlobal("fetch", async () => { + ++fetches; + markFetchStarted(); + await fetchGate; + return Response.json({ ok: true }); + }); + + const leader = timeoutFetch(VOLATILE_URL); + await fetchStarted; + const joiner = timeoutFetch(VOLATILE_URL); + releaseFetch(); + const [, joinerResponse] = await Promise.all([leader, joiner]); + + expect(fetches).toBe(1); + expect(joinerResponse.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBe("coalesced"); + }); +});