From fdd6f49fcddd65c29a59f1a838bd586e16d31422 Mon Sep 17 00:00:00 2001 From: Alan Daniel Date: Fri, 10 Apr 2026 15:42:47 -0400 Subject: [PATCH 1/2] Split GitHub cache and harden rate limits --- .../drizzle/0003_github_cache_namespace.sql | 5 + .../pulls/detail/pull-detail-activity.tsx | 2 - .../pulls/detail/pull-detail-page.tsx | 1 - apps/dashboard/src/db/schema.ts | 6 + apps/dashboard/src/env.d.ts | 13 + apps/dashboard/src/lib/github-cache.test.ts | 145 +++++- apps/dashboard/src/lib/github-cache.ts | 422 +++++++++++++++++- apps/dashboard/src/lib/github-revalidation.ts | 5 + apps/dashboard/src/lib/github.functions.ts | 297 +++++++----- apps/dashboard/src/lib/github.server.test.ts | 145 ++++++ apps/dashboard/src/lib/github.server.ts | 103 ++++- apps/dashboard/worker-configuration.d.ts | 24 +- apps/dashboard/wrangler.jsonc | 7 + 13 files changed, 1023 insertions(+), 152 deletions(-) create mode 100644 apps/dashboard/drizzle/0003_github_cache_namespace.sql create mode 100644 apps/dashboard/src/lib/github.server.test.ts diff --git a/apps/dashboard/drizzle/0003_github_cache_namespace.sql b/apps/dashboard/drizzle/0003_github_cache_namespace.sql new file mode 100644 index 0000000..e709248 --- /dev/null +++ b/apps/dashboard/drizzle/0003_github_cache_namespace.sql @@ -0,0 +1,5 @@ +CREATE TABLE `github_cache_namespace` ( + `namespace_key` text PRIMARY KEY NOT NULL, + `version` integer NOT NULL, + `updated_at` integer NOT NULL +); diff --git a/apps/dashboard/src/components/pulls/detail/pull-detail-activity.tsx b/apps/dashboard/src/components/pulls/detail/pull-detail-activity.tsx index 04e7397..00b3448 100644 --- a/apps/dashboard/src/components/pulls/detail/pull-detail-activity.tsx +++ b/apps/dashboard/src/components/pulls/detail/pull-detail-activity.tsx @@ -48,7 +48,6 @@ import type { PullDetail, PullStatus, } from "#/lib/github.types"; -import { githubCachePolicy } from "#/lib/github-cache-policy"; import { checkPermissionWarning } from "#/lib/warning-store"; export function PullDetailActivitySection({ @@ -149,7 +148,6 @@ function MergeStatusSection({ const statusQuery = useQuery({ ...githubPullStatusQueryOptions(scope, { owner, repo, pullNumber }), refetchOnWindowFocus: "always", - refetchInterval: githubCachePolicy.status.staleTimeMs, }); const status = statusQuery.data ?? null; diff --git a/apps/dashboard/src/components/pulls/detail/pull-detail-page.tsx b/apps/dashboard/src/components/pulls/detail/pull-detail-page.tsx index d53ec15..c6f7251 100644 --- a/apps/dashboard/src/components/pulls/detail/pull-detail-page.tsx +++ b/apps/dashboard/src/components/pulls/detail/pull-detail-page.tsx @@ -29,7 +29,6 @@ export function PullDetailPage() { ...githubPullPageQueryOptions(scope, { owner, repo, pullNumber }), enabled: hasMounted, }); - const viewerQuery = useQuery({ ...githubViewerQueryOptions(scope), enabled: hasMounted, diff --git a/apps/dashboard/src/db/schema.ts b/apps/dashboard/src/db/schema.ts index d12c687..a6b45d0 100644 --- a/apps/dashboard/src/db/schema.ts +++ b/apps/dashboard/src/db/schema.ts @@ -87,3 +87,9 @@ export const githubRevalidationSignal = sqliteTable( updatedAt: integer("updated_at").notNull(), }, ); + +export const githubCacheNamespace = sqliteTable("github_cache_namespace", { + namespaceKey: text("namespace_key").primaryKey(), + version: integer("version").notNull(), + updatedAt: integer("updated_at").notNull(), +}); diff --git a/apps/dashboard/src/env.d.ts b/apps/dashboard/src/env.d.ts index 88b6ad5..b0b0760 100644 --- a/apps/dashboard/src/env.d.ts +++ b/apps/dashboard/src/env.d.ts @@ -1,2 +1,15 @@ // Env types are generated by `wrangler types` in worker-configuration.d.ts // The cloudflare:workers module exports env: Cloudflare.Env from worker-configuration.d.ts + +declare namespace Cloudflare { + interface Env { + GITHUB_APP_CLIENT_ID?: string; + GITHUB_APP_CLIENT_SECRET?: string; + GITHUB_APP_SLUG?: string; + GITHUB_WEBHOOK_SECRET?: string; + GITHUB_CLIENT_ID?: string; + GITHUB_CLIENT_SECRET?: string; + BETTER_AUTH_SECRET: string; + BETTER_AUTH_URL: string; + } +} diff --git a/apps/dashboard/src/lib/github-cache.test.ts b/apps/dashboard/src/lib/github-cache.test.ts index 2dcf571..b1148b7 100644 --- a/apps/dashboard/src/lib/github-cache.test.ts +++ b/apps/dashboard/src/lib/github-cache.test.ts @@ -4,6 +4,7 @@ import { type GitHubCacheStore, type GitHubCacheStoreEntry, type GitHubFetchResult, + type GitHubPayloadCacheStore, getOrRevalidateGitHubResource, } from "./github-cache"; @@ -157,9 +158,9 @@ describe("getOrRevalidateGitHubResource", () => { fetcher, }); - await Promise.resolve(); - - expect(fetcher).toHaveBeenCalledTimes(1); + await vi.waitFor(() => { + expect(fetcher).toHaveBeenCalledTimes(1); + }); resolveFetch?.({ kind: "success", @@ -254,4 +255,142 @@ describe("getOrRevalidateGitHubResource", () => { expect(result).toEqual({ title: "New title" }); expect(fetcher).toHaveBeenCalledTimes(1); }); + + it("returns a fresh split-cache payload from the payload store", async () => { + const get = vi.fn(async () => buildEntry()); + const put = vi.fn(async () => undefined); + const payloadStore = { get, put } satisfies GitHubPayloadCacheStore; + const fetcher = + vi.fn< + (parameters: { + etag?: string | null; + lastModified?: string | null; + }) => Promise> + >(); + + const result = await getOrRevalidateGitHubResource({ + userId: "user-1", + resource: "viewer", + freshForMs: 60_000, + cacheMode: "split", + namespaceKeys: ["viewer"], + payloadStore, + getNamespaceVersions: async () => ({ viewer: 3 }), + now: () => 150, + fetcher, + }); + + expect(result).toEqual({ login: "adn" }); + expect(fetcher).not.toHaveBeenCalled(); + expect(get).toHaveBeenCalledTimes(1); + }); + + it("hydrates the split payload store from a fresh legacy entry on KV miss", async () => { + const store = createMemoryStore([buildEntry()]); + const storedPayloadEntries = new Map(); + const get = vi.fn(async () => null); + const put = vi.fn( + async (storageKey: string, entry: GitHubCacheStoreEntry) => { + storedPayloadEntries.set(storageKey, structuredClone(entry)); + }, + ); + const payloadStore = { get, put } satisfies GitHubPayloadCacheStore; + const fetcher = + vi.fn< + (parameters: { + etag?: string | null; + lastModified?: string | null; + }) => Promise> + >(); + + const result = await getOrRevalidateGitHubResource({ + userId: "user-1", + resource: "viewer", + freshForMs: 60_000, + cacheMode: "split", + namespaceKeys: ["viewer"], + store, + payloadStore, + getNamespaceVersions: async () => ({ viewer: 0 }), + now: () => 150, + fetcher, + }); + + expect(result).toEqual({ login: "adn" }); + expect(fetcher).not.toHaveBeenCalled(); + expect(get).toHaveBeenCalledTimes(1); + expect(put).toHaveBeenCalledTimes(1); + expect(storedPayloadEntries.size).toBe(1); + expect(Array.from(storedPayloadEntries.values())).toEqual([buildEntry()]); + }); + + it("extends freshness when GitHub budget is low", async () => { + const store = createMemoryStore([ + buildEntry({ + freshUntil: 50, + }), + ]); + const fetcher = vi.fn< + (parameters: { + etag?: string | null; + lastModified?: string | null; + }) => Promise> + >(async () => ({ + kind: "success", + data: { login: "adn" }, + metadata: createGitHubResponseMetadata(200, { + etag: '"next"', + "x-ratelimit-remaining": "10", + "x-ratelimit-reset": "0", + }), + })); + + const result = await getOrRevalidateGitHubResource({ + userId: "user-1", + resource: "viewer", + freshForMs: 15_000, + store, + now: () => 500, + fetcher, + }); + + expect(result).toEqual({ login: "adn" }); + + const updatedEntry = await store.get("user-1::viewer::null"); + expect(updatedEntry?.freshUntil).toBe(300_500); + expect(updatedEntry?.rateLimitRemaining).toBe(10); + }); + + it("serves stale cache when GitHub is rate limited", async () => { + const store = createMemoryStore([ + buildEntry({ + freshUntil: 50, + }), + ]); + + const result = await getOrRevalidateGitHubResource({ + userId: "user-1", + resource: "viewer", + freshForMs: 1_000, + store, + now: () => 500, + fetcher: vi.fn(async () => { + throw { + status: 403, + response: { + headers: { + "x-ratelimit-remaining": "0", + "x-ratelimit-reset": "2", + }, + }, + }; + }), + }); + + expect(result).toEqual({ login: "adn" }); + + const updatedEntry = await store.get("user-1::viewer::null"); + expect(updatedEntry?.freshUntil).toBe(60_500); + expect(updatedEntry?.statusCode).toBe(403); + }); }); diff --git a/apps/dashboard/src/lib/github-cache.ts b/apps/dashboard/src/lib/github-cache.ts index 6daaaa3..de0e940 100644 --- a/apps/dashboard/src/lib/github-cache.ts +++ b/apps/dashboard/src/lib/github-cache.ts @@ -32,6 +32,15 @@ export type GitHubCacheStore = { delete(cacheKey: string): Promise; }; +export type GitHubPayloadCacheStore = { + get(storageKey: string): Promise; + put( + storageKey: string, + entry: GitHubCacheStoreEntry, + expirationTtlSeconds: number, + ): Promise; +}; + export type GitHubFetchResult = | { kind: "not-modified"; @@ -49,15 +58,30 @@ type GetOrRevalidateGitHubResourceOptions = { params?: unknown; freshForMs: number; signalKeys?: string[]; + namespaceKeys?: string[]; + cacheMode?: "legacy" | "split"; + payloadRetentionSeconds?: number; fetcher: ( conditionals: GitHubConditionalHeaders, ) => Promise>; store?: GitHubCacheStore; + payloadStore?: GitHubPayloadCacheStore | null; inFlightCache?: Map>; getLatestSignalUpdatedAt?: (signalKeys: string[]) => Promise; + getNamespaceVersions?: ( + namespaceKeys: string[], + ) => Promise>; now?: () => number; }; +const DEFAULT_GITHUB_PAYLOAD_RETENTION_SECONDS = 7 * 24 * 60 * 60; +const GITHUB_RATE_LIMIT_LOW_REMAINING = 100; +const GITHUB_RATE_LIMIT_CRITICAL_REMAINING = 25; +const GITHUB_RATE_LIMIT_LOW_FRESH_FLOOR_MS = 2 * 60 * 1000; +const GITHUB_RATE_LIMIT_CRITICAL_FRESH_FLOOR_MS = 5 * 60 * 1000; +const GITHUB_RATE_LIMIT_RESET_BUFFER_MS = 5 * 1000; +const GITHUB_STALE_IF_RATE_LIMITED_FALLBACK_MS = 60 * 1000; + const requestScopedInFlightGitHubCacheReads = new WeakMap< Request, Map> @@ -139,6 +163,171 @@ function parseCachedPayload(payloadJson: string) { return JSON.parse(payloadJson) as TData; } +function isRecord(value: unknown): value is Record { + return Boolean(value) && typeof value === "object"; +} + +function getRateLimitResetMs(rateLimitReset: number | null | undefined) { + if (typeof rateLimitReset !== "number" || !Number.isFinite(rateLimitReset)) { + return null; + } + + return rateLimitReset * 1_000; +} + +function getAdaptiveFreshForMs( + currentTime: number, + baseFreshForMs: number, + metadata: Pick< + GitHubResponseMetadata, + "rateLimitRemaining" | "rateLimitReset" + >, +) { + if ( + typeof metadata.rateLimitRemaining !== "number" || + !Number.isFinite(metadata.rateLimitRemaining) + ) { + return baseFreshForMs; + } + + if (metadata.rateLimitRemaining <= GITHUB_RATE_LIMIT_CRITICAL_REMAINING) { + const untilReset = getRateLimitResetMs(metadata.rateLimitReset); + const resetExtendedFreshForMs = + typeof untilReset === "number" + ? Math.max( + untilReset - currentTime + GITHUB_RATE_LIMIT_RESET_BUFFER_MS, + 0, + ) + : 0; + + return Math.max( + baseFreshForMs, + GITHUB_RATE_LIMIT_CRITICAL_FRESH_FLOOR_MS, + resetExtendedFreshForMs, + ); + } + + if (metadata.rateLimitRemaining <= GITHUB_RATE_LIMIT_LOW_REMAINING) { + return Math.max(baseFreshForMs, GITHUB_RATE_LIMIT_LOW_FRESH_FLOOR_MS); + } + + return baseFreshForMs; +} + +function getErrorStatusCode(error: unknown) { + if (!isRecord(error)) { + return null; + } + + return typeof error.status === "number" ? error.status : null; +} + +function getErrorResponseHeaders(error: unknown) { + if (!isRecord(error) || !isRecord(error.response)) { + return null; + } + + return error.response.headers as Record | null; +} + +function normalizeUnknownHeaders( + headers: Record | null | undefined, +) { + if (!headers) { + return {}; + } + + return Object.entries(headers).reduce>( + (accumulator, [key, value]) => { + accumulator[key.toLowerCase()] = + typeof value === "string" + ? value + : value == null + ? null + : String(value); + return accumulator; + }, + {}, + ); +} + +function isGitHubRateLimitError(error: unknown) { + const statusCode = getErrorStatusCode(error); + if (statusCode !== 403 && statusCode !== 429) { + return false; + } + + const headers = normalizeUnknownHeaders(getErrorResponseHeaders(error)); + const retryAfter = headers["retry-after"]; + const remaining = parseNullableInt(headers["x-ratelimit-remaining"]); + + return retryAfter !== null || remaining === 0 || statusCode === 429; +} + +function getRateLimitedStaleFreshUntil(currentTime: number, error: unknown) { + const headers = normalizeUnknownHeaders(getErrorResponseHeaders(error)); + const retryAfterSeconds = parseNullableInt(headers["retry-after"]); + if (typeof retryAfterSeconds === "number" && retryAfterSeconds > 0) { + return ( + currentTime + + retryAfterSeconds * 1_000 + + GITHUB_RATE_LIMIT_RESET_BUFFER_MS + ); + } + + const resetAtMs = getRateLimitResetMs( + parseNullableInt(headers["x-ratelimit-reset"]), + ); + if (typeof resetAtMs === "number") { + return Math.max( + currentTime + GITHUB_STALE_IF_RATE_LIMITED_FALLBACK_MS, + resetAtMs + GITHUB_RATE_LIMIT_RESET_BUFFER_MS, + ); + } + + return currentTime + GITHUB_STALE_IF_RATE_LIMITED_FALLBACK_MS; +} + +async function hashText(value: string) { + const encoded = new TextEncoder().encode(value); + const digest = await crypto.subtle.digest("SHA-256", encoded); + + return Array.from(new Uint8Array(digest), (byte) => + byte.toString(16).padStart(2, "0"), + ).join(""); +} + +async function buildGitHubPayloadStorageKey({ + userId, + resource, + paramsJson, + namespaceKeys, + namespaceVersions, +}: { + userId: string; + resource: string; + paramsJson: string; + namespaceKeys: string[]; + namespaceVersions: Record; +}) { + const paramsHash = (await hashText(paramsJson)).slice(0, 16); + const namespaceVersionHash = + namespaceKeys.length === 0 + ? "static" + : ( + await hashText( + stableSerialize( + namespaceKeys.map((namespaceKey) => [ + namespaceKey, + namespaceVersions[namespaceKey] ?? 0, + ]), + ), + ) + ).slice(0, 16); + + return `gh:${userId}:${resource}:${paramsHash}:v${namespaceVersionHash}`; +} + async function getGitHubCacheStore(): Promise { const [{ eq }, { getDb }, { githubResponseCache }] = await Promise.all([ import("drizzle-orm"), @@ -186,6 +375,36 @@ async function getGitHubCacheStore(): Promise { }; } +async function getGitHubPayloadCacheStore(): Promise { + try { + const { env } = await import("cloudflare:workers"); + const workerEnv = env as typeof env & { + GITHUB_CACHE_KV?: KVNamespace; + }; + const kv = workerEnv.GITHUB_CACHE_KV; + + if (!kv) { + return null; + } + + return { + async get(storageKey) { + const entry = await kv.get(storageKey, { + type: "json", + }); + return entry ?? null; + }, + async put(storageKey, entry, expirationTtlSeconds) { + await kv.put(storageKey, JSON.stringify(entry), { + expirationTtl: expirationTtlSeconds, + }); + }, + }; + } catch { + return null; + } +} + async function getLatestGitHubRevalidationSignalUpdatedAt( signalKeys: string[], ) { @@ -214,6 +433,72 @@ async function getLatestGitHubRevalidationSignalUpdatedAt( return Math.max(...signals.map((signal) => signal.updatedAt)); } +export async function getGitHubCacheNamespaceVersions(namespaceKeys: string[]) { + if (namespaceKeys.length === 0) { + return {}; + } + + const uniqueNamespaceKeys = Array.from(new Set(namespaceKeys)); + const [{ inArray }, { getDb }, { githubCacheNamespace }] = await Promise.all([ + import("drizzle-orm"), + import("../db"), + import("../db/schema"), + ]); + const db = getDb(); + const rows = await db + .select({ + namespaceKey: githubCacheNamespace.namespaceKey, + version: githubCacheNamespace.version, + }) + .from(githubCacheNamespace) + .where(inArray(githubCacheNamespace.namespaceKey, uniqueNamespaceKeys)); + + return uniqueNamespaceKeys.reduce>( + (accumulator, namespaceKey) => { + accumulator[namespaceKey] = + rows.find((row) => row.namespaceKey === namespaceKey)?.version ?? 0; + return accumulator; + }, + {}, + ); +} + +export async function bumpGitHubCacheNamespaces( + namespaceKeys: string[], + at = Date.now(), +) { + if (namespaceKeys.length === 0) { + return 0; + } + + const uniqueNamespaceKeys = Array.from(new Set(namespaceKeys)); + const [{ sql }, { getDb }, { githubCacheNamespace }] = await Promise.all([ + import("drizzle-orm"), + import("../db"), + import("../db/schema"), + ]); + const db = getDb(); + + await db + .insert(githubCacheNamespace) + .values( + uniqueNamespaceKeys.map((namespaceKey) => ({ + namespaceKey, + version: 1, + updatedAt: at, + })), + ) + .onConflictDoUpdate({ + target: githubCacheNamespace.namespaceKey, + set: { + version: sql`${githubCacheNamespace.version} + 1`, + updatedAt: at, + }, + }); + + return uniqueNamespaceKeys.length; +} + export async function markGitHubRevalidationSignals( signalKeys: string[], at = Date.now(), @@ -244,6 +529,8 @@ export async function markGitHubRevalidationSignals( }, }); + await bumpGitHubCacheNamespaces(uniqueSignalKeys, at); + return uniqueSignalKeys.length; } @@ -294,19 +581,54 @@ export function createGitHubResponseMetadata( }; } +async function persistGitHubCacheEntry({ + entry, + legacyStore, + payloadStore, + payloadStorageKey, + payloadRetentionSeconds, +}: { + entry: GitHubCacheStoreEntry; + legacyStore: GitHubCacheStore; + payloadStore: GitHubPayloadCacheStore | null; + payloadStorageKey: string | null; + payloadRetentionSeconds: number; +}) { + const writes: Array> = [legacyStore.upsert(entry)]; + + if (payloadStore && payloadStorageKey) { + writes.push( + payloadStore.put(payloadStorageKey, entry, payloadRetentionSeconds), + ); + } + + await Promise.all(writes); +} + export async function getOrRevalidateGitHubResource({ userId, resource, params, freshForMs, signalKeys = [], + namespaceKeys = [], + cacheMode = "legacy", + payloadRetentionSeconds = DEFAULT_GITHUB_PAYLOAD_RETENTION_SECONDS, fetcher, now = Date.now, store, + payloadStore, inFlightCache, getLatestSignalUpdatedAt = getLatestGitHubRevalidationSignalUpdatedAt, + getNamespaceVersions = getGitHubCacheNamespaceVersions, }: GetOrRevalidateGitHubResourceOptions): Promise { - const resolvedStore = store ?? (await getGitHubCacheStore()); + let resolvedStore = store ?? null; + const resolvedPayloadStore = + typeof payloadStore !== "undefined" + ? payloadStore + : cacheMode === "split" + ? await getGitHubPayloadCacheStore() + : null; const paramsJson = stableSerialize(params); const cacheKey = buildGitHubCacheKey({ userId, resource, paramsJson }); const resolvedInFlightCache = @@ -318,7 +640,36 @@ export async function getOrRevalidateGitHubResource({ } const task = (async () => { - const existingEntry = await resolvedStore.get(cacheKey); + const getResolvedStore = async () => { + if (!resolvedStore) { + resolvedStore = await getGitHubCacheStore(); + } + + return resolvedStore; + }; + const uniqueNamespaceKeys = Array.from(new Set(namespaceKeys)); + const namespaceVersions = + cacheMode === "split" + ? await getNamespaceVersions(uniqueNamespaceKeys) + : {}; + const payloadStorageKey = + cacheMode === "split" && resolvedPayloadStore + ? await buildGitHubPayloadStorageKey({ + userId, + resource, + paramsJson, + namespaceKeys: uniqueNamespaceKeys, + namespaceVersions, + }) + : null; + const splitEntry = + resolvedPayloadStore && payloadStorageKey + ? await resolvedPayloadStore.get(payloadStorageKey) + : null; + const legacyEntry = splitEntry + ? null + : await (await getResolvedStore()).get(cacheKey); + const existingEntry = splitEntry ?? legacyEntry; const currentTime = now(); const latestSignalUpdatedAt = signalKeys.length > 0 ? await getLatestSignalUpdatedAt(signalKeys) : null; @@ -333,13 +684,44 @@ export async function getOrRevalidateGitHubResource({ existingEntry.freshUntil > currentTime && !isSignalNewerThanCache ) { + if (legacyEntry && resolvedPayloadStore && payloadStorageKey) { + await resolvedPayloadStore.put( + payloadStorageKey, + legacyEntry, + payloadRetentionSeconds, + ); + } + return parseCachedPayload(existingEntry.payloadJson); } - const result = await fetcher({ - etag: existingEntry?.etag ?? null, - lastModified: existingEntry?.lastModified ?? null, - }); + let result: GitHubFetchResult; + try { + result = await fetcher({ + etag: existingEntry?.etag ?? null, + lastModified: existingEntry?.lastModified ?? null, + }); + } catch (error) { + if (existingEntry && isGitHubRateLimitError(error)) { + const staleEntry = { + ...existingEntry, + freshUntil: getRateLimitedStaleFreshUntil(currentTime, error), + statusCode: getErrorStatusCode(error) ?? existingEntry.statusCode, + }; + + await persistGitHubCacheEntry({ + entry: staleEntry, + legacyStore: await getResolvedStore(), + payloadStore: resolvedPayloadStore, + payloadStorageKey, + payloadRetentionSeconds, + }); + + return parseCachedPayload(existingEntry.payloadJson); + } + + throw error; + } if (result.kind === "not-modified") { if (!existingEntry) { @@ -348,22 +730,32 @@ export async function getOrRevalidateGitHubResource({ ); } - await resolvedStore.upsert({ + const refreshedEntry = { ...existingEntry, etag: result.metadata.etag ?? existingEntry.etag, lastModified: result.metadata.lastModified ?? existingEntry.lastModified, fetchedAt: currentTime, - freshUntil: currentTime + freshForMs, + freshUntil: + currentTime + + getAdaptiveFreshForMs(currentTime, freshForMs, result.metadata), rateLimitRemaining: result.metadata.rateLimitRemaining, rateLimitReset: result.metadata.rateLimitReset, statusCode: result.metadata.statusCode, + }; + + await persistGitHubCacheEntry({ + entry: refreshedEntry, + legacyStore: await getResolvedStore(), + payloadStore: resolvedPayloadStore, + payloadStorageKey, + payloadRetentionSeconds, }); return parseCachedPayload(existingEntry.payloadJson); } - await resolvedStore.upsert({ + const nextEntry = { cacheKey, userId, resource, @@ -372,10 +764,20 @@ export async function getOrRevalidateGitHubResource({ lastModified: result.metadata.lastModified, payloadJson: JSON.stringify(result.data), fetchedAt: currentTime, - freshUntil: currentTime + freshForMs, + freshUntil: + currentTime + + getAdaptiveFreshForMs(currentTime, freshForMs, result.metadata), rateLimitRemaining: result.metadata.rateLimitRemaining, rateLimitReset: result.metadata.rateLimitReset, statusCode: result.metadata.statusCode, + }; + + await persistGitHubCacheEntry({ + entry: nextEntry, + legacyStore: await getResolvedStore(), + payloadStore: resolvedPayloadStore, + payloadStorageKey, + payloadRetentionSeconds, }); return result.data; diff --git a/apps/dashboard/src/lib/github-revalidation.ts b/apps/dashboard/src/lib/github-revalidation.ts index 2019136..3f1be7d 100644 --- a/apps/dashboard/src/lib/github-revalidation.ts +++ b/apps/dashboard/src/lib/github-revalidation.ts @@ -3,6 +3,11 @@ import type { Tab } from "./tab-store"; export const githubRevalidationSignalKeys = { pullsMine: "pulls.mine", issuesMine: "issues.mine", + repoLabels: (input: { owner: string; repo: string }) => + `repoLabels:${input.owner}/${input.repo}`, + repoCollaborators: (input: { owner: string; repo: string }) => + `repoCollaborators:${input.owner}/${input.repo}`, + orgTeams: (input: { org: string }) => `orgTeams:${input.org}`, actionsRepo: (input: { owner: string; repo: string }) => `actions:${input.owner}/${input.repo}`, pullEntity: (input: { owner: string; repo: string; pullNumber: number }) => diff --git a/apps/dashboard/src/lib/github.functions.ts b/apps/dashboard/src/lib/github.functions.ts index b85ee6e..2918708 100644 --- a/apps/dashboard/src/lib/github.functions.ts +++ b/apps/dashboard/src/lib/github.functions.ts @@ -39,6 +39,7 @@ import { } from "./github-access"; import { getGitHubAppSlug } from "./github-app.server"; import { + bumpGitHubCacheNamespaces, bustGitHubCache, createGitHubResponseMetadata, type GitHubConditionalHeaders, @@ -637,6 +638,8 @@ async function getCachedGitHubRequest({ params, freshForMs, signalKeys, + namespaceKeys, + cacheMode, request, mapData, }: { @@ -645,6 +648,8 @@ async function getCachedGitHubRequest({ params: unknown; freshForMs: number; signalKeys?: string[]; + namespaceKeys?: string[]; + cacheMode?: "legacy" | "split"; request: ( headers: Record, ) => Promise>; @@ -656,6 +661,8 @@ async function getCachedGitHubRequest({ params, freshForMs, signalKeys, + namespaceKeys, + cacheMode, fetcher: async (conditionals) => { const result = await executeGitHubRequest(request, conditionals); @@ -677,6 +684,8 @@ async function getCachedPaginatedGitHubRequest({ params, freshForMs, signalKeys, + namespaceKeys, + cacheMode, request, mapData, pageSize = 100, @@ -686,6 +695,8 @@ async function getCachedPaginatedGitHubRequest({ params: unknown; freshForMs: number; signalKeys?: string[]; + namespaceKeys?: string[]; + cacheMode?: "legacy" | "split"; request: (page: number) => Promise>; mapData: (items: TGitHubItem[]) => TResult; pageSize?: number; @@ -696,6 +707,8 @@ async function getCachedPaginatedGitHubRequest({ params, freshForMs, signalKeys, + namespaceKeys, + cacheMode, fetcher: async () => { const items: TGitHubItem[] = []; let page = 1; @@ -738,18 +751,20 @@ async function getCachedPullResponse({ resource: string; freshForMs: number; }) { + const pullNamespaceKey = githubRevalidationSignalKeys.pullEntity({ + owner: data.owner, + repo: data.repo, + pullNumber: data.pullNumber, + }); + return getCachedGitHubRequest({ context, resource, params: data, freshForMs, - signalKeys: [ - githubRevalidationSignalKeys.pullEntity({ - owner: data.owner, - repo: data.repo, - pullNumber: data.pullNumber, - }), - ], + signalKeys: [pullNamespaceKey], + namespaceKeys: [pullNamespaceKey], + cacheMode: "split", request: (headers) => context.octokit.rest.pulls.get({ owner: data.owner, @@ -782,19 +797,20 @@ async function getPullCommentsResult( type IssueComment = Awaited< ReturnType >["data"][number]; + const pullNamespaceKey = githubRevalidationSignalKeys.pullEntity({ + owner: data.owner, + repo: data.repo, + pullNumber: data.pullNumber, + }); return getCachedGitHubRequest({ context, resource: "pulls.comments", params: data, freshForMs: githubCachePolicy.activity.staleTimeMs, - signalKeys: [ - githubRevalidationSignalKeys.pullEntity({ - owner: data.owner, - repo: data.repo, - pullNumber: data.pullNumber, - }), - ], + signalKeys: [pullNamespaceKey], + namespaceKeys: [pullNamespaceKey], + cacheMode: "split", request: (headers) => context.octokit.rest.issues.listComments({ owner: data.owner, @@ -827,19 +843,20 @@ async function getPullCommitsResult( type PullCommitResponse = Awaited< ReturnType >["data"][number]; + const pullNamespaceKey = githubRevalidationSignalKeys.pullEntity({ + owner: data.owner, + repo: data.repo, + pullNumber: data.pullNumber, + }); return getCachedGitHubRequest({ context, resource: "pulls.commits", params: data, freshForMs: githubCachePolicy.activity.staleTimeMs, - signalKeys: [ - githubRevalidationSignalKeys.pullEntity({ - owner: data.owner, - repo: data.repo, - pullNumber: data.pullNumber, - }), - ], + signalKeys: [pullNamespaceKey], + namespaceKeys: [pullNamespaceKey], + cacheMode: "split", request: (headers) => context.octokit.rest.pulls.listCommits({ owner: data.owner, @@ -978,18 +995,20 @@ async function getPullStatusResult( data: PullFromRepoInput, pull?: RepoPullDetail, ): Promise { + const pullNamespaceKey = githubRevalidationSignalKeys.pullEntity({ + owner: data.owner, + repo: data.repo, + pullNumber: data.pullNumber, + }); + return getOrRevalidateGitHubResource({ userId: context.session.user.id, resource: "pulls.status.v1", params: data, freshForMs: githubCachePolicy.status.staleTimeMs, - signalKeys: [ - githubRevalidationSignalKeys.pullEntity({ - owner: data.owner, - repo: data.repo, - pullNumber: data.pullNumber, - }), - ], + signalKeys: [pullNamespaceKey], + namespaceKeys: [pullNamespaceKey], + cacheMode: "split", fetcher: async () => { const pullForStatus = pull ?? @@ -1036,18 +1055,20 @@ async function getIssueDetailResult( context: GitHubContext, data: IssueFromRepoInput, ): Promise { + const issueNamespaceKey = githubRevalidationSignalKeys.issueEntity({ + owner: data.owner, + repo: data.repo, + issueNumber: data.issueNumber, + }); + return getCachedGitHubRequest({ context, resource: "issues.detail", params: data, freshForMs: githubCachePolicy.detail.staleTimeMs, - signalKeys: [ - githubRevalidationSignalKeys.issueEntity({ - owner: data.owner, - repo: data.repo, - issueNumber: data.issueNumber, - }), - ], + signalKeys: [issueNamespaceKey], + namespaceKeys: [issueNamespaceKey], + cacheMode: "split", request: (headers) => context.octokit.rest.issues.get({ owner: data.owner, @@ -1072,19 +1093,20 @@ async function getIssueCommentsResult( type RawIssueComment = Awaited< ReturnType >["data"][number]; + const issueNamespaceKey = githubRevalidationSignalKeys.issueEntity({ + owner: data.owner, + repo: data.repo, + issueNumber: data.issueNumber, + }); return getCachedGitHubRequest({ context, resource: "issues.comments", params: data, freshForMs: githubCachePolicy.activity.staleTimeMs, - signalKeys: [ - githubRevalidationSignalKeys.issueEntity({ - owner: data.owner, - repo: data.repo, - issueNumber: data.issueNumber, - }), - ], + signalKeys: [issueNamespaceKey], + namespaceKeys: [issueNamespaceKey], + cacheMode: "split", request: (headers) => context.octokit.rest.issues.listComments({ owner: data.owner, @@ -1155,6 +1177,8 @@ async function getViewer(context: GitHubContext): Promise { resource: "viewer", params: null, freshForMs: githubCachePolicy.viewer.staleTimeMs, + namespaceKeys: ["viewer"], + cacheMode: "split", request: (headers) => context.octokit.rest.users.getAuthenticated({ headers }), mapData: (viewer) => viewer, @@ -1187,6 +1211,8 @@ async function getMyPullSlice({ params: { username, role }, freshForMs: githubCachePolicy.list.staleTimeMs, signalKeys: [githubRevalidationSignalKeys.pullsMine], + namespaceKeys: [githubRevalidationSignalKeys.pullsMine], + cacheMode: "split", request: (headers) => context.octokit.rest.search.issuesAndPullRequests({ q: buildUserSearchQuery({ @@ -1221,6 +1247,8 @@ async function getMyIssueSlice({ params: { username, role }, freshForMs: githubCachePolicy.list.staleTimeMs, signalKeys: [githubRevalidationSignalKeys.issuesMine], + namespaceKeys: [githubRevalidationSignalKeys.issuesMine], + cacheMode: "split", request: (headers) => context.octokit.rest.search.issuesAndPullRequests({ q: buildUserSearchQuery({ @@ -1397,6 +1425,8 @@ export const getUserRepos = createServerFn({ method: "GET" }).handler( resource: "repos.list", params: { sort: "updated", perPage: 10 }, freshForMs: githubCachePolicy.reposList.staleTimeMs, + namespaceKeys: ["repos.list"], + cacheMode: "split", request: (headers) => context.octokit.rest.repos.listForAuthenticatedUser({ sort: "updated", @@ -1847,6 +1877,11 @@ async function getPullFilesResult( ): Promise { const page = clampPage(data.page); const perPage = clampPerPage(data.perPage, 20); + const pullNamespaceKey = githubRevalidationSignalKeys.pullEntity({ + owner: data.owner, + repo: data.repo, + pullNumber: data.pullNumber, + }); return getCachedGitHubRequest({ context, @@ -1859,13 +1894,9 @@ async function getPullFilesResult( perPage, }, freshForMs: githubCachePolicy.detail.staleTimeMs, - signalKeys: [ - githubRevalidationSignalKeys.pullEntity({ - owner: data.owner, - repo: data.repo, - pullNumber: data.pullNumber, - }), - ], + signalKeys: [pullNamespaceKey], + namespaceKeys: [pullNamespaceKey], + cacheMode: "split", request: (headers) => context.octokit.rest.pulls.listFiles({ owner: data.owner, @@ -1895,18 +1926,20 @@ async function getPullFileSummariesResult( context: GitHubContext, data: PullFromRepoInput, ): Promise { + const pullNamespaceKey = githubRevalidationSignalKeys.pullEntity({ + owner: data.owner, + repo: data.repo, + pullNumber: data.pullNumber, + }); + return getCachedPaginatedGitHubRequest({ context, resource: "pulls.fileSummaries", params: data, freshForMs: githubCachePolicy.detail.staleTimeMs, - signalKeys: [ - githubRevalidationSignalKeys.pullEntity({ - owner: data.owner, - repo: data.repo, - pullNumber: data.pullNumber, - }), - ], + signalKeys: [pullNamespaceKey], + namespaceKeys: [pullNamespaceKey], + cacheMode: "split", request: (page) => context.octokit.rest.pulls.listFiles({ owner: data.owner, @@ -1953,6 +1986,12 @@ async function getPullReviewCommentsResult( context: GitHubContext, data: PullFromRepoInput, ): Promise { + const pullNamespaceKey = githubRevalidationSignalKeys.pullEntity({ + owner: data.owner, + repo: data.repo, + pullNumber: data.pullNumber, + }); + return getCachedPaginatedGitHubRequest< RepoPullReviewComment, PullReviewComment[] @@ -1961,13 +2000,9 @@ async function getPullReviewCommentsResult( resource: "pulls.reviewComments", params: data, freshForMs: githubCachePolicy.activity.staleTimeMs, - signalKeys: [ - githubRevalidationSignalKeys.pullEntity({ - owner: data.owner, - repo: data.repo, - pullNumber: data.pullNumber, - }), - ], + signalKeys: [pullNamespaceKey], + namespaceKeys: [pullNamespaceKey], + cacheMode: "split", request: (page) => context.octokit.rest.pulls.listReviewComments({ owner: data.owner, @@ -2110,28 +2145,41 @@ export const getRepoCollaborators = createServerFn({ method: "GET" }) return []; } - try { - const allCollaborators = await context.octokit.paginate( - context.octokit.rest.repos.listCollaborators, - { + return getCachedPaginatedGitHubRequest< + Awaited< + ReturnType + >["data"][number], + RepoCollaborator[] + >({ + context, + resource: "repos.collaborators", + params: data, + freshForMs: githubCachePolicy.viewer.staleTimeMs, + namespaceKeys: [ + githubRevalidationSignalKeys.repoCollaborators({ + owner: data.owner, + repo: data.repo, + }), + ], + cacheMode: "split", + request: (page) => + context.octokit.rest.repos.listCollaborators({ owner: data.owner, repo: data.repo, + page, per_page: 100, - }, - ); - - return allCollaborators.map((c) => ({ - login: c.login, - avatarUrl: c.avatar_url, - permissions: { - admin: c.permissions?.admin ?? false, - push: c.permissions?.push ?? false, - pull: c.permissions?.pull ?? false, - }, - })); - } catch { - return []; - } + }), + mapData: (allCollaborators) => + allCollaborators.map((c) => ({ + login: c.login, + avatarUrl: c.avatar_url, + permissions: { + admin: c.permissions?.admin ?? false, + push: c.permissions?.push ?? false, + pull: c.permissions?.pull ?? false, + }, + })), + }).catch(() => []); }); export type OrgTeamsInput = { @@ -2146,22 +2194,30 @@ export const getOrgTeams = createServerFn({ method: "GET" }) return []; } - try { - const allTeams = await context.octokit.paginate( - context.octokit.rest.teams.list, - { + return getCachedPaginatedGitHubRequest< + Awaited< + ReturnType + >["data"][number], + OrgTeam[] + >({ + context, + resource: "org.teams", + params: data, + freshForMs: githubCachePolicy.viewer.staleTimeMs, + namespaceKeys: [githubRevalidationSignalKeys.orgTeams({ org: data.org })], + cacheMode: "split", + request: (page) => + context.octokit.rest.teams.list({ org: data.org, + page, per_page: 100, - }, - ); - - return allTeams.map((t) => ({ - slug: t.slug, - name: t.name, - })); - } catch { - return []; - } + }), + mapData: (allTeams) => + allTeams.map((t) => ({ + slug: t.slug, + name: t.name, + })), + }).catch(() => []); }); export const requestPullReviewers = createServerFn({ method: "POST" }) @@ -2266,24 +2322,37 @@ export const getRepoLabels = createServerFn({ method: "GET" }) return []; } - try { - const allLabels = await context.octokit.paginate( - context.octokit.rest.issues.listLabelsForRepo, - { + return getCachedPaginatedGitHubRequest< + Awaited< + ReturnType + >["data"][number], + GitHubLabel[] + >({ + context, + resource: "repos.labels", + params: data, + freshForMs: githubCachePolicy.viewer.staleTimeMs, + namespaceKeys: [ + githubRevalidationSignalKeys.repoLabels({ + owner: data.owner, + repo: data.repo, + }), + ], + cacheMode: "split", + request: (page) => + context.octokit.rest.issues.listLabelsForRepo({ owner: data.owner, repo: data.repo, + page, per_page: 100, - }, - ); - - return allLabels.map((l) => ({ - name: l.name, - color: l.color ?? "000000", - description: l.description ?? null, - })); - } catch { - return []; - } + }), + mapData: (allLabels) => + allLabels.map((l) => ({ + name: l.name, + color: l.color ?? "000000", + description: l.description ?? null, + })), + }).catch(() => []); }); export const setIssueLabels = createServerFn({ method: "POST" }) @@ -2335,6 +2404,12 @@ export const createRepoLabel = createServerFn({ method: "POST" }) name: data.name, color: data.color, }); + await bumpGitHubCacheNamespaces([ + githubRevalidationSignalKeys.repoLabels({ + owner: data.owner, + repo: data.repo, + }), + ]); return { name: response.data.name, color: response.data.color ?? "000000", diff --git a/apps/dashboard/src/lib/github.server.test.ts b/apps/dashboard/src/lib/github.server.test.ts new file mode 100644 index 0000000..165e5bb --- /dev/null +++ b/apps/dashboard/src/lib/github.server.test.ts @@ -0,0 +1,145 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const octokitInstances: Array<{ + hookBefore: ReturnType; + log: { warn: ReturnType; info: ReturnType }; + options: Record; +}> = []; +const octokitConstructor = vi.fn((options: Record) => { + const instance = { + hookBefore: vi.fn(), + log: { + warn: vi.fn(), + info: vi.fn(), + }, + options, + }; + + octokitInstances.push(instance); + + return { + hook: { + before: instance.hookBefore, + }, + log: instance.log, + }; +}); +const getGitHubAccessTokenByUserId = vi.fn(async () => "github-token"); + +vi.mock("octokit", () => ({ + Octokit: octokitConstructor, +})); + +vi.mock("./github-app.server", () => ({ + getGitHubAccessTokenByUserId, +})); + +beforeEach(() => { + octokitInstances.length = 0; + octokitConstructor.mockClear(); + getGitHubAccessTokenByUserId.mockClear(); +}); + +describe("getGitHubClient", () => { + it("configures Octokit throttling and safe-method retries", async () => { + const { getGitHubClient } = await import("./github.server"); + + await getGitHubClient("user-123"); + + expect(getGitHubAccessTokenByUserId).toHaveBeenCalledWith("user-123"); + expect(octokitConstructor).toHaveBeenCalledTimes(1); + + const [instance] = octokitInstances; + const options = instance.options as { + auth: string; + userAgent: string; + retry: { enabled: boolean }; + throttle: { + enabled: boolean; + id: string; + fallbackSecondaryRateRetryAfter: number; + onRateLimit: ( + retryAfter: number, + options: { method?: string; url: string }, + octokit: { + log: { + warn: (message: string) => void; + info: (message: string) => void; + }; + }, + retryCount: number, + ) => boolean; + onSecondaryRateLimit: ( + retryAfter: number, + options: { method?: string; url: string }, + octokit: { + log: { + warn: (message: string) => void; + info: (message: string) => void; + }; + }, + retryCount: number, + ) => boolean; + }; + }; + + expect(options.auth).toBe("github-token"); + expect(options.userAgent).toBe("quickhub-dashboard"); + expect(options.retry).toEqual({ enabled: true }); + expect(options.throttle.enabled).toBe(true); + expect(options.throttle.id).toBe("github-user:user-123"); + expect(options.throttle.fallbackSecondaryRateRetryAfter).toBe(60); + + expect(instance.hookBefore).toHaveBeenCalledTimes(1); + const [hookEvent, hookHandler] = instance.hookBefore.mock.calls[0] as [ + string, + (options: { method?: string; request?: { retries?: number } }) => void, + ]; + expect(hookEvent).toBe("request"); + + const getOptions = { method: "GET" } as { + method?: string; + request?: { retries?: number }; + }; + hookHandler(getOptions); + expect(getOptions.request?.retries).toBe(2); + + const postOptions = { method: "POST" } as { + method?: string; + request?: { retries?: number }; + }; + hookHandler(postOptions); + expect(postOptions.request?.retries).toBe(0); + + expect( + options.throttle.onRateLimit( + 30, + { method: "GET", url: "/repos" }, + { + log: instance.log, + }, + 0, + ), + ).toBe(true); + expect( + options.throttle.onRateLimit( + 30, + { method: "POST", url: "/repos" }, + { + log: instance.log, + }, + 0, + ), + ).toBe(false); + expect( + options.throttle.onSecondaryRateLimit( + 30, + { method: "GET", url: "/search/issues" }, + { log: instance.log }, + 1, + ), + ).toBe(false); + expect(instance.log.warn).toHaveBeenCalled(); + expect(instance.log.info).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/dashboard/src/lib/github.server.ts b/apps/dashboard/src/lib/github.server.ts index 776a810..736e665 100644 --- a/apps/dashboard/src/lib/github.server.ts +++ b/apps/dashboard/src/lib/github.server.ts @@ -2,10 +2,107 @@ import "@tanstack/react-start/server-only"; import { Octokit, type Octokit as OctokitType } from "octokit"; import { getGitHubAccessTokenByUserId } from "./github-app.server"; +const GITHUB_CLIENT_USER_AGENT = "quickhub-dashboard"; +const GITHUB_READ_RETRY_COUNT = 2; +const GITHUB_RATE_LIMIT_RETRY_COUNT = 1; +const GITHUB_SECONDARY_RATE_LIMIT_FALLBACK_SECONDS = 60; + +type GitHubThrottleRequestOptions = { + method?: string; + url: string; +}; + +type GitHubThrottleClient = Pick; + +function isSafeGitHubRetryMethod(method: string | undefined) { + return method === "GET" || method === "HEAD" || method === "OPTIONS"; +} + +function shouldRetryGitHubRateLimitedRequest({ + method, + retryCount, +}: { + method: string | undefined; + retryCount: number; +}) { + return ( + isSafeGitHubRetryMethod(method) && + retryCount < GITHUB_RATE_LIMIT_RETRY_COUNT + ); +} + +function configureGitHubRequestPolicies(octokit: OctokitType) { + octokit.hook.before("request", (options) => { + const requestOptions = options.request ?? {}; + options.request = requestOptions; + requestOptions.retries = isSafeGitHubRetryMethod(options.method) + ? GITHUB_READ_RETRY_COUNT + : 0; + }); +} + export async function getGitHubClient(userId: string): Promise { - return new Octokit({ + const octokit = new Octokit({ auth: await getGitHubAccessTokenByUserId(userId), - retry: { enabled: false }, - throttle: { enabled: false }, + userAgent: GITHUB_CLIENT_USER_AGENT, + retry: { enabled: true }, + throttle: { + enabled: true, + id: `github-user:${userId}`, + fallbackSecondaryRateRetryAfter: + GITHUB_SECONDARY_RATE_LIMIT_FALLBACK_SECONDS, + onRateLimit: ( + retryAfter: number, + options: GitHubThrottleRequestOptions, + throttledOctokit: GitHubThrottleClient, + retryCount: number, + ) => { + throttledOctokit.log.warn( + `GitHub rate limit for ${options.method} ${options.url}; retryAfter=${retryAfter}s retryCount=${retryCount}`, + ); + + if ( + shouldRetryGitHubRateLimitedRequest({ + method: options.method, + retryCount, + }) + ) { + throttledOctokit.log.info( + `Retrying ${options.method} ${options.url} after ${retryAfter}s`, + ); + return true; + } + + return false; + }, + onSecondaryRateLimit: ( + retryAfter: number, + options: GitHubThrottleRequestOptions, + throttledOctokit: GitHubThrottleClient, + retryCount: number, + ) => { + throttledOctokit.log.warn( + `GitHub secondary rate limit for ${options.method} ${options.url}; retryAfter=${retryAfter}s retryCount=${retryCount}`, + ); + + if ( + shouldRetryGitHubRateLimitedRequest({ + method: options.method, + retryCount, + }) + ) { + throttledOctokit.log.info( + `Retrying ${options.method} ${options.url} after secondary rate limit (${retryAfter}s)`, + ); + return true; + } + + return false; + }, + }, }); + + configureGitHubRequestPolicies(octokit); + + return octokit; } diff --git a/apps/dashboard/worker-configuration.d.ts b/apps/dashboard/worker-configuration.d.ts index 4fc4bbc..2a9322b 100644 --- a/apps/dashboard/worker-configuration.d.ts +++ b/apps/dashboard/worker-configuration.d.ts @@ -1,33 +1,13 @@ /* eslint-disable */ -// Generated by Wrangler by running `wrangler types` (hash: c6b7e2b2686641ee1202aed2c24b79c5) +// Generated by Wrangler by running `wrangler types` (hash: 8cd79b63058024e6be00b9483134f0d0) // Runtime types generated with workerd@1.20260401.1 2025-09-02 nodejs_compat declare namespace Cloudflare { interface Env { + GITHUB_CACHE_KV: KVNamespace; DB: D1Database; - GITHUB_CLIENT_ID: string; - GITHUB_CLIENT_SECRET: string; - BETTER_AUTH_SECRET: string; - BETTER_AUTH_URL: string; } } interface Env extends Cloudflare.Env {} -type StringifyValues> = { - [Binding in keyof EnvType]: EnvType[Binding] extends string - ? EnvType[Binding] - : string; -}; -declare namespace NodeJS { - interface ProcessEnv - extends StringifyValues< - Pick< - Cloudflare.Env, - | "GITHUB_CLIENT_ID" - | "GITHUB_CLIENT_SECRET" - | "BETTER_AUTH_SECRET" - | "BETTER_AUTH_URL" - > - > {} -} // Begin runtime types /*! ***************************************************************************** diff --git a/apps/dashboard/wrangler.jsonc b/apps/dashboard/wrangler.jsonc index 009e89b..410dc19 100644 --- a/apps/dashboard/wrangler.jsonc +++ b/apps/dashboard/wrangler.jsonc @@ -20,5 +20,12 @@ "database_id": "7a94a843-0906-416c-9908-4c9fe5339db7", "migrations_dir": "drizzle" } + ], + "kv_namespaces": [ + { + "binding": "GITHUB_CACHE_KV", + "id": "2e46aa8dd4554072a64e1aad955d6adc", + "preview_id": "373842a94f694ce48f5f5376ab622b18" + } ] } From 6165847f8ebec991fdbac974dff265138f3ba487 Mon Sep 17 00:00:00 2001 From: Alan Daniel Date: Fri, 10 Apr 2026 15:50:56 -0400 Subject: [PATCH 2/2] Document GitHub cache architecture --- docs/github-cache-architecture.md | 292 ++++++++++++++++++++++++++++++ 1 file changed, 292 insertions(+) create mode 100644 docs/github-cache-architecture.md diff --git a/docs/github-cache-architecture.md b/docs/github-cache-architecture.md new file mode 100644 index 0000000..dd41aab --- /dev/null +++ b/docs/github-cache-architecture.md @@ -0,0 +1,292 @@ +# GitHub Cache Architecture + +This document describes the GitHub caching and rate-limit architecture currently +implemented in the dashboard app. + +The system is designed for three goals: + +- Reduce repeated GitHub API calls. +- Keep cached GitHub data invalidated by durable server-side signals. +- Degrade gracefully when GitHub rate limits are low or exhausted. + +## Components + +### Browser cache + +The browser keeps the fastest cache layer through React Query and persisted +query state. This layer is responsible for fast route transitions, reloads, and +recently visited views. + +Browser cache is not the source of truth for invalidation. Server-side +revalidation signals decide whether cached GitHub data must be refreshed. + +### D1 control plane + +Cloudflare D1 stores durable app data and cache control state. + +Relevant tables: + +- `github_response_cache`: legacy durable response cache and migration fallback. +- `github_revalidation_signal`: timestamped invalidation signals used by + webhook and client revalidation flows. +- `github_cache_namespace`: version rows used by the split KV cache. + +The namespace table is created by: + +```text +apps/dashboard/drizzle/0003_github_cache_namespace.sql +``` + +D1 remains authoritative for cache invalidation because it gives us durable, +consistent control records. KV is used for hot payloads, not for authoritative +invalidation state. + +### KV payload cache + +Cloudflare KV stores hot GitHub response payload envelopes when a resource opts +into `cacheMode: "split"`. + +The binding is: + +```text +GITHUB_CACHE_KV +``` + +It is configured in: + +```text +apps/dashboard/wrangler.jsonc +``` + +Each KV entry stores the same envelope shape as the legacy D1 response cache: + +- `payloadJson` +- `etag` +- `lastModified` +- `fetchedAt` +- `freshUntil` +- `rateLimitRemaining` +- `rateLimitReset` +- `statusCode` + +Entries default to a 7 day KV TTL. + +## Split Cache Read Flow + +The central implementation is: + +```text +apps/dashboard/src/lib/github-cache.ts +``` + +For a split-cache read: + +1. Normalize the request params into stable JSON. +2. Resolve the namespace versions from D1. +3. Build a KV storage key from: + - user id + - resource name + - params hash + - namespace version hash +4. Read the KV payload entry. +5. If KV misses, read the legacy D1 cache entry as a fallback. +6. If the entry is fresh and no newer invalidation signal exists, return it. +7. If the entry is stale, call GitHub with conditional headers when available. +8. If GitHub returns `304`, refresh metadata and freshness without replacing + the payload. +9. If GitHub returns `200`, write the new payload envelope. +10. Writes are mirrored to both KV and the legacy D1 response cache. + +The legacy D1 cache is intentionally still written. It provides a safe fallback +while the split cache is rolled out and lets KV hydrate on misses. + +## Cache Keys And Namespaces + +There are two related but separate concepts: + +- Cache key: identifies one resource plus one param set for one user. +- Namespace key: identifies the invalidation group for one or more cache keys. + +Examples: + +- `viewer` +- `repos.list` +- `pulls.mine` +- `issues.mine` +- `pull:{owner}/{repo}#{pullNumber}` +- `issue:{owner}/{repo}#{issueNumber}` +- `repoLabels:{owner}/{repo}` +- `repoCollaborators:{owner}/{repo}` +- `orgTeams:{org}` + +Namespace helpers live in: + +```text +apps/dashboard/src/lib/github-revalidation.ts +``` + +When a namespace is bumped, future reads build a different KV key. Old KV values +are not synchronously deleted. They expire naturally by TTL, which avoids delete +storms and avoids relying on immediate KV delete consistency. + +## Invalidation Flow + +Webhook and mutation invalidation is durable: + +1. A webhook or mutation resolves affected signal keys. +2. `markGitHubRevalidationSignals()` writes the latest signal timestamp to D1. +3. The same signal keys are passed to `bumpGitHubCacheNamespaces()`. +4. Namespace rows are inserted or incremented in D1. +5. Future split-cache reads use the newer namespace version and bypass old KV + entries. + +This also handles offline users. If a user is not in the app when a webhook +arrives, the D1 signal and namespace version remain durable. When the user comes +back later, their next read observes the newer server-side state. + +## Resources Currently On The Split Path + +The first implemented split-cache coverage includes: + +- Authenticated viewer. +- Authenticated user repo list. +- My pull request dashboard slices. +- My issue dashboard slices. +- Pull detail data. +- Pull comments. +- Pull commits. +- Pull status. +- Pull files pages. +- Pull file summaries. +- Pull review comments. +- Issue detail data. +- Issue comments. +- Repo collaborators. +- Org teams. +- Repo labels. + +Most resource opt-ins are in: + +```text +apps/dashboard/src/lib/github.functions.ts +``` + +## Rate-Limit Behavior + +There are two layers of rate-limit protection. + +### Cache-level stale fallback + +`github-cache.ts` tracks GitHub response metadata, including remaining quota and +reset time. + +When GitHub budget gets low: + +- remaining quota at or below `100` extends freshness to at least 2 minutes. +- remaining quota at or below `25` extends freshness to at least 5 minutes or + until just after reset, whichever is longer. + +When GitHub returns a primary or secondary rate-limit style error and a cached +payload exists: + +- the cached payload is served instead of failing the request. +- `freshUntil` is extended using `retry-after`, `x-ratelimit-reset`, or a + fallback 60 second window. +- the status code is persisted for observability. + +This is stale-if-rate-limited behavior. It prioritizes keeping the app usable +over forcing a live GitHub refresh during quota pressure. + +### Octokit-level throttling and retry + +The Octokit client factory lives in: + +```text +apps/dashboard/src/lib/github.server.ts +``` + +It enables Octokit's retry and throttling plugins with these rules: + +- Throttling is grouped by user with `github-user:{userId}`. +- Safe read methods, `GET`, `HEAD`, and `OPTIONS`, get up to 2 transient + retries. +- Non-idempotent writes get 0 automatic retries to avoid replaying mutations. +- Primary and secondary rate-limit callbacks retry safe reads at most once. +- Writes are throttled and logged, but not automatically replayed. + +This keeps read paths resilient while avoiding duplicated side effects from +`POST`, `PATCH`, or `DELETE` requests. + +## PR Status Refresh + +The pull detail page no longer polls PR status every 15 seconds. + +Status fetching is owned by the merge status section in: + +```text +apps/dashboard/src/components/pulls/detail/pull-detail-activity.tsx +``` + +It still refreshes on window focus, but it avoids constant background polling. +Webhook invalidation and server cache freshness now carry more of the load. + +## Operational Requirements + +Before this architecture is active in an environment: + +1. The `GITHUB_CACHE_KV` binding must exist in Wrangler. +2. The `github_cache_namespace` D1 migration must be applied. +3. Existing legacy D1 cache entries can remain in place. + +Migration commands: + +```sh +pnpm --filter @diffkit/dashboard migrate:local +pnpm --filter @diffkit/dashboard migrate:remote +``` + +If the KV binding is unavailable, the cache layer falls back to legacy D1 +behavior. + +## Testing Coverage + +The focused tests cover: + +- fresh legacy cache reads. +- conditional `304` refresh. +- request-scoped stale refresh deduplication. +- split-cache KV hits. +- KV hydration from legacy D1. +- adaptive freshness when GitHub quota is low. +- stale serving when GitHub is rate limited. +- Octokit throttling and safe-method retry policy. + +Relevant test files: + +```text +apps/dashboard/src/lib/github-cache.test.ts +apps/dashboard/src/lib/github-cache-invalidation.test.ts +apps/dashboard/src/lib/github.server.test.ts +``` + +## Known Limitations + +- Split-cache writes still mirror payloads to legacy D1, so D1 cache churn is + reduced but not eliminated yet. +- In-flight deduplication is request-scoped, not cross-request or cross-worker. +- Realtime client updates still depend on the existing client revalidation + model rather than server push. +- KV is used for payloads only. It should not become the source of truth for + invalidation. + +## Future Work + +Likely next steps: + +- Remove or reduce legacy D1 payload mirroring after the split cache has run in + production safely. +- Add cross-request coalescing for hot GitHub reads. +- Add metrics for KV hit rate, D1 fallback rate, stale-if-rate-limited events, + and GitHub quota pressure. +- Move more realtime behavior toward push-based delivery if 10 second + revalidation polling becomes too noisy.