From 942b74f2b8c53fe102da464cc9be54ba06cbdf7c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 11 Jul 2026 03:24:37 -0700 Subject: [PATCH] fix(review): retry a 503 REES startup ping before escalating (#5006) REES's own /v1/ping returns 503 specifically to mean "not configured/ ready yet" (server.ts checks its own REES_SHARED_SECRET before anything else) -- the same benign startup-ordering race the engine's probe already extends grace to for a refused connection, just via an HTTP response instead of a connection failure. All 7 GITTENSORY-1J events clustered in one ~5h window and never recurred, consistent with a one-time deploy/restart race rather than a persistent misconfiguration. Retry a 503 up to twice (500ms apart) before logging rees_ping_error; any other status is still final immediately, and a persistent 503 still escalates after the retries exhaust. --- src/review/enrichment-wire.ts | 38 ++++++++++++++++++++--------- test/unit/enrichment-wire.test.ts | 40 +++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/src/review/enrichment-wire.ts b/src/review/enrichment-wire.ts index 103d801b1f..0de1e833f7 100644 --- a/src/review/enrichment-wire.ts +++ b/src/review/enrichment-wire.ts @@ -53,6 +53,32 @@ function sharedSecretWasNormalized( return (normalized ?? "") !== raw; } +// REES's own /v1/ping returns 503 specifically to mean "not configured/ready yet" (server.ts: no +// REES_SHARED_SECRET set on that side) -- the same benign startup-ordering race probeReesSecretAtStartup's +// catch block already extends grace to for a refused connection (GITTENSORY-1J: 7 Sentry events, all in one +// ~5h window, never recurring -- consistent with a one-time deploy/restart race, not a persistent +// misconfiguration). Retry a few times before escalating; any other status is final on the first response. +const REES_PING_NOT_READY_RETRIES = 2; +const REES_PING_NOT_READY_RETRY_DELAY_MS = 500; + +async function fetchReesPingWithRetry(url: string, secret: string): Promise { + const request = () => + fetch(url, { + method: "POST", + headers: { + "user-agent": "gittensory-selfhost/1.0", + authorization: `Bearer ${secret}`, + }, + signal: AbortSignal.timeout(5000), + }); + let response = await request(); + for (let attempt = 0; attempt < REES_PING_NOT_READY_RETRIES && response.status === 503; attempt += 1) { + await new Promise((resolve) => setTimeout(resolve, REES_PING_NOT_READY_RETRY_DELAY_MS)); + response = await request(); + } + return response; +} + // Set true once the startup probe confirms REES rejects the shared secret (401/403). Once set, // buildReviewEnrichment skips every /v1/enrich call for the rest of this process's lifetime instead of // repeating a call that's confirmed to fail on every PR review, each one logging review_context_fetch_failed. @@ -104,17 +130,7 @@ export function probeReesSecretAtStartup(env: Env): void { // Probe asynchronously — never block the server from starting. void (async () => { try { - const response = await fetch( - `${base.replace(/\/+$/, "")}/v1/ping`, - { - method: "POST", - headers: { - "user-agent": "gittensory-selfhost/1.0", - authorization: `Bearer ${sharedSecret}`, - }, - signal: AbortSignal.timeout(5000), - }, - ); + const response = await fetchReesPingWithRetry(`${base.replace(/\/+$/, "")}/v1/ping`, sharedSecret); if (response.ok) { console.log( JSON.stringify({ diff --git a/test/unit/enrichment-wire.test.ts b/test/unit/enrichment-wire.test.ts index 0aad2c1b66..36cc381c49 100644 --- a/test/unit/enrichment-wire.test.ts +++ b/test/unit/enrichment-wire.test.ts @@ -144,6 +144,46 @@ describe("probeReesSecretAtStartup", () => { errSpy.mockRestore(); }); + it("REGRESSION (#5006, GITTENSORY-1J): retries a 503 ('not ready yet') a few times before escalating to rees_ping_error", async () => { + const fetchSpy = vi.fn(async () => ({ ok: false, status: 503 }) as Response); + globalThis.fetch = fetchSpy as unknown as typeof fetch; + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + probeReesSecretAtStartup(env({ REES_URL: "https://rees.example", REES_SHARED_SECRET: "s3cret" })); + await new Promise((resolve) => setTimeout(resolve, 1100)); + expect(fetchSpy).toHaveBeenCalledTimes(3); // the first attempt + 2 retries, all still 503 + const parsed = errSpy.mock.calls.map((c) => JSON.parse(c[0] as string)); + expect(parsed.some((p) => p.event === "rees_ping_error" && p.status === 503)).toBe(true); + errSpy.mockRestore(); + }); + + it("REGRESSION (#5006): a 503 that clears on retry succeeds without ever escalating to rees_ping_error", async () => { + let calls = 0; + const fetchSpy = vi.fn(async () => { + calls += 1; + return (calls < 2 ? { ok: false, status: 503 } : { ok: true }) as Response; + }); + globalThis.fetch = fetchSpy as unknown as typeof fetch; + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + probeReesSecretAtStartup(env({ REES_URL: "https://rees.example", REES_SHARED_SECRET: "s3cret" })); + await new Promise((resolve) => setTimeout(resolve, 1100)); + expect(fetchSpy).toHaveBeenCalledTimes(2); + expect(logSpy.mock.calls.some((c) => JSON.parse(c[0] as string).event === "rees_ping_ok")).toBe(true); + expect(errSpy).not.toHaveBeenCalled(); + logSpy.mockRestore(); + errSpy.mockRestore(); + }); + + it("does not retry a non-503 non-ok status — a single attempt is final", async () => { + const fetchSpy = vi.fn(async () => ({ ok: false, status: 500 }) as Response); + globalThis.fetch = fetchSpy as unknown as typeof fetch; + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + probeReesSecretAtStartup(env({ REES_URL: "https://rees.example", REES_SHARED_SECRET: "s3cret" })); + await flush(); + expect(fetchSpy).toHaveBeenCalledTimes(1); + errSpy.mockRestore(); + }); + it("warns rees_ping_error (not throw) when the fetch itself rejects — REES may not be up yet", async () => { const fetchSpy = vi.fn(async () => { throw new Error("connect ECONNREFUSED");