From 268455ad17c351cb2c33b481ef861f6f121a456f Mon Sep 17 00:00:00 2001 From: oktofeesh1 <287075021+oktofeesh1@users.noreply.github.com> Date: Tue, 30 Jun 2026 00:10:38 -0700 Subject: [PATCH] feat(selfhost): cache safe GitHub GET reads Cache safe GitHub REST GETs through one auth-aware response cache used by Octokit and raw backfill reads. Add per-endpoint TTL overrides, single-flight cold misses, Prometheus counters, and self-host dashboard/docs coverage. --- .env.example | 1 + .../docs.self-hosting-configuration.tsx | 17 + docker-compose.yml | 1 + grafana/dashboards/gittensory.json | 118 +++++ src/github/app.ts | 139 +----- src/github/backfill.ts | 9 +- src/github/client.ts | 251 +++++++++- src/selfhost/redis-response-cache.ts | 28 +- test/unit/backfill.test.ts | 41 ++ test/unit/github-app.test.ts | 13 +- test/unit/github-client.test.ts | 467 +++++++++++++++++- test/unit/queue.test.ts | 9 +- test/unit/selfhost-grafana-dashboard.test.ts | 18 +- .../selfhost-redis-response-cache.test.ts | 53 ++ 14 files changed, 1005 insertions(+), 160 deletions(-) diff --git a/.env.example b/.env.example index 6ae9422d8c..b9f55eac06 100644 --- a/.env.example +++ b/.env.example @@ -163,6 +163,7 @@ REDIS_URL=redis://redis:6379 # REQUIRED for the self-host review # GITTENSORY_IMAGE=ghcr.io/jsonbored/gittensory-selfhost:latest # image used by scripts/deploy-selfhost-image.sh; # # pin production rollouts to a release tag such as :orb-v0.1.0 # # or to an immutable @sha256 digest. +# GITHUB_CACHE_TTL_SECONDS=20 # Short default Redis TTL for safe GitHub GET response caching. Set 0 to disable. # QDRANT_URL= # set to http://qdrant:6333 to use Qdrant as the RAG vector store # # (--profile qdrant). Overrides the built-in sqlite-vec / pgvector. # DISCORD_WEBHOOK_URL= # one Discord channel for per-action notifications (merged/closed/ diff --git a/apps/gittensory-ui/src/routes/docs.self-hosting-configuration.tsx b/apps/gittensory-ui/src/routes/docs.self-hosting-configuration.tsx index daba9c4839..1bd4054be8 100644 --- a/apps/gittensory-ui/src/routes/docs.self-hosting-configuration.tsx +++ b/apps/gittensory-ui/src/routes/docs.self-hosting-configuration.tsx @@ -77,6 +77,23 @@ INTERNAL_JOB_TOKEN=`} FOO wins over the file variant.

+

GitHub API cache

+

+ Redis backs shared caching for stable GitHub GET responses, including repeated installation, + repo/user metadata, and branch-protection required-status reads. Keys include the caller + identity and response-shaping headers, and cold misses are single-flighted so concurrent + jobs do not stampede GitHub. +

+ + + GITHUB_CACHE_TTL_SECONDS is the short default for repeated safe GitHub GETs. + Stable repo/user metadata and branch-protection required-status reads use longer internal + TTLs. Live CI status, check-run, check-suite, pull/issue subresources, pull mergeability, + token minting, rate-limit, and collaborator-permission endpoints are never served from this + cache. Prometheus exports gittensory_github_response_cache_total, and the + bundled self-host Grafana dashboard includes the hit/miss/coalesced/error breakdown. + +

Per-PR feature flags

