fix(audit): back off and retry on 429 instead of recording the page as blocked (#562)
This commit is contained in:
parent
accac73e16
commit
bb099ad65a
42
src/server/lib/audit/crawl-throttle.test.ts
Normal file
42
src/server/lib/audit/crawl-throttle.test.ts
Normal file
@ -0,0 +1,42 @@
|
||||
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);
|
||||
});
|
||||
});
|
||||
58
src/server/lib/audit/crawl-throttle.ts
Normal file
58
src/server/lib/audit/crawl-throttle.ts
Normal file
@ -0,0 +1,58 @@
|
||||
/**
|
||||
* 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,
|
||||
),
|
||||
};
|
||||
}
|
||||
@ -146,6 +146,11 @@ 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 }>;
|
||||
|
||||
@ -4,6 +4,7 @@ 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;
|
||||
@ -127,6 +128,10 @@ 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,
|
||||
});
|
||||
}
|
||||
|
||||
@ -248,6 +253,7 @@ function emptyPageResult(input: {
|
||||
crawlDepth: number | null;
|
||||
inSitemap: boolean;
|
||||
htmlBytes?: number;
|
||||
retryAfterMs?: number;
|
||||
}): CrawledPageResult {
|
||||
return {
|
||||
id: crypto.randomUUID(),
|
||||
@ -275,6 +281,7 @@ function emptyPageResult(input: {
|
||||
contentHash: null,
|
||||
isHtml: false,
|
||||
htmlBytes: input.htmlBytes ?? 0,
|
||||
retryAfterMs: input.retryAfterMs,
|
||||
imagesTotal: 0,
|
||||
imagesMissingAlt: 0,
|
||||
images: [],
|
||||
|
||||
@ -19,6 +19,12 @@ 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";
|
||||
@ -106,6 +112,9 @@ 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;
|
||||
@ -119,6 +128,7 @@ 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
|
||||
@ -129,6 +139,7 @@ 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.
|
||||
@ -145,12 +156,14 @@ 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;
|
||||
@ -171,6 +184,7 @@ async function runCrawlChunk(
|
||||
attempted: stats.attempted,
|
||||
pending: stats.pending,
|
||||
endWindow: input.startWindow,
|
||||
endLaunchGapMs: input.startLaunchGapMs,
|
||||
};
|
||||
}
|
||||
|
||||
@ -187,6 +201,15 @@ async function runCrawlChunk(
|
||||
let nextIndex = 0;
|
||||
let attemptedInChunk = 0;
|
||||
const inFlight = new Set<Promise<void>>();
|
||||
// 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<string, number>();
|
||||
let persistThreshold = FIRST_PERSIST_BATCH_SIZE;
|
||||
let batch: CrawledPageResult[] = [];
|
||||
// Persistence runs concurrently with fetching (pipelined) but sequentially
|
||||
@ -222,6 +245,15 @@ 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();
|
||||
@ -232,18 +264,26 @@ async function runCrawlChunk(
|
||||
inFlight.add(promise);
|
||||
};
|
||||
|
||||
const hasNext = () => retryQueue.length > 0 || nextIndex < claimed.length;
|
||||
|
||||
while (true) {
|
||||
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;
|
||||
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;
|
||||
}
|
||||
if (inFlight.size > 0) {
|
||||
await Promise.race(inFlight);
|
||||
@ -254,8 +294,8 @@ async function runCrawlChunk(
|
||||
// chunk is done (leases exhausted or soft deadline hit).
|
||||
if (
|
||||
queuedPersists > MAX_QUEUED_PERSIST_BATCHES &&
|
||||
nextIndex < claimed.length &&
|
||||
Date.now() < deadlineAt
|
||||
hasNext() &&
|
||||
now < deadlineAt
|
||||
) {
|
||||
await persistChain;
|
||||
continue;
|
||||
@ -265,8 +305,12 @@ async function runCrawlChunk(
|
||||
flush();
|
||||
await persistChain;
|
||||
|
||||
// Leases we never launched (soft deadline) go back to the queue.
|
||||
const unattempted = claimed.slice(nextIndex).map((entry) => entry.url);
|
||||
// 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),
|
||||
];
|
||||
if (unattempted.length > 0) {
|
||||
await scratchpad.releaseUrls(unattempted);
|
||||
}
|
||||
@ -280,9 +324,14 @@ async function runCrawlChunk(
|
||||
attempted: stats.attempted,
|
||||
pending: stats.pending,
|
||||
endWindow: windowSize,
|
||||
endLaunchGapMs: throttle.launchGapMs,
|
||||
};
|
||||
}
|
||||
|
||||
function sleep(ms: number) {
|
||||
return new Promise<void>((resolve) => setTimeout(resolve, ms));
|
||||
}
|
||||
|
||||
async function persistCrawledPages(input: {
|
||||
auditId: string;
|
||||
workflowInstanceId: string;
|
||||
|
||||
@ -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 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.",
|
||||
"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.",
|
||||
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.',
|
||||
},
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user