refactor!: Add checkReadiness() and recordPacingSignal() to IRequestManager to fix leaky abstraction of ThrottlingRequestManager - #4061
Conversation
…ess() `isEmpty()` and `isFinished()` become one `readiness()`, returning `ready` | `waiting` | `stalled` | `finished`. Two probes per task-loop tick become one, which is the point: the crawler asks several times a second, and the two booleans were always read together anyway - `isEmpty() && !isFinished()` was the whole vocabulary for "not now, but not done either". The two extra states are things the pair could not express: - `waiting` carries an optional `readyAt`, so a source that is deliberately holding requests back can say *when* it will have work instead of leaving the caller to poll. `BasicCrawler` turns that into a single rescheduled `pool.notify()` timer, replaced only by an earlier one and `unref`'d so a pending wake-up never keeps the process alive. - `stalled` replaces `ThrottlingRequestManager.assertNoStalledDomains()`. The crawler raises the `PersistentRateLimitError` itself, from the one call site where nothing is in flight - which is both where a crawl that cannot progress becomes distinguishable from one that is merely waiting, and the only point at which throwing does not abandon requests mid-processing. `joinRequestSourceStates()` folds two sources read as one, with the precedence `ready` > `stalled` > `waiting` > `finished`. It is `@internal`: the tandem and the throttler are its only callers, and it is reached from tests the way the other internal helpers are. `ready` outranking `stalled` is deliberate - a stalled source is masked while the other one still has work, which is parity with the old call site and stops one hopeless domain from ending a crawl that is getting somewhere. `ThrottlingRequestManager.readiness()` answers all of it in one traversal of the domain clocks. Only domains whose delays have run out are probed, so the cost is one call per *dispatchable* source rather than one per throttled domain, and a stall candidate is kept out of that set: its backoff lapses between one 429 and the next, so a probe landing in that window would otherwise see its sub-queue as `ready` and mask the very state we are here to report. Storage backends keep their two booleans. That is the layer where the two questions really are two separate lookups, and a frontend that split them too would either probe twice per scheduling decision or lose the distinction.
…ignal()
`SupportsDomainThrottling` and `supportsDomainThrottling()` are gone. In their place
one required member on `IRequestManager`:
recordPacingSignal(url: string, signal: PacingSignal): boolean;
type PacingSignal =
| { reason: 'rateLimited'; waitMs?: number; scope?: PacingScope }
| { reason: 'minInterval'; intervalMs: number; scope: PacingScope };
Three things change, and each one is why the previous shape had to go.
**It lives on the interface, so a nested pacer still hears about it.**
`RequestManagerTandem` forwards the call, which means a `ThrottlingRequestManager`
sitting *inside* a composition receives the crawler's 429s and `Crawl-delay`
directives. Structural detection could never do that: it only ever found the
outermost manager, so a pacer under a tandem was silently deaf. Forwarding needs the
manager resolved, so the tandem now resolves a concrete one in its constructor - an
unresolved one is by construction a lazily-opened default queue, which paces nothing.
**One method taking a value, not a method per channel.** A wrapper forwards the
payload without knowing what is in it, and a new kind of signal costs implementors
nothing. Nothing in it names the mechanism it came from - status codes, response
headers and robots.txt are the crawler's business - and every delay is in
milliseconds, which incidentally fixes a `retryAfterMs` sitting next to a
`delaySeconds`. The arms are named for what they mean rather than where they came
from: a refusal is reactive and transient, a declared floor is a property of the site
and outlives the run.
**It is required, not optional.** A `?` here would be a capability probe wearing
different clothes - `if (record === undefined)` asks "does this manager pace?" at
runtime, which is the question we just deleted. `RequestQueue` answers `false`, the
tandem forwards, the throttler paces. The cost is that "nothing paces at all" and
"does not pace this domain" were told apart by `undefined` versus `false`; they
collapse into one warning naming both fixes, and which reader it is for is obvious
from what they have configured.
`scope` says how much of the URL space a signal covers, as an open `LiteralUnion`:
the two values Crawlee's own reporters send are suggested, but a pacer keyed on an
account or an API key can be reported to in its own vocabulary. A manager may apply a
signal to a **wider** scope than it was given - a floor for one host still holds when
a whole site is paced by it - but never a narrower one, which would leave part of what
it covers unpaced. `ThrottlingRequestManager` holds one queue per group, so
`throttleBy` is the finest granularity it can express; it widens a `hostname` signal
under registrable-domain grouping and **throws** on anything it cannot honour rather
than quietly under-applying it. Its own `throttleBy` option stays a closed union:
open interface, strict implementation.
The crawler's conflict check becomes `requestManager instanceof
ThrottlingRequestManager`. Probing for a method is no longer meaningful now that
every wrapping manager forwards it: its presence says nothing about whether anything
underneath actually paces, and a tandem over a plain queue must not trip the check.
…n ownership `sameDomainDelaySecs` used to wrap the request manager in `getRequestManager()`, at first use. That is the wrong place: by then the tandem the crawler builds for a `requestList` already exists, so the pacer went *outside* it, and requests transferred out of a list went straight into the wrapped queue without ever passing a per-domain clock. The shorthand quietly did nothing for a `requestList` crawl. It is built in the constructor now, which is the only position from which it can be placed *inside* the tandem. Doing that needs `ThrottlingRequestManagerOptions.inner` to accept a factory, since the default queue is only opened on first use. The factory is memoized for identity as much as for cost: `addRequestsBatched` groups a chunk by manager identity and `reclaimRequest` decides where an in-flight request goes back by comparing against it, so a fresh instance per call would split batches and reclaim requests into a manager that never handed them out. Bookkeeping - purging, persisting, dropping, hinting - deliberately never forces it; a hint arriving early is remembered and applied on resolution. That is also why `innerManager` is now `T | undefined`, and why reading it does not open anything: a getter should not open a queue behind a caller's back. The purge between repeated `run()` calls follows from ownership, which the constructor now knows in full, so `#ownedRequestQueue` and `#sameDomainDelaySecs` both go away: - `all` - nothing came from the caller, so one `purge()` from the outside in covers everything, per-domain queues included. - `none` - the caller supplied the manager and the crawler put nothing of its own inside it. - `ambiguous` - the caller supplied it *and* `sameDomainDelaySecs` put per-domain queues underneath. A purge empties their storage along with ours; skipping it leaves ours stale. There is no right answer, so a second `run()` throws and asks for `purgeRequestQueue` explicitly rather than guessing. That last case is what `ThrottlingRequestManager.purgeDomainQueues()` existed for - it was the crawler reaching in to empty the half it owned. With the decision made on ownership up front, `purge()` splits into two private halves and the method leaves the public surface.
46d7ca5 to
7b08dba
Compare
barjin
left a comment
There was a problem hiding this comment.
Thanks @janbuchar !
Just one note regarding the wrapped managers, the rest is naming nits:
| persistState(): Promise<void>; | ||
| // (undocumented) | ||
| readiness(): Promise<RequestLoaderState>; |
There was a problem hiding this comment.
nit: Seeing the persistState method and RequestLoaderState interface next to each other, it seems that state is a rather overloaded term.
How about calling the latter ...Status or something similar?
There was a problem hiding this comment.
Renamed: RequestSourceStatus / RequestLoaderStatus, and joinRequestSourceStatuses with them.
| persistState(): Promise<void>; | ||
| // (undocumented) | ||
| readiness(): Promise<RequestLoaderState>; |
There was a problem hiding this comment.
I'm not too keen on the readiness name. How about (get)?Status or sth more descriptive?
There was a problem hiding this comment.
checkReadiness() now. Kept "readiness" over a bare getStatus(); status of what was the part that read badly IMO. What do you think?
| } | ||
| // Both would pace the same domains, from different keys and with no idea of one another. | ||
| if (sameDomainDelaySecs > 0 && supportsDomainThrottling(requestManager)) { | ||
| if (sameDomainDelaySecs > 0 && requestManager instanceof ThrottlingRequestManager) { |
There was a problem hiding this comment.
What if the ThrottlingRequestManager is wrapped inside a Tandem? Then this check doesn't fire and we can still get double pacing, right?
There was a problem hiding this comment.
Correct, it didn't fire — requestList.toTandem(throttler) wasn't picked up by the check and got a second pacer wrapped around it. Fixed by deleting the check:
sameDomainDelaySecsnow goes in as a pacing signal —{ reason: 'minIntervalEverywhere', intervalMs, scope: 'registrableDomain' }— offered to whatever the crawler was going to read from.- Anything that paces takes it as its
minCrawlDelaySecsfloor, through any number of wrappers, since they all forward. The crawler builds aThrottlingRequestManagerof its own only when nothing takes it, so a domain never ends up with two clocks. - A manager pacing only some of its domains throws instead — same rule as a signal scoped wider than the grouping.
- Bonus: with no pacer of ours underneath a manager you own, the
purgeRequestQueueambiguity stops applying to that case too.
That leaves no instanceof anywhere, which is what actually closes #3999 — the delegation walk from #4038 would have kept BasicCrawler enumerating wrapper types instead, which sucked
…throttling managers hidden under layers of composition
…ecs to throttling managers hidden under layers of composition
Resolves the conflicts with apify#4061, which reshaped the same file. - Stall detection and emptiness are now one answer, `checkReadiness()`, so the fix goes through it: `#checkInnerReadiness()` downgrades the wrapped manager from `ready` to `waiting` while a sweep has found nothing in it but requests whose domains are backing off, and `hasInnerBacklog` keeps a domain whose only work sits there in view of the stall check. - The sweep itself is unchanged apart from the wrapped manager now being resolved lazily: `#getInner()` in `#fetchFromInner`, `#resolvedInner` where the sweep's conclusion is invalidated. - The tests moved from `isEmpty()`, `isFinished()` and `assertNoStalledDomains()` to `checkReadiness()`, and from `recordDomainDelay()` to `recordPacingSignal()`. The stall test now runs on a manager with a backoff long enough to outlive the sweep, because a wrapped manager reporting `ready` outranks a stalling domain by design.
ThrottlingRequestManagerare lost when it is wrapped in another manager #3999inneroption ofThrottlingRequestManageroptional #4027Worth reviewing in order — each change stands alone, with the reasoning in its own message.
checkReadiness()replacesisEmpty()/isFinished()onIRequestLoaderandIRequestManager.IRequestManager.recordPacingSignal()replaces theSupportsDomainThrottlingcapability probe.sameDomainDelaySecsthrottler in its constructor, which is what puts it inside the tandem.sameDomainDelaySecsgoes in throughrecordPacingSignal()as a floor covering every domain, so a manager that already paces takes it instead of getting a second pacer wrapped around it.ThrottlingRequestManager'sinneris optional — omitted, it opens the default queue on first use. Same feature as feat(core): make inner option of ThrottlingRequestManager optional #4037, which is a much smaller patch on top of the lazyinnerfactory this branch already has.On #3999 this takes the first option — wrappers forward — but the objection there was that it makes
RequestManagerTandemknow throttling exists. It doesn't: pacing signals areIRequestManagermembers, so the tandem forwards aPacingSignalit never looks inside, and learns nothing throttling-specific. That deletes the capability-discovery problem rather than solving it, which is why the delegation interface from #4038 isn't here.One capability check did survive that — the guard refusing
sameDomainDelaySecsover aThrottlingRequestManager— and it missed a throttler behind a tandem (#discussion_r3851833671). The fourth change removes it rather than deepening it: the floor is aPacingSignallike any other, so whatever paces takes it wherever it sits in a composition, and a manager pacing only some of its domains throws rather than under-applying it. Noinstanceofleft anywhere.#4039 is the shape
sameDomainDelaySecswas producing on its own: wrapping happened at first use, by which point the tandem for arequestListalready existed, so the pacer went outside it and a list's requests reached the queue without passing a per-domain clock. The third commit builds the pacer in the constructor instead, so those requests are routed by domain like any other. The hand-built form in #4039's reproduction is untouched —fetchNextRequeststill takes whateverinneroffers without checking the clocks — so #4040 is still needed.Migration notes in
docs/upgrading/upgrading_v4.md.