Skip to content

fix(selfhost): wire GITHUB_CACHE_TTL_SECONDS or remove the dead config #2573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 2, 2026
Merged

fix(selfhost): wire GITHUB_CACHE_TTL_SECONDS or remove the dead config #2573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion src/github/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ export interface CachedGitHubResponse {
}
export interface GitHubResponseCache {
get(key: string): Promise<CachedGitHubResponse | null>;
set(key: string, value: CachedGitHubResponse, ttlSeconds?: number): Promise<void>;
// 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<void>;
}
let responseCache: GitHubResponseCache | null = null;
export function setGitHubResponseCache(cache: GitHubResponseCache | null): void {
Expand Down
12 changes: 6 additions & 6 deletions src/selfhost/redis-response-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
Expand Down Expand Up @@ -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");
Expand Down
8 changes: 5 additions & 3 deletions src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,11 +442,13 @@ async function main(): Promise<void> {
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",
Expand All @@ -456,7 +458,7 @@ async function main(): Promise<void> {
JSON.stringify({
event: "selfhost_redis_ready",
backend: "redis",
githubResponseCacheTtl: ghCacheTtl,
githubResponseCacheEnabled: ghCacheTtl > 0,
}),
);

Expand Down
72 changes: 42 additions & 30 deletions test/unit/selfhost-redis-response-cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: '<https://github.com/ghapi/repos/o/r/pulls?page=2>; 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: '<https://github.com/ghapi/repos/o/r/pulls?page=2>; 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,
Expand All @@ -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"}',
Expand All @@ -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,
Expand All @@ -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',
);
Expand All @@ -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',
);
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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(
Expand All @@ -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',
Expand Down
Loading