diff --git a/src/github/backfill.ts b/src/github/backfill.ts index ff5c7b2a1..30031edb3 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 13af732d1..22a3bf192 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 8b2fff89b..5abc43bcb 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 f22b37404..cc25c73de 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"); + }); +});