Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/github/backfill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> => new Promise((resolve) => setTimeout(resolve, ms));

Expand Down Expand Up @@ -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) {
Expand Down
15 changes: 14 additions & 1 deletion src/github/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> => 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<void> =>
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
Expand Down
8 changes: 8 additions & 0 deletions test/helpers/d1.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
13 changes: 13 additions & 0 deletions test/helpers/vitest-setup.ts
Original file line number Diff line number Diff line change
@@ -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);
1 change: 1 addition & 0 deletions vitest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down