mirror of
https://github.com/RayLabsHQ/gitea-mirror.git
synced 2026-08-12 03:42:44 +02:00
feat(github): reuse ETags across syncs via conditional requests (#356)
* feat(github): reuse ETags across syncs via conditional requests The mirror re-lists every repository's pull requests on each scheduled sync with `state: "all"`, and the Octokit client was created with the throttling plugin but no conditional-request/ETag layer. Every sync therefore re-downloaded unchanged data as full 200s and spent full rate-limit budget. Add an ETag cache wired into `createGitHubClient`: for each GET it replays the previously stored `If-None-Match`, so GitHub returns `304 Not Modified` (which does not count against the token's primary rate limit) when nothing changed, and the cached body is reused. The store is process-lifetime and scoped per user so ETags survive the per-sync client re-creation without leaking data across tokens. Non-GET requests are untouched. Refs #355 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 * test(github): cover conditional-request ETag cache Add unit tests driving a real Octokit instance with a stubbed fetch: the second GET replays `If-None-Match`, a `304` is transparently served from cache as a 200 with the same body, responses without an ETag are re-fetched, non-GET requests are never made conditional, and cache entries are isolated by scope. Refs #355 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 * fix: key conditional-request cache by expanded URL The request hook built the cache key from requestOptions.url, which is still the route template (/repos/{owner}/{repo}/pulls) at hook time. Every repo therefore shared one entry per user + endpoint, so with more than one repo per user the stored ETag never matched and the 304 path stopped firing. Expand the route via octokit.request.endpoint.parse before building the key, and add a two-repo regression test that fails on the shared-key behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79
This commit is contained in:
Josh Free
@@ -0,0 +1,177 @@
|
||||
import { describe, expect, test } from "bun:test";
|
||||
import { Octokit } from "@octokit/rest";
|
||||
import {
|
||||
applyConditionalRequests,
|
||||
conditionalRequestCacheKey,
|
||||
InMemoryConditionalRequestStore,
|
||||
} from "@/lib/github-conditional-requests";
|
||||
|
||||
function jsonResponse(
|
||||
body: unknown,
|
||||
init: { status: number; etag?: string; link?: string },
|
||||
): Response {
|
||||
const headers: Record<string, string> = {
|
||||
"content-type": "application/json",
|
||||
};
|
||||
if (init.etag) headers.etag = init.etag;
|
||||
if (init.link) headers.link = init.link;
|
||||
return new Response(init.status === 304 ? null : JSON.stringify(body), {
|
||||
status: init.status,
|
||||
headers,
|
||||
});
|
||||
}
|
||||
|
||||
function clientWithFetch(fetch: (url: string, init: any) => Promise<Response>) {
|
||||
return new Octokit({
|
||||
auth: "test-token",
|
||||
request: { fetch: fetch as unknown as typeof globalThis.fetch },
|
||||
});
|
||||
}
|
||||
|
||||
describe("applyConditionalRequests", () => {
|
||||
test("replays If-None-Match and serves the cached body on 304", async () => {
|
||||
const etag = 'W/"abc123"';
|
||||
const pulls = [{ number: 1, title: "one" }];
|
||||
let calls = 0;
|
||||
let secondRequestIfNoneMatch: string | undefined;
|
||||
|
||||
const octokit = clientWithFetch(async (_url, init) => {
|
||||
calls += 1;
|
||||
if (calls === 1) {
|
||||
return jsonResponse(pulls, { status: 200, etag });
|
||||
}
|
||||
secondRequestIfNoneMatch = init?.headers?.["if-none-match"];
|
||||
return jsonResponse(null, { status: 304, etag });
|
||||
});
|
||||
applyConditionalRequests(octokit, {
|
||||
store: new InMemoryConditionalRequestStore(),
|
||||
scope: "user-1",
|
||||
});
|
||||
|
||||
const first = await octokit.request("GET /repos/{owner}/{repo}/pulls", {
|
||||
owner: "o",
|
||||
repo: "r",
|
||||
state: "all",
|
||||
});
|
||||
const second = await octokit.request("GET /repos/{owner}/{repo}/pulls", {
|
||||
owner: "o",
|
||||
repo: "r",
|
||||
state: "all",
|
||||
});
|
||||
|
||||
expect(calls).toBe(2);
|
||||
expect(secondRequestIfNoneMatch).toBe(etag);
|
||||
// A 304 is transparently presented as a 200 carrying the cached body.
|
||||
expect(second.status).toBe(200);
|
||||
expect(second.data).toEqual(first.data);
|
||||
expect(second.data).toEqual(pulls);
|
||||
});
|
||||
|
||||
test("does not attach conditional headers to non-GET requests", async () => {
|
||||
let sawIfNoneMatch = false;
|
||||
const octokit = clientWithFetch(async (_url, init) => {
|
||||
if (init?.headers?.["if-none-match"]) sawIfNoneMatch = true;
|
||||
return jsonResponse({ ok: true }, { status: 201 });
|
||||
});
|
||||
applyConditionalRequests(octokit, {
|
||||
store: new InMemoryConditionalRequestStore(),
|
||||
scope: "user-1",
|
||||
});
|
||||
|
||||
await octokit.request("POST /repos/{owner}/{repo}/pulls", {
|
||||
owner: "o",
|
||||
repo: "r",
|
||||
title: "x",
|
||||
head: "a",
|
||||
base: "b",
|
||||
});
|
||||
|
||||
expect(sawIfNoneMatch).toBe(false);
|
||||
});
|
||||
|
||||
test("makes a full request again when the response carries no ETag", async () => {
|
||||
let calls = 0;
|
||||
let secondRequestIfNoneMatch: string | undefined;
|
||||
const octokit = clientWithFetch(async (_url, init) => {
|
||||
calls += 1;
|
||||
if (calls >= 2) secondRequestIfNoneMatch = init?.headers?.["if-none-match"];
|
||||
return jsonResponse([{ number: 1 }], { status: 200 }); // no etag
|
||||
});
|
||||
applyConditionalRequests(octokit, {
|
||||
store: new InMemoryConditionalRequestStore(),
|
||||
scope: "user-1",
|
||||
});
|
||||
|
||||
await octokit.request("GET /repos/{owner}/{repo}/pulls", {
|
||||
owner: "o",
|
||||
repo: "r",
|
||||
});
|
||||
const second = await octokit.request("GET /repos/{owner}/{repo}/pulls", {
|
||||
owner: "o",
|
||||
repo: "r",
|
||||
});
|
||||
|
||||
expect(calls).toBe(2);
|
||||
expect(secondRequestIfNoneMatch).toBeUndefined();
|
||||
expect(second.status).toBe(200);
|
||||
});
|
||||
|
||||
test("keys cache entries per expanded URL so repos do not collide", async () => {
|
||||
// Each resource gets its own ETag; the stub returns 304 only when the
|
||||
// presented If-None-Match matches the ETag issued for that exact URL.
|
||||
const etagByUrl = new Map<string, string>();
|
||||
let notModifiedCount = 0;
|
||||
|
||||
const octokit = clientWithFetch(async (url, init) => {
|
||||
const ifNoneMatch = init?.headers?.["if-none-match"] as
|
||||
| string
|
||||
| undefined;
|
||||
let etag = etagByUrl.get(url);
|
||||
if (!etag) {
|
||||
etag = `W/"etag-${etagByUrl.size + 1}"`;
|
||||
etagByUrl.set(url, etag);
|
||||
}
|
||||
if (ifNoneMatch && ifNoneMatch === etag) {
|
||||
notModifiedCount += 1;
|
||||
return jsonResponse(null, { status: 304, etag });
|
||||
}
|
||||
return jsonResponse([{ url }], { status: 200, etag });
|
||||
});
|
||||
applyConditionalRequests(octokit, {
|
||||
store: new InMemoryConditionalRequestStore(),
|
||||
scope: "user-1",
|
||||
});
|
||||
|
||||
const get = (owner: string, repo: string) =>
|
||||
octokit.request("GET /repos/{owner}/{repo}/pulls", {
|
||||
owner,
|
||||
repo,
|
||||
state: "all",
|
||||
});
|
||||
|
||||
const first = await get("alpha", "one");
|
||||
await get("beta", "two");
|
||||
const third = await get("alpha", "one");
|
||||
|
||||
// The repeated first request must revalidate against its OWN ETag and come
|
||||
// back as a 304 cache hit, not be clobbered by the second repo's entry.
|
||||
expect(notModifiedCount).toBe(1);
|
||||
expect(third.status).toBe(200);
|
||||
expect(third.data).toEqual(first.data);
|
||||
});
|
||||
|
||||
test("isolates cached entries by scope", () => {
|
||||
const store = new InMemoryConditionalRequestStore();
|
||||
store.set(conditionalRequestCacheKey("user-1", "GET", "/x"), {
|
||||
etag: "a",
|
||||
data: 1,
|
||||
status: 200,
|
||||
});
|
||||
expect(
|
||||
store.get(conditionalRequestCacheKey("user-2", "GET", "/x")),
|
||||
).toBeUndefined();
|
||||
expect(
|
||||
store.get(conditionalRequestCacheKey("user-1", "GET", "/x"))?.etag,
|
||||
).toBe("a");
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,140 @@
|
||||
import type { Octokit } from "@octokit/rest";
|
||||
|
||||
/**
|
||||
* A single cached GitHub response, retained so that a later conditional request
|
||||
* which comes back `304 Not Modified` can be served from memory instead of
|
||||
* downloading the full body again. Only the fields the mirror actually consumes
|
||||
* are stored.
|
||||
*/
|
||||
export interface CachedResponse {
|
||||
etag: string;
|
||||
data: unknown;
|
||||
status: number;
|
||||
/** Preserved so `octokit.paginate` can keep following pages on a cache hit. */
|
||||
link?: string;
|
||||
}
|
||||
|
||||
export interface ConditionalRequestStore {
|
||||
get(key: string): CachedResponse | undefined;
|
||||
set(key: string, value: CachedResponse): void;
|
||||
}
|
||||
|
||||
/**
|
||||
* Process-lifetime, bounded in-memory store. Gitea Mirror runs as a long-lived
|
||||
* server whose scheduler re-syncs on an interval, so keeping ETags in memory is
|
||||
* enough to turn each repeated poll into a cheap `304`. The bound keeps memory
|
||||
* predictable; evicting an entry only costs one full response on the next sync.
|
||||
*/
|
||||
export class InMemoryConditionalRequestStore implements ConditionalRequestStore {
|
||||
private readonly entries = new Map<string, CachedResponse>();
|
||||
|
||||
constructor(private readonly maxEntries = 5000) {}
|
||||
|
||||
get(key: string): CachedResponse | undefined {
|
||||
return this.entries.get(key);
|
||||
}
|
||||
|
||||
set(key: string, value: CachedResponse): void {
|
||||
// Delete-then-set so Map iteration order approximates LRU for eviction.
|
||||
this.entries.delete(key);
|
||||
this.entries.set(key, value);
|
||||
if (this.entries.size > this.maxEntries) {
|
||||
const oldest = this.entries.keys().next().value;
|
||||
if (oldest !== undefined) this.entries.delete(oldest);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Shared across every client so ETags survive the per-sync re-creation of the
|
||||
* Octokit instance (a fresh client is built on each scheduled sync).
|
||||
*/
|
||||
export const defaultConditionalRequestStore =
|
||||
new InMemoryConditionalRequestStore();
|
||||
|
||||
export function conditionalRequestCacheKey(
|
||||
scope: string,
|
||||
method: string,
|
||||
url: string,
|
||||
): string {
|
||||
return `${scope} ${method.toUpperCase()} ${url}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Adds ETag-based conditional requests to an Octokit instance. For every GET it
|
||||
* replays the previously stored `If-None-Match`; GitHub then answers `304 Not
|
||||
* Modified` — which does not count against the token's primary rate limit —
|
||||
* whenever nothing changed, and the cached body is returned in place of a full
|
||||
* re-download. Non-GET requests are passed through untouched.
|
||||
*
|
||||
* The scope isolates cache entries per token/user so one account never reads
|
||||
* another's cached data.
|
||||
*/
|
||||
export function applyConditionalRequests(
|
||||
octokit: Octokit,
|
||||
options: { store?: ConditionalRequestStore; scope?: string } = {},
|
||||
): void {
|
||||
// Some tests stub Octokit without the hook system; skip wiring in that case.
|
||||
if (typeof (octokit as any)?.hook?.wrap !== "function") return;
|
||||
|
||||
const store = options.store ?? defaultConditionalRequestStore;
|
||||
const scope = options.scope ?? "default";
|
||||
|
||||
octokit.hook.wrap("request", async (request: any, requestOptions: any): Promise<any> => {
|
||||
const method = String(requestOptions.method ?? "GET").toUpperCase();
|
||||
if (method !== "GET") {
|
||||
return request(requestOptions);
|
||||
}
|
||||
|
||||
// Build the key from the expanded absolute URL, not the route template.
|
||||
// Inside the hook `requestOptions.url` is still `/repos/{owner}/{repo}/...`,
|
||||
// so keying on it would collapse every repo into a single entry per user +
|
||||
// endpoint and stop the 304 path from firing once more than one repo syncs.
|
||||
// `octokit.request.endpoint.parse` expands the route (owner/repo + query);
|
||||
// the chained `request` argument has no `.endpoint`, so it must come from
|
||||
// `octokit.request`.
|
||||
const parseEndpoint = (octokit as any)?.request?.endpoint?.parse;
|
||||
const expandedUrl =
|
||||
typeof parseEndpoint === "function"
|
||||
? parseEndpoint(requestOptions).url
|
||||
: requestOptions.url;
|
||||
const key = conditionalRequestCacheKey(scope, method, expandedUrl);
|
||||
const cached = store.get(key);
|
||||
if (cached?.etag) {
|
||||
requestOptions.headers = {
|
||||
...requestOptions.headers,
|
||||
"if-none-match": cached.etag,
|
||||
};
|
||||
}
|
||||
|
||||
try {
|
||||
const response = await request(requestOptions);
|
||||
const etag = response?.headers?.etag;
|
||||
if (etag && response.status >= 200 && response.status < 300) {
|
||||
store.set(key, {
|
||||
etag,
|
||||
data: response.data,
|
||||
status: response.status,
|
||||
link: response.headers?.link,
|
||||
});
|
||||
}
|
||||
return response;
|
||||
} catch (error: any) {
|
||||
// GitHub answered "not modified": reuse the stored body, presented as a
|
||||
// 200 so callers (including octokit.paginate) are unaffected.
|
||||
if (error?.status === 304 && cached) {
|
||||
return {
|
||||
status: 200,
|
||||
url: expandedUrl,
|
||||
headers: {
|
||||
...(error.response?.headers ?? {}),
|
||||
etag: cached.etag,
|
||||
...(cached.link ? { link: cached.link } : {}),
|
||||
},
|
||||
data: cached.data,
|
||||
} as any;
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
});
|
||||
}
|
||||
@@ -3,6 +3,7 @@ import type { GitRepo, RepoStatus } from "@/types/Repository";
|
||||
import { Octokit } from "@octokit/rest";
|
||||
import { throttling } from "@octokit/plugin-throttling";
|
||||
import type { Config } from "@/types/config";
|
||||
import { applyConditionalRequests } from "@/lib/github-conditional-requests";
|
||||
// Conditionally import rate limit manager (not available in test environment)
|
||||
let RateLimitManager: any = null;
|
||||
let publishEvent: any = null;
|
||||
@@ -202,6 +203,10 @@ export function createGitHubClient(
|
||||
});
|
||||
}
|
||||
|
||||
// Reuse ETags across syncs so unchanged lists (e.g. pull requests) come back
|
||||
// as rate-limit-free 304s instead of full downloads on every scheduled poll.
|
||||
applyConditionalRequests(octokit, { scope: userId ?? username });
|
||||
|
||||
return octokit;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user