Sharing the `RequestManager` instance breaks `maxRequestsPerCrawl` limits

[barjin]

The BasicCrawler.loadHandledRequestCount implementation considers only request sources exclusive to the current crawler instance.

crawlee/packages/basic-crawler/src/internals/basic-crawler.ts

Line 1970 in 72e82cd

this.handledRequestsCount = await this.requestManager.handledCount();

If the request source has been used before (has handledCount > 0), the BasicCrawler.maxRequestsPerCrawl limits won't work correctly, see below:

import { RequestQueueV2, CheerioCrawler } from 'crawlee';

const queue = await RequestQueueV2.open();
    queue.addRequestsBatched(
        Array.from({ length: 100 }, (_, i) => (`https://example.com/page/${i}`)),
    );

for (const crawlerId of [1, 2]) {
    const crawler = new CheerioCrawler({
        requestQueue: queue,
        requestHandler: async ({ request }) => {
            console.log(`[${crawlerId}] Crawling ${request.url}...`);
        },
        maxRequestsPerCrawl: 10,
        maxConcurrency: 1,
    });

    await crawler.run();
}

cc @Pijukatel

[janbuchar]

Just to be clear, the expected behavior is that the crawlers will crawl 10 pages each? If a crawler wakes up after a migration, is it the same crawler or a new one?

[barjin]

the expected behavior is that the crawlers will crawl 10 pages each

IMO yes, these two crawlers could have been of different classes (CheerioCrawler / PlaywrightCrawler) etc. The limit is set on the crawler, not the request source. On the other hand, the current approach will work correctly with .stop() and .run() on a single crawler instance.

a crawler wakes up after a migration, is it the same crawler or a new one?

IMO the only right answer is the same crawler here - as a user, you don't really care for the migrations and if you set maxRequestsPerCrawl to a number, you expect this number of results.

[janbuchar]

That's a bit of a conundrum then. We'd need to make sure that all crawler instances in a script are considered "different", while at the same time, corresponding instances in a separate execution of a script should be considered "equal".

If that's really the right way to go about this, I suppose we could utilize persistence IDs based on instantiation order.

[barjin]

That's actually how we do it in Python (via the Statistics class)

https://github.com/apify/crawlee-python/blob/master/src/crawlee/crawlers/_basic/_basic_crawler.py#L555-L558

Alternatively we could also use the explicit crawler ids from #3309 (v4 only, though).

[janbuchar]

That's actually how we do it in Python (via the Statistics class)

apify/crawlee-python@master/src/crawlee/crawlers/_basic/_basic_crawler.py#L555-L558

Oh yeah, that makes sense. We should definitely unify this and what you describe sounds less confusing.

Alternatively we could also use the explicit crawler ids from #3309 (v4 only, though).

Ultimately, it would be nice if we could use the crawler IDs for the persistence of all crawler-related stuff (like Statistics). But just using the value in Statistics to detect when we have crawled enough is just fine for now.

[B4nan]

Do we need to care about this for v3?