Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 12 additions & 2 deletions src/selfhost/redis-token-cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,24 @@ const REDIS_TOKEN_CACHE_METRIC = "loopover_redis_token_cache_total";
const keyFor = (installationId: number): string =>
`gh:insttoken:${installationId}`;

function recordTokenCacheMetric(result: "hit" | "miss"): void {
function recordTokenCacheMetric(result: "hit" | "miss" | "error"): void {
incr(REDIS_TOKEN_CACHE_METRIC, { result });
}

export function createRedisTokenCache(redis: Redis): InstallationTokenStore {
return {
async get(installationId: number) {
const raw = await redis.get(keyFor(installationId));
// Fail open on a connection error, same contract as redis-cache.ts's webhook-dedup cache: the caller
// (github/app.ts's readCachedToken -> createInstallationToken) has no try/catch of its own, so an
// uncaught error here would hard-fail GitHub App token minting on every Redis hiccup instead of just
// costing one extra real mint. Unlike redis-cache.ts, still record a metric so the failure isn't invisible.
let raw: string | null;
try {
raw = await redis.get(keyFor(installationId));
} catch {
recordTokenCacheMetric("error");
return null;
}
if (!raw) {
recordTokenCacheMetric("miss");
return null;
Expand Down
16 changes: 15 additions & 1 deletion test/unit/selfhost-redis-token-cache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics";
import { createRedisTokenCache } from "../../src/selfhost/redis-token-cache";

/** Minimal ioredis stand-in that records the TTL passed to set(). */
function fakeRedis(): {
function fakeRedis(options: { getThrows?: boolean } = {}): {
redis: Redis;
store: Map<string, string>;
ttl: () => number;
Expand All @@ -13,6 +13,7 @@ function fakeRedis(): {
let lastTtl = -1;
const redis = {
async get(k: string) {
if (options.getThrows) throw new Error("connection refused");
return store.get(k) ?? null;
},
async set(k: string, v: string, _ex: "EX", ttl: number) {
Expand Down Expand Up @@ -96,4 +97,17 @@ describe("createRedisTokenCache (#perf installation-token persistence)", () => {
'loopover_redis_token_cache_total{result="miss"} 1',
);
});

it("regression: fails open (returns null) and records an error metric on a Redis connection failure (#6288)", async () => {
// Unlike a cache miss/malformed value, a connection failure must never throw uncaught here: the caller
// (github/app.ts's readCachedToken -> createInstallationToken) has no try/catch of its own, so an uncaught
// rejection would hard-fail GitHub App token minting on every Redis hiccup instead of costing one extra
// real mint. This must still be observable, unlike redis-cache.ts's silent fail-open.
const { redis } = fakeRedis({ getThrows: true });
await expect(createRedisTokenCache(redis).get(9)).resolves.toBeNull();

expect(await renderMetrics()).toContain(
'loopover_redis_token_cache_total{result="error"} 1',
);
});
});
Loading