Repository navigation
Make a page budget mean a page budget - #74
Merged
Merged
Conversation
Two defects in the crawl budget layer, both of which let a run blow past a stated limit and then report the wrong reason for stopping. A soft stop hid the hard stop behind it. CrawlBudget.stop() was first-write-wins across a single field, so once a soft reason latched (max-query-variants, max-depth, max-links-per-page) a later hard reason could not replace it. The fetch loop continues while the reason is not a hard stop, so the crawler kept issuing real requests at the audited origin and discarding every response until the frontier drained. Reproduced: a 1,000,000-byte budget read 4,197,578 bytes across 85 requests and reported max-query-variants. Now hard and soft latch separately and the getter prefers hard: the same crawl reads 1,234,598 bytes across 25 requests and reports max-bytes. The fetch-side budgets did not relate to the page budget. maxPages and maxBytes each resolved against an independent flat default, so the smaller silently won: asking for 1,000 pages of a site serving ~745KB each returned 140, with no error and a partial flag naming the wrong budget. maxFetches, maxDurationMs and maxBytes now derive from maxPages when the caller has not set them. This raises unset ceilings only; a stated budget is still honoured exactly, and asking for nothing still gets the documented flat defaults. Exports resolveSiteCrawlLimits so a caller can ask what budgets a crawl will run under rather than recomputing the derivation and drifting from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The publish-workflow test requires a CHANGELOG heading matching package.json, which the version bump did not have, so CI failed on a convention the local suite could not see: it had been run before the bump, not after. Bumps CRAWL_ENGINE_VERSION 2.0.0 -> 2.1.0. The report shape is unchanged so CRAWL_SCHEMA_VERSION stays 2.0, but that constant exists so persisted checkpoints can detect a change in crawl semantics, and the same options can now produce a larger crawl than 6.0.0 did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The scaled() helper shipped as `Math.ceil(derived)` where it needed `Math.max(flat, Math.ceil(derived))`, so for a small page count it did the exact opposite of what it claims: at maxPages 1 it produced maxFetches 2 and maxDurationMs 400, below the documented defaults of 5,000 and 120,000. A crawl could then not afford its own auxiliary fetches. Six tests in crawl.test.ts caught it on the Node 20 and 22 CI legs, where the 400ms cap made the crawl a race it lost; the same tests passed on Node 24, which is why the local run was green. Scaling is a ceiling RAISE. It may never lower a budget the caller did not ask to lower, which is what the commit message, the changelog entry and the tests all already claimed. The new test walks page counts from 1 to 1,000 and asserts no derived budget ever falls under its default; it fails when the floor is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-ups. Four defects, three of them cases where the scaling
introduced here still could not deliver the page count it promised.
The derivation keyed off whether `maxPages` was PRESENT rather than off the
resolved page count, which made the default path the one case the fix did
not reach: `runSiteCrawl(url)` resolves to the documented 1,000 pages but
kept the flat 100MB, so it still returned ~140 pages of a media-heavy site,
while `runSiteCrawl(url, {maxPages: 1_000})` got a 1.5GB ceiling. Identical
work, budgets 14x apart, decided by whether a key was present. A test
asserted that discontinuity as if it were the guarantee.
A derived budget could overflow. `maxPages: Number.MAX_VALUE` produced
Infinity for all three fetch-side budgets; JSON.stringify writes that as
null, so a report could state a crawl ran under no byte budget at all, and
a duration past 2^31-1 ms makes setTimeout fire immediately. Every resolved
limit is now clamped to a safe integer, duration to the setTimeout ceiling.
Fetch scaling ignored retries and duration scaling ignored pacing, though
both are the caller's own knobs. A retry is a real request, so one
recoverable 429 per page exhausted a 5,000-page fetch budget at ~3,700
pages; and requestDelayMs is time the crawl must spend NOT fetching, so
5,000 paced pages were budgeted 2,000s for work that cannot finish in under
5,000s. Politeness was causing the truncation.
Bumps to 7.0.0: a default crawl now returns more pages, reads more bytes and
takes longer than 6.0.0, so stored crawls are not comparable across it. The
README badge, which ships in the npm tarball, was stale at 6.0.0.
Adds an end-to-end test that crawls until the flat byte ceiling is crossed,
which fails if runSiteCrawl stops routing through the derivation. The suite
passed with that wiring severed before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in the crawl budget layer. Both let a crawl exceed a stated limit and then report the wrong reason for stopping.
A soft stop hid the hard stop behind it
CrawlBudget.stop()was first-write-wins across a single field, so once a soft reason latched (max-query-variants,max-depth,max-links-per-page) a later hard reason could not replace it.The fetch loop continues while the reason is not a hard stop:
So a latched soft reason kept the crawler issuing real requests at the audited origin and discarding every response until the frontier drained. The run then reported the soft reason, sending anyone diagnosing it to the wrong budget.
Hard and soft now latch separately and the getter prefers hard. First-write-wins still applies within a class, never across them.
Measured against a fixture origin with a 1,000,000-byte budget:
max-query-variantsmax-bytesThe fetch budgets had no relationship to the page budget
maxPagesandmaxByteseach resolved against an independent flat default, so the smaller silently won. Asking for 1,000 pages of a site serving ~745 KB each returned 140, because the 100 MB byte default ran out first, with no error, no warning, and apartialflag naming the wrong budget.maxFetches,maxDurationMsandmaxBytesnow derive frommaxPageswhen the caller has not set them. This raises unset ceilings only:maxPagesstill gets the documented flat defaultsAlso exports
resolveSiteCrawlLimitsso a caller can ask what budgets a crawl will run under, rather than recomputing the derivation and drifting from it.Tests
test/crawl-budget-coherence.test.tscovers both defects against a real fixture HTTP server, plus unit coverage of the derivation.Worth noting: two earlier versions of this test file passed without the fix. The fixture linked paths with no query strings, so
maxQueryVariantsnever latched and the bug was never triggered. The current fixture uses small pages so many are crawled and links enqueued before the byte cap trips. Verified by stashingsrc/crawl.tsand re-running: the two latch tests fail without the fix, and the derivation assertions compare directly againstDEFAULT_SITE_CRAWL_LIMITSso they cannot pass at the flat values.The scaling only binds above ~67 pages (bytes) or ~300 (duration), so it is not behaviourally testable at reasonable cost. That is why the derivation is exported and unit-tested directly instead of shipped unverified.
624 tests pass across 53 files on Node 20, 22 and 24 (reproduced locally in containers matching both CI legs). Typecheck and lint clean.
Also bumps
CRAWL_ENGINE_VERSION2.0.0 to 2.1.0. The report shape is unchanged soCRAWL_SCHEMA_VERSIONstays at 2.0, but that constant exists so persisted checkpoints can detect a change in crawl semantics, and the same options can now produce a larger crawl than 6.0.0 did.Minor version bump for the new exported helper.
🤖 Generated with Claude Code
Review follow-ups
Six findings survived an adversarial pass and a round of review on
0cdb2ff. One (no floor under the derivation) was already fixed by the third commit. The rest are addressed here.The default path was the one case the fix did not reach. The derivation keyed off whether
maxPageswas present, not off the resolved page count. SorunSiteCrawl(url)resolved to the documented 1,000 pages but kept the flat 100MB and still truncated at ~140 pages, whilerunSiteCrawl(url, {maxPages: 1000})got 1.5GB. Identical work, budgets 14x apart, decided by whether a key was written down. A test asserted that discontinuity as if it were the guarantee.A derived budget could overflow.
maxPages: Number.MAX_VALUEproducedInfinityfor all three fetch-side budgets.JSON.stringifywrites that asnull, so a report could state a crawl ran under no byte budget at all; and a duration past 2^31-1 ms makessetTimeoutfire immediately, turning the longest possible budget into none. Every resolved limit is clamped to a safe integer, duration to thesetTimeoutceiling.Scaling ignored retries and pacing, though both are the caller's own knobs. A retry is a real request against
maxFetches, so one recoverable429per page exhausted a 5,000-page budget at ~3,700 pages.requestDelayMsis time the crawl is required to spend not fetching, so 5,000 paced pages were budgeted 2,000s for work that cannot finish in under 5,000s: the politeness knob was causing the truncation.The wiring was unguarded. The derivation tests all called the pure function directly; the whole suite passed with
runSiteCrawlreverted to flat defaults. There is now an end-to-end test that crawls past the flat byte ceiling and fails if that wiring is severed. It gzips its fixture, so it crosses a 100MB budget over ~1.5KB of loopback traffic.Why this is 7.0.0 and not 6.1.0
A default crawl now returns more pages, reads more bytes, and takes longer than 6.0.0 did, so stored crawls are not comparable across the boundary. Under this repo's own rule that is a major. The README badge, which ships in the npm tarball, was stale at 6.0.0.