diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 6a041e950c..5a57c54b34 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2424,6 +2424,20 @@ function toPullRequestFileRecordFromGitHub(repoFullName: string, pullNumber: num // manualReview alone — it might be a human's own hold, not just a stale bot one) — so a guardrail-configured // repo's PR could otherwise sit "held for manual review" indefinitely over nothing but a fetch timing gap. const REVIEW_FILES_EMPTY_RETRY_DELAY_MS = 500; +// Test-only override (#test-hotspots): the dedicated retry tests (backfill-2.test.ts) pin retry BEHAVIOR +// (attempt count, which response wins), never the wall-clock delay itself. Any OTHER test whose fetch stub +// happens to return an empty files array (a common, often-incidental stub shape) pays this real 500ms sleep +// unknowingly — confirmed as a major contributor to queue-lifecycle-guards.test.ts's slowest tests. Same +// `...ForTest` convention as clearInstallationTokenCacheForTest / setMaxChunksPerRepoForTest; the whole +// suite defaults it near-zero via test/helpers/vitest-setup.ts (vitest.config.ts's setupFiles), so this +// fires for every current AND future test with this shape, not just ones an author remembers to opt in. +let reviewFilesEmptyRetryDelayMsOverride: number | null = null; +export function reviewFilesEmptyRetryDelayMs(): number { + return reviewFilesEmptyRetryDelayMsOverride ?? REVIEW_FILES_EMPTY_RETRY_DELAY_MS; +} +export function setReviewFilesEmptyRetryDelayMsForTest(value: number | null): void { + reviewFilesEmptyRetryDelayMsOverride = value; +} const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); @@ -2457,7 +2471,7 @@ export async function fetchAndStorePullRequestFilesForReview( const fetchOnce = () => fetchPullRequestFiles(env, repoFullName, pullNumber, token, warnings, admissionKey, "live_review").catch(() => [] as GitHubFilePayload[]); let files = await fetchOnce(); if (files.length === 0) { - await sleep(REVIEW_FILES_EMPTY_RETRY_DELAY_MS); + await sleep(reviewFilesEmptyRetryDelayMs()); files = await fetchOnce(); } if (warnings.length > 0) { diff --git a/src/github/client.ts b/src/github/client.ts index f5014628fa..75fe561124 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -389,7 +389,20 @@ export function isGitHubResponseCacheReplay(response: Response): boolean { const GITHUB_RATE_LIMIT_MAX_RETRIES = 3; const GITHUB_RATE_LIMIT_MAX_DELAY_MS = 8_000; -const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); +// Test-only override (#test-hotspots): several tests deliberately drive fetchWithGitHubRetry through its +// FULL rate-limit backoff (up to 500+1000+2000=3500ms real wall time) to assert the resulting sync state +// (rate_limited status, capped segments) -- not the delay itself. This caps what's actually AWAITED +// without touching rateLimitRetryMs's own return value, so its dedicated pure-function correctness test +// (github-app.test.ts, asserting exact backoff math) stays exercising the real numbers unmodified. Same +// `...ForTest` convention as reviewFilesEmptyRetryDelayMs; the suite defaults it near-zero suite-wide via +// test/helpers/vitest-setup.ts. +let githubRateLimitRetrySleepCapMsOverride: number | null = null; +export function setGithubRateLimitRetrySleepCapMsForTest(value: number | null): void { + githubRateLimitRetrySleepCapMsOverride = value; +} + +const sleep = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, githubRateLimitRetrySleepCapMsOverride ?? ms)); /** Does this GitHub response signal a rate limit (primary or secondary)? 403/429 with a Retry-After header, an * exhausted x-ratelimit-remaining, or a secondary-limit/abuse body. A 403 with NONE of these is a real diff --git a/test/helpers/d1.ts b/test/helpers/d1.ts index 778817fb3e..80cdd9ae03 100644 --- a/test/helpers/d1.ts +++ b/test/helpers/d1.ts @@ -86,6 +86,14 @@ export class TestD1Database { ); copyFileSync(getMigratedTemplatePath(), clonePath); this.db = new DatabaseSync(clonePath); + // #test-hotspots: SQLite's defaults (journal_mode=DELETE, synchronous=FULL) fsync on every + // autocommit write -- each of the app's `.prepare(...).run()` calls is its own implicit + // transaction, so a write-heavy test (audit events, PR upserts, gate results) pays a real disk + // fsync per statement. Under concurrent CI/local load this dominated wall time far more than a + // quiet-machine benchmark suggests (queue-lifecycle-guards.test.ts: ~1000ms on several tests). + // Both are safe to disable for a throwaway per-test clone that's unlinked as soon as it's open: + // crash-consistency durability is meaningless for data nothing outlives the test to read back. + this.db.exec("PRAGMA journal_mode = MEMORY; PRAGMA synchronous = OFF;"); state.clonePaths.push(clonePath); if (!state.exitSweepRegistered) { state.exitSweepRegistered = true; diff --git a/test/helpers/vitest-setup.ts b/test/helpers/vitest-setup.ts new file mode 100644 index 0000000000..766732677f --- /dev/null +++ b/test/helpers/vitest-setup.ts @@ -0,0 +1,13 @@ +// Runs once per test file (Vitest's setupFiles, distinct from globalSetup which runs once for the +// whole run in the main process — this needs to touch each file's own module registry). Defaults +// production retry/backoff delays that exist for real network-timing reasons to near-zero so a test +// whose stub incidentally triggers one (e.g. an empty-files fetch stub tripping +// fetchAndStorePullRequestFilesForReview's empty-retry) doesn't pay real wall-clock time for it +// (#test-hotspots). The delay CONSTANT and its production default are untouched — this only sets the +// `...ForTest` override every such helper already exposes; a dedicated test asserting the retry's own +// behavior (attempt count, precedence) still exercises the identical code path, just without the sleep. +import { setReviewFilesEmptyRetryDelayMsForTest } from "../../src/github/backfill"; +import { setGithubRateLimitRetrySleepCapMsForTest } from "../../src/github/client"; + +setReviewFilesEmptyRetryDelayMsForTest(0); +setGithubRateLimitRetrySleepCapMsForTest(0); diff --git a/vitest.config.ts b/vitest.config.ts index 79e280f5fe..a42547bfca 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -17,6 +17,7 @@ export default defineConfig({ globals: true, testTimeout: 15000, globalSetup: ["./test/helpers/vitest-global-setup-node-version.ts"], + setupFiles: ["./test/helpers/vitest-setup.ts"], // Retry a failed test once before failing the run. The loopover gate auto-CLOSES a contributor PR // on a red required CI, so a single transient flake must not kill an honest PR; a deterministic // failure still fails both attempts (and vitest flags the retried test as flaky so it stays visible).