fix: Add is_finished and correct is_empty semantics in RequestQueueClient

[Mantisus]

Description

• Adds an is_finished method to the RequestQueueClient interface. It's not a breaking change. The base class defaults is_finished to is_empty when it isn't overridden. Splits the queue predicates. is_empty means there are no requests available to fetch. is_finished means all requests are fully processed (nothing to fetch and nothing in progress).
• Implements is_finished in all built-in RequestQueueClient.
• Updates is_empty in all built-in RequestQueueClient to match the new meaning.
• Affects the AutoscaledPool. It now skips scheduling new tasks when no requests are available to fetch. For example, while the only request is being processed.

Issues

• Closes: Add an is_finished method to the StorageClient interface #1966
• Closes: Crawler can get softlocked when consuming the RequestQueue #1598

Testing

• Added a test for RequestQueue that checks the is_empty and is_finished values through the queue lifecycle. It covers all built-in clients.
• Added a regression test that checks the AutoscaledPool doesn't schedule new tasks while the only request is being processed.

[janbuchar]

Looks sound, the only thing I'm mildly concerned about is the removal of the is_empty return value caching — I'd feel better if I knew some historical context and why it's safe to do this.

[vdusek]

Thanks! Could you please implement apify/apify-sdk-python#987 as well?

That way, we can verify that it works on the platform too.

I'd like this to be part of SDK v4, built on top of Crawlee v1.8 with this included.

[Mantisus]

Thanks! Could you please implement apify/apify-sdk-python#987 as well?

Sure.

[janbuchar]

Looks about right to me. @vdusek please review it too 🙂

[vdusek]

A few comments

[vdusek]

A few more comments (three nits and one thing that might have a performance impact)

[vdusek]

is_finished calls await self.is_empty() here, which opens a DB session, and then opens a second session below for get_metadata() and the remaining queries. Since the autoscaled pool polls is_finished/is_empty while scheduling requests, this could have a performance impact.

Maybe we could handle the empty check directly inside is_finished, using only a single session?

[Mantisus]

Getting a session from the pool is quite cheap, but it can actually impact performance if the pool is small. However, this will only become apparent when the queue is empty but not yet finished.

But merging is_empty and get_metadata into a single session in is_finished will make the method harder to read.

If we need to optimize these methods, I would consider removing the _add_buffer_record calls in is_empty and is_finished, just to update accessed_at in the metadata. This will have a greater impact on performance.

[vdusek]

I'd probably use a fix prefix for the PR title. It does add a new method, true, but only because its absence is the root cause of the bug. Something like this:

fix: add is_finished and correct is_empty semantics in RequestQueueClient

would be better in my opinion.