Skip to content

fix(core): keep crawler enqueue limits when options carry explicit undefined - #3927

Merged
B4nan merged 2 commits into
masterfrom
claude/nervous-khayyam-aa886d
Aug 4, 2026
Merged

fix(core): keep crawler enqueue limits when options carry explicit undefined#3927
B4nan merged 2 commits into
masterfrom
claude/nervous-khayyam-aa886d

Conversation

@B4nan

@B4nan B4nan commented Jul 29, 2026

Copy link
Copy Markdown
Member

Follow-up to #3924, same bug class one level down.

BasicCrawler's context-bound enqueueLinks built its call as { requestQueue, robotsTxtFile, respectRobotsTxtFile, onSkippedRequest, limit, ...options }, so a user options object carrying an explicitly present undefined key silently discarded the computed value:

await enqueueLinks({ urls, limit: config.limit }); // config.limit is not set

Neither ow.optional.number nor TypeScript (without exactOptionalPropertyTypes) rejects an explicit undefined, so the calculateEnqueuedRequestLimit() result was dropped — the queue kept growing past the remaining maxRequestsPerCrawl budget and the enqueueLimit skips were never reported to onSkippedRequest. maxRequestsPerCrawl is still enforced as a stop condition via isMaxPagesExceeded(), so the effect was queue bloat and missing reporting rather than an unbounded crawl. The same applied to robotsTxtFile and respectRobotsTxtFile (robots.txt filtering silently disabled at enqueue time), and to requestQueue, where the explicit undefined made ow throw instead.

Unlike #3924 this isn't a pure reordering, so the semantics for each computed key:

  • limit — now min(user limit, remaining maxRequestsPerCrawl budget). This is what addRequests() (maxNewRequests after the spread) and the per-crawler *CrawlerEnqueueLinks helpers have always done; this path was the outlier. A user limit larger than the remaining budget is now capped, which is a user-visible change.
  • onSkippedRequest — composed instead of replaced: the crawler's reporting (logging, crawler-level onSkippedRequest) runs, then the user's callback. Previously passing a callback to enqueueLinks silently disabled the crawler-level one.
  • requestQueue, robotsTxtFile, respectRobotsTxtFile?? fallbacks, so a defined user value still wins.

The enqueue-limit log message now also distinguishes the two causes, instead of blaming the user's limit when the maxRequestsPerCrawl clamp is what bit.

Not covered here: JSDOMCrawler and LinkeDOMCrawler never route context.enqueueLinks through this wrapper at all (they call the standalone enqueueLinks() directly, unlike CheerioCrawler/BrowserCrawler which forward the bound context function), so crawlDepth injection and maxCrawlDepth are inert there. Separate issue, separate fix.

…defined

`BasicCrawler`'s context-bound `enqueueLinks` built its call as `{ requestQueue,
robotsTxtFile, respectRobotsTxtFile, onSkippedRequest, limit, ...options }`, so a
user options object carrying an explicitly present `undefined` key silently
discarded the computed value:

    await enqueueLinks({ urls, limit: config.limit }); // config.limit is not set

Neither `ow.optional.number` nor TypeScript (without `exactOptionalPropertyTypes`)
rejects an explicit `undefined`, so the `calculateEnqueuedRequestLimit()` result
was dropped - the queue kept growing past the remaining `maxRequestsPerCrawl`
budget and the `enqueueLimit` skips were never reported to `onSkippedRequest`.
`maxRequestsPerCrawl` is still enforced as a stop condition, so the effect was
queue bloat and missing reporting rather than an unbounded crawl. The same
applied to `robotsTxtFile` and `respectRobotsTxtFile` (robots.txt filtering
silently disabled at enqueue time), and to `requestQueue`, where the explicit
`undefined` made `ow` throw instead.

The crawler-derived values now merge after the user options instead of before:

- `limit` becomes `min(user limit, remaining maxRequestsPerCrawl budget)`, which
  is what `addRequests()` and the per-crawler `*CrawlerEnqueueLinks` helpers have
  always done - this path was the outlier
- `onSkippedRequest` is composed, so the crawler reporting keeps running when the
  user passes their own callback, instead of being replaced by it
- `requestQueue`, `robotsTxtFile` and `respectRobotsTxtFile` fall back with `??`,
  so a defined user value still wins

The enqueue-limit log message now also distinguishes the two causes, instead of
blaming the user's `limit` when the `maxRequestsPerCrawl` clamp is what bit.
@B4nan B4nan added the adhoc Ad-hoc unplanned task added during the sprint. label Jul 29, 2026
@github-actions github-actions Bot added this to the 146th sprint - Tooling team milestone Jul 29, 2026
@github-actions github-actions Bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Jul 29, 2026
@B4nan
B4nan requested a review from barjin August 3, 2026 14:21

@barjin barjin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thank you @B4nan !

I just have one nit regarding the new changes ⬇️

Comment thread packages/basic-crawler/src/internals/basic-crawler.ts
…contexts

CheerioCrawler and BrowserCrawler pre-clamped `options.limit` before calling
the bound `enqueueLinks`, so the enqueueLimit log attributed skips to an
explicit enqueueLinks limit even when only the maxRequestsPerCrawl budget was
clamping. The bound path clamps authoritatively, so the pre-clamp is dropped
there (jsdom/linkedom keep theirs, they use the non-bound path).

Also documents that the crawler-level and per-call onSkippedRequest callbacks
compose instead of replacing each other.
@B4nan
B4nan merged commit 5f1b0ab into master Aug 4, 2026
8 checks passed
@B4nan
B4nan deleted the claude/nervous-khayyam-aa886d branch August 4, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants