Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions apps/switchyard/server/github-checks.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(Object.assign(async (input: Parameters<typeof fetch>[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);
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<string, string>[]) {
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(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");
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(); }
});
62 changes: 53 additions & 9 deletions apps/switchyard/server/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export class GitHubError extends Error {
constructor(
readonly status: number,
message: string,
readonly retryAtMs?: number,
) {
super(message);
}
Expand All @@ -52,6 +53,9 @@ export class GitHub {
private readonly api: string;
private readonly web: string;

private readonly checksCache = new Map<string, { expiresAt: number; result: Promise<ChecksReport> }>();
private readonly rateLimits = new Map<number, { until: number; error: GitHubError }>();

/**
* Installation tokens, cached until a minute before they expire.
*
Expand Down Expand Up @@ -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<RepoRef> {
const token = await this.installationToken(installationId);
const raw = await this.request<RawRepo>("GET", `/repos/${fullName}`, { auth: `Bearer ${token}` });
const raw = await this.request<RawRepo>("GET", `/repos/${fullName}`, { auth: `Bearer ${token}`, installationId });
return toRepoRef(raw, installationId);
}

Expand All @@ -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 }))
Expand All @@ -252,7 +256,7 @@ export class GitHub {
const raw = await this.request<RawPull[]>(
"GET",
`/repos/${fullName}/pulls?state=open&sort=updated&direction=desc&per_page=50`,
{ auth: `Bearer ${token}` },
{ auth: `Bearer ${token}`, installationId },
);
return raw.map(toPullRef);
}
Expand All @@ -269,7 +273,7 @@ export class GitHub {
const raw = await this.request<RawIssue[]>(
"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)
Expand All @@ -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<ChecksReport> {
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<ChecksReport> {
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) {
Expand All @@ -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
Expand All @@ -317,7 +342,7 @@ export class GitHub {
this.request<RawPull[]>(
"GET",
`/repos/${fullName}/pulls?state=all&per_page=20&head=${encodeURIComponent(fullName.split("/")[0] + ":" + ref)}`,
{ auth: `Bearer ${token}` },
{ auth: `Bearer ${token}`, installationId },
),
]);

Expand Down Expand Up @@ -363,9 +388,13 @@ export class GitHub {
): Promise<PullRef & { url: string }> {
const token = await this.installationToken(installationId);
const raw = await this.request<RawPull & { html_url: string }>("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 };
}

Expand All @@ -374,8 +403,11 @@ export class GitHub {
private async request<T>(
method: string,
path: string,
init: { auth: string; body?: unknown },
init: { auth: string; body?: unknown; installationId?: number },
): Promise<T> {
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: {
Expand All @@ -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;
Expand All @@ -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}`);
}
Expand Down
2 changes: 1 addition & 1 deletion apps/switchyard/src/components/TrackPull.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down
Loading