From 0335605363e36abf447c6cf62f0353e7948ae716 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 02:18:59 -0700 Subject: [PATCH] fix(selfhost): remove the dead GITHUB_CACHE_TTL_SECONDS default fallback GITHUB_CACHE_TTL_SECONDS (threaded into createRedisResponseCache as the fallback ttlSeconds) was dead beyond its >0/=0 enable/disable gate: every real call site into GitHubResponseCache.set() (client.ts, graphql-cache.ts) already resolves its own per-class TTL env var (GITHUB_BRANCH_PROTECTION_CACHE_TTL_SECONDS, GITHUB_COMMIT_CACHE_TTL_SECONDS, GITHUB_METADATA_CACHE_TTL_SECONDS, GITHUB_GRAPHQL_CACHE_TTL_SECONDS) and always passes it explicitly, so ttlOverrideSeconds ?? ttlSeconds never fell through to the configured value. An operator setting GITHUB_CACHE_TTL_SECONDS=5 or =120 (per .env.example's own "short default Redis TTL" documentation) got no change in actual cache freshness -- only the gate had any effect. Chose removal over wiring it in as an actual fallback default: the per-class TTLs default to 10-20 minutes, but docker-compose.yml already sets GITHUB_CACHE_TTL_SECONDS=20 unconditionally in every self-host deployment's environment -- wiring it in as a fallback would silently collapse every existing deployment's caching from 10-20 minutes down to 20 seconds on upgrade, a surprising regression for a "fix the docs" change. Make ttlSeconds a required parameter on GitHubResponseCache.set() and createRedisResponseCache() instead of an optional one with a dead fallback, and correct the misleading .env.example/docker-compose.yml/ server.ts documentation to describe the actual enable/disable-gate behavior. --- .env.example | 5 +- docker-compose.yml | 1 + src/github/client.ts | 4 +- src/selfhost/redis-response-cache.ts | 12 ++-- src/server.ts | 8 ++- .../selfhost-redis-response-cache.test.ts | 72 +++++++++++-------- 6 files changed, 61 insertions(+), 41 deletions(-) diff --git a/.env.example b/.env.example index 79106403f6..e63a609dab 100644 --- a/.env.example +++ b/.env.example @@ -170,7 +170,10 @@ 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. +# GITHUB_CACHE_TTL_SECONDS=20 # Enables the Redis-backed GitHub GET-response cache when >0; set 0 +# # to disable. NOT a per-entry TTL duration — each cached class +# # resolves its own TTL below (GITHUB_BRANCH_PROTECTION_CACHE_TTL_SECONDS +# # etc.); the numeric value here is otherwise unused. # GITHUB_BRANCH_PROTECTION_CACHE_TTL_SECONDS=1200 # TTL for required-status branch protection reads. # GITHUB_METADATA_CACHE_TTL_SECONDS=600 # TTL for stable repo/user/installation metadata reads. # GITHUB_COMMIT_CACHE_TTL_SECONDS=900 # TTL for bare /commits/{ref} resolves; dedups the two hourly upstream ref→SHA reads. diff --git a/docker-compose.yml b/docker-compose.yml index b3ebd56f22..32fa0790a3 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}" + # Enables the GitHub GET-response cache when >0 (0 disables); not a per-entry TTL, see .env.example. GITHUB_CACHE_TTL_SECONDS: "${GITHUB_CACHE_TTL_SECONDS:-20}" # Uncomment for Qdrant RAG vector store (--profile qdrant): # QDRANT_URL: http://qdrant:6333 diff --git a/src/github/client.ts b/src/github/client.ts index 06ef5116fa..a4922816a9 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -42,7 +42,9 @@ export interface CachedGitHubResponse { } export interface GitHubResponseCache { get(key: string): Promise; - set(key: string, value: CachedGitHubResponse, ttlSeconds?: number): Promise; + // Required: every real call site resolves a per-class TTL (githubResponseCacheTtlSeconds / + // githubGraphQlCacheTtlSeconds) before calling set(), so there is no sensible cache-wide fallback (#2505). + set(key: string, value: CachedGitHubResponse, ttlSeconds: number): Promise; } let responseCache: GitHubResponseCache | null = null; export function setGitHubResponseCache(cache: GitHubResponseCache | null): void { diff --git a/src/selfhost/redis-response-cache.ts b/src/selfhost/redis-response-cache.ts index f1b772de1d..9e07e685ad 100644 --- a/src/selfhost/redis-response-cache.ts +++ b/src/selfhost/redis-response-cache.ts @@ -4,6 +4,9 @@ // 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). +// GITHUB_CACHE_TTL_SECONDS only gates whether this cache is constructed at all (server.ts: >0 enables, 0 +// disables) -- it is NOT a per-entry default. Every real caller (client.ts, graphql-cache.ts) resolves its own +// per-class TTL env var before calling set(), so set() takes the TTL as a required argument (#2505). import type { Redis } from "ioredis"; import type { CachedGitHubResponse, GitHubResponseCache } from "../github/client"; import { incr } from "./metrics"; @@ -19,10 +22,7 @@ function recordRedisResponseCacheMetric(result: "hit" | "miss" | "set" | "error" incr(REDIS_GITHUB_RESPONSE_CACHE_METRIC, { result }); } -export function createRedisResponseCache( - redis: Redis, - ttlSeconds: number, -): GitHubResponseCache { +export function createRedisResponseCache(redis: Redis): GitHubResponseCache { return { async get(key: string) { let raw: string | null; @@ -58,13 +58,13 @@ export function createRedisResponseCache( return null; } }, - async set(key: string, value: CachedGitHubResponse, ttlOverrideSeconds?: number) { + async set(key: string, value: CachedGitHubResponse, ttlSeconds: number) { try { await redis.set( keyFor(key), JSON.stringify(value), "EX", - Math.max(1, ttlOverrideSeconds ?? ttlSeconds), + Math.max(1, ttlSeconds), ); } catch (error) { recordRedisResponseCacheMetric("error"); diff --git a/src/server.ts b/src/server.ts index 7aa8422e90..83c9a4d17c 100644 --- a/src/server.ts +++ b/src/server.ts @@ -442,11 +442,13 @@ async function main(): Promise { const { createRedisTokenCache } = await import("./selfhost/redis-token-cache"); const { setInstallationTokenStore, setGitHubResponseCache } = await import("./github/app"); setInstallationTokenStore(createRedisTokenCache(redisClient)); - // Short-TTL cache for safe GitHub GET responses (dedups the ~24 reads per review). Default 20s; 0 disables. + // Enable/disable gate for the GitHub GET-response cache (dedups the ~24 reads per review); NOT a per-entry + // TTL — each cached class (branch-protection/metadata/commit/GraphQL) resolves its own TTL env var, so the + // value here only matters as >0 (enabled) vs 0 (disabled) (#2505). const ghCacheTtl = Math.max(0, Number(process.env.GITHUB_CACHE_TTL_SECONDS ?? "20")); if (ghCacheTtl > 0) { const { createRedisResponseCache } = await import("./selfhost/redis-response-cache"); - setGitHubResponseCache(createRedisResponseCache(redisClient, ghCacheTtl)); + setGitHubResponseCache(createRedisResponseCache(redisClient)); } readinessProbes.push({ name: "redis", @@ -456,7 +458,7 @@ async function main(): Promise { JSON.stringify({ event: "selfhost_redis_ready", backend: "redis", - githubResponseCacheTtl: ghCacheTtl, + githubResponseCacheEnabled: ghCacheTtl > 0, }), ); diff --git a/test/unit/selfhost-redis-response-cache.test.ts b/test/unit/selfhost-redis-response-cache.test.ts index 2e008c7616..047e1db7e6 100644 --- a/test/unit/selfhost-redis-response-cache.test.ts +++ b/test/unit/selfhost-redis-response-cache.test.ts @@ -30,24 +30,28 @@ afterEach(() => resetMetrics()); describe("createRedisResponseCache (#perf GitHub GET cache)", () => { it("get returns null for a missing url", async () => { expect( - await createRedisResponseCache(fakeRedis().redis, 20).get(URL_A), + await createRedisResponseCache(fakeRedis().redis).get(URL_A), ).toBeNull(); expect(await renderMetrics()).toContain( 'gittensory_redis_gh_response_cache_total{result="miss"} 1', ); }); - it("set then get round-trips status/body/content-type with the configured TTL", async () => { + it("set then get round-trips status/body/content-type at the caller-supplied TTL", async () => { const f = fakeRedis(); - const cache = createRedisResponseCache(f.redis, 30); - await cache.set(URL_A, { - status: 200, - body: '{"x":1}', - contentType: "application/json", - link: '; rel="next"', - etag: '"abc123"', - lastModified: "Mon, 29 Jun 2026 20:00:00 GMT", - }); + const cache = createRedisResponseCache(f.redis); + await cache.set( + URL_A, + { + status: 200, + body: '{"x":1}', + contentType: "application/json", + link: '; rel="next"', + etag: '"abc123"', + lastModified: "Mon, 29 Jun 2026 20:00:00 GMT", + }, + 30, + ); expect(f.ttl()).toBe(30); expect(await cache.get(URL_A)).toEqual({ status: 200, @@ -68,7 +72,7 @@ describe("createRedisResponseCache (#perf GitHub GET cache)", () => { it("replays cached branch-protection permission denials and missing resources", async () => { const f = fakeRedis(); - const cache = createRedisResponseCache(f.redis, 30); + const cache = createRedisResponseCache(f.redis); const forbidden = { status: 403, body: '{"message":"Resource not accessible by integration"}', @@ -90,9 +94,9 @@ describe("createRedisResponseCache (#perf GitHub GET cache)", () => { expect(await cache.get("branch-protection-missing")).toEqual(missing); }); - it("honors a per-entry TTL override from the shared GitHub client", async () => { + it("uses the caller-supplied per-entry TTL from the shared GitHub client", async () => { const f = fakeRedis(); - await createRedisResponseCache(f.redis, 30).set( + await createRedisResponseCache(f.redis).set( URL_A, { status: 200, @@ -106,18 +110,22 @@ describe("createRedisResponseCache (#perf GitHub GET cache)", () => { it("floors the TTL at 1s", async () => { const f = fakeRedis(); - await createRedisResponseCache(f.redis, 0).set(URL_A, { - status: 200, - body: "{}", - contentType: "application/json", - }); + await createRedisResponseCache(f.redis).set( + URL_A, + { + status: 200, + body: "{}", + contentType: "application/json", + }, + 0, + ); expect(f.ttl()).toBe(1); }); it("get returns null on malformed JSON", async () => { const f = fakeRedis(); f.store.set("gh:resp:" + URL_A, "{nope"); - expect(await createRedisResponseCache(f.redis, 20).get(URL_A)).toBeNull(); + expect(await createRedisResponseCache(f.redis).get(URL_A)).toBeNull(); expect(await renderMetrics()).toContain( 'gittensory_redis_gh_response_cache_total{result="miss"} 1', ); @@ -126,7 +134,7 @@ describe("createRedisResponseCache (#perf GitHub GET cache)", () => { it("get returns null when the stored shape is wrong", async () => { const f = fakeRedis(); f.store.set("gh:resp:" + URL_A, JSON.stringify({ status: "200", body: 1 })); - expect(await createRedisResponseCache(f.redis, 20).get(URL_A)).toBeNull(); + expect(await createRedisResponseCache(f.redis).get(URL_A)).toBeNull(); expect(await renderMetrics()).toContain( 'gittensory_redis_gh_response_cache_total{result="miss"} 1', ); @@ -142,12 +150,12 @@ describe("createRedisResponseCache (#perf GitHub GET cache)", () => { contentType: "text/plain", }), ); - expect(await createRedisResponseCache(f.redis, 20).get(URL_A)).toBeNull(); + expect(await createRedisResponseCache(f.redis).get(URL_A)).toBeNull(); }); it("get returns null for malformed replayable status values", async () => { const f = fakeRedis(); - const cache = createRedisResponseCache(f.redis, 20); + const cache = createRedisResponseCache(f.redis); f.store.set( "gh:resp:string-status", @@ -192,7 +200,7 @@ describe("createRedisResponseCache (#perf GitHub GET cache)", () => { lastModified: {}, }), ); - expect(await createRedisResponseCache(f.redis, 20).get(URL_A)).toEqual({ + expect(await createRedisResponseCache(f.redis).get(URL_A)).toEqual({ status: 200, body: "{}", contentType: "application/json", @@ -209,7 +217,7 @@ describe("createRedisResponseCache (#perf GitHub GET cache)", () => { }, } as unknown as Redis; - await expect(createRedisResponseCache(redis, 20).get(URL_A)).rejects.toThrow( + await expect(createRedisResponseCache(redis).get(URL_A)).rejects.toThrow( "redis read failed", ); expect(await renderMetrics()).toContain( @@ -225,11 +233,15 @@ describe("createRedisResponseCache (#perf GitHub GET cache)", () => { } as unknown as Redis; await expect( - createRedisResponseCache(redis, 20).set(URL_A, { - status: 200, - body: "{}", - contentType: "application/json", - }), + createRedisResponseCache(redis).set( + URL_A, + { + status: 200, + body: "{}", + contentType: "application/json", + }, + 20, + ), ).rejects.toThrow("redis write failed"); expect(await renderMetrics()).toContain( 'gittensory_redis_gh_response_cache_total{result="error"} 1',