From ac9ee482d2b4cd8f472065d6f9b57db35cec560e Mon Sep 17 00:00:00 2001 From: Ben Senescu <44480372+bensenescu@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:16:44 -0400 Subject: [PATCH] =?UTF-8?q?Revert=20"fix(audit):=20back=20off=20and=20retr?= =?UTF-8?q?y=20on=20429=20instead=20of=20recording=20the=20page=20a?= =?UTF-8?q?=E2=80=A6"=20(#564)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit bb099ad65ae9ac50a6d900c5f61bd0a942661333. --- src/server/lib/audit/crawl-throttle.test.ts | 42 ---------- src/server/lib/audit/crawl-throttle.ts | 58 -------------- src/server/lib/audit/types.ts | 5 -- .../workflows/site-audit-workflow-helpers.ts | 7 -- .../workflows/siteAuditWorkflowCrawl.ts | 79 ++++--------------- src/shared/audit-issues.ts | 2 +- 6 files changed, 16 insertions(+), 177 deletions(-) delete mode 100644 src/server/lib/audit/crawl-throttle.test.ts delete mode 100644 src/server/lib/audit/crawl-throttle.ts diff --git a/src/server/lib/audit/crawl-throttle.test.ts b/src/server/lib/audit/crawl-throttle.test.ts deleted file mode 100644 index 120ea39..0000000 --- a/src/server/lib/audit/crawl-throttle.test.ts +++ /dev/null @@ -1,42 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { - backOff, - IDLE_THROTTLE, - parseRetryAfterMs, -} from "@/server/lib/audit/crawl-throttle"; - -describe("parseRetryAfterMs", () => { - it("reads delay-seconds and HTTP-dates, ignores garbage", () => { - expect(parseRetryAfterMs("5")).toBe(5_000); - const inTenSeconds = new Date(Date.now() + 10_000).toUTCString(); - const ms = parseRetryAfterMs(inTenSeconds); - expect(ms).toBeGreaterThan(8_000); - expect(ms).toBeLessThanOrEqual(10_000); - expect(parseRetryAfterMs("soon")).toBeUndefined(); - expect(parseRetryAfterMs(null)).toBeUndefined(); - }); -}); - -describe("backOff", () => { - it("pauses for Retry-After within bounds and starts spacing launches", () => { - expect(backOff(IDLE_THROTTLE, 5_000, 1_000)).toEqual({ - pausedUntil: 6_000, - launchGapMs: 250, - }); - expect(backOff(IDLE_THROTTLE, undefined, 1_000).pausedUntil).toBe(3_000); - expect(backOff(IDLE_THROTTLE, 0, 1_000).pausedUntil).toBe(2_000); - expect(backOff(IDLE_THROTTLE, 3_600_000, 1_000).pausedUntil).toBe(21_000); - }); - - it("treats a burst during one pause as one signal, then doubles per episode", () => { - const first = backOff(IDLE_THROTTLE, 2_000, 1_000); - const burst = backOff(first, 2_000, 1_500); - expect(burst).toEqual({ pausedUntil: 3_500, launchGapMs: 250 }); - - let throttle = burst; - for (const now of [10_000, 20_000, 30_000, 40_000, 50_000]) { - throttle = backOff(throttle, 2_000, now); - } - expect(throttle.launchGapMs).toBe(4_000); - }); -}); diff --git a/src/server/lib/audit/crawl-throttle.ts b/src/server/lib/audit/crawl-throttle.ts deleted file mode 100644 index 056cf75..0000000 --- a/src/server/lib/audit/crawl-throttle.ts +++ /dev/null @@ -1,58 +0,0 @@ -/** - * Site-wide backoff for rate limiting. A 429 means "too fast", not "go - * away": the whole crawl pauses for the server's Retry-After (or a default), - * every launch after that is spaced out, and the URL is retried a bounded - * number of times before it is recorded as blocked. - */ -export interface CrawlThrottle { - /** No fetch launches before this time (ms epoch). */ - pausedUntil: number; - /** Minimum spacing between launches; 0 until the site rate-limits us. */ - launchGapMs: number; -} - -/** Retries per URL within one chunk before a 429 is recorded as blocked. */ -export const MAX_RATE_LIMIT_RETRIES = 3; -const DEFAULT_PAUSE_MS = 2_000; -const MIN_PAUSE_MS = 1_000; -/** Longer Retry-After values are cut short: the chunk has a 90s deadline. */ -const MAX_PAUSE_MS = 20_000; -const MIN_LAUNCH_GAP_MS = 250; -const MAX_LAUNCH_GAP_MS = 4_000; - -export const IDLE_THROTTLE: CrawlThrottle = { pausedUntil: 0, launchGapMs: 0 }; - -/** `Retry-After` is either delay-seconds or an HTTP-date. */ -export function parseRetryAfterMs(header: string | null): number | undefined { - if (!header) return undefined; - const value = header.trim(); - if (/^\d+$/.test(value)) return Number(value) * 1000; - const at = Date.parse(value); - return Number.isNaN(at) ? undefined : Math.max(0, at - Date.now()); -} - -/** - * Register a 429. The concurrent fetches of one window tend to fail - * together, so a burst arriving while already paused counts as one signal: - * it can extend the pause but does not compound the launch gap. - */ -export function backOff( - throttle: CrawlThrottle, - retryAfterMs: number | undefined, - now = Date.now(), -): CrawlThrottle { - const pause = Math.min( - Math.max(retryAfterMs ?? DEFAULT_PAUSE_MS, MIN_PAUSE_MS), - MAX_PAUSE_MS, - ); - const alreadyPaused = now < throttle.pausedUntil; - return { - pausedUntil: Math.max(throttle.pausedUntil, now + pause), - launchGapMs: alreadyPaused - ? throttle.launchGapMs - : Math.min( - Math.max(throttle.launchGapMs * 2, MIN_LAUNCH_GAP_MS), - MAX_LAUNCH_GAP_MS, - ), - }; -} diff --git a/src/server/lib/audit/types.ts b/src/server/lib/audit/types.ts index 253617a..2fbe499 100644 --- a/src/server/lib/audit/types.ts +++ b/src/server/lib/audit/types.ts @@ -146,11 +146,6 @@ export interface CrawledPageResult { * response time is measured at headers and says nothing about body size. */ htmlBytes: number; - /** - * Server-requested wait before retrying, from a 429's Retry-After header. - * Only set on 429 responses. Transient — drives the crawl throttle. - */ - retryAfterMs?: number; imagesTotal: number; imagesMissingAlt: number; images: Array<{ src: string | null; alt: string | null }>; diff --git a/src/server/workflows/site-audit-workflow-helpers.ts b/src/server/workflows/site-audit-workflow-helpers.ts index d18bcb9..a03463f 100644 --- a/src/server/workflows/site-audit-workflow-helpers.ts +++ b/src/server/workflows/site-audit-workflow-helpers.ts @@ -4,7 +4,6 @@ import type { } from "@/server/lib/audit/types"; import { sha256Hex } from "@/server/lib/audit/ids"; import { normalizeUrl } from "@/server/lib/audit/url-utils"; -import { parseRetryAfterMs } from "@/server/lib/audit/crawl-throttle"; const CRAWL_USER_AGENT = "OpenSEO-Audit/1.0"; const MAX_HTML_BYTES = 1024 * 1024; @@ -128,10 +127,6 @@ export async function crawlPage( // The body was still fetched and buffered; report its size so the // crawl window's byte budget sees blocked/error pages too. htmlBytes: body.length, - retryAfterMs: - statusCode === 429 - ? parseRetryAfterMs(response.headers.get("retry-after")) - : undefined, }); } @@ -253,7 +248,6 @@ function emptyPageResult(input: { crawlDepth: number | null; inSitemap: boolean; htmlBytes?: number; - retryAfterMs?: number; }): CrawledPageResult { return { id: crypto.randomUUID(), @@ -281,7 +275,6 @@ function emptyPageResult(input: { contentHash: null, isHtml: false, htmlBytes: input.htmlBytes ?? 0, - retryAfterMs: input.retryAfterMs, imagesTotal: 0, imagesMissingAlt: 0, images: [], diff --git a/src/server/workflows/siteAuditWorkflowCrawl.ts b/src/server/workflows/siteAuditWorkflowCrawl.ts index ac16017..b090955 100644 --- a/src/server/workflows/siteAuditWorkflowCrawl.ts +++ b/src/server/workflows/siteAuditWorkflowCrawl.ts @@ -19,12 +19,6 @@ import { CRAWL_WINDOW, RETRY_CRAWL_WINDOW, } from "@/server/lib/audit/crawl-window"; -import { - backOff, - IDLE_THROTTLE, - MAX_RATE_LIMIT_RETRIES, - type CrawlThrottle, -} from "@/server/lib/audit/crawl-throttle"; import { crawlPage } from "@/server/workflows/site-audit-workflow-helpers"; import { pgStep } from "@/server/workflows/pgStep"; import { CRAWL_CHUNK_STEP } from "@/server/workflows/auditStepConfigs"; @@ -112,9 +106,6 @@ export async function runCrawlPhase( // the site's page weight the hard way — on heavy-page sites that meant an // exceededMemory death every ~200 pages. let windowHint = CRAWL_WINDOW.initial; - // Same for the launch gap a rate-limiting site taught us: relearning it - // per chunk would cost a fresh round of 429s every 200 pages. - let launchGapHint = IDLE_THROTTLE.launchGapMs; while (pending > 0 && attemptedTotal < params.maxPages) { chunkNo += 1; @@ -128,7 +119,6 @@ export async function runCrawlPhase( chunkNo, attemptedBefore: attemptedTotal, startWindow: windowHint, - startLaunchGapMs: launchGapHint, }), ); // Apply the chunk's counters even when it did no new work (a retried @@ -139,7 +129,6 @@ export async function runCrawlPhase( // `?? initial`: an instance in flight across a deploy replays cached // step results from before endWindow existed. windowHint = result.endWindow ?? CRAWL_WINDOW.initial; - launchGapHint = result.endLaunchGapMs ?? IDLE_THROTTLE.launchGapMs; // One zero-attempt chunk is normal (retry of a completed chunk number); // two in a row means the frontier is unservable — stop with what we // have instead of spinning forever. @@ -156,14 +145,12 @@ async function runCrawlChunk( chunkNo: number; attemptedBefore: number; startWindow: number; - startLaunchGapMs: number; }, ): Promise<{ attemptedInChunk: number; attempted: number; pending: number; endWindow: number; - endLaunchGapMs: number; }> { const { auditId, workflowInstanceId, origin, maxPages, robots, chunkNo } = input; @@ -184,7 +171,6 @@ async function runCrawlChunk( attempted: stats.attempted, pending: stats.pending, endWindow: input.startWindow, - endLaunchGapMs: input.startLaunchGapMs, }; } @@ -201,15 +187,6 @@ async function runCrawlChunk( let nextIndex = 0; let attemptedInChunk = 0; const inFlight = new Set>(); - // 429s are the site saying "slower", not "blocked": the URL goes back on - // the queue, the whole window pauses, and launches are spaced out. - let throttle: CrawlThrottle = { - ...IDLE_THROTTLE, - launchGapMs: input.startLaunchGapMs, - }; - let nextLaunchAt = 0; - const retryQueue: ClaimedUrl[] = []; - const retriesByUrl = new Map(); let persistThreshold = FIRST_PERSIST_BATCH_SIZE; let batch: CrawledPageResult[] = []; // Persistence runs concurrently with fetching (pipelined) but sequentially @@ -245,15 +222,6 @@ async function runCrawlChunk( const launch = (entry: ClaimedUrl) => { const promise = crawlPage(entry.url, entry.depth, entry.inSitemap) .then((page) => { - if (page.statusCode === 429) { - throttle = backOff(throttle, page.retryAfterMs); - const retries = retriesByUrl.get(entry.url) ?? 0; - if (retries < MAX_RATE_LIMIT_RETRIES) { - retriesByUrl.set(entry.url, retries + 1); - retryQueue.push(entry); - return; - } - } attemptedInChunk += 1; batch.push(page); if (batch.length >= persistThreshold) flush(); @@ -264,26 +232,18 @@ async function runCrawlChunk( inFlight.add(promise); }; - const hasNext = () => retryQueue.length > 0 || nextIndex < claimed.length; - while (true) { - const now = Date.now(); - const canLaunch = - hasNext() && - now < deadlineAt && - queuedPersists <= MAX_QUEUED_PERSIST_BATCHES; - if (canLaunch && inFlight.size < windowSize) { - const waitMs = Math.min( - Math.max(throttle.pausedUntil, nextLaunchAt) - now, - deadlineAt - now, - ); - if (waitMs > 0) { - await sleep(waitMs); - continue; - } - launch(retryQueue.shift() ?? claimed[nextIndex++]); - nextLaunchAt = Date.now() + throttle.launchGapMs; - continue; + while ( + inFlight.size < windowSize && + nextIndex < claimed.length && + Date.now() < deadlineAt + ) { + // queuedPersists changes when persistChain settles. Keep it out of the + // loop condition because the type-aware linter cannot see that async + // mutation and flags the otherwise valid backpressure check. + if (queuedPersists > MAX_QUEUED_PERSIST_BATCHES) break; + launch(claimed[nextIndex]); + nextIndex += 1; } if (inFlight.size > 0) { await Promise.race(inFlight); @@ -294,8 +254,8 @@ async function runCrawlChunk( // chunk is done (leases exhausted or soft deadline hit). if ( queuedPersists > MAX_QUEUED_PERSIST_BATCHES && - hasNext() && - now < deadlineAt + nextIndex < claimed.length && + Date.now() < deadlineAt ) { await persistChain; continue; @@ -305,12 +265,8 @@ async function runCrawlChunk( flush(); await persistChain; - // Leases we never launched or finished retrying (soft deadline) go back - // to the queue; the next chunk retries them with a fresh budget. - const unattempted = [ - ...retryQueue.map((entry) => entry.url), - ...claimed.slice(nextIndex).map((entry) => entry.url), - ]; + // Leases we never launched (soft deadline) go back to the queue. + const unattempted = claimed.slice(nextIndex).map((entry) => entry.url); if (unattempted.length > 0) { await scratchpad.releaseUrls(unattempted); } @@ -324,14 +280,9 @@ async function runCrawlChunk( attempted: stats.attempted, pending: stats.pending, endWindow: windowSize, - endLaunchGapMs: throttle.launchGapMs, }; } -function sleep(ms: number) { - return new Promise((resolve) => setTimeout(resolve, ms)); -} - async function persistCrawledPages(input: { auditId: string; workflowInstanceId: string; diff --git a/src/shared/audit-issues.ts b/src/shared/audit-issues.ts index 8b306a6..baddfa7 100644 --- a/src/shared/audit-issues.ts +++ b/src/shared/audit-issues.ts @@ -20,7 +20,7 @@ export const AUDIT_ISSUE_TYPES = { severity: "critical", title: "Crawler was blocked", explanation: - "The site returned a bot challenge or access denial (e.g. a Cloudflare challenge, 403, or a 429 that persisted after we slowed down and retried) instead of the page. We report this honestly rather than pretending the page is broken — but it means this page could not be audited, and other crawlers like search engines may face similar friction.", + "The site returned a bot challenge or access denial (e.g. a Cloudflare challenge, 403, or 429) instead of the page. We report this honestly rather than pretending the page is broken — but it means this page could not be audited, and other crawlers like search engines may face similar friction.", howToFix: 'If you own this site, allowlist the "OpenSEO-Audit" user agent in your WAF/bot-protection settings (on Cloudflare: a WAF custom rule that skips bot protection when the user agent contains "OpenSEO-Audit"; on some free tiers you may need to relax bot protection). Then re-run the audit.', },