From 54065266ea8cb573334e476aea24e082a529346a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 24 Jul 2026 14:53:04 -0700 Subject: [PATCH 1/2] perf(test): eliminate two more real-wait retry-backoff hotspots fetchReesPingWithRetry (REES /v1/ping 503 retry) and netuid-verification's fetchWithRetry both paid their real production backoff delay in tests that exercise the retry path, adding ~4s of pure wall-clock wait across the affected suites. Add the same settable-override pattern already used for the other suite-wide retry delays, defaulting to the real production value and only overridden to 0 in test/helpers/vitest-setup.ts. Also switch the two probeReesSecretAtStartup regression tests off a fixed 1100ms sleep-then-assert (needed because the probe is fire-and-forget) onto vi.waitFor, so they settle as soon as the now-fast retries actually finish instead of always paying the old worst-case wait. --- src/review/content-lane/netuid-verification.ts | 10 +++++++++- src/review/enrichment-wire.ts | 14 +++++++++++++- test/helpers/vitest-setup.ts | 4 ++++ test/unit/enrichment-wire.test.ts | 8 ++++---- 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/review/content-lane/netuid-verification.ts b/src/review/content-lane/netuid-verification.ts index 63bee12035..47d9a5c0ae 100644 --- a/src/review/content-lane/netuid-verification.ts +++ b/src/review/content-lane/netuid-verification.ts @@ -37,6 +37,14 @@ const RETRYABLE_STATUS = new Set([408, 425, 429, 500, 502, 503, 504]); const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); +let netuidRetryBaseDelayMsOverride: number | null = null; + +/** Test-only: collapses fetchWithRetry's exponential backoff to near-zero so a retry/exhaustion test + * doesn't pay real wall-clock time (the DEFAULT_BASE_DELAY_MS constant and production default are unchanged). */ +export function setNetuidRetryBaseDelayMsForTest(value: number | null): void { + netuidRetryBaseDelayMsOverride = value; +} + /** Minimal fetch-with-retry (inlined from reviewbot core/fetch-retry.ts defaults). Retries on a * thrown error or a retryable status, with exponential backoff + a per-attempt timeout. */ async function fetchWithRetry( @@ -46,7 +54,7 @@ async function fetchWithRetry( opts: { retries?: number; baseDelayMs?: number; timeoutMs?: number } = {}, ): Promise { const retries = opts.retries ?? DEFAULT_RETRIES; - const baseDelayMs = opts.baseDelayMs ?? DEFAULT_BASE_DELAY_MS; + const baseDelayMs = opts.baseDelayMs ?? netuidRetryBaseDelayMsOverride ?? DEFAULT_BASE_DELAY_MS; const timeoutMs = opts.timeoutMs ?? DEFAULT_TIMEOUT_MS; let lastError: unknown; for (let attempt = 0; attempt <= retries; attempt += 1) { diff --git a/src/review/enrichment-wire.ts b/src/review/enrichment-wire.ts index 5526d1f397..066ce7a74c 100644 --- a/src/review/enrichment-wire.ts +++ b/src/review/enrichment-wire.ts @@ -73,6 +73,18 @@ function sharedSecretWasNormalized( const REES_PING_NOT_READY_RETRIES = 2; const REES_PING_NOT_READY_RETRY_DELAY_MS = 500; +let reesPingNotReadyRetryDelayMsOverride: number | null = null; + +/** Test-only: collapses the real inter-retry wait so probeReesSecretAtStartup's retry tests don't pay + * REES_PING_NOT_READY_RETRIES * REES_PING_NOT_READY_RETRY_DELAY_MS of real wall-clock time. */ +export function setReesPingNotReadyRetryDelayMsForTest(value: number | null): void { + reesPingNotReadyRetryDelayMsOverride = value; +} + +function reesPingNotReadyRetryDelayMs(): number { + return reesPingNotReadyRetryDelayMsOverride ?? REES_PING_NOT_READY_RETRY_DELAY_MS; +} + async function fetchReesPingWithRetry(url: string, secret: string): Promise { const request = () => fetch(url, { @@ -85,7 +97,7 @@ async function fetchReesPingWithRetry(url: string, secret: string): Promise setTimeout(resolve, REES_PING_NOT_READY_RETRY_DELAY_MS)); + await new Promise((resolve) => setTimeout(resolve, reesPingNotReadyRetryDelayMs())); response = await request(); } return response; diff --git a/test/helpers/vitest-setup.ts b/test/helpers/vitest-setup.ts index 1223a0989a..5bf538265c 100644 --- a/test/helpers/vitest-setup.ts +++ b/test/helpers/vitest-setup.ts @@ -9,7 +9,11 @@ import { setReviewFilesEmptyRetryDelayMsForTest } from "../../src/github/backfill"; import { setGithubRateLimitRetrySleepCapMsForTest } from "../../src/github/client"; import { setMergeStateUnknownRetryDelayMsForTest } from "../../src/queue/ci-resolution"; +import { setReesPingNotReadyRetryDelayMsForTest } from "../../src/review/enrichment-wire"; +import { setNetuidRetryBaseDelayMsForTest } from "../../src/review/content-lane/netuid-verification"; setReviewFilesEmptyRetryDelayMsForTest(0); setGithubRateLimitRetrySleepCapMsForTest(0); setMergeStateUnknownRetryDelayMsForTest(0); +setReesPingNotReadyRetryDelayMsForTest(0); +setNetuidRetryBaseDelayMsForTest(0); diff --git a/test/unit/enrichment-wire.test.ts b/test/unit/enrichment-wire.test.ts index 23832654f9..0493462114 100644 --- a/test/unit/enrichment-wire.test.ts +++ b/test/unit/enrichment-wire.test.ts @@ -150,8 +150,9 @@ describe("probeReesSecretAtStartup", () => { 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 + // fetchReesPingWithRetry is fire-and-forget; poll instead of sleeping the retry budget's worst case + // (the test's own vitest-setup override collapses the real inter-retry delay to 0, so this settles fast). + await vi.waitFor(() => 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(); @@ -167,8 +168,7 @@ describe("probeReesSecretAtStartup", () => { 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); + await vi.waitFor(() => 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(); From d5dd1ccc6a6be73baa716a59eec26bb70039218a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 24 Jul 2026 15:00:29 -0700 Subject: [PATCH 2/2] perf(test): eliminate real ECONNRESET backoff sleeps in pg-queue tests retryPoolQuery's real 500ms-multiplier backoff ran for real in the three PG connection-resilience tests in selfhost-pg-queue.test.ts (only Date was faked, not setTimeout) -- ~6.5s of pure wall-clock wait across those three tests alone. Add the same settable-override pattern used elsewhere in this file's fixtures, scoped to this test file only (not the global vitest-setup default) since pg-queue.ts isn't otherwise imported broadly across the suite. Production default and behavior unchanged. --- src/selfhost/pg-queue.ts | 11 ++++++++++- test/unit/selfhost-pg-queue.test.ts | 7 ++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/selfhost/pg-queue.ts b/src/selfhost/pg-queue.ts index 19e284ad02..e74512c067 100644 --- a/src/selfhost/pg-queue.ts +++ b/src/selfhost/pg-queue.ts @@ -86,9 +86,18 @@ function isPgSqlStateConnectionError(err: unknown): boolean { return hasErrorCode(err, PG_SQLSTATE_CONNECTION_CODES); } +let pgRetryPoolQueryDelayMsOverride: number | null = null; + +/** Test-only: collapses retryPoolQuery's per-attempt backoff to near-zero so a connection-error retry + * test doesn't pay real wall-clock time (the delayMs default and production behavior are unchanged). */ +export function setPgRetryPoolQueryDelayMsForTest(value: number | null): void { + pgRetryPoolQueryDelayMsOverride = value; +} + /** Retry a pool query up to `retries` times on transient connection errors, with a short delay * between attempts. The pool will establish a new connection automatically. */ async function retryPoolQuery(fn: () => Promise, retries = 3, delayMs = 500): Promise { + const effectiveDelayMs = pgRetryPoolQueryDelayMsOverride ?? delayMs; let lastErr: unknown; for (let attempt = 0; attempt <= retries; attempt++) { try { @@ -96,7 +105,7 @@ async function retryPoolQuery(fn: () => Promise, retries = 3, delayMs = 50 } catch (err) { lastErr = err; if (!isPgConnectionError(err) || attempt === retries) throw err; - await new Promise((resolve) => setTimeout(resolve, delayMs * (attempt + 1))); + await new Promise((resolve) => setTimeout(resolve, effectiveDelayMs * (attempt + 1))); } } throw lastErr; diff --git a/test/unit/selfhost-pg-queue.test.ts b/test/unit/selfhost-pg-queue.test.ts index 8cb89c6bae..ae13dbd42f 100644 --- a/test/unit/selfhost-pg-queue.test.ts +++ b/test/unit/selfhost-pg-queue.test.ts @@ -2,7 +2,7 @@ // Real-Postgres integration paths (migrations, pg-adapter translation) live in test/integration/selfhost-pg.test.ts. import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { Pool, QueryResult } from "pg"; -import { createPgQueue } from "../../src/selfhost/pg-queue"; +import { createPgQueue, setPgRetryPoolQueryDelayMsForTest } from "../../src/selfhost/pg-queue"; import { queueSnapshotFromBinding } from "../../src/selfhost/queue-common"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { RetryableJobError } from "../../src/queue/retryable"; @@ -15,6 +15,11 @@ import type { JobMessage } from "../../src/types"; // "unavailable" (null, never gates) here; individual host-load tests override the mock explicitly. vi.mock("../../src/selfhost/host-pressure", () => ({ hostLoadAvg1PerCore: vi.fn(() => null) })); +// The PG connection-resilience tests below deliberately trigger retryPoolQuery's real ECONNRESET retry +// path; collapse its per-attempt backoff to near-zero so they don't pay real wall-clock time for it +// (the delayMs default and production behavior in src/selfhost/pg-queue.ts are unchanged). +setPgRetryPoolQueryDelayMsForTest(0); + const msg = (t: string): JobMessage => ({ type: t }) as unknown as JobMessage; const webhook = (sender: { login: string; type: string }, eventName = "issue_comment", action = "edited"): JobMessage => ({