Most review capabilities need both their own flag and the repo in{" "} diff --git a/docker-compose.yml b/docker-compose.yml index f66fa43921..90db922769 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -62,6 +62,7 @@ services: # DATABASE_URL: postgres://gittensory:${POSTGRES_PASSWORD:-CHANGEME}@pgbouncer:5432/gittensory # Required shared transient state for review correctness, webhook dedup, and rate limiting. REDIS_URL: "${REDIS_URL:-redis://redis:6379}" + GITHUB_CACHE_TTL_SECONDS: "${GITHUB_CACHE_TTL_SECONDS:-20}" # Uncomment for Qdrant RAG vector store (--profile qdrant): # QDRANT_URL: http://qdrant:6333 # Uncomment for Ollama AI (--profile ollama): diff --git a/grafana/dashboards/gittensory.json b/grafana/dashboards/gittensory.json index 90216fd8fd..af8c1844fa 100644 --- a/grafana/dashboards/gittensory.json +++ b/grafana/dashboards/gittensory.json @@ -1376,6 +1376,124 @@ "unit": "short" } } + }, + { + "collapsed": false, + "gridPos": { + "h": 1, + "w": 24, + "x": 0, + "y": 90 + }, + "id": 113, + "title": "GitHub API Cache", + "type": "row" + }, + { + "datasource": { + "type": "prometheus", + "uid": "${DS_PROMETHEUS}" + }, + "fieldConfig": { + "defaults": { + "color": { + "mode": "palette-classic" + }, + "custom": { + "lineWidth": 2, + "fillOpacity": 10, + "stacking": { + "mode": "normal", + "group": "A" + } + }, + "unit": "reqps" + } + }, + "gridPos": { + "h": 8, + "w": 12, + "x": 0, + "y": 91 + }, + "id": 114, + "options": { + "legend": { + "calcs": [ + "lastNotNull" + ], + "displayMode": "table", + "placement": "bottom" + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, + "targets": [ + { + "datasource": { + "type": "prometheus", + "uid": "${DS_PROMETHEUS}" + }, + "expr": "sum by (result) (rate(gittensory_github_response_cache_total[5m]))", + "legendFormat": "{{result}}", + "refId": "A" + } + ], + "title": "GitHub Cache Rate", + "type": "timeseries" + }, + { + "datasource": { + "type": "prometheus", + "uid": "${DS_PROMETHEUS}" + }, + "fieldConfig": { + "defaults": { + "color": { + "mode": "palette-classic" + }, + "custom": { + "lineWidth": 2, + "fillOpacity": 10 + }, + "unit": "short" + } + }, + "gridPos": { + "h": 8, + "w": 12, + "x": 12, + "y": 91 + }, + "id": 115, + "options": { + "legend": { + "calcs": [ + "lastNotNull" + ], + "displayMode": "table", + "placement": "bottom" + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, + "targets": [ + { + "datasource": { + "type": "prometheus", + "uid": "${DS_PROMETHEUS}" + }, + "expr": "sum by (class, result) (gittensory_github_response_cache_total)", + "legendFormat": "{{class}} {{result}}", + "refId": "A" + } + ], + "title": "GitHub Cache Totals", + "type": "timeseries" } ], "refresh": "30s", diff --git a/src/github/app.ts b/src/github/app.ts index 22a6adbfa2..2913142265 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -3,7 +3,11 @@ import { fetchBrokeredInstallationToken, isOrbBrokerMode, } from "../orb/broker-client"; -import { makeInstallationOctokit } from "./client"; +import { + clearGitHubResponseCacheForTest, + makeInstallationOctokit, + timeoutFetch, +} from "./client"; import { maintainerControlPanelUrl } from "./footer"; import type { AgentActionMode } from "../settings/agent-execution"; import { signRs256Jwt } from "../utils/crypto"; @@ -29,6 +33,13 @@ export { GITTENSORY_GATE_CHECK_NAME, GITTENSORY_LEGACY_GATE_CHECK_NAME, } from "../review/check-names"; +export type { CachedGitHubResponse, GitHubResponseCache } from "./client"; +export { + isCacheableGithubUrl, + isRateLimitedResponse, + rateLimitRetryMs, + setGitHubResponseCache, +} from "./client"; type CheckRunResponse = { id: number; @@ -55,130 +66,6 @@ type GitHubCheckConclusion = | "skipped"; type GitHubCheckStatus = "queued" | "in_progress" | "completed"; -/** Hard cap on a single GitHub API request. Without it a slow/half-open GitHub connection can hang the - * Worker — e.g. the Gate's own completing PATCH stalling after the pending check was posted, which leaves - * the check in_progress forever. A bounded timeout turns a hang into a catchable error the caller can - * finalize. Applied to every raw fetch here and to the Octokit instances (via a timeout-injecting fetch). */ -const GITHUB_FETCH_TIMEOUT_MS = 12_000; - -/** A short-TTL cache for safe GitHub GET responses (e.g. Redis on the self-host). Stores only status/body/ - * content-type — never rate-limit or encoding headers. Set on the self-host; the Worker leaves it null. */ -export interface CachedGitHubResponse { - status: number; - body: string; - contentType: string; -} -export interface GitHubResponseCache { - get(url: string): Promise; - set(url: string, value: CachedGitHubResponse): Promise; -} -let responseCache: GitHubResponseCache | null = null; -export function setGitHubResponseCache( - cache: GitHubResponseCache | null, -): void { - responseCache = cache; -} - -/** Only cache safe GETs to the GitHub REST API. Never cache token-minting, rate-limit, or - * authorization/permission endpoints whose response must reflect the live caller context. Exported for tests. */ -export function isCacheableGithubUrl(url: string): boolean { - if (!url.startsWith("https://api.github.com/")) return false; - if (url.includes("/access_tokens") || url.includes("/rate_limit")) - return false; - return !/\/repos\/[^/]+\/[^/]+\/collaborators\/[^/]+\/permission(?:$|[?#])/.test( - url, - ); -} - -// Transient GitHub rate-limit handling (#ratelimit-resilience). A primary (x-ratelimit-remaining:0) or secondary -// (Retry-After / "secondary rate limit" body) limit returns 403/429. Instead of surfacing it as a failure — or -// MISCLASSIFYING a 403 as a permission gap — back off a few times and retry. A sustained limit exhausts the -// retries and the response is returned so the caller (and the queue) handles it. Bounded so a review never stalls. -const GITHUB_RATE_LIMIT_MAX_RETRIES = 3; -const GITHUB_RATE_LIMIT_MAX_DELAY_MS = 8_000; - -const sleep = (ms: number): Promise => - new Promise((resolve) => setTimeout(resolve, ms)); - -/** Does this GitHub response signal a rate limit (primary or secondary)? 403/429 with a Retry-After header, an - * exhausted x-ratelimit-remaining, or a secondary-limit/abuse body. A 403 with NONE of these is a real - * permission/other error and must surface — not retry, not be mistaken for a rate limit. Exported for tests. */ -export async function isRateLimitedResponse( - response: Response, -): Promise { - if (response.status !== 403 && response.status !== 429) return false; - if (response.headers.get("retry-after") != null) return true; - if (response.headers.get("x-ratelimit-remaining") === "0") return true; - try { - return /secondary rate limit|\babuse\b|api rate limit exceeded/i.test( - await response.clone().text(), - ); - /* v8 ignore next 3 -- defensive: a cloned Response body that fails to read isn't reachable in practice */ - } catch { - return false; - } -} - -/** How long to wait before the next rate-limit retry: honor a valid Retry-After (seconds), else exponential - * backoff — each capped so a review can never stall on one call. A sustained PRIMARY limit (reset up to an hour - * out) simply exhausts the few inline retries and the queue retries the job later. Exported for tests. */ -export function rateLimitRetryMs(response: Response, attempt: number): number { - const retryAfterHeader = response.headers.get("retry-after"); - if (retryAfterHeader != null) { - const retryAfter = Number(retryAfterHeader); - if (Number.isFinite(retryAfter) && retryAfter >= 0) - return Math.min(retryAfter * 1000, GITHUB_RATE_LIMIT_MAX_DELAY_MS); - } - return Math.min(500 * 2 ** attempt, GITHUB_RATE_LIMIT_MAX_DELAY_MS); -} - -async function timeoutFetch( - input: RequestInfo | URL, - init?: RequestInit, -): Promise { - const method = (init?.method ?? "GET").toUpperCase(); - const url = String(input); // timeoutFetch is only ever called with string URLs (app template strings + octokit) - const useCache = - responseCache !== null && method === "GET" && isCacheableGithubUrl(url); - if (useCache) { - const hit = await responseCache!.get(url).catch(() => null); // a cache read must never break the fetch - if (hit) - return new Response(hit.body, { - status: hit.status, - headers: { "content-type": hit.contentType }, - }); - } - let response: Response; - for (let attempt = 0; ; attempt += 1) { - response = init?.signal - ? await fetch(input, init) - : await fetch(input, { - ...(init ?? {}), - signal: AbortSignal.timeout(GITHUB_FETCH_TIMEOUT_MS), - }); - // Retry a transient rate-limit (with backoff) instead of surfacing it; stop once exhausted or it's not a limit. - if ( - attempt >= GITHUB_RATE_LIMIT_MAX_RETRIES || - !(await isRateLimitedResponse(response)) - ) - break; - await sleep(rateLimitRetryMs(response, attempt)); - } - if (useCache && response.status === 200) { - try { - const body = await response.clone().text(); // clone leaves the returned response readable - await responseCache!.set(url, { - status: 200, - body, - contentType: response.headers.get("content-type") ?? "application/json", - }); - } catch { - /* caching is best-effort */ - } - } - return response; -} - // In-isolate installation-token cache. GitHub installation tokens are valid ~1h; minting a fresh one on EVERY // call (the previous behavior) multiplied GitHub API usage enormously — each review path mints several tokens, // and across the sweep + re-reviews that exhausted the hourly rate limit (observed min_remaining=0 → reviews @@ -402,7 +289,7 @@ export function isForeignAppInstallation( export function clearInstallationTokenCacheForTest(): void { installationTokenCache.clear(); externalTokenStore = null; - responseCache = null; + clearGitHubResponseCacheForTest(); } export async function getAppInstallation( diff --git a/src/github/backfill.ts b/src/github/backfill.ts index ba1b51809e..b7cca72da0 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -67,6 +67,7 @@ import { } from "../review/check-names"; import { buildReviewThreadBlocker, type ReviewThreadBlocker } from "../review/review-thread-findings"; import { delayUntil, shouldWaitForGitHubRateLimit } from "./rate-limit"; +import { isGitHubResponseCacheReplay, timeoutFetch } from "./client"; type GitHubLabelPayload = { name: string; @@ -2687,10 +2688,12 @@ async function githubJsonWithHeaders( ): Promise<{ data: T; link: string | null; etag: string | null; lastModified: string | null }> { const { owner, name } = repoParts(repoFullName); const url = `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}${path}`; - let response = await fetch(url, { headers: githubRestHeaders(token) }); - await recordGitHubResponse(env, repoFullName, path, response, "rest"); + let response = await timeoutFetch(url, { headers: githubRestHeaders(token) }); + if (!isGitHubResponseCacheReplay(response)) { + await recordGitHubResponse(env, repoFullName, path, response, "rest"); + } if (response.status === 404 && token && token === env.GITHUB_PUBLIC_TOKEN) { - response = await fetch(url, { headers: githubRestHeaders() }); + response = await timeoutFetch(url, { headers: githubRestHeaders() }); // Do not persist unauthenticated fallback rate-limit headers into the shared REST backoff state. // GitHub's unauthenticated REST bucket is capped below LOW_REST_RATE_LIMIT_REMAINING, so recording // successful fallback responses can incorrectly stall later token-backed segment jobs. diff --git a/src/github/client.ts b/src/github/client.ts index f57a8abd59..251a4139b8 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -1,6 +1,7 @@ import { Octokit } from "@octokit/core"; import { isGlobalAgentFrozen, recordAuditEvent } from "../db/repositories"; import { isGlobalAgentPause, resolveAgentActionMode, type AgentActionMode } from "../settings/agent-execution"; +import { incr } from "../selfhost/metrics"; import type { RepositorySettings } from "../types"; // The SINGLE place an installation-scoped Octokit is built. Every GitHub write in src/github/** routes through @@ -16,12 +17,252 @@ import type { RepositorySettings } from "../types"; // covered here — they carry their own mode guard / are a separate actor class. const GITHUB_FETCH_TIMEOUT_MS = 12_000; +const GITHUB_API_PREFIX = "https://api.github.com"; +const GITHUB_RESPONSE_CACHE_METRIC = "gittensory_github_response_cache_total"; +const BRANCH_PROTECTION_TTL_SECONDS = 20 * 60; +const METADATA_TTL_SECONDS = 10 * 60; +export const GITHUB_RESPONSE_CACHE_REPLAY_HEADER = "x-gittensory-cache"; -// A 12s hard cap on every GitHub request. Centralised here so the comment / label / pr-action helpers — which -// previously built a bare `new Octokit({ auth })` with no cap — all inherit the bound for free. -export function timeoutFetch(input: RequestInfo | URL, init?: RequestInit): Promise { - if (init?.signal) return fetch(input, init); - return fetch(input, { ...(init ?? {}), signal: AbortSignal.timeout(GITHUB_FETCH_TIMEOUT_MS) }); +/** A shared cache for safe GitHub GET responses (e.g. Redis on the self-host). Stores only status/body/ + * content-type plus pagination/validator headers — never rate-limit or encoding headers. Set on the self-host; + * the Worker leaves it null. */ +export interface CachedGitHubResponse { + status: number; + body: string; + contentType: string; + link?: string; + etag?: string; + lastModified?: string; +} +export interface GitHubResponseCache { + get(key: string): Promise; + set(key: string, value: CachedGitHubResponse, ttlSeconds?: number): Promise; +} +let responseCache: GitHubResponseCache | null = null; +export function setGitHubResponseCache(cache: GitHubResponseCache | null): void { + responseCache = cache; +} + +type GitHubCacheClass = "branch_protection" | "metadata"; + +/** Only cache explicitly stable GitHub REST reads. PR/issue/comment/label/event/check/status reads are mutable + * review inputs and must always reflect the current GitHub state. Exported for tests. */ +export function isCacheableGithubUrl(url: string): boolean { + return githubCacheClassForUrl(url) !== null; +} + +function githubApiPath(url: string): string { + return url.slice(GITHUB_API_PREFIX.length); +} + +function githubCacheClassForUrl(url: string): GitHubCacheClass | null { + if (!url.startsWith(`${GITHUB_API_PREFIX}/`)) return null; + const path = githubApiPath(url); + if (/^\/repos\/[^/]+\/[^/]+\/branches\/[^/]+\/protection\/required_status_checks(?:$|[?#])/.test(path)) return "branch_protection"; + if ( + (/^\/users\/[^/?#]+(?:$|[?#])/.test(path) || + /^\/repos\/[^/?#]+\/[^/?#]+(?:$|[?#])/.test(path) || + /^\/app\/installations\/\d+(?:$|[?#])/.test(path)) + ) { + return "metadata"; + } + return null; +} + +function githubCacheTtlSeconds(cls: GitHubCacheClass): number { + if (cls === "branch_protection") return BRANCH_PROTECTION_TTL_SECONDS; + return METADATA_TTL_SECONDS; +} + +function hasConditionalRequestHeader(headers: Headers): boolean { + return headers.has("if-none-match") || headers.has("if-modified-since") || headers.has("if-match") || headers.has("if-unmodified-since"); +} + +function cacheBypassClass(method: string, url: string, headers: Headers): string { + if (responseCache === null) return "disabled"; + if (method !== "GET") return "non_get"; + if (!url.startsWith(`${GITHUB_API_PREFIX}/`)) return "non_github"; + if (hasConditionalRequestHeader(headers)) return "conditional"; + return "sensitive"; +} + +function recordGitHubCacheMetric(result: "hit" | "miss" | "set" | "coalesced" | "bypassed" | "error", cls: string): void { + incr(GITHUB_RESPONSE_CACHE_METRIC, { result, class: cls }); +} + +async function sha256Short(value: string): Promise { + const digest = await crypto.subtle.digest("SHA-256", new TextEncoder().encode(value)); + return Array.from(new Uint8Array(digest), (byte) => byte.toString(16).padStart(2, "0")).join("").slice(0, 16); +} + +async function responseCacheKey(url: string, headers: Headers): Promise { + const authHash = await sha256Short(headers.get("authorization") || ""); + const accept = encodeURIComponent(headers.get("accept") || ""); + const apiVersion = encodeURIComponent(headers.get("x-github-api-version") || ""); + return `v2:${authHash}:${accept}:${apiVersion}:${url}`; +} + +function requestHeaders(input: RequestInfo | URL, init: RequestInit | undefined): Headers { + const headers = new Headers(typeof Request !== "undefined" && input instanceof Request ? input.headers : undefined); + new Headers(init?.headers).forEach((value, key) => headers.set(key, value)); + return headers; +} + +function requestMethod(input: RequestInfo | URL, init: RequestInit | undefined): string { + return (init?.method ?? (typeof Request !== "undefined" && input instanceof Request ? input.method : undefined) ?? "GET").toUpperCase(); +} + +function requestUrl(input: RequestInfo | URL): string { + return typeof Request !== "undefined" && input instanceof Request ? input.url : String(input); +} + +export function isGitHubResponseCacheReplay(response: Response): boolean { + return response.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER) !== null; +} + +// Transient GitHub rate-limit handling (#ratelimit-resilience). A primary (x-ratelimit-remaining:0) or secondary +// (Retry-After / "secondary rate limit" body) limit returns 403/429. Instead of surfacing it as a failure — or +// MISCLASSIFYING a 403 as a permission gap — back off a few times and retry. A sustained limit exhausts the +// retries and the response is returned so the caller (and the queue) handles it. Bounded so a review never stalls. +const GITHUB_RATE_LIMIT_MAX_RETRIES = 3; +const GITHUB_RATE_LIMIT_MAX_DELAY_MS = 8_000; + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +/** Does this GitHub response signal a rate limit (primary or secondary)? 403/429 with a Retry-After header, an + * exhausted x-ratelimit-remaining, or a secondary-limit/abuse body. A 403 with NONE of these is a real + * permission/other error and must surface — not retry, not be mistaken for a rate limit. Exported for tests. */ +export async function isRateLimitedResponse(response: Response): Promise { + if (response.status !== 403 && response.status !== 429) return false; + if (response.headers.get("retry-after") != null) return true; + if (response.headers.get("x-ratelimit-remaining") === "0") return true; + try { + return /secondary rate limit|\babuse\b|api rate limit exceeded/i.test(await response.clone().text()); + /* v8 ignore next 3 -- defensive: a cloned Response body that fails to read isn't reachable in practice */ + } catch { + return false; + } +} + +/** How long to wait before the next rate-limit retry: honor a valid Retry-After (seconds), else exponential + * backoff — each capped so a review can never stall on one call. A sustained PRIMARY limit (reset up to an hour + * out) simply exhausts the few inline retries and the queue retries the job later. Exported for tests. */ +export function rateLimitRetryMs(response: Response, attempt: number): number { + const retryAfterHeader = response.headers.get("retry-after"); + if (retryAfterHeader != null) { + const retryAfter = Number(retryAfterHeader); + if (Number.isFinite(retryAfter) && retryAfter >= 0) return Math.min(retryAfter * 1000, GITHUB_RATE_LIMIT_MAX_DELAY_MS); + } + return Math.min(500 * 2 ** attempt, GITHUB_RATE_LIMIT_MAX_DELAY_MS); +} + +function responseFromCached(hit: CachedGitHubResponse, replayKind: "hit" | "coalesced"): Response { + const headers = new Headers({ "content-type": hit.contentType, [GITHUB_RESPONSE_CACHE_REPLAY_HEADER]: replayKind }); + if (hit.link) headers.set("link", hit.link); + if (hit.etag) headers.set("etag", hit.etag); + if (hit.lastModified) headers.set("last-modified", hit.lastModified); + return new Response(hit.body, { + status: hit.status, + headers, + }); +} + +async function fetchWithGitHubRetry(input: RequestInfo | URL, init?: RequestInit): Promise { + let response: Response; + for (let attempt = 0; ; attempt += 1) { + response = init?.signal + ? await fetch(input, init) + : await fetch(input, { + ...(init ?? {}), + signal: AbortSignal.timeout(GITHUB_FETCH_TIMEOUT_MS), + }); + // Retry a transient rate-limit (with backoff) instead of surfacing it; stop once exhausted or it's not a limit. + if (attempt >= GITHUB_RATE_LIMIT_MAX_RETRIES || !(await isRateLimitedResponse(response))) break; + await sleep(rateLimitRetryMs(response, attempt)); + } + return response; +} + +async function fetchAndMaybeCacheGitHubGet( + input: RequestInfo | URL, + init: RequestInit | undefined, + url: string, + cacheKey: string, + cls: GitHubCacheClass, +): Promise<{ response: Response; cached: CachedGitHubResponse | null }> { + const response = await fetchWithGitHubRetry(input, init); + if (response.status !== 200) return { response, cached: null }; + try { + const cached = { + status: 200, + body: await response.clone().text(), + contentType: response.headers.get("content-type") ?? "application/json", + ...(response.headers.get("link") ? { link: response.headers.get("link")! } : {}), + ...(response.headers.get("etag") ? { etag: response.headers.get("etag")! } : {}), + ...(response.headers.get("last-modified") ? { lastModified: response.headers.get("last-modified")! } : {}), + }; + await responseCache!.set(cacheKey, cached, githubCacheTtlSeconds(cls)); + recordGitHubCacheMetric("set", cls); + return { response, cached }; + } catch { + recordGitHubCacheMetric("error", cls); + return { response, cached: null }; + } +} + +// Single-flight cacheable GETs inside one isolate: a webhook burst often asks for the same metadata +// before Redis has been populated. Join those cold misses so GitHub sees one request, then replay the cached body. +const inFlightCacheableGets = new Map>(); + +// A 12s hard cap on every GitHub request. Centralised here so the app token/installation raw fetches plus comment / +// label / check-run / pr-action Octokit helpers all inherit the cache boundary, retry, and timeout behavior. +export async function timeoutFetch(input: RequestInfo | URL, init?: RequestInit): Promise { + const method = requestMethod(input, init); + const url = requestUrl(input); + const headers = requestHeaders(input, init); + const conditional = hasConditionalRequestHeader(headers); + const cls = method === "GET" && !conditional ? githubCacheClassForUrl(url) : null; + const useCache = responseCache !== null && cls !== null; + if (!useCache) { + recordGitHubCacheMetric("bypassed", cacheBypassClass(method, url, headers)); + return fetchWithGitHubRetry(input, init); + } + + const cacheKey = await responseCacheKey(url, headers); + let hit: CachedGitHubResponse | null = null; + try { + hit = await responseCache!.get(cacheKey); + } catch { + recordGitHubCacheMetric("error", cls); + } + if (hit) { + recordGitHubCacheMetric("hit", cls); + return responseFromCached(hit, "hit"); + } + recordGitHubCacheMetric("miss", cls); + + const existing = inFlightCacheableGets.get(cacheKey); + if (existing) { + recordGitHubCacheMetric("coalesced", cls); + const replay = await existing; + if (replay) return responseFromCached(replay, "coalesced"); + } + + const request = fetchAndMaybeCacheGitHubGet(input, init, url, cacheKey, cls); + const shared = request.then( + (result) => result.cached, + () => null, + ); + const sharedWithCleanup = shared.finally(() => inFlightCacheableGets.delete(cacheKey)); + inFlightCacheableGets.set(cacheKey, sharedWithCleanup); + const result = await request; + return result.response; +} + +/** Test-only: reset shared GitHub response cache state between tests. */ +export function clearGitHubResponseCacheForTest(): void { + responseCache = null; + inFlightCacheableGets.clear(); } const WRITE_METHODS = new Set(["POST", "PATCH", "PUT", "DELETE"]); diff --git a/src/selfhost/redis-response-cache.ts b/src/selfhost/redis-response-cache.ts index 83edcb1fb4..e197e43107 100644 --- a/src/selfhost/redis-response-cache.ts +++ b/src/selfhost/redis-response-cache.ts @@ -1,43 +1,47 @@ // Redis-backed GitHub GET-response cache (#perf). The self-host runtime requires REDIS_URL; when -// GITHUB_CACHE_TTL_SECONDS>0, it caches safe GitHub API GET responses for a short TTL. A single review pass makes ~24 GitHub -// fetches (PR data, files, user/org lookups) — many repeated — all network-bound and rate-limited. A short-TTL -// cache dedups those within and across rapid re-reviews, cutting latency and rate-limit pressure, and it -// persists across restarts. Keyed by URL; the TTL bounds staleness. Only the status + body + content-type are -// stored — NOT rate-limit headers (a cache hit consumed no quota) or content-encoding (the body is decoded). +// GITHUB_CACHE_TTL_SECONDS>0, it caches explicitly stable GitHub API GET responses. A review pass can repeat +// branch-protection and metadata reads across jobs, but mutable PR/issue/check/status reads must stay live. The +// shared GitHub client picks per-endpoint TTL overrides for stable metadata. Keyed by the caller identity + URL + +// response-shaping headers. Only the status + body + content-type plus pagination/validator headers are stored — +// NOT rate-limit headers (a cache hit consumed no quota) or content-encoding (the body is decoded). import type { Redis } from "ioredis"; -import type { CachedGitHubResponse, GitHubResponseCache } from "../github/app"; +import type { CachedGitHubResponse, GitHubResponseCache } from "../github/client"; -const keyFor = (url: string): string => `gh:resp:${url}`; +const keyFor = (key: string): string => `gh:resp:${key}`; export function createRedisResponseCache( redis: Redis, ttlSeconds: number, ): GitHubResponseCache { return { - async get(url: string) { - const raw = await redis.get(keyFor(url)); + async get(key: string) { + const raw = await redis.get(keyFor(key)); if (!raw) return null; try { const value = JSON.parse(raw) as Partial; return typeof value.status === "number" && + value.status === 200 && typeof value.body === "string" && typeof value.contentType === "string" ? { status: value.status, body: value.body, contentType: value.contentType, + ...(typeof value.link === "string" ? { link: value.link } : {}), + ...(typeof value.etag === "string" ? { etag: value.etag } : {}), + ...(typeof value.lastModified === "string" ? { lastModified: value.lastModified } : {}), } : null; } catch { return null; } }, - async set(url: string, value: CachedGitHubResponse) { + async set(key: string, value: CachedGitHubResponse, ttlOverrideSeconds?: number) { await redis.set( - keyFor(url), + keyFor(key), JSON.stringify(value), "EX", - Math.max(1, ttlSeconds), + Math.max(1, ttlOverrideSeconds ?? ttlSeconds), ); }, }; diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 2f140406c2..5474eb7cb0 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -40,12 +40,18 @@ import { refreshInstallationHealth, refreshPullRequestDetails, } from "../../src/github/backfill"; +import { + clearGitHubResponseCacheForTest, + setGitHubResponseCache, + type CachedGitHubResponse, +} from "../../src/github/client"; import { normalizeRegistryPayload } from "../../src/registry/normalize"; import { persistRegistrySnapshot } from "../../src/registry/sync"; import { createTestEnv } from "../helpers/d1"; describe("GitHub backfill", () => { afterEach(() => { + clearGitHubResponseCacheForTest(); vi.unstubAllGlobals(); }); @@ -3990,6 +3996,41 @@ describe("GitHub backfill", () => { expect([...(required as Set)].sort()).toEqual(["Superagent Security Scan", "validate"]); }); + it("uses the shared GitHub GET cache for raw branch-protection reads without double-counting rate-limit observations", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + const store = new Map(); + setGitHubResponseCache({ + get: async (key) => store.get(key) ?? null, + set: async (key, value) => void store.set(key, value), + }); + let fetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + fetches += 1; + expect(input.toString()).toContain("/branches/main/protection/required_status_checks"); + return Response.json( + { contexts: ["validate"], checks: [] }, + { headers: { "x-ratelimit-limit": "5000", "x-ratelimit-remaining": "4999", "x-ratelimit-reset": "1782802800" } }, + ); + }); + + const first = await fetchRequiredStatusContexts(env, "JSONbored/gittensory", "main", "public-token"); + const second = await fetchRequiredStatusContexts(env, "JSONbored/gittensory", "main", "public-token"); + + expect([...(first as Set)]).toEqual(["validate"]); + expect([...(second as Set)]).toEqual(["validate"]); + expect(fetches).toBe(1); + expect([...store.keys()].some((key) => key.includes("/branches/main/protection/required_status_checks"))).toBe(true); + const observations = await listLatestGitHubRateLimitObservations(env); + expect(observations).toHaveLength(1); + expect(observations[0]).toMatchObject({ + repoFullName: "JSONbored/gittensory", + resource: "rest", + path: "/branches/main/protection/required_status_checks", + statusCode: 200, + remaining: 4999, + }); + }); + it("returns null when the live read fails, even if a stale global fallback is configured (conservative fold-all)", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); (env as Env & { GITTENSORY_REQUIRED_CI_CONTEXTS?: string }).GITTENSORY_REQUIRED_CI_CONTEXTS = "stale-required-context"; diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 8650c922be..abc507544d 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -1904,8 +1904,12 @@ describe("self-host Redis token store + GitHub GET response cache", () => { it("isCacheableGithubUrl: caches safe GitHub GETs but not sensitive endpoints", () => { expect( - isCacheableGithubUrl("https://api.github.com/repos/o/r/pulls/1"), + isCacheableGithubUrl("https://api.github.com/repos/o/r"), ).toBe(true); + expect(isCacheableGithubUrl("https://api.github.com/repos/o/r/pulls/1")).toBe(false); + expect(isCacheableGithubUrl("https://api.github.com/repos/o/r/pulls/1/files")).toBe(false); + expect(isCacheableGithubUrl("https://api.github.com/repos/o/r/issues/1/labels")).toBe(false); + expect(isCacheableGithubUrl("https://api.github.com/repos/o/r/issues/1/comments")).toBe(false); expect( isCacheableGithubUrl( "https://api.github.com/app/installations/1/access_tokens", @@ -1985,7 +1989,8 @@ describe("self-host Redis token store + GitHub GET response cache", () => { expect(a.id).toBe(42); expect(b.id).toBe(42); expect(getFetches).toBe(1); // second call served from the response cache - expect(store.has("https://api.github.com/app/installations/42")).toBe(true); + expect([...store.keys()].some((key) => key.includes("https://api.github.com/app/installations/42"))).toBe(true); + expect([...store.keys()].some((key) => key.includes("Bearer "))).toBe(false); }); it("does not cache a non-200 GitHub GET", async () => { @@ -2001,9 +2006,7 @@ describe("self-host Redis token store + GitHub GET response cache", () => { vi.stubGlobal("fetch", async () => new Response("nope", { status: 500 })); const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); await expect(getAppInstallation(env, 99)).rejects.toThrow(); - expect(store.has("https://api.github.com/app/installations/99")).toBe( - false, - ); // non-200 not cached + expect(store.size).toBe(0); // non-200 not cached }); }); diff --git a/test/unit/github-client.test.ts b/test/unit/github-client.test.ts index fe5821d434..a28b826d0d 100644 --- a/test/unit/github-client.test.ts +++ b/test/unit/github-client.test.ts @@ -1,6 +1,18 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { forcedSelfhostMode, makeInstallationOctokit, resolveRepoActionMode, timeoutFetch } from "../../src/github/client"; +import { + clearGitHubResponseCacheForTest, + forcedSelfhostMode, + GITHUB_RESPONSE_CACHE_REPLAY_HEADER, + isCacheableGithubUrl, + isRateLimitedResponse, + makeInstallationOctokit, + resolveRepoActionMode, + setGitHubResponseCache, + timeoutFetch, + type CachedGitHubResponse, +} from "../../src/github/client"; import { setGlobalAgentFrozen } from "../../src/db/repositories"; +import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { createTestEnv } from "../helpers/d1"; type RecordedCall = { url: string; method: string }; @@ -12,7 +24,20 @@ function stubFetchRecording(calls: RecordedCall[], body: unknown = { id: 5 }): v }); } -afterEach(() => vi.unstubAllGlobals()); +function installMemoryResponseCache(): Map { + const store = new Map(); + setGitHubResponseCache({ + get: async (url) => store.get(url) ?? null, + set: async (url, value) => void store.set(url, value), + }); + return store; +} + +afterEach(() => { + clearGitHubResponseCacheForTest(); + resetMetrics(); + vi.unstubAllGlobals(); +}); describe("makeInstallationOctokit", () => { it("live mode lets a write reach GitHub (no suppression hook)", async () => { @@ -141,4 +166,442 @@ describe("timeoutFetch", () => { await timeoutFetch("https://example.test"); expect(injected).toBeInstanceOf(AbortSignal); }); + + it("serves stable installation Octokit metadata GETs from the shared GitHub response cache", async () => { + const store = installMemoryResponseCache(); + let getFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = String(input); + if (url === "https://api.github.com/repos/o/r") { + getFetches += 1; + return Response.json({ full_name: "o/r", fetches: getFetches }); + } + return new Response("not found", { status: 404 }); + }); + + const octokit = makeInstallationOctokit(createTestEnv(), "tok"); + const first = await octokit.request("GET /repos/{owner}/{repo}", { owner: "o", repo: "r" }); + const second = await octokit.request("GET /repos/{owner}/{repo}", { owner: "o", repo: "r" }); + + expect(first.data).toMatchObject({ full_name: "o/r", fetches: 1 }); + expect(second.data).toMatchObject({ full_name: "o/r", fetches: 1 }); + expect(getFetches).toBe(1); + expect([...store.keys()].some((url) => url.endsWith("/repos/o/r"))).toBe(true); + const metrics = await renderMetrics(); + expect(metrics).toContain('gittensory_github_response_cache_total{class="metadata",result="miss"} 1'); + expect(metrics).toContain('gittensory_github_response_cache_total{class="metadata",result="hit"} 1'); + expect(metrics).toContain('gittensory_github_response_cache_total{class="metadata",result="set"} 1'); + }); + + it("single-flights concurrent cacheable Octokit GET misses before Redis is warm", async () => { + let cacheReads = 0; + let resolveBothCacheReads!: () => void; + const bothCacheReads = new Promise((resolve) => { + resolveBothCacheReads = resolve; + }); + setGitHubResponseCache({ + get: async () => { + cacheReads += 1; + if (cacheReads === 2) resolveBothCacheReads(); + return null; + }, + set: async () => undefined, + }); + let releaseFetch!: () => void; + const fetchGate = new Promise((resolve) => { + releaseFetch = resolve; + }); + let getFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (String(input).includes("/repos/o/r/branches/main/protection/required_status_checks")) { + getFetches += 1; + await fetchGate; + return Response.json({ contexts: ["ci"] }); + } + return new Response("not found", { status: 404 }); + }); + + const octokit = makeInstallationOctokit(createTestEnv(), "tok"); + const first = octokit.request("GET /repos/{owner}/{repo}/branches/{branch}/protection/required_status_checks", { owner: "o", repo: "r", branch: "main" }); + const second = octokit.request("GET /repos/{owner}/{repo}/branches/{branch}/protection/required_status_checks", { owner: "o", repo: "r", branch: "main" }); + await bothCacheReads; + releaseFetch(); + + await expect(Promise.all([first, second])).resolves.toEqual([ + expect.objectContaining({ data: { contexts: ["ci"] } }), + expect.objectContaining({ data: { contexts: ["ci"] } }), + ]); + expect(getFetches).toBe(1); + expect(await renderMetrics()).toContain('gittensory_github_response_cache_total{class="branch_protection",result="coalesced"} 1'); + }); + + it("keys safe GitHub GETs by auth identity and response-shaping headers without storing the token", async () => { + const store = installMemoryResponseCache(); + let getFetches = 0; + vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => { + getFetches += 1; + const authorization = new Headers(init?.headers).get("authorization"); + return Response.json({ caller: authorization?.endsWith("token-a") ? "a" : "b" }); + }); + + const url = "https://api.github.com/repos/o/r"; + const firstA = await timeoutFetch(url, { headers: { authorization: "Bearer token-a", accept: "application/vnd.github+json" } }); + const firstB = await timeoutFetch(url, { headers: { authorization: "Bearer token-b", accept: "application/vnd.github+json" } }); + const secondA = await timeoutFetch(url, { headers: { authorization: "Bearer token-a", accept: "application/vnd.github+json" } }); + + expect(await firstA.json()).toEqual({ caller: "a" }); + expect(await firstB.json()).toEqual({ caller: "b" }); + expect(await secondA.json()).toEqual({ caller: "a" }); + expect(secondA.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBe("hit"); + expect(getFetches).toBe(2); + expect([...store.keys()].some((key) => key.includes("token-a") || key.includes("token-b"))).toBe(false); + expect([...store.keys()].filter((key) => key.includes(url))).toHaveLength(2); + }); + + it("replays pagination and validator headers while dropping rate-limit headers", async () => { + installMemoryResponseCache(); + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + getFetches += 1; + return Response.json( + [{ number: getFetches }], + { + headers: { + link: '; rel="next"', + etag: '"abc123"', + "last-modified": "Mon, 29 Jun 2026 20:00:00 GMT", + "x-ratelimit-remaining": "4999", + }, + }, + ); + }); + + const url = "https://api.github.com/repos/o/r/branches/main/protection/required_status_checks"; + expect(await (await timeoutFetch(url)).json()).toEqual([{ number: 1 }]); + const replay = await timeoutFetch(url); + + expect(await replay.json()).toEqual([{ number: 1 }]); + expect(replay.headers.get("link")).toBe('; rel="next"'); + expect(replay.headers.get("etag")).toBe('"abc123"'); + expect(replay.headers.get("last-modified")).toBe("Mon, 29 Jun 2026 20:00:00 GMT"); + expect(replay.headers.get("x-ratelimit-remaining")).toBeNull(); + expect(getFetches).toBe(1); + }); + + it("defaults cached replays to application/json when GitHub omits content-type", async () => { + const store = installMemoryResponseCache(); + vi.stubGlobal("fetch", async () => new Response(new TextEncoder().encode('{"ok":true}'), { status: 200 })); + + const url = "https://api.github.com/users/alice"; + expect(await (await timeoutFetch(url)).json()).toEqual({ ok: true }); + const replay = await timeoutFetch(url); + + expect([...store.values()][0]?.contentType).toBe("application/json"); + expect(replay.headers.get("content-type")).toBe("application/json"); + expect(await replay.json()).toEqual({ ok: true }); + }); + + it("uses longer TTL overrides for stable GitHub metadata and branch-protection reads", async () => { + const ttlByUrl = new Map(); + setGitHubResponseCache({ + get: async () => null, + set: async (key, _value, ttl) => void ttlByUrl.set(key, ttl), + }); + vi.stubGlobal("fetch", async () => Response.json({ ok: true })); + + await timeoutFetch("https://api.github.com/repos/o/r/branches/main/protection/required_status_checks"); + await timeoutFetch("https://api.github.com/repos/o/r"); + await timeoutFetch("https://api.github.com/users/alice"); + await timeoutFetch("https://api.github.com/app/installations/123"); + await timeoutFetch("https://api.github.com/repos/o/r/commits/abc/status"); + + const branchTtl = [...ttlByUrl].find(([key]) => key.includes("/required_status_checks"))?.[1]; + const repoTtl = [...ttlByUrl].find(([key]) => key.endsWith("/repos/o/r"))?.[1]; + const userTtl = [...ttlByUrl].find(([key]) => key.endsWith("/users/alice"))?.[1]; + const installationTtl = [...ttlByUrl].find(([key]) => key.endsWith("/app/installations/123"))?.[1]; + const statusTtl = [...ttlByUrl].find(([key]) => key.includes("/commits/abc/status"))?.[1]; + expect(branchTtl).toBe(20 * 60); + expect(repoTtl).toBe(10 * 60); + expect(userTtl).toBe(10 * 60); + expect(installationTtl).toBe(10 * 60); + expect(statusTtl).toBeUndefined(); + expect([...ttlByUrl.keys()].some((key) => key.includes("/commits/abc/status"))).toBe(false); + }); + + it("bypasses live CI and mergeability decision endpoints instead of replaying stale Redis data", async () => { + const stale = { + status: 200, + body: JSON.stringify({ state: "stale" }), + contentType: "application/json", + }; + setGitHubResponseCache({ + get: async () => stale, + set: async () => undefined, + }); + const decisionCases = [ + { url: "https://api.github.com/repos/o/r/commits/abc/status?per_page=100&page=1", first: { state: "pending" }, second: { state: "success" } }, + { + url: "https://api.github.com/repos/o/r/commits/abc/check-runs?per_page=100&page=1", + first: { total_count: 1, check_runs: [{ status: "queued" }] }, + second: { total_count: 0, check_runs: [] }, + }, + { + url: "https://api.github.com/repos/o/r/commits/abc/check-suites?per_page=100", + first: { check_suites: [{ status: "in_progress" }] }, + second: { check_suites: [] }, + }, + { url: "https://api.github.com/repos/o/r/pulls/7", first: { mergeable_state: "unknown" }, second: { mergeable_state: "clean" } }, + { url: "https://api.github.com/repos/o/r/pulls/7/merge", first: { merged: false }, second: { merged: true } }, + { url: "https://api.github.com/repos/o/r/check-runs/99", first: { status: "queued" }, second: { status: "completed" } }, + { url: "https://api.github.com/repos/o/r/check-suites/99", first: { status: "in_progress" }, second: { status: "completed" } }, + ]; + const responses = new Map(decisionCases.map(({ url, first, second }) => [url, [first, second]])); + let getFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + getFetches += 1; + const url = String(input); + return Response.json(responses.get(url)?.shift() ?? { state: "unexpected" }); + }); + + for (const { url, first, second } of decisionCases) { + expect(isCacheableGithubUrl(url)).toBe(false); + expect(await (await timeoutFetch(url)).json()).toEqual(first); + expect(await (await timeoutFetch(url)).json()).toEqual(second); + } + + expect(getFetches).toBe(decisionCases.length * 2); + expect(await renderMetrics()).toContain(`gittensory_github_response_cache_total{class="sensitive",result="bypassed"} ${decisionCases.length * 2}`); + }); + + it("bypasses mutable PR and issue subresources instead of replaying stale coordination state", async () => { + const stale = { + status: 200, + body: JSON.stringify({ state: "stale" }), + contentType: "application/json", + }; + setGitHubResponseCache({ + get: async () => stale, + set: async () => undefined, + }); + const mutableCases = [ + { url: "https://api.github.com/repos/o/r/pulls/7/files?per_page=100&page=1", first: [{ filename: "old.ts" }], second: [{ filename: "new.ts" }] }, + { url: "https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100&page=1", first: [{ state: "COMMENTED" }], second: [{ state: "APPROVED" }] }, + { url: "https://api.github.com/repos/o/r/pulls/7/commits?per_page=100&page=1", first: [{ sha: "old" }], second: [{ sha: "new" }] }, + { url: "https://api.github.com/repos/o/r/pulls?state=open&per_page=100&page=1", first: [{ number: 7, head: { sha: "old" } }], second: [{ number: 7, head: { sha: "new" } }] }, + { url: "https://api.github.com/repos/o/r/issues/7/comments?per_page=100&page=1", first: [], second: [{ id: 1 }] }, + { url: "https://api.github.com/repos/o/r/issues/7/labels", first: [], second: [{ name: "ready" }] }, + { url: "https://api.github.com/repos/o/r/issues/7/events?per_page=100&page=1", first: [{ event: "labeled" }], second: [{ event: "closed" }] }, + ]; + const responses = new Map(mutableCases.map(({ url, first, second }) => [url, [first, second]])); + let getFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + getFetches += 1; + const url = String(input); + return Response.json(responses.get(url)?.shift() ?? { state: "unexpected" }); + }); + + for (const { url, first, second } of mutableCases) { + expect(isCacheableGithubUrl(url)).toBe(false); + expect(await (await timeoutFetch(url)).json()).toEqual(first); + expect(await (await timeoutFetch(url)).json()).toEqual(second); + } + + expect(getFetches).toBe(mutableCases.length * 2); + expect(await renderMetrics()).toContain(`gittensory_github_response_cache_total{class="sensitive",result="bypassed"} ${mutableCases.length * 2}`); + }); + + it("bypasses conditional GitHub GETs so validator headers keep shaping the live response", async () => { + const store = installMemoryResponseCache(); + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + getFetches += 1; + return Response.json({ fetches: getFetches }); + }); + + const url = "https://api.github.com/repos/o/r"; + const first = await timeoutFetch(url, { headers: { "if-none-match": '"cached-etag"' } }); + const second = await timeoutFetch(url, { headers: { "if-none-match": '"cached-etag"' } }); + + expect(await first.json()).toEqual({ fetches: 1 }); + expect(await second.json()).toEqual({ fetches: 2 }); + expect(store.size).toBe(0); + expect(await renderMetrics()).toContain('gittensory_github_response_cache_total{class="conditional",result="bypassed"} 2'); + }); + + it("normalizes Request inputs for GitHub cache detection and auth-aware keys", async () => { + const store = installMemoryResponseCache(); + let getFetches = 0; + vi.stubGlobal("fetch", async () => { + getFetches += 1; + return Response.json({ fetches: getFetches }); + }); + + const request = () => + new Request("https://api.github.com/repos/o/r", { + headers: { + authorization: "Bearer request-token", + accept: "application/vnd.github+json", + }, + }); + const first = await timeoutFetch(request()); + const second = await timeoutFetch(request()); + + expect(await first.json()).toEqual({ fetches: 1 }); + expect(await second.json()).toEqual({ fetches: 1 }); + expect(second.headers.get(GITHUB_RESPONSE_CACHE_REPLAY_HEADER)).toBe("hit"); + expect(getFetches).toBe(1); + expect([...store.keys()].some((key) => key.includes("request-token"))).toBe(false); + }); + + it("falls back to a fresh request when the in-flight GET cannot be replayed", async () => { + let cacheReads = 0; + let resolveBothCacheReads!: () => void; + const bothCacheReads = new Promise((resolve) => { + resolveBothCacheReads = resolve; + }); + setGitHubResponseCache({ + get: async () => { + cacheReads += 1; + if (cacheReads === 2) resolveBothCacheReads(); + return null; + }, + set: async () => undefined, + }); + let releaseFetch!: () => void; + const fetchGate = new Promise((resolve) => { + releaseFetch = resolve; + }); + let getFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (String(input).includes("/repos/o/r/branches/main/protection/required_status_checks")) { + getFetches += 1; + if (getFetches === 1) { + await fetchGate; + return new Response("temporary failure", { status: 500 }); + } + return Response.json({ contexts: ["after-fallback"] }); + } + return new Response("not found", { status: 404 }); + }); + + const octokit = makeInstallationOctokit(createTestEnv(), "tok"); + const first = octokit + .request("GET /repos/{owner}/{repo}/branches/{branch}/protection/required_status_checks", { owner: "o", repo: "r", branch: "main" }) + .catch((error: { status?: number }) => error.status); + const second = octokit.request("GET /repos/{owner}/{repo}/branches/{branch}/protection/required_status_checks", { owner: "o", repo: "r", branch: "main" }); + await bothCacheReads; + releaseFetch(); + + await expect(first).resolves.toBe(500); + await expect(second).resolves.toEqual(expect.objectContaining({ data: { contexts: ["after-fallback"] } })); + expect(getFetches).toBe(2); + }); + + it("also falls back when the shared in-flight GET leader throws before a response exists", async () => { + let cacheReads = 0; + let resolveBothCacheReads!: () => void; + const bothCacheReads = new Promise((resolve) => { + resolveBothCacheReads = resolve; + }); + setGitHubResponseCache({ + get: async () => { + cacheReads += 1; + if (cacheReads === 2) resolveBothCacheReads(); + return null; + }, + set: async () => undefined, + }); + let releaseFetch!: () => void; + const fetchGate = new Promise((resolve) => { + releaseFetch = resolve; + }); + let getFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (String(input).includes("/repos/o/r/branches/main/protection/required_status_checks")) { + getFetches += 1; + if (getFetches === 1) { + await fetchGate; + throw new Error("network down"); + } + return Response.json({ contexts: ["after-throw"] }); + } + return new Response("not found", { status: 404 }); + }); + + const octokit = makeInstallationOctokit(createTestEnv(), "tok"); + const first = octokit + .request("GET /repos/{owner}/{repo}/branches/{branch}/protection/required_status_checks", { owner: "o", repo: "r", branch: "main" }) + .catch((error: Error) => error.message); + const second = octokit.request("GET /repos/{owner}/{repo}/branches/{branch}/protection/required_status_checks", { owner: "o", repo: "r", branch: "main" }); + await bothCacheReads; + releaseFetch(); + + await expect(first).resolves.toContain("network down"); + await expect(second).resolves.toEqual(expect.objectContaining({ data: { contexts: ["after-throw"] } })); + expect(getFetches).toBe(2); + }); + + it("fails open when the shared response cache throws on read or write", async () => { + let cacheReads = 0; + let cacheWrites = 0; + setGitHubResponseCache({ + get: async () => { + cacheReads += 1; + throw new Error("redis read unavailable"); + }, + set: async () => { + cacheWrites += 1; + throw new Error("redis write unavailable"); + }, + }); + let getFetches = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (String(input) === "https://api.github.com/repos/o/r") { + getFetches += 1; + return Response.json({ number: 4 }); + } + return new Response("not found", { status: 404 }); + }); + + const response = await timeoutFetch("https://api.github.com/repos/o/r"); + + expect(await response.json()).toEqual({ number: 4 }); + expect(cacheReads).toBe(1); + expect(cacheWrites).toBe(1); + expect(getFetches).toBe(1); + expect(await renderMetrics()).toContain('gittensory_github_response_cache_total{class="metadata",result="error"} 2'); + }); + + it("counts bypassed non-GET, non-GitHub, and sensitive GitHub requests", async () => { + setGitHubResponseCache({ + get: async () => null, + set: async () => undefined, + }); + vi.stubGlobal("fetch", async () => new Response("ok")); + + await timeoutFetch("https://api.github.com/repos/o/r/issues", { method: "POST" }); + await timeoutFetch("https://example.test/health"); + await timeoutFetch("https://api.github.com/repos/o/r/collaborators/alice/permission"); + + const metrics = await renderMetrics(); + expect(metrics).toContain('gittensory_github_response_cache_total{class="non_get",result="bypassed"} 1'); + expect(metrics).toContain('gittensory_github_response_cache_total{class="non_github",result="bypassed"} 1'); + expect(metrics).toContain('gittensory_github_response_cache_total{class="sensitive",result="bypassed"} 1'); + }); +}); + +describe("isRateLimitedResponse", () => { + it("fails closed to non-rate-limited when the defensive cloned body read throws", async () => { + const response = { + status: 403, + headers: new Headers(), + clone: () => ({ + text: async () => { + throw new Error("body read failed"); + }, + }), + } as unknown as Response; + + await expect(isRateLimitedResponse(response)).resolves.toBe(false); + }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index c5631c1ab7..d9bdf5799b 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -3621,10 +3621,9 @@ describe("queue processors", () => { if (url.includes("/check-runs/971") && method === "PATCH") { const body = JSON.parse(String(init?.body ?? "{}")) as { status?: string; conclusion?: string; output?: { title?: string } }; patchBodies.push(body); - // First PATCH = the gate completion. A rate-limit 403 must propagate to the queue instead of being - // swallowed as nonfatal; the pending check remains in_progress while the queue backs off and retries. - if (patchBodies.length === 1) return new Response(JSON.stringify({ message: "You have exceeded a secondary rate limit" }), { status: 403 }); - return Response.json({ id: 971 }); + // Gate completion stays rate-limited through the inline retry budget. It must propagate to the queue instead + // of being swallowed as nonfatal; the pending check remains in_progress while the queue backs off and retries. + return new Response(JSON.stringify({ message: "You have exceeded a secondary rate limit" }), { status: 403, headers: { "retry-after": "0" } }); } return new Response("not found", { status: 404 }); }); @@ -3643,7 +3642,7 @@ describe("queue processors", () => { }), ).rejects.toThrow(/rate limit/i); - expect(patchBodies).toHaveLength(1); + expect(patchBodies).toHaveLength(4); // initial attempt + GITHUB_RATE_LIMIT_MAX_RETRIES (3) expect(patchBodies[0]?.status).toBe("completed"); }); diff --git a/test/unit/selfhost-grafana-dashboard.test.ts b/test/unit/selfhost-grafana-dashboard.test.ts index c1e7d14703..1fcbdac679 100644 --- a/test/unit/selfhost-grafana-dashboard.test.ts +++ b/test/unit/selfhost-grafana-dashboard.test.ts @@ -5,6 +5,8 @@ import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; type DashboardTarget = { + expr?: string; + legendFormat?: string; queryText?: string; rawQueryText?: string; }; @@ -20,6 +22,7 @@ type Dashboard = { const tmpRoots: string[] = []; const dashboardPath = join(process.cwd(), "grafana/dashboards/maintainer-reviews.json"); +const selfhostDashboardPath = join(process.cwd(), "grafana/dashboards/gittensory.json"); const timeFrom = "${__from:date:seconds}"; const timeTo = "${__to:date:seconds}"; @@ -32,8 +35,8 @@ const sqliteCliAvailable = (() => { } })(); -function readDashboard(): Dashboard { - return JSON.parse(readFileSync(dashboardPath, "utf8")) as Dashboard; +function readDashboard(path = dashboardPath): Dashboard { + return JSON.parse(readFileSync(path, "utf8")) as Dashboard; } function reviewTargets(dashboard = readDashboard()): DashboardTarget[] { @@ -69,6 +72,17 @@ afterEach(() => { for (const dir of tmpRoots.splice(0)) rmSync(dir, { force: true, recursive: true }); }); +describe("Gittensory Self-Host Grafana dashboard", () => { + it("surfaces the GitHub response cache Prometheus counters", () => { + const dashboard = readDashboard(selfhostDashboardPath); + const targets = dashboard.panels.flatMap((panel) => panel.targets ?? []); + + expect(targets.some((target) => target.expr === "sum by (result) (rate(gittensory_github_response_cache_total[5m]))")).toBe(true); + expect(targets.some((target) => target.expr === "sum by (class, result) (gittensory_github_response_cache_total)")).toBe(true); + expect(targets.some((target) => target.legendFormat === "{{class}} {{result}}")).toBe(true); + }); +}); + describe("maintainer Reviews & PRs Grafana dashboard", () => { it("binds every review_targets panel query to Grafana's selected time range", () => { const targets = reviewTargets(); diff --git a/test/unit/selfhost-redis-response-cache.test.ts b/test/unit/selfhost-redis-response-cache.test.ts index fa127e9e35..5796885345 100644 --- a/test/unit/selfhost-redis-response-cache.test.ts +++ b/test/unit/selfhost-redis-response-cache.test.ts @@ -38,15 +38,35 @@ describe("createRedisResponseCache (#perf GitHub GET cache)", () => { status: 200, body: '{"x":1}', contentType: "application/json", + link: '; rel="next"', + etag: '"abc123"', + lastModified: "Mon, 29 Jun 2026 20:00:00 GMT", }); expect(f.ttl()).toBe(30); expect(await cache.get(URL_A)).toEqual({ status: 200, body: '{"x":1}', contentType: "application/json", + link: '; rel="next"', + etag: '"abc123"', + lastModified: "Mon, 29 Jun 2026 20:00:00 GMT", }); }); + it("honors a per-entry TTL override from the shared GitHub client", async () => { + const f = fakeRedis(); + await createRedisResponseCache(f.redis, 30).set( + URL_A, + { + status: 200, + body: "{}", + contentType: "application/json", + }, + 600, + ); + expect(f.ttl()).toBe(600); + }); + it("floors the TTL at 1s", async () => { const f = fakeRedis(); await createRedisResponseCache(f.redis, 0).set(URL_A, { @@ -68,4 +88,37 @@ describe("createRedisResponseCache (#perf GitHub GET cache)", () => { f.store.set("gh:resp:" + URL_A, JSON.stringify({ status: "200", body: 1 })); expect(await createRedisResponseCache(f.redis, 20).get(URL_A)).toBeNull(); }); + + it("get returns null for non-200 cached responses", async () => { + const f = fakeRedis(); + f.store.set( + "gh:resp:" + URL_A, + JSON.stringify({ + status: 500, + body: "temporary failure", + contentType: "text/plain", + }), + ); + expect(await createRedisResponseCache(f.redis, 20).get(URL_A)).toBeNull(); + }); + + it("ignores malformed optional replay headers while keeping the valid cached response", async () => { + const f = fakeRedis(); + f.store.set( + "gh:resp:" + URL_A, + JSON.stringify({ + status: 200, + body: "{}", + contentType: "application/json", + link: 42, + etag: null, + lastModified: {}, + }), + ); + expect(await createRedisResponseCache(f.redis, 20).get(URL_A)).toEqual({ + status: 200, + body: "{}", + contentType: "application/json", + }); + }); });