From 0c2337f8e08d09c14d33beb9b667dc832f41d65a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 24 Jul 2026 13:24:27 -0700 Subject: [PATCH] perf(test): eliminate SQLite fsync and real rate-limit backoff wait in tests Profiling queue-lifecycle-guards.test.ts (33.7s) and backfill.test.ts (19.2s) found two more real per-test costs, confirmed via a CPU profile showing wall time going to idle (real I/O wait), not compute. SQLite fsyncs on every autocommit write by default; TestD1Database's clones are real temp files (needed for the template-clone trick from the earlier fix), so every app .prepare(...).run() paid a real fsync. journal_mode=MEMORY + synchronous=OFF are safe for a throwaway clone unlinked as soon as it's open -- crash-consistency durability is meaningless for data nothing outlives the test to read back. Benchmarked: 50 inserts, 13.9ms default vs 0.6ms tuned. fetchWithGitHubRetry's exponential backoff (500/1000/2000ms real setTimeout) is legitimate production behavior for a genuine 403/429, but several tests deliberately drive a fixture through the full retry loop to assert the resulting sync state, not the delay -- paying the full 3.5s every time. client.ts gains setGithubRateLimitRetrySleepCapMsForTest(), capping only what is actually awaited, not rateLimitRetryMs's return value -- its dedicated pure-function backoff-math test stays untouched and still exercises the real numbers. Both wired once, suite-wide, via a new test/helpers/vitest-setup.ts (vitest.config.ts setupFiles) -- same ...ForTest convention as clearInstallationTokenCacheForTest, applied automatically so every current AND future test with this shape benefits without opting in. queue-lifecycle-guards.test.ts: 33.7s -> 7.2s. backfill.test.ts: 19.2s -> 1.7s. --- src/github/backfill.ts | 16 +++++++++++++++- src/github/client.ts | 15 ++++++++++++++- test/helpers/d1.ts | 8 ++++++++ test/helpers/vitest-setup.ts | 13 +++++++++++++ vitest.config.ts | 1 + 5 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 test/helpers/vitest-setup.ts 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).