From 33c19aa64e470e0fb41ce44d7ad918021a7dab56 Mon Sep 17 00:00:00 2001 From: AoD Date: Sun, 6 Sep 2026 11:24:28 +0000 Subject: [PATCH 1/2] fix(switchyard): cache checks and back off on GitHub rate limits --- apps/switchyard/server/github-checks.test.ts | 66 ++++++++++++++++++++ apps/switchyard/server/github.ts | 62 +++++++++++++++--- apps/switchyard/src/components/TrackPull.tsx | 2 +- 3 files changed, 120 insertions(+), 10 deletions(-) diff --git a/apps/switchyard/server/github-checks.test.ts b/apps/switchyard/server/github-checks.test.ts index e421645..448bd05 100644 --- a/apps/switchyard/server/github-checks.test.ts +++ b/apps/switchyard/server/github-checks.test.ts @@ -43,3 +43,69 @@ for (const scenario of [ } finally { request.mockRestore(); token.mockRestore(); } }); } + +function client() { + return new GitHub({ appId: "1", slug: "test", clientId: "test", clientSecret: "test", privateKeyPem: "", webhookSecret: null, apiUrl: "https://api.github.com", webUrl: "https://github.com" }); +} + +test("viewers share in-flight checks and cached reports, then refresh after five minutes", async () => { + const gh = client(); + const token = spyOn(gh, "installationToken").mockResolvedValue("test"); + let now = 1_000_000; + const clock = spyOn(Date, "now").mockImplementation(() => now); + const fetcher = spyOn(globalThis, "fetch").mockImplementation(async input => { + const url = String(input); + return Response.json(url.includes("/branches/") ? { commit: { sha: "abc" } } : url.includes("check-runs") ? { check_runs: [] } : []); + }); + try { + await Promise.all(Array.from({ length: 20 }, () => gh.checks(1, "o/r", "branch"))); + expect(fetcher).toHaveBeenCalledTimes(3); + await gh.checks(1, "o/r", "branch"); + expect(fetcher).toHaveBeenCalledTimes(3); + now += 300_001; + await gh.checks(1, "o/r", "branch"); + expect(fetcher).toHaveBeenCalledTimes(6); + await gh.checks(2, "o/r", "branch"); + expect(fetcher).toHaveBeenCalledTimes(9); + } finally { fetcher.mockRestore(); token.mockRestore(); clock.mockRestore(); } +}); + +for (const headers of [ + { "x-ratelimit-remaining": "0", "x-ratelimit-reset": "1600" }, + { "retry-after": "600" }, +] as Record[]) { + test(`rate limits stop all reads for an installation until reset: ${JSON.stringify(headers)}`, async () => { + const gh = client(); + const token = spyOn(gh, "installationToken").mockResolvedValue("test"); + let now = 1_000_000; + const clock = spyOn(Date, "now").mockImplementation(() => now); + const fetcher = spyOn(globalThis, "fetch") + .mockResolvedValueOnce(Response.json({ message: "API rate limit exceeded" }, { status: 403, headers })) + .mockImplementation(async () => Response.json([])); + try { + await expect(gh.checks(1, "o/r", "a")).rejects.toThrow("rate limit"); + await expect(gh.checks(1, "o/r", "a")).rejects.toThrow("rate limit"); + await expect(gh.checks(1, "o/r", "b")).rejects.toThrow("rate limit"); + await expect(gh.pulls(1, "o/other")).rejects.toThrow("rate limit"); + expect(fetcher).toHaveBeenCalledTimes(1); + await gh.pulls(2, "o/r"); + expect(fetcher).toHaveBeenCalledTimes(2); + now = 1_600_001; + await gh.pulls(1, "o/r"); + expect(fetcher).toHaveBeenCalledTimes(3); + } finally { fetcher.mockRestore(); token.mockRestore(); clock.mockRestore(); } + }); +} + +test("permission failures do not block unrelated installation reads", async () => { + const gh = client(); + const token = spyOn(gh, "installationToken").mockResolvedValue("test"); + const fetcher = spyOn(globalThis, "fetch") + .mockResolvedValueOnce(Response.json({ message: "Forbidden" }, { status: 403 })) + .mockResolvedValueOnce(Response.json([])); + try { + await expect(gh.checks(1, "o/r", "a")).rejects.toThrow("Forbidden"); + await gh.pulls(1, "o/r"); + expect(fetcher).toHaveBeenCalledTimes(2); + } finally { fetcher.mockRestore(); token.mockRestore(); } +}); diff --git a/apps/switchyard/server/github.ts b/apps/switchyard/server/github.ts index 3549130..6acc803 100644 --- a/apps/switchyard/server/github.ts +++ b/apps/switchyard/server/github.ts @@ -30,6 +30,7 @@ export class GitHubError extends Error { constructor( readonly status: number, message: string, + readonly retryAtMs?: number, ) { super(message); } @@ -52,6 +53,9 @@ export class GitHub { private readonly api: string; private readonly web: string; + private readonly checksCache = new Map }>(); + private readonly rateLimits = new Map(); + /** * Installation tokens, cached until a minute before they expire. * @@ -229,7 +233,7 @@ export class GitHub { /** One repository, read as the installation — so it works for private ones. */ async repository(installationId: number, fullName: string): Promise { const token = await this.installationToken(installationId); - const raw = await this.request("GET", `/repos/${fullName}`, { auth: `Bearer ${token}` }); + const raw = await this.request("GET", `/repos/${fullName}`, { auth: `Bearer ${token}`, installationId }); return toRepoRef(raw, installationId); } @@ -240,7 +244,7 @@ export class GitHub { const raw = await this.request<{ name: string; commit: { sha: string } }[]>( "GET", `/repos/${fullName}/branches?per_page=100`, - { auth: `Bearer ${token}` }, + { auth: `Bearer ${token}`, installationId }, ); return raw .map((b) => ({ name: b.name, sha: b.commit.sha, isDefault: b.name === defaultBranch })) @@ -252,7 +256,7 @@ export class GitHub { const raw = await this.request( "GET", `/repos/${fullName}/pulls?state=open&sort=updated&direction=desc&per_page=50`, - { auth: `Bearer ${token}` }, + { auth: `Bearer ${token}`, installationId }, ); return raw.map(toPullRef); } @@ -269,7 +273,7 @@ export class GitHub { const raw = await this.request( "GET", `/repos/${fullName}/issues?state=open&sort=updated&direction=desc&per_page=50`, - { auth: `Bearer ${token}` }, + { auth: `Bearer ${token}`, installationId }, ); return raw .filter((i) => !i.pull_request) @@ -292,13 +296,34 @@ export class GitHub { * renders "nothing pushed yet" rather than an empty list that looks broken. */ async checks(installationId: number, fullName: string, ref: string, track?: { createdAt: string; originNumber: number | null }): Promise { + const key = JSON.stringify([installationId, fullName, ref, track?.createdAt, track?.originNumber]); + const cached = this.checksCache.get(key); + if (cached && cached.expiresAt > Date.now()) return cached.result; + // Share both the report and in-flight work across rows, tabs, and viewers. + for (const [key, entry] of this.checksCache) { + if (entry.expiresAt <= Date.now()) this.checksCache.delete(key); + } + const entry = { expiresAt: Infinity, result: this.readChecks(installationId, fullName, ref, track) }; + this.checksCache.set(key, entry); + try { + const report = await entry.result; + entry.expiresAt = Date.now() + 5 * 60_000; + return report; + } catch (err) { + // Failed refreshes must not turn every mounted row into a retry loop. + entry.expiresAt = Math.max(Date.now() + 60_000, err instanceof GitHubError ? err.retryAtMs ?? 0 : 0); + throw err; + } + } + + private async readChecks(installationId: number, fullName: string, ref: string, track?: { createdAt: string; originNumber: number | null }): Promise { const token = await this.installationToken(installationId); let sha: string | null = null; try { const branch = await this.request<{ commit: { sha: string } }>( "GET", `/repos/${fullName}/branches/${encodeURIComponent(ref)}`, - { auth: `Bearer ${token}` }, + { auth: `Bearer ${token}`, installationId }, ); sha = branch.commit.sha; } catch (err) { @@ -308,7 +333,7 @@ export class GitHub { const [runs, pulls] = await Promise.all([ sha ? this.request<{ check_runs: RawCheckRun[] }>("GET", `/repos/${fullName}/commits/${sha}/check-runs?per_page=50`, { - auth: `Bearer ${token}`, + auth: `Bearer ${token}`, installationId, }) : Promise.resolve({ check_runs: [] as RawCheckRun[] }), // `state=all`, not `state=open`. A branch whose pull request has been // merged is the *most* interesting case — it is the one where the work @@ -317,7 +342,7 @@ export class GitHub { this.request( "GET", `/repos/${fullName}/pulls?state=all&per_page=20&head=${encodeURIComponent(fullName.split("/")[0] + ":" + ref)}`, - { auth: `Bearer ${token}` }, + { auth: `Bearer ${token}`, installationId }, ), ]); @@ -363,9 +388,13 @@ export class GitHub { ): Promise { const token = await this.installationToken(installationId); const raw = await this.request("POST", `/repos/${fullName}/pulls`, { - auth: `Bearer ${token}`, + auth: `Bearer ${token}`, installationId, body: input, }); + for (const key of this.checksCache.keys()) { + const [installation, repo, ref] = JSON.parse(key); + if (installation === installationId && repo === fullName && ref === input.head) this.checksCache.delete(key); + } return { ...toPullRef(raw), url: raw.html_url }; } @@ -374,8 +403,11 @@ export class GitHub { private async request( method: string, path: string, - init: { auth: string; body?: unknown }, + init: { auth: string; body?: unknown; installationId?: number }, ): Promise { + const blocked = init.installationId === undefined ? undefined : this.rateLimits.get(init.installationId); + if (blocked && blocked.until > Date.now()) throw blocked.error; + if (init.installationId !== undefined) this.rateLimits.delete(init.installationId); const res = await fetch(path.startsWith("http") ? path : `${this.api}${path}`, { method, headers: { @@ -400,6 +432,17 @@ export class GitHub { } catch { /* not JSON — the status line is the whole story */ } + const limited = res.status === 429 || (res.status === 403 && ( + res.headers.get("x-ratelimit-remaining") === "0" || res.headers.has("retry-after") || /rate limit/i.test(message) + )); + if (limited) { + const retrySeconds = Number(res.headers.get("retry-after")); + const reset = Number(res.headers.get("x-ratelimit-reset")) * 1000; + const until = Math.max(Date.now() + (retrySeconds > 0 ? retrySeconds * 1000 : 60_000), Number.isFinite(reset) ? reset : 0); + const error = new GitHubError(res.status, message, until); + if (init.installationId !== undefined) this.rateLimits.set(init.installationId, { until, error }); + throw error; + } throw new GitHubError(res.status, message); } return (text ? JSON.parse(text) : undefined) as T; @@ -409,6 +452,7 @@ export class GitHub { /** A GitHub failure as one of ours, keeping the part a person can act on. */ export function asHttpError(err: unknown, whatFor: string): HttpError { if (err instanceof GitHubError) { + if (err.retryAtMs) return new HttpError(503, "github_rate_limited", `GitHub's request limit was reached. Switchyard will retry after ${new Date(err.retryAtMs).toISOString()}.`); if (err.status === 401 || err.status === 403) { return new HttpError(502, "github_rejected", `GitHub would not let switchyard ${whatFor}: ${err.message}`); } diff --git a/apps/switchyard/src/components/TrackPull.tsx b/apps/switchyard/src/components/TrackPull.tsx index f4e0b67..a55208b 100644 --- a/apps/switchyard/src/components/TrackPull.tsx +++ b/apps/switchyard/src/components/TrackPull.tsx @@ -35,7 +35,7 @@ export function TrackPull({ track }: { track: Track }) { } }; void refresh(); - const interval = window.setInterval(() => void refresh(), 60_000); + const interval = window.setInterval(() => void refresh(), 5 * 60_000); document.addEventListener("visibilitychange", refresh); return () => { live = false; window.clearInterval(interval); document.removeEventListener("visibilitychange", refresh); }; }, [track.id, track.branch, track.status]); From e24a4215073e65b178f83217ea57572614c4f697 Mon Sep 17 00:00:00 2001 From: AoD Date: Sun, 6 Sep 2026 11:26:05 +0000 Subject: [PATCH 2/2] test(switchyard): satisfy Bun fetch mock types --- apps/switchyard/server/github-checks.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/switchyard/server/github-checks.test.ts b/apps/switchyard/server/github-checks.test.ts index 448bd05..6cc5812 100644 --- a/apps/switchyard/server/github-checks.test.ts +++ b/apps/switchyard/server/github-checks.test.ts @@ -53,10 +53,10 @@ test("viewers share in-flight checks and cached reports, then refresh after five const token = spyOn(gh, "installationToken").mockResolvedValue("test"); let now = 1_000_000; const clock = spyOn(Date, "now").mockImplementation(() => now); - const fetcher = spyOn(globalThis, "fetch").mockImplementation(async input => { + const fetcher = spyOn(globalThis, "fetch").mockImplementation(Object.assign(async (input: Parameters[0]) => { const url = String(input); return Response.json(url.includes("/branches/") ? { commit: { sha: "abc" } } : url.includes("check-runs") ? { check_runs: [] } : []); - }); + }, { preconnect: fetch.preconnect })); try { await Promise.all(Array.from({ length: 20 }, () => gh.checks(1, "o/r", "branch"))); expect(fetcher).toHaveBeenCalledTimes(3); @@ -81,7 +81,7 @@ for (const headers of [ const clock = spyOn(Date, "now").mockImplementation(() => now); const fetcher = spyOn(globalThis, "fetch") .mockResolvedValueOnce(Response.json({ message: "API rate limit exceeded" }, { status: 403, headers })) - .mockImplementation(async () => Response.json([])); + .mockImplementation(Object.assign(async () => Response.json([]), { preconnect: fetch.preconnect })); try { await expect(gh.checks(1, "o/r", "a")).rejects.toThrow("rate limit"); await expect(gh.checks(1, "o/r", "a")).rejects.toThrow("rate limit");