feat: redesign request timeouts and allow per-route overrides - #3860
Conversation
| test('should expose the per-route timeouts, falling back the same way as getHandler', async () => { | ||
| const router = Router.create(); | ||
|
|
||
| router.addHandler('SLOW', async () => {}, { requestHandlerTimeoutSecs: 120 }); |
There was a problem hiding this comment.
this is sick, per-router timeouts is such a good move
Pre- and post-navigation hooks had no timeout at all, so a hook that hangs stalled the request forever. In v3 the request handler timeout was summed with the navigation timeout, and that sum happened to bound the hooks too - dropping the sum in v4 removed the bound along with it. Each hook now gets its own window, separate from `navigationTimeoutSecs` and `requestHandlerTimeoutSecs`, so a slow hook is reported as a slow hook rather than being charged to a phase it does not belong to. Defaults to the same value as the navigation timeout of the given crawler (30s for HTTP, 60s for browsers). Relates to #2951
Navigation, the navigation hooks and the request handler each have their own timeout, but the phases between them have none - `extendContext`, the robots.txt check or the response processing can hang and stall the crawler forever. This adds the backstop #2951 asks for: it is not any single phase's timeout, and its message does not blame one. Uses `internalTimeoutMillis`, which is already at least 5 minutes and derived from the request handler timeout, so it only fires when something is genuinely stuck rather than merely slow. Deliberately a bare timer rather than `addTimeoutToPromise`: nested calls share a single `AbortController`, so wrapping the request in one meant the request handler timing out also aborted this outer context - the error handling that follows it was then cancelled before it could reclaim the request, and the crawler hung instead of retrying. Relates to #2951
`requestHandlerTimeoutSecs` applies to every request alike, so a single page type
that needs longer - a listing behind an infinite scroll, say - forced the timeout
up for everything else too. Routes can now opt out of the crawler's default:
router.addHandler('LIST', handler, { requestHandlerTimeoutSecs: 120 });
The crawler-level option is unchanged and still the default for anything a route
does not override. The label is already known before the handler starts, so the
timeout is resolved per request, without the router having to reach back into the
crawler mid-flight.
Also stretches the two things derived from the handler timeout to account for
routes that ask for more than it: the internal backstop (per request, and only
when a route actually overrides, so an explicitly configured internal timeout
stays as configured), and the request manager's processing-time hint, which has
to be applied up front and so uses the longest timeout any route asked for.
`AdaptivePlaywrightCrawler` times the handler itself instead of going through
`runRequestHandler`, so it resolves the override too - with its own timeout as
the fallback, which is not the crawler-level one.
Closes #1485
Covers what a per-route timeout cannot express: a handler that only discovers
once it is running how much longer it needs - a listing page that turns out to
have far more to scroll through than usual, say.
router.addHandler('LIST', async ({ extendTimeout, page }) => {
const pageCount = await countPages(page);
extendTimeout(pageCount * 10);
await scrapeAllPages(page);
});
Extends the request handler's own window and the internal backstop together -
extending only the handler would be pointless, since the backstop would cut the
request down moments later. The backstop is a bare timer rather than an
`addTimeoutToPromise` frame, so it cannot be reached through `@apify/timeout`
and is wired up explicitly.
`@apify/timeout` is patched via pnpm for now, so this can be built and tested
here and in CI ahead of the release. To be replaced by a plain version bump once
apify/apify-shared-js#669 is out - the patch is only the built `dist` of that PR.
Regenerated with `pnpm api:extract` for the new timeout options (`navigationHooksTimeoutSecs`, `RouteOptions`, `context.extendTimeout`).
`AdaptivePlaywrightCrawler` destructures `requestHandlerTimeoutSecs` and never forwards it, so the base class saw the default 60s no matter what was set, and derived a flat 300s internal timeout from it. One request can run the handler twice in sequence though - the static attempt falling through to the browser, or the browser run followed by a rendering type detection - so the new backstop would cut off legitimate handlers whenever two runs exceeded 300s, i.e. anything above ~150s, and report it as a retry rather than a timeout. Forwards twice the configured timeout, which is what the base class needs to size the bounds around a request that may genuinely run the handler twice. Also documents the redesign in the v4 upgrading guide, covers the new router methods with unit tests, and makes the backstop extension test deterministic rather than a race it happened to win.
The per-route `requestHandlerTimeoutSecs` override and `context.extendTimeout` were only mentioned in the v4 upgrading guide. Adds them where someone reaching for the feature would look: the `Router` class JSDoc (with the `addHandler` / `addDefaultHandler` method docs noting the options arg), and the routing section of the refactoring tutorial, where a per-route timeout has a natural motivation.
`context.extendTimeout` needed an `@apify/timeout` change that was not yet released when this branch opened, so it rode on a local pnpm patch of 0.3.3. That change shipped in 0.4.0, so this drops the patch and bumps every package to `^0.4.4`. Also brings `jsdom-crawler` in line - it declared `^0.3.0` while the rest were on `^0.3.2`.
The name reads as one window shared by all navigation hooks; it is actually applied to each hook individually. Spell that out in the option's docs so the behaviour is not a surprise.
A navigation hook can call `context.extendTimeout` to push back its own window, because the extension lands on whichever timeout frame is currently innermost - and while a hook runs, that is the hook's own. It works today but only as a consequence of how the frames nest, so pin it: a pre- and a post-navigation hook that each run past `navigationHooksTimeoutSecs` but extend first must still succeed. The existing "after navigationHooksTimeoutSecs" tests cover the other direction (no extension -> the hook times out).
The timeout behaviour was only exercised on the HTTP crawlers. Add the browser equivalents on a real PlaywrightCrawler: a hanging pre-navigation hook times out against `navigationHooksTimeoutSecs`, `context.extendTimeout` rescues both a navigation hook and the request handler, and a per-route `requestHandlerTimeoutSecs` override lets one label run long while the others keep the crawler default.
This diverges from what crawlee-python does — https://github.com/apify/crawlee-python/blob/master/src/crawlee/crawlers/_abstract_http/_abstract_http_crawler.py#L149-L150 — there is a shared timeout for hooks and navigation. |
janbuchar
left a comment
There was a problem hiding this comment.
Apart from #3860 (comment), I have no further objections. Please request a re-review once that's resolved 🙂
Replace the separate per-hook `navigationHooksTimeoutSecs` with a single `navigationTimeoutSecs` budget shared by the `preNavigationHooks`, the navigation, and the `postNavigationHooks`, matching Crawlee for Python. A slow hook now eats into the same window the navigation uses instead of each step being timed on its own. Response parsing stays outside the window, bounded only by the whole-request backstop. `context.extendTimeout()` also pushes the shared window deadline, so extending from any navigation hook grows the whole navigation budget rather than just that step. Bind the HTTP request to the navigation frame's `@apify/timeout` cancel signal instead of a fixed `AbortSignal.timeout`: the fixed timer would fire on its own and abort the lazily-read response body when a post-navigation hook (even one that called `extendTimeout`) ran past the original window. A genuine navigation timeout still aborts the socket via that signal. BREAKING CHANGE: the `navigationHooksTimeoutSecs` option has been removed; use `navigationTimeoutSecs` (now covering the hooks too) or `context.extendTimeout()`.
The navigation unification made the shared window report `navigation timed out` (lowercase), but the driver-timeout fallback in `handleRequestTimeout` still threw `Navigation timed out` (capital). With the window and the driver's own goto timeout set to the same duration, whichever fires first decides the casing - so the message was effectively racy, and the puppeteer `#1216` test (unchanged by the refactor) asserted the capital form and failed once the window started winning. Lower-case the fallback to match, and update the test.
`Navigation timed out after N seconds.` reads as its own sentence in the logs (e.g. after `Reclaiming failed request ... queue. `), so it should start with a capital, like the sibling `Request timed out` / `Fetching next request timed out` messages. `requestHandler` stays lower-case because it is an identifier, but `navigation` is a plain word. Supersedes the previous commit, which had lower-cased it: this instead capitalizes consistently across the unified navigation window and the driver-timeout fallback, so the two paths agree regardless of which fires first.
Same sentence-casing as the navigation message: the HTTP navigation-timeout fallback threw a lower-case `request timed out`, while the whole-request backstop already throws `Request timed out`. Capitalize the fallback so the two agree.
# Conflicts: # docs/public-api/crawlee-core.api.md # packages/core/src/router.ts
The internal timeout was read with a hand-rolled `tryEnv(process.env...)` in `BasicCrawler`. Route it through `Configuration` as `internalTimeoutMillis` (`CRAWLEE_INTERNAL_TIMEOUT`), like the other environment-backed options, so it is resolved and coerced the same way. This also drops the old `tryEnv`, whose `+val` turned an empty-string env var into `0`. Tests that exercised it now inject a `Configuration` instead of mutating `process.env` mid-run, which matches how the value is actually resolved.
…hort internal timeout Two follow-ups on the request backstop: - `context.extendTimeout` now also raises the request manager's processing-time reservation, so a request that asks for much longer is not handed out again by a locking backend mid-flight. Best-effort: the hint is process-wide and raise-only, and locking is opt-in in v4. - `_init` warns once when the configured internal timeout is shorter than the navigation and request handler timeouts combined. The backstop is floored per request so it will not actually cut them short, but the configured value is then effectively ignored, which is worth surfacing. The internal-timeout tests inject a `Configuration` via the service locator instead of mutating `process.env`, matching how the value is resolved now that it goes through `Configuration`.
The window helper, the context symbols, and the race that backs the internal request timeout have grown enough to live on their own. Move them into `request-backstop.ts` as `raceWithBackstop`, `remainingNavigationWindowMillis`, and the `navigationDeadline` / `extendBackstop` / `backstopExpired` symbols; `BasicCrawler` keeps only the crawler-specific sizing (which phases the backstop must outlast) and delegates the rest. No behaviour change.
…han cutting phases
|
I'll run a staff review loop before requesting new reviews. I forgot to do that after the nav timeout window refactor, which introduced quite a lot of missed bugs, but all comments should now be addressed. |
This PR replaced the browser crawler's use of the core `handleRequestTimeout` (from `crawler_utils.ts`) with a local navigation-timeout path, leaving the helper with no consumers. It was `@internal` and the only thing in the file, so drop both the file and its re-export.
Completes the previous commit: `crawler_utils.ts` was deleted but its re-export in `crawlers/index.ts` was left behind, which would fail the build.
…er runs `AdaptivePlaywrightCrawler` runs the request handler up to twice per request, and covered that by passing `requestHandlerTimeoutSecs * 2` to the base. That only doubled the crawler-level default, so a per-route `requestHandlerTimeoutSecs` override (which the base resolves un-doubled) left the backstop and the request reservation sized for a single run - a request with a large route override could be cut short, or handed out again by a locking backend, mid-second-run. Replace the ad-hoc doubling with a `getRequestHandlerRunCount` hook (1 by default, 2 for the adaptive crawler) applied wherever a whole-request budget is derived: the backstop, the reservation, the extendTimeout reservation bump, and the startup warning. The per-run handler window is unchanged.
Restore the first line of `Router.addDefaultHandler`'s JSDoc, dropped by accident when the per-route options docs were added (it left the public doc starting mid-sentence). Also fold in the review-loop comment refinements (the backstop comment points at `raceWithBackstop`; the HTTP `cancelSignal` comment describes the actual header-phase-only abort) and drop a stray blank line.
# Conflicts: # packages/basic-crawler/src/internals/basic-crawler.ts # packages/http-crawler/src/internals/http-crawler.ts
# Conflicts: # docs/public-api/crawlee-core.api.md
# Conflicts: # docs/public-api/crawlee-basic.api.md
janbuchar
left a comment
There was a problem hiding this comment.
https://github.com/apify/crawlee/pull/3860/changes#r3705566607 — I think this needs to be addressed before release, but perhaps we can defer it to a follow up issue so that we don't stall this PR anymore.
… notes Rename the request-backstop module to request-timeout and its symbols (raceWithTimeout, extendTimeoutKey, timeoutExpiredKey, RequestTimeoutContext) to speak in terms of the timeout rather than the internal 'backstop' jargon, drop the @internal tags, trim a verbose comment, and remove the stray Python references from the HTTP and browser navigation-timeout messages.
navigationDeadlineKey and remainingNavigationWindowMillis are re-exported from the package index only so the HTTP and browser crawler subclasses can reach them; they are not user-facing, so restore their @internal tag to keep them out of the public API report. The module-local symbols stay untagged.
extendTimeout no longer raises the request reservation. setExpectedRequestProcessingTimeSecs is a process-wide, raise-only hint, so bumping it per request inflates the expected processing time for every request dequeued afterwards and is not retroactive - the wrong contract for a per-request extension. A proper per-request reservation is deferred to a follow-up. Replace the getRequestHandlerRunCount() extension point with an override of the existing resolveRequestHandlerTimeoutMillis: the adaptive crawler, which runs the handler up to twice, scales the whole-request budgets (internal timeout, reservation) by wrapping super in that override, while its per-run window resolves through super directly. The method now takes the route label instead of a Request.
…wler The internal timeout is a coarse safety net, not a precise budget - sizing it to exactly two handler runs was over-engineering, and doubling the reservation hint with it had the same problem. Both now use the plain single-run numbers; the rare adaptive setup with a very large per-route timeout can raise CRAWLEE_INTERNAL_TIMEOUT (and gets the startup warning when it is too low). This removes the last reason for resolveRequestHandlerTimeoutMillis to be protected: the adaptive crawler resolves per-route overrides through the router's public getTimeoutSecs instead, so the method is private again and the crawlers expose no new API surface for the timeout redesign.
Timeout redesign (#2951)
v4 had already dropped the old
navigationTimeoutSecs + requestHandlerTimeoutSecs + buffersum, so the request handler timeout covers only the user's function and the confusing "timed out after 130 seconds" messages are gone. What it did not do is put back the pieces that sum incidentally bounded.navigationTimeoutSecscovers thepreNavigationHooks, the navigation itself, and thepostNavigationHooksas a single shared budget, matching Crawlee for Python. A hook that hangs no longer stalls the request forever; it eats into the same window the navigation uses. (This replaces an earlier attempt at a separate per-hooknavigationHooksTimeoutSecs, which is gone.)extendContext, the robots.txt check, response processing) could hang indefinitely. An internal backstop now bounds the whole request. It is sized to outlast the phases that have their own timeout (navigation plus the handler), so a legitimately slow request is never cut short and it only fires when something is genuinely stuck. It is configured withCRAWLEE_INTERNAL_TIMEOUT, now resolved throughConfigurationlike the other env-backed options. Set it below the phase timeouts and the crawler raises it per request and warns at startup, rather than cutting a phase short.context.extendTimeout()When the time needed is only apparent once a hook or handler is already running,
context.extendTimeout(secs)buys more. From inside the navigation phase it pushes the shared navigation window; from the request handler it pushes the handler timeout. Either way it also pushes the backstop and raises the request-manager reservation, so the extra time is neither clipped by the backstop nor undone by a locking backend handing the request out again.Per-route timeouts (#1485)
requestHandlerTimeoutSecsis unchanged and stays the default for anything a route does not override; a route's value may be longer or shorter than it. The label is known before the handler starts, so the timeout is resolved per request and the router never reaches back into the crawler mid-flight. The backstop and the reservation both account for the longest route in play.Browser navigation
A
preNavigationHookshook can still overridegotoOptions.timeout(including0, Playwright's "no timeout"); the shared window no longer clamps it to 1ms or discards a larger value. A navigation timeout, whether ours or the driver's own, is reported asNavigation timed out after N secondsinstead of the driver's raw millisecond value.Adaptive crawler
AdaptivePlaywrightCrawlerruns the handler up to twice per request (a static attempt falling through to the browser). AgetRequestHandlerRunCounthook (2 for adaptive, 1 everywhere else) sizes the whole-request budgets for both runs, while each run keeps its own handler window. This replaces an earlier one-off doubling that missed per-route overrides.Notes on the implementation
The backstop is a bare timer, not
addTimeoutToPromise: nestedaddTimeoutToPromisecalls share oneAbortController, so wrapping the whole request in one would let the handler timing out abort the outer context and cancel the error handling that reclaims the request. The plumbing lives in its ownrequest-backstop.tsmodule.For the same reason the HTTP navigation binds its request to the navigation frame's
@apify/timeoutcancel signal rather than a fixedAbortSignal.timeout: the response body is read lazily, after the post-navigation hooks, so a fixed timer would abort a body a hook is legitimately still keeping alive viaextendTimeout. A genuine navigation timeout still fails the request; the body read is bounded at the parse step.Depends on the
extendTimeoutaddition in@apify/timeout(apify/apify-shared-js#669), released as 0.4.4 and consumed here.Closes #1485
Closes #2951