From c356dc714a8c3e89b83b843321dbb2a7c9138126 Mon Sep 17 00:00:00 2001 From: galuis116 Date: Fri, 17 Jul 2026 14:36:57 -0400 Subject: [PATCH] fix(selfhost): fail open on a Redis write failure in the installation-token cache set() had no try/catch around its redis.set call, unlike get()'s explicit fail-open contract. The token is already successfully minted from GitHub before set() is called, and the caller (createInstallationToken) has no try/catch of its own, so a transient Redis write failure was turning an otherwise-successful mint into a hard failure instead of just costing one extra real mint next time. --- src/selfhost/redis-token-cache.ts | 21 ++++++++++----- test/unit/github-app.test.ts | 28 ++++++++++++++++++++ test/unit/selfhost-redis-token-cache.test.ts | 18 ++++++++++++- 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/src/selfhost/redis-token-cache.ts b/src/selfhost/redis-token-cache.ts index 035436d158..9680b7255e 100644 --- a/src/selfhost/redis-token-cache.ts +++ b/src/selfhost/redis-token-cache.ts @@ -64,12 +64,21 @@ export function createRedisTokenCache(redis: Redis): InstallationTokenStore { 1, Math.floor((value.expiresAtMs - Date.now()) / 1000), ); - await redis.set( - keyFor(installationId), - JSON.stringify(value), - "EX", - ttlSeconds, - ); + // Fail open on a connection error, same contract as get() above: the caller (github/app.ts's + // createInstallationToken, right after successfully minting a fresh token) has no try/catch of its own, + // so an uncaught error here would turn an otherwise-successful mint into a hard failure over a transient + // cache-write hiccup. The token was already obtained from GitHub before this call, so a write failure + // just costs one extra real mint next time -- never the caller's job to fail. + try { + await redis.set( + keyFor(installationId), + JSON.stringify(value), + "EX", + ttlSeconds, + ); + } catch { + recordTokenCacheMetric("error"); + } }, }; } diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 0f233d8a39..7d0ec4affa 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -28,6 +28,8 @@ import type { Advisory } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; import { getInstallation, listLatestGitHubRateLimitObservations, upsertInstallation } from "../../src/db/repositories"; import { clockSkewSecondsSample, resetClockSkewForTest } from "../../src/selfhost/clock-skew"; +import { createRedisTokenCache } from "../../src/selfhost/redis-token-cache"; +import type { Redis } from "ioredis"; beforeEach(() => { clearInstallationTokenCacheForTest(); @@ -2713,6 +2715,32 @@ describe("self-host Redis token store + GitHub GET response cache", () => { await expect(getAppInstallation(env, 99)).rejects.toThrow(); expect(store.size).toBe(0); // non-200 not cached }); + + it("REGRESSION (#6999): a Redis write failure never fails an otherwise-successful token mint", async () => { + // The real createRedisTokenCache implementation (not a hand-rolled store), wired to a Redis stand-in whose + // set() always throws -- pins that the fail-open fix actually reaches createInstallationToken's uncaught + // writeCachedToken call, not just the unit-level contract on redis-token-cache.ts's own set(). + const throwingRedis = { + get: async () => null, + set: async () => { + throw new Error("connection refused"); + }, + } as unknown as Redis; + setInstallationTokenStore(createRedisTokenCache(throwingRedis)); + const privateKey = await generatePrivateKeyPem(); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString().includes("/access_tokens")) { + return Response.json({ + token: "minted-despite-cache-failure", + expires_at: new Date(Date.now() + 60 * 60_000).toISOString(), + }); + } + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + await expect(createInstallationToken(env, 888)).resolves.toBe("minted-despite-cache-failure"); + }); }); describe("GitHub rate-limit handling (#ratelimit-resilience)", () => { diff --git a/test/unit/selfhost-redis-token-cache.test.ts b/test/unit/selfhost-redis-token-cache.test.ts index 43fb08cb2f..bed7a53af4 100644 --- a/test/unit/selfhost-redis-token-cache.test.ts +++ b/test/unit/selfhost-redis-token-cache.test.ts @@ -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(options: { getThrows?: boolean } = {}): { +function fakeRedis(options: { getThrows?: boolean; setThrows?: boolean } = {}): { redis: Redis; store: Map; ttl: () => number; @@ -17,6 +17,7 @@ function fakeRedis(options: { getThrows?: boolean } = {}): { return store.get(k) ?? null; }, async set(k: string, v: string, _ex: "EX", ttl: number) { + if (options.setThrows) throw new Error("connection refused"); store.set(k, v); lastTtl = ttl; return "OK"; @@ -110,4 +111,19 @@ describe("createRedisTokenCache (#perf installation-token persistence)", () => { 'loopover_redis_token_cache_total{result="error"} 1', ); }); + + it("regression: set() fails open (does not throw) and records an error metric on a Redis connection failure (#6999)", async () => { + // The token was already successfully minted from GitHub before set() is called (github/app.ts's + // createInstallationToken has no try/catch around this write), so a transient cache-write failure must + // never surface as a token-mint failure -- same fail-open contract as get()'s own regression test above. + const { redis, store } = fakeRedis({ setThrows: true }); + await expect( + createRedisTokenCache(redis).set(9, { token: "sensitive-value", expiresAtMs: Date.now() + 60_000 }), + ).resolves.toBeUndefined(); + + expect(store.has("gh:insttoken:9")).toBe(false); // the write never actually landed + const metrics = await renderMetrics(); + expect(metrics).toContain('loopover_redis_token_cache_total{result="error"} 1'); + expect(metrics).not.toContain("sensitive-value"); + }); });