Skip to content

Commit d67ba99

Browse files
committed
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
1 parent 3b2dd1e commit d67ba99

4 files changed

Lines changed: 167 additions & 2 deletions

File tree

src/github/backfill.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4883,7 +4883,14 @@ async function githubJsonWithHeaders<T>(
48834883
}
48844884
if (response.status === 304 && options?.allowNotModified) return notModifiedResponse(response);
48854885
if (response.status === 404 && token && token === env.GITHUB_PUBLIC_TOKEN) {
4886-
response = await timeoutFetch(url, { headers: githubRestHeaders(undefined, options?.validators) });
4886+
// The bypass is a liveness guarantee that must survive the fallback: a read whose whole purpose is a live
4887+
// base tip must not silently become a cacheable `commit`-class GET answered from the persistent response
4888+
// cache on this retry (#10032). Propagate the flag exactly as the first request spreads it above. The
4889+
// rateLimitAdmission omission below is separate and deliberate -- it is NOT propagated here on purpose.
4890+
response = await timeoutFetch(url, {
4891+
headers: githubRestHeaders(undefined, options?.validators),
4892+
...(options?.bypassResponseCache ? { githubBypassResponseCache: true } : {}),
4893+
});
48874894
// Do not persist unauthenticated fallback rate-limit headers into the shared REST backoff state.
48884895
// GitHub's unauthenticated REST bucket is capped below LOW_REST_RATE_LIMIT_REMAINING, so recording
48894896
// successful fallback responses can incorrectly stall later token-backed segment jobs.

src/github/client.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -633,7 +633,12 @@ export async function timeoutFetch(input: RequestInfo | URL, init?: GitHubTimeou
633633
const headers = requestHeaders(input, init);
634634
const conditional = hasConditionalRequestHeader(headers);
635635
const cls = method === "GET" && !conditional && !init?.githubBypassResponseCache ? githubCacheClassForUrl(url) : null;
636-
if (method === "GET" && !conditional && cls === null && isVolatileSingleFlightEligibleGithubUrl(url, headers)) {
636+
// A bypass read forces `cls = null` (line above), which is exactly the condition that admits a URL to the
637+
// volatile coalescer -- so without this guard the flag would ROUTE the read INTO single-flight, letting one
638+
// caller's transient failure become every concurrent caller's answer (#10032). Gate the volatile branch on
639+
// the flag too, the same way the cache branch already is, so a bypass read goes straight to the network and
640+
// never publishes itself into `inFlightVolatileGets` for another caller to replay.
641+
if (method === "GET" && !conditional && !init?.githubBypassResponseCache && cls === null && isVolatileSingleFlightEligibleGithubUrl(url, headers)) {
637642
return fetchWithVolatileSingleFlight(input, init, volatileSingleFlightScope(url, headers));
638643
}
639644
const useCache = responseCache !== null && cls !== null;

test/unit/backfill.test.ts

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ import {
5050
clearGitHubResponseCacheForTest,
5151
githubRateLimitAdmissionKeyForInstallation,
5252
githubRateLimitAdmissionKeyForPublicToken,
53+
GITHUB_RESPONSE_CACHE_REPLAY_HEADER,
5354
setGitHubResponseCache,
5455
type CachedGitHubResponse,
5556
} from "../../src/github/client";
@@ -291,6 +292,57 @@ describe("GitHub backfill", () => {
291292
expect(await listLatestGitHubRateLimitObservations(env)).toEqual([]);
292293
});
293294

295+
it("#10032: the 404 unauthenticated retry still bypasses the response cache, not a cache replay", async () => {
296+
// The bypass is a liveness guarantee that must survive the public-token 404 fallback. A response cache is
297+
// installed and would replay a stale commit tip on a cacheable read -- the retry must NOT let it.
298+
const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
299+
const cacheGet = vi.fn(async () => ({
300+
status: 200,
301+
body: JSON.stringify({ commit: { committer: { date: "2000-01-01T00:00:00Z" } } }),
302+
contentType: "application/json",
303+
}));
304+
setGitHubResponseCache({ get: cacheGet, set: async () => undefined });
305+
let getFetches = 0;
306+
vi.stubGlobal("fetch", async () => {
307+
getFetches += 1;
308+
if (getFetches === 1) return new Response("not found", { status: 404 });
309+
return Response.json({ commit: { committer: { date: "2026-07-02T23:32:36.181Z" } } });
310+
});
311+
312+
// token === GITHUB_PUBLIC_TOKEN, so the first request 404s and the unauthenticated retry fires.
313+
await expect(
314+
fetchLiveBaseBranchAdvancedAt(env, "JSONbored/gittensory", "main", "public-token", githubRateLimitAdmissionKeyForPublicToken()),
315+
).resolves.toBe("2026-07-02T23:32:36.181Z");
316+
317+
expect(getFetches).toBe(2);
318+
// Neither leg was answered from (or wrote to) the persistent cache -- had the retry dropped the flag, this
319+
// cacheable commit read would have replayed the stale 2000 date instead of issuing its own request.
320+
expect(cacheGet).not.toHaveBeenCalled();
321+
});
322+
323+
it("#10032: the 404 unauthenticated retry sends no rate-limit-admission headers (deliberate omission preserved)", async () => {
324+
const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
325+
const admissionFlags: Array<boolean> = [];
326+
const replayHeaders: Array<string | null> = [];
327+
let getFetches = 0;
328+
vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => {
329+
getFetches += 1;
330+
// GitHubTimeoutFetchInit is not a plain RequestInit; read the admission flag off the passed init object.
331+
admissionFlags.push(Boolean((init as { githubRateLimitAdmission?: boolean } | undefined)?.githubRateLimitAdmission));
332+
replayHeaders.push(new Headers(init?.headers).get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER));
333+
if (getFetches === 1) return new Response("not found", { status: 404 });
334+
return Response.json({ commit: { committer: { date: "2026-07-02T23:32:36.181Z" } } });
335+
});
336+
337+
await expect(
338+
fetchLiveBaseBranchAdvancedAt(env, "JSONbored/gittensory", "main", "public-token", githubRateLimitAdmissionKeyForPublicToken()),
339+
).resolves.toBe("2026-07-02T23:32:36.181Z");
340+
341+
expect(getFetches).toBe(2);
342+
// The retry (second call) carries no admission flag -- that omission is documented and must be preserved.
343+
expect(admissionFlags[1]).toBe(false);
344+
});
345+
294346
it("fetches how far the default branch has advanced beyond this PR's base commit via the compare API", async () => {
295347
const env = createTestEnv();
296348
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {

test/unit/github-client.test.ts

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1443,3 +1443,104 @@ describe("isRateLimitedResponse", () => {
14431443
await expect(isRateLimitedResponse(response)).resolves.toBe(false);
14441444
});
14451445
});
1446+
1447+
describe("timeoutFetch — githubBypassResponseCache vs the volatile single-flight coalescer (#10032)", () => {
1448+
// A sensitive-class GitHub read (cls===null but volatile-eligible) is exactly the shape the coalescer serves:
1449+
// two concurrent NON-bypass reads share one in-flight promise. A bypass read forces cls=null too, so without
1450+
// the fix it would be ADMITTED to that same coalescer and answered by another caller's in-flight response
1451+
// (its transient failure included) instead of performing its own live network read.
1452+
const VOLATILE_URL = "https://api.github.com/repos/o/r/issues/7/events?per_page=100&page=1";
1453+
1454+
it("REGRESSION: githubBypassResponseCache must not be answered by the volatile single-flight coalescer", async () => {
1455+
let markFetchStarted!: () => void;
1456+
const fetchStarted = new Promise<void>((resolve) => {
1457+
markFetchStarted = resolve;
1458+
});
1459+
let releaseFetch!: () => void;
1460+
const fetchGate = new Promise<void>((resolve) => {
1461+
releaseFetch = resolve;
1462+
});
1463+
let fetches = 0;
1464+
vi.stubGlobal("fetch", async () => {
1465+
const mine = ++fetches;
1466+
if (mine === 1) {
1467+
markFetchStarted();
1468+
await fetchGate;
1469+
}
1470+
return Response.json({ body: mine });
1471+
});
1472+
1473+
// Leader bypass read is in flight (had it published into the coalescer, a follower would replay it); the
1474+
// second bypass read must still issue its OWN request, not join the leader.
1475+
const first = timeoutFetch(VOLATILE_URL, { githubBypassResponseCache: true });
1476+
await fetchStarted;
1477+
const second = timeoutFetch(VOLATILE_URL, { githubBypassResponseCache: true });
1478+
releaseFetch();
1479+
const [a, b] = await Promise.all([first, second]);
1480+
1481+
expect(fetches).toBe(2);
1482+
expect(a.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBeNull();
1483+
expect(b.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBeNull();
1484+
});
1485+
1486+
it("a bypass GET concurrent with a NON-bypass leader gets its own body, not the leader's replay", async () => {
1487+
let markFetchStarted!: () => void;
1488+
const fetchStarted = new Promise<void>((resolve) => {
1489+
markFetchStarted = resolve;
1490+
});
1491+
let releaseFetch!: () => void;
1492+
const fetchGate = new Promise<void>((resolve) => {
1493+
releaseFetch = resolve;
1494+
});
1495+
let fetches = 0;
1496+
vi.stubGlobal("fetch", async () => {
1497+
const mine = ++fetches;
1498+
if (mine === 1) {
1499+
markFetchStarted();
1500+
await fetchGate;
1501+
return Response.json({ caller: "leader" });
1502+
}
1503+
return Response.json({ caller: "bypass-own" });
1504+
});
1505+
1506+
// The non-bypass leader registers its shared promise in the coalescer first; the bypass read must not join
1507+
// it -- it fetches for itself and gets its own body.
1508+
const leader = timeoutFetch(VOLATILE_URL);
1509+
await fetchStarted;
1510+
const bypass = timeoutFetch(VOLATILE_URL, { githubBypassResponseCache: true });
1511+
releaseFetch();
1512+
const bypassResponse = await bypass;
1513+
1514+
expect(bypassResponse.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBeNull();
1515+
expect(await bypassResponse.json()).toEqual({ caller: "bypass-own" });
1516+
await leader;
1517+
expect(fetches).toBe(2);
1518+
});
1519+
1520+
it("leaves the volatile path unchanged for NON-bypass reads: two concurrent GETs still coalesce to one fetch", async () => {
1521+
let markFetchStarted!: () => void;
1522+
const fetchStarted = new Promise<void>((resolve) => {
1523+
markFetchStarted = resolve;
1524+
});
1525+
let releaseFetch!: () => void;
1526+
const fetchGate = new Promise<void>((resolve) => {
1527+
releaseFetch = resolve;
1528+
});
1529+
let fetches = 0;
1530+
vi.stubGlobal("fetch", async () => {
1531+
++fetches;
1532+
markFetchStarted();
1533+
await fetchGate;
1534+
return Response.json({ ok: true });
1535+
});
1536+
1537+
const leader = timeoutFetch(VOLATILE_URL);
1538+
await fetchStarted;
1539+
const joiner = timeoutFetch(VOLATILE_URL);
1540+
releaseFetch();
1541+
const [, joinerResponse] = await Promise.all([leader, joiner]);
1542+
1543+
expect(fetches).toBe(1);
1544+
expect(joinerResponse.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBe("coalesced");
1545+
});
1546+
});

0 commit comments

Comments
 (0)