From bd79039b9a83ad8347b28a6d7521d320e980d32b Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Fri, 21 Aug 2026 13:36:29 +0200 Subject: [PATCH 01/17] fix(scrapy): stop silently dropping in-flight requests and redirects --- src/apify/scrapy/_async_thread.py | 28 +++++ src/apify/scrapy/requests.py | 30 ++--- src/apify/scrapy/scheduler.py | 106 ++++++++++++++---- .../scrapy/requests/test_to_apify_request.py | 24 ++++ tests/unit/scrapy/test_async_thread.py | 52 +++++++++ tests/unit/scrapy/test_scheduler.py | 90 ++++++++++++++- 6 files changed, 293 insertions(+), 37 deletions(-) diff --git a/src/apify/scrapy/_async_thread.py b/src/apify/scrapy/_async_thread.py index 90f6f4cb..6c543b27 100644 --- a/src/apify/scrapy/_async_thread.py +++ b/src/apify/scrapy/_async_thread.py @@ -74,6 +74,25 @@ def run_coro( future.cancel() raise + def submit_coro(self, coro: Coroutine) -> None: + """Schedule a coroutine on the event loop without waiting for its result. + + Use this for work whose result nothing depends on, so the calling thread is not blocked by the round + trip. Failures are logged, as there is no caller left to propagate them to, and a coroutine still + pending when `close` runs is cancelled along with the rest. + + Args: + coro: The coroutine to run. + + Raises: + RuntimeError: If the event loop has been closed. + """ + if self._eventloop.is_closed(): + raise RuntimeError(f'The coroutine {coro} cannot be executed because the event loop is closed.') + + future = asyncio.run_coroutine_threadsafe(coro, self._eventloop) + future.add_done_callback(self._log_failure) + def close(self, timeout: timedelta | None = None) -> None: """Close the event loop and its thread gracefully. @@ -110,6 +129,15 @@ def close(self, timeout: timedelta | None = None) -> None: logger.warning('Event loop thread did not exit cleanly! Forcing shutdown...') self._force_exit_event_loop() + @staticmethod + def _log_failure(future: futures.Future) -> None: + """Log the failure of a coroutine submitted via `submit_coro`.""" + if future.cancelled(): + return + + if (exc := future.exception()) is not None: + logger.error('A coroutine submitted to the event loop failed.', exc_info=exc) + def _start_event_loop(self) -> None: """Set up and run the asyncio event loop in the dedicated thread.""" asyncio.set_event_loop(self._eventloop) diff --git a/src/apify/scrapy/requests.py b/src/apify/scrapy/requests.py index 8bf99ec5..e8333886 100644 --- a/src/apify/scrapy/requests.py +++ b/src/apify/scrapy/requests.py @@ -76,8 +76,15 @@ def to_apify_request(scrapy_request: ScrapyRequest, spider: Spider) -> ApifyRequ try: if scrapy_request.dont_filter: request_kwargs['always_enqueue'] = True - elif scrapy_request.meta.get('apify_request_unique_key'): - request_kwargs['unique_key'] = scrapy_request.meta['apify_request_unique_key'] + # Reuse the queue's own unique key only while this is still the very request it was minted for. + # Scrapy derives new requests from a fetched one with `Request.replace()` (redirects) and spiders + # often forward `meta` verbatim to another URL; both inherit the stamp, and reusing it there would + # deduplicate the derived request against its parent and silently drop it. A stamp without a URL + # beside it was set by hand rather than by `to_scrapy_request`, so it is taken at face value. + elif (unique_key := scrapy_request.meta.get('apify_request_unique_key')) and ( + scrapy_request.meta.get('apify_request_url', scrapy_request.url) == scrapy_request.url + ): + request_kwargs['unique_key'] = unique_key # Serialize the Scrapy request now, before `Request.from_url()` runs below. `from_url()` mutates the # `user_data` dict it receives in place (it injects a live `CrawleeRequestData` under `__crawlee`), and that @@ -187,21 +194,14 @@ def to_scrapy_request(apify_request: ApifyRequest, spider: Spider) -> ScrapyRequ if not isinstance(scrapy_request, ScrapyRequest): raise TypeError('scrapy_request must be an instance of the ScrapyRequest class') - # Update the meta field with the meta field from the apify_request - meta = scrapy_request.meta or {} - meta.update({'apify_request_unique_key': apify_request.unique_key}) - # scrapy_request.meta is a property, so we have to set it like this - scrapy_request._meta = meta # noqa: SLF001 - # If the apify_request comes directly from the Scrapy, typically start URLs. else: - scrapy_request = ScrapyRequest( - url=apify_request.url, - method=apify_request.method, - meta={ - 'apify_request_unique_key': apify_request.unique_key, - }, - ) + scrapy_request = ScrapyRequest(url=apify_request.url, method=apify_request.method) + + # Stamp the queue's unique key together with the URL it belongs to, so that `to_apify_request` can tell + # this request apart from the ones Scrapy derives from it. + scrapy_request.meta['apify_request_unique_key'] = apify_request.unique_key + scrapy_request.meta['apify_request_url'] = scrapy_request.url # Add optional 'headers' field if apify_request.headers: diff --git a/src/apify/scrapy/scheduler.py b/src/apify/scrapy/scheduler.py index 0646d3d6..1a9f7924 100644 --- a/src/apify/scrapy/scheduler.py +++ b/src/apify/scrapy/scheduler.py @@ -19,6 +19,8 @@ from scrapy.http.request import Request from twisted.internet.defer import Deferred + from apify import Request as ApifyRequest + logger = getLogger(__name__) @@ -28,7 +30,11 @@ class ApifyScheduler(BaseScheduler): This scheduler requires the asyncio Twisted reactor to be installed. """ - def __init__(self, async_thread_timeout: timedelta = timedelta(seconds=60)) -> None: + def __init__( + self, + async_thread_timeout: timedelta = timedelta(seconds=60), + crawler: Crawler | None = None, + ) -> None: if not is_asyncio_reactor_installed(): raise ValueError( f'{ApifyScheduler.__qualname__} requires the asyncio Twisted reactor. ' @@ -37,6 +43,10 @@ def __init__(self, async_thread_timeout: timedelta = timedelta(seconds=60)) -> N ) self._rq: RequestQueue | None = None self.spider: Spider | None = None + self._crawler = crawler + + self._requests_in_flight: list[tuple[ApifyRequest, Request]] = [] + """Requests handed over to Scrapy and not resolved in the request queue yet.""" # A thread with the asyncio event loop to run coroutines on. self._async_thread = AsyncThread(default_timeout=async_thread_timeout) @@ -49,7 +59,7 @@ def from_crawler(cls, crawler: Crawler) -> ApifyScheduler: background event loop may take before timing out; it defaults to 60 seconds. """ timeout_secs = crawler.settings.getint('APIFY_ASYNC_THREAD_TIMEOUT_SECS', 60) - return cls(async_thread_timeout=timedelta(seconds=timeout_secs)) + return cls(async_thread_timeout=timedelta(seconds=timeout_secs), crawler=crawler) def open(self, spider: Spider) -> Deferred[None] | None: """Open the scheduler. @@ -86,12 +96,26 @@ async def open_rq() -> RequestQueue: def close(self, reason: str) -> None: """Close the scheduler. - Shut down the event loop and its thread gracefully. + Resolve the requests Scrapy still holds, then shut down the event loop and its thread gracefully. Args: reason: The reason for closing the spider. """ logger.debug(f'Closing {self.__class__.__name__} due to {reason}...') + + rq = self._rq + if isinstance(rq, RequestQueue): + try: + # Resolve what Scrapy holds while the event loop is still around. Whatever it did not finish - + # an interrupted run, an Actor migration - goes back to the queue, so the next run picks it up + # instead of waiting for its lock to expire. + self._resolve_finished_requests(wait=True) + for apify_request, _ in self._requests_in_flight: + self._async_thread.run_coro(rq.reclaim_request(apify_request)) + self._requests_in_flight.clear() + except Exception: + logger.exception('Failed to resolve the requests still in flight in the request queue.') + try: self._async_thread.close() @@ -113,6 +137,11 @@ def has_pending_requests(self) -> bool: if not isinstance(self._rq, RequestQueue): raise TypeError('self._rq must be an instance of the RequestQueue class') + # Scrapy asks this only once both its downloader and its scraper are idle, so everything still tracked + # as in flight is provably finished. Wait for those updates to land: the queue reports itself unfinished + # while any request it handed out is still unresolved. + self._resolve_finished_requests(wait=True) + # Log here before re-raising: this coroutine ran on a separate event-loop thread, and the failure is # otherwise easy to lose as it crosses that thread boundary back into Scrapy's synchronous machinery. try: @@ -164,6 +193,10 @@ def next_request(self) -> Request | None: if not isinstance(self._rq, RequestQueue): raise TypeError('self._rq must be an instance of the RequestQueue class') + # Resolve whatever Scrapy has finished since the last call. The engine polls this method throughout the + # crawl, which keeps the queue's view of progress current without blocking on the round trips. + self._resolve_finished_requests(wait=False) + # Log here before re-raising: this coroutine ran on a separate event-loop thread, and the failure is # otherwise easy to lose as it crosses that thread boundary back into Scrapy's synchronous machinery. try: @@ -178,26 +211,61 @@ def next_request(self) -> Request | None: if not isinstance(self.spider, Spider): raise TypeError('self.spider must be an instance of the Spider class') - # Reconstruct the Scrapy request before consuming the queue entry. A malformed entry must not crash - # the whole run, so on failure it is logged and skipped (None) rather than propagating. + # A malformed entry must not crash the whole run, so on failure it is logged and skipped rather than + # propagating. Such an unrecoverable entry (a corrupt or legacy payload) is marked as handled right + # away, otherwise the queue would keep handing it back forever. try: scrapy_request = to_scrapy_request(apify_request, spider=self.spider) except Exception as exc: logger.warning(f'Failed to convert Apify request {apify_request} to a Scrapy request; skipping it: {exc}') - scrapy_request = None - - # Mark the request as handled. This runs even when reconstruction failed above: an unrecoverable entry - # (a corrupt or legacy payload) must still be consumed, otherwise the queue would keep handing it back - # forever. Retrying genuine failures is the RetryMiddleware's job. - # Log here before re-raising: this coroutine ran on a separate event-loop thread, and the failure is - # otherwise easy to lose as it crosses that thread boundary back into Scrapy's synchronous machinery. - try: - self._async_thread.run_coro(self._rq.mark_request_as_handled(apify_request)) - except Exception: - logger.exception('Failed to mark the request as handled in the request queue.') - raise - - if scrapy_request is None: + try: + self._async_thread.run_coro(self._rq.mark_request_as_handled(apify_request)) + except Exception: + logger.exception('Failed to mark the request as handled in the request queue.') + raise return None + # The entry stays unresolved in the queue until Scrapy is done with the request, so a run interrupted + # mid-flight leaves it pending instead of silently handled. + self._requests_in_flight.append((apify_request, scrapy_request)) + return scrapy_request + + def _requests_busy_in_scrapy(self) -> set[Request]: + """Return the requests Scrapy is still working on. + + A request handed out by `next_request` stays in the downloader's or the scraper's active set until its + download, the downloader middleware chain and the spider callback have all finished, so absence from + both means Scrapy is done with it. Requests a middleware drops before the download reach neither set, + but Scrapy only asks `has_pending_requests` once both are empty, which is what settles those too. + """ + engine = self._crawler.engine if self._crawler is not None else None + if engine is None: + return set() + + scraper_slot = engine.scraper.slot + return engine.downloader.active | (scraper_slot.active if scraper_slot is not None else set()) + + def _resolve_finished_requests(self, *, wait: bool) -> None: + """Mark every request Scrapy has finished processing as handled in the request queue. + + Args: + wait: Whether to block until the queue has been updated. Pass False on the crawl's hot path, where + nothing depends on the result and blocking would stall the Twisted reactor. + """ + rq = self._rq + if not self._requests_in_flight or not isinstance(rq, RequestQueue): + return + + busy = self._requests_busy_in_scrapy() + still_in_flight = [] + + for apify_request, scrapy_request in self._requests_in_flight: + if scrapy_request in busy: + still_in_flight.append((apify_request, scrapy_request)) + elif wait: + self._async_thread.run_coro(rq.mark_request_as_handled(apify_request)) + else: + self._async_thread.submit_coro(rq.mark_request_as_handled(apify_request)) + + self._requests_in_flight = still_in_flight diff --git a/tests/unit/scrapy/requests/test_to_apify_request.py b/tests/unit/scrapy/requests/test_to_apify_request.py index 97902f7d..1ce15fe9 100644 --- a/tests/unit/scrapy/requests/test_to_apify_request.py +++ b/tests/unit/scrapy/requests/test_to_apify_request.py @@ -10,6 +10,7 @@ from crawlee._types import HttpHeaders +from apify import Request as ApifyRequest from apify.scrapy.requests import to_apify_request, to_scrapy_request @@ -187,3 +188,26 @@ def test_apify_request_id_in_meta_is_ignored(spider: Spider) -> None: assert apify_request is not None assert apify_request.unique_key == 'https://example.com' + + +def test_redirected_request_does_not_inherit_the_parents_unique_key(spider: Spider) -> None: + """A redirect derived from a fetched request gets its own unique key instead of the parent's stamp.""" + parent = to_scrapy_request(ApifyRequest.from_url('https://example.com/redirect'), spider) + redirected = parent.replace(url='https://example.com/target') + + apify_request = to_apify_request(redirected, spider) + + assert apify_request is not None + assert apify_request.url == 'https://example.com/target' + assert apify_request.unique_key != parent.meta['apify_request_unique_key'] + + +def test_follow_up_request_with_propagated_meta_gets_its_own_unique_key(spider: Spider) -> None: + """A spider callback forwarding `meta` verbatim to another URL must not reuse the parent's unique key.""" + parent = to_scrapy_request(ApifyRequest.from_url('https://example.com/listing'), spider) + follow_up = Request(url='https://example.com/detail', meta=parent.meta) + + apify_request = to_apify_request(follow_up, spider) + + assert apify_request is not None + assert apify_request.unique_key != parent.meta['apify_request_unique_key'] diff --git a/tests/unit/scrapy/test_async_thread.py b/tests/unit/scrapy/test_async_thread.py index 3cf51b62..df0f52c2 100644 --- a/tests/unit/scrapy/test_async_thread.py +++ b/tests/unit/scrapy/test_async_thread.py @@ -161,3 +161,55 @@ async def boom() -> None: # The loop was stopped and its thread joined despite the failing cancellation, so nothing is left running. assert not thread._thread.is_alive() assert thread._eventloop.is_closed() + + +def test_submit_coro_runs_the_coroutine_without_blocking() -> None: + """`submit_coro` schedules the coroutine on the background loop and returns before it completes.""" + thread = AsyncThread() + _wait_until_running(thread) + + release = threading.Event() + finished = threading.Event() + + async def gated() -> None: + await asyncio.to_thread(release.wait) + finished.set() + + thread.submit_coro(gated()) + + # The call returned while the coroutine is still parked on the gate. + assert not finished.is_set() + + release.set() + assert finished.wait(timeout=2) + + thread.close() + + +def test_submit_coro_logs_a_failing_coroutine(caplog: pytest.LogCaptureFixture) -> None: + """A coroutine submitted without a caller to propagate to has its failure logged instead of swallowed.""" + thread = AsyncThread() + _wait_until_running(thread) + + async def boom() -> None: + raise RuntimeError('boom') + + with caplog.at_level(logging.ERROR, logger='apify.scrapy._async_thread'): + thread.submit_coro(boom()) + thread.close() + + errors = [record for record in caplog.records if record.levelno >= logging.ERROR] + assert len(errors) == 1 + assert errors[0].exc_info is not None + assert str(errors[0].exc_info[1]) == 'boom' + + +def test_submit_coro_raises_after_close() -> None: + """`submit_coro` raises `RuntimeError` once the loop has been closed.""" + thread = AsyncThread() + thread.close() + + coro = _return(42) + with pytest.raises(RuntimeError): + thread.submit_coro(coro) + coro.close() diff --git a/tests/unit/scrapy/test_scheduler.py b/tests/unit/scrapy/test_scheduler.py index a7cc4445..5413a3ac 100644 --- a/tests/unit/scrapy/test_scheduler.py +++ b/tests/unit/scrapy/test_scheduler.py @@ -25,6 +25,15 @@ def spider() -> DummySpider: return DummySpider() +def fake_crawler(busy: set[Request]) -> Any: + """Build a crawler stub whose engine reports `busy` as the requests Scrapy is still working on.""" + engine = SimpleNamespace( + downloader=SimpleNamespace(active=busy), + scraper=SimpleNamespace(slot=None), + ) + return SimpleNamespace(engine=engine) + + @pytest.fixture def scheduler(monkeypatch: pytest.MonkeyPatch, spider: DummySpider) -> ApifyScheduler: """Create a scheduler with its reactor check and async thread stubbed out.""" @@ -124,8 +133,7 @@ def test_next_request_skips_request_that_fails_to_convert( def test_next_request_returns_converted_request(scheduler: ApifyScheduler) -> None: - """A valid queue entry is reconstructed into a Scrapy request and marked handled.""" - rq = cast('mock.MagicMock', scheduler._rq) + """A valid queue entry is reconstructed into a Scrapy request.""" async_thread = cast('mock.MagicMock', scheduler._async_thread) apify_request = ApifyRequest( @@ -140,7 +148,6 @@ def test_next_request_returns_converted_request(scheduler: ApifyScheduler) -> No assert isinstance(result, Request) assert result.url == apify_request.url - rq.mark_request_as_handled.assert_called_once_with(apify_request) def test_next_request_returns_none_when_queue_empty(scheduler: ApifyScheduler) -> None: @@ -190,3 +197,80 @@ def __init__(self, default_timeout: timedelta | None = None) -> None: ApifyScheduler.from_crawler(cast('Any', crawler)) assert captured['default_timeout'] == timedelta(seconds=123) + + +def test_next_request_leaves_the_request_unhandled(scheduler: ApifyScheduler) -> None: + """A request handed to Scrapy stays unhandled in the queue until Scrapy has finished processing it.""" + rq = cast('mock.MagicMock', scheduler._rq) + async_thread = cast('mock.MagicMock', scheduler._async_thread) + + apify_request = ApifyRequest( + url='https://example.com', + method='GET', + unique_key='https://example.com', + user_data={}, + ) + async_thread.run_coro.return_value = apify_request + + result = scheduler.next_request() + + assert isinstance(result, Request) + rq.mark_request_as_handled.assert_not_called() + + +def test_has_pending_requests_marks_finished_requests_as_handled(scheduler: ApifyScheduler) -> None: + """Requests Scrapy has finished with are marked as handled once it goes idle and asks about pending work.""" + rq = cast('mock.MagicMock', scheduler._rq) + async_thread = cast('mock.MagicMock', scheduler._async_thread) + + apify_request = ApifyRequest( + url='https://example.com', + method='GET', + unique_key='https://example.com', + user_data={}, + ) + async_thread.run_coro.return_value = apify_request + scheduler.next_request() + rq.mark_request_as_handled.assert_not_called() + + # Scrapy asks about pending work only once it is idle, which is also how a request a middleware dropped + # before the download - an offsite or robots.txt denial - is settled. + scheduler._crawler = fake_crawler(busy=set()) + async_thread.run_coro.return_value = True # the queue reports itself finished + assert scheduler.has_pending_requests() is False + + rq.mark_request_as_handled.assert_called_once_with(apify_request) + + +def test_close_reclaims_requests_scrapy_never_finished(scheduler: ApifyScheduler) -> None: + """Requests still being processed when the scheduler closes go back to the queue instead of being lost.""" + rq = cast('mock.MagicMock', scheduler._rq) + async_thread = cast('mock.MagicMock', scheduler._async_thread) + + apify_request = ApifyRequest( + url='https://example.com', + method='GET', + unique_key='https://example.com', + user_data={}, + ) + async_thread.run_coro.return_value = apify_request + scrapy_request = scheduler.next_request() + + # Scrapy is still downloading the request when the run is interrupted. + scheduler._crawler = fake_crawler(busy={cast('Request', scrapy_request)}) + + scheduler.close('shutdown') + + rq.reclaim_request.assert_called_once_with(apify_request) + rq.mark_request_as_handled.assert_not_called() + + +def test_from_crawler_keeps_the_crawler(monkeypatch: pytest.MonkeyPatch) -> None: + """`from_crawler` keeps the crawler, which is how the scheduler learns what Scrapy is still working on.""" + monkeypatch.setattr('apify.scrapy.scheduler.is_asyncio_reactor_installed', lambda: True) + monkeypatch.setattr('apify.scrapy.scheduler.AsyncThread', mock.MagicMock()) + + crawler = SimpleNamespace(settings=Settings()) + scheduler = ApifyScheduler.from_crawler(cast('Any', crawler)) + + assert scheduler._crawler is crawler From 08251656e1d86212c18bb6cb27317ed2d6b5e343 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Fri, 21 Aug 2026 18:50:03 +0200 Subject: [PATCH 02/17] fix(scrapy): resolve request queue updates reliably on the shutdown and failure paths --- src/apify/scrapy/_async_thread.py | 35 ++++- src/apify/scrapy/scheduler.py | 63 ++++++--- tests/unit/scrapy/test_async_thread.py | 64 ++++++++- tests/unit/scrapy/test_scheduler.py | 182 +++++++++++++++++++++---- 4 files changed, 300 insertions(+), 44 deletions(-) diff --git a/src/apify/scrapy/_async_thread.py b/src/apify/scrapy/_async_thread.py index 6c543b27..3e3d57ee 100644 --- a/src/apify/scrapy/_async_thread.py +++ b/src/apify/scrapy/_async_thread.py @@ -12,6 +12,9 @@ logger = getLogger(__name__) +_SUBMITTED_PRUNE_THRESHOLD = 128 +"""How many `submit_coro` futures may pile up before the finished ones are dropped from the tracking list.""" + class AsyncThread: """Run an asyncio event loop in a dedicated background thread. @@ -26,6 +29,9 @@ def __init__(self, default_timeout: timedelta = timedelta(seconds=60)) -> None: self._default_timeout = default_timeout self._eventloop = asyncio.new_event_loop() + self._submitted: list[futures.Future] = [] + """Futures of the coroutines submitted via `submit_coro` that may still be running.""" + # Start the event loop in a dedicated daemon thread. self._thread = threading.Thread( target=self._start_event_loop, @@ -79,7 +85,8 @@ def submit_coro(self, coro: Coroutine) -> None: Use this for work whose result nothing depends on, so the calling thread is not blocked by the round trip. Failures are logged, as there is no caller left to propagate them to, and a coroutine still - pending when `close` runs is cancelled along with the rest. + pending when `close` runs is cancelled along with the rest - call `wait_for_submitted` before anything + that must not see that happen. Args: coro: The coroutine to run. @@ -90,8 +97,34 @@ def submit_coro(self, coro: Coroutine) -> None: if self._eventloop.is_closed(): raise RuntimeError(f'The coroutine {coro} cannot be executed because the event loop is closed.') + # Drop the futures that already finished. `wait_for_submitted` only runs once Scrapy goes idle, so + # without this the list would hold every coroutine the whole crawl ever submitted, with its result. + if len(self._submitted) >= _SUBMITTED_PRUNE_THRESHOLD: + self._submitted = [submitted for submitted in self._submitted if not submitted.done()] + future = asyncio.run_coroutine_threadsafe(coro, self._eventloop) future.add_done_callback(self._log_failure) + self._submitted.append(future) + + def wait_for_submitted(self, timeout: timedelta | None = None) -> None: + """Block until the coroutines submitted via `submit_coro` have finished. + + Use this before anything that would observe their effects, or before `close`, which cancels whatever is + still running. Coroutines that do not finish within the timeout stay tracked for the next call. + + Args: + timeout: The maximum time to wait for the submitted coroutines. Pass `None` to use the + `default_timeout` passed to the constructor. + """ + if timeout is None: + timeout = self._default_timeout + + self._submitted = list(futures.wait(self._submitted, timeout=timeout.total_seconds()).not_done) + + # Returning with coroutines still pending breaks the guarantee the callers rely on, so say so rather + # than letting them act on effects that have not landed. + if self._submitted: + logger.warning(f'{len(self._submitted)} submitted coroutines did not finish within the timeout.') def close(self, timeout: timedelta | None = None) -> None: """Close the event loop and its thread gracefully. diff --git a/src/apify/scrapy/scheduler.py b/src/apify/scrapy/scheduler.py index 1a9f7924..94096176 100644 --- a/src/apify/scrapy/scheduler.py +++ b/src/apify/scrapy/scheduler.py @@ -106,16 +106,25 @@ def close(self, reason: str) -> None: rq = self._rq if isinstance(rq, RequestQueue): try: - # Resolve what Scrapy holds while the event loop is still around. Whatever it did not finish - - # an interrupted run, an Actor migration - goes back to the queue, so the next run picks it up - # instead of waiting for its lock to expire. self._resolve_finished_requests(wait=True) - for apify_request, _ in self._requests_in_flight: - self._async_thread.run_coro(rq.reclaim_request(apify_request)) - self._requests_in_flight.clear() except Exception: logger.exception('Failed to resolve the requests still in flight in the request queue.') + # Whatever Scrapy did not finish - an interrupted run, an Actor migration - goes back to the queue + # while the event loop is still around, so the next run gets it as pending. Each request is + # reclaimed on its own, so one failure does not strand the rest. + for apify_request, _ in self._requests_in_flight: + try: + self._async_thread.run_coro(rq.reclaim_request(apify_request)) + except Exception: + logger.exception(f'Failed to reclaim the request {apify_request} in the request queue.') + + self._requests_in_flight.clear() + + # Let the updates fired off on the hot path finish: closing the event loop cancels them silently, which + # would leave those requests unhandled in the queue. + self._async_thread.wait_for_submitted() + try: self._async_thread.close() @@ -131,6 +140,8 @@ def close(self, reason: str) -> None: def has_pending_requests(self) -> bool: """Check if the scheduler has any pending requests. + Resolves the requests Scrapy has finished with first, as their outcome is what decides the answer. + Returns: True if the scheduler has any pending requests, False otherwise. """ @@ -138,10 +149,13 @@ def has_pending_requests(self) -> bool: raise TypeError('self._rq must be an instance of the RequestQueue class') # Scrapy asks this only once both its downloader and its scraper are idle, so everything still tracked - # as in flight is provably finished. Wait for those updates to land: the queue reports itself unfinished - # while any request it handed out is still unresolved. + # as in flight is provably finished. self._resolve_finished_requests(wait=True) + # The queue answers from its own bookkeeping, so an update still in flight would let it report itself + # finished while a request is unhandled - and closing the crawl would then cancel that update. + self._async_thread.wait_for_submitted() + # Log here before re-raising: this coroutine ran on a separate event-loop thread, and the failure is # otherwise easy to lose as it crosses that thread boundary back into Scrapy's synchronous machinery. try: @@ -234,10 +248,10 @@ def next_request(self) -> Request | None: def _requests_busy_in_scrapy(self) -> set[Request]: """Return the requests Scrapy is still working on. - A request handed out by `next_request` stays in the downloader's or the scraper's active set until its - download, the downloader middleware chain and the spider callback have all finished, so absence from - both means Scrapy is done with it. Requests a middleware drops before the download reach neither set, - but Scrapy only asks `has_pending_requests` once both are empty, which is what settles those too. + A request handed out by `next_request` joins the downloader's active set before the middleware chain + runs, and leaves the scraper's only once the spider callback and the item pipeline have finished with + it. Absence from both therefore means Scrapy is done with the request, whether it was downloaded, + dropped by a middleware or errored out. """ engine = self._crawler.engine if self._crawler is not None else None if engine is None: @@ -249,6 +263,11 @@ def _requests_busy_in_scrapy(self) -> set[Request]: def _resolve_finished_requests(self, *, wait: bool) -> None: """Mark every request Scrapy has finished processing as handled in the request queue. + Only a failure to dispatch the update - a timed-out or closed event loop - keeps a request tracked for + the next call to retry, without stopping the rest of the list from being resolved. The queue reports + the update's own failures by returning `None`, which is indistinguishable from success here, so such a + request is left unhandled for the next run to pick up. + Args: wait: Whether to block until the queue has been updated. Pass False on the crawl's hot path, where nothing depends on the result and blocking would stall the Twisted reactor. @@ -258,14 +277,20 @@ def _resolve_finished_requests(self, *, wait: bool) -> None: return busy = self._requests_busy_in_scrapy() - still_in_flight = [] + unresolved: list[tuple[ApifyRequest, Request]] = [] for apify_request, scrapy_request in self._requests_in_flight: if scrapy_request in busy: - still_in_flight.append((apify_request, scrapy_request)) - elif wait: - self._async_thread.run_coro(rq.mark_request_as_handled(apify_request)) - else: - self._async_thread.submit_coro(rq.mark_request_as_handled(apify_request)) + unresolved.append((apify_request, scrapy_request)) + continue + + try: + if wait: + self._async_thread.run_coro(rq.mark_request_as_handled(apify_request)) + else: + self._async_thread.submit_coro(rq.mark_request_as_handled(apify_request)) + except Exception: + logger.exception(f'Failed to mark the request {apify_request} as handled in the request queue.') + unresolved.append((apify_request, scrapy_request)) - self._requests_in_flight = still_in_flight + self._requests_in_flight = unresolved diff --git a/tests/unit/scrapy/test_async_thread.py b/tests/unit/scrapy/test_async_thread.py index df0f52c2..994db6e8 100644 --- a/tests/unit/scrapy/test_async_thread.py +++ b/tests/unit/scrapy/test_async_thread.py @@ -11,7 +11,7 @@ import pytest from ..._utils import poll_until_condition -from apify.scrapy._async_thread import AsyncThread +from apify.scrapy._async_thread import _SUBMITTED_PRUNE_THRESHOLD, AsyncThread async def _return(value: int) -> int: @@ -213,3 +213,65 @@ def test_submit_coro_raises_after_close() -> None: with pytest.raises(RuntimeError): thread.submit_coro(coro) coro.close() + + +def test_wait_for_submitted_blocks_until_the_coroutines_finish() -> None: + """`wait_for_submitted` waits for the fire-and-forget coroutines, so `close` cannot cancel them.""" + thread = AsyncThread() + _wait_until_running(thread) + + release = threading.Event() + finished = threading.Event() + + async def gated() -> None: + await asyncio.to_thread(release.wait) + finished.set() + + thread.submit_coro(gated()) + release.set() + + thread.wait_for_submitted() + + assert finished.is_set() + thread.close() + + +def test_wait_for_submitted_keeps_an_unfinished_coroutine_tracked(caplog: pytest.LogCaptureFixture) -> None: + """A coroutine that outlasts the timeout stays tracked and is reported, so a later call can wait for it.""" + thread = AsyncThread() + _wait_until_running(thread) + + release = threading.Event() + + async def gated() -> None: + await asyncio.to_thread(release.wait) + + thread.submit_coro(gated()) + + with caplog.at_level(logging.WARNING, logger='apify.scrapy._async_thread'): + thread.wait_for_submitted(timeout=timedelta(seconds=0.01)) + assert len(thread._submitted) == 1 + assert [record for record in caplog.records if record.levelno == logging.WARNING] + + release.set() + thread.wait_for_submitted() + assert thread._submitted == [] + + thread.close() + + +def test_submit_coro_drops_the_finished_futures() -> None: + """Only the coroutines still running stay tracked, so a long crawl does not pile up finished futures.""" + thread = AsyncThread() + _wait_until_running(thread) + + for _ in range(_SUBMITTED_PRUNE_THRESHOLD): + thread.submit_coro(_return(1)) + + assert futures.wait(list(thread._submitted), timeout=2).not_done == set() + + # Every tracked coroutine has finished, so this submission drops them instead of growing the list. + thread.submit_coro(_return(1)) + assert len(thread._submitted) == 1 + + thread.close() diff --git a/tests/unit/scrapy/test_scheduler.py b/tests/unit/scrapy/test_scheduler.py index 5413a3ac..c4032955 100644 --- a/tests/unit/scrapy/test_scheduler.py +++ b/tests/unit/scrapy/test_scheduler.py @@ -25,11 +25,16 @@ def spider() -> DummySpider: return DummySpider() -def fake_crawler(busy: set[Request]) -> Any: - """Build a crawler stub whose engine reports `busy` as the requests Scrapy is still working on.""" +def fake_crawler( + *, + downloader_busy: set[Request] | None = None, + scraper_busy: set[Request] | None = None, +) -> Any: + """Build a crawler stub reporting the given requests as busy; without `scraper_busy` its scraper slot is None.""" + scraper_slot = SimpleNamespace(active=scraper_busy) if scraper_busy is not None else None engine = SimpleNamespace( - downloader=SimpleNamespace(active=busy), - scraper=SimpleNamespace(slot=None), + downloader=SimpleNamespace(active=downloader_busy if downloader_busy is not None else set()), + scraper=SimpleNamespace(slot=scraper_slot), ) return SimpleNamespace(engine=engine) @@ -133,7 +138,8 @@ def test_next_request_skips_request_that_fails_to_convert( def test_next_request_returns_converted_request(scheduler: ApifyScheduler) -> None: - """A valid queue entry is reconstructed into a Scrapy request.""" + """A valid queue entry is reconstructed into a Scrapy request and left unhandled until Scrapy is done.""" + rq = cast('mock.MagicMock', scheduler._rq) async_thread = cast('mock.MagicMock', scheduler._async_thread) apify_request = ApifyRequest( @@ -142,12 +148,13 @@ def test_next_request_returns_converted_request(scheduler: ApifyScheduler) -> No unique_key='https://example.com', user_data={}, ) - async_thread.run_coro.side_effect = [apify_request, None] + async_thread.run_coro.return_value = apify_request result = scheduler.next_request() assert isinstance(result, Request) assert result.url == apify_request.url + rq.mark_request_as_handled.assert_not_called() def test_next_request_returns_none_when_queue_empty(scheduler: ApifyScheduler) -> None: @@ -199,8 +206,8 @@ def __init__(self, default_timeout: timedelta | None = None) -> None: assert captured['default_timeout'] == timedelta(seconds=123) -def test_next_request_leaves_the_request_unhandled(scheduler: ApifyScheduler) -> None: - """A request handed to Scrapy stays unhandled in the queue until Scrapy has finished processing it.""" +def test_has_pending_requests_marks_finished_requests_as_handled(scheduler: ApifyScheduler) -> None: + """Requests Scrapy has finished with are marked as handled once it goes idle and asks about pending work.""" rq = cast('mock.MagicMock', scheduler._rq) async_thread = cast('mock.MagicMock', scheduler._async_thread) @@ -211,15 +218,19 @@ def test_next_request_leaves_the_request_unhandled(scheduler: ApifyScheduler) -> user_data={}, ) async_thread.run_coro.return_value = apify_request + scheduler.next_request() + rq.mark_request_as_handled.assert_not_called() - result = scheduler.next_request() + # Scrapy asks about pending work only once its downloader and its scraper are both idle. + scheduler._crawler = fake_crawler(scraper_busy=set()) + async_thread.run_coro.return_value = True # the queue reports itself finished + assert scheduler.has_pending_requests() is False - assert isinstance(result, Request) - rq.mark_request_as_handled.assert_not_called() + rq.mark_request_as_handled.assert_called_once_with(apify_request) -def test_has_pending_requests_marks_finished_requests_as_handled(scheduler: ApifyScheduler) -> None: - """Requests Scrapy has finished with are marked as handled once it goes idle and asks about pending work.""" +def test_next_request_marks_finished_requests_without_blocking(scheduler: ApifyScheduler) -> None: + """On the crawl's hot path a finished request is marked as handled without blocking the reactor on it.""" rq = cast('mock.MagicMock', scheduler._rq) async_thread = cast('mock.MagicMock', scheduler._async_thread) @@ -230,19 +241,47 @@ def test_has_pending_requests_marks_finished_requests_as_handled(scheduler: Apif user_data={}, ) async_thread.run_coro.return_value = apify_request - scheduler.next_request() - rq.mark_request_as_handled.assert_not_called() + scrapy_request = scheduler.next_request() - # Scrapy asks about pending work only once it is idle, which is also how a request a middleware dropped - # before the download - an offsite or robots.txt denial - is settled. - scheduler._crawler = fake_crawler(busy=set()) - async_thread.run_coro.return_value = True # the queue reports itself finished - assert scheduler.has_pending_requests() is False + # The queue is drained from here on, so no further request is handed out. + async_thread.run_coro.return_value = None + + # Scrapy is still downloading the request, so it stays unresolved. + scheduler._crawler = fake_crawler(downloader_busy={cast('Request', scrapy_request)}) + assert scheduler.next_request() is None + async_thread.submit_coro.assert_not_called() + + # Scrapy is done with it, so it is resolved off the reactor thread instead of blocking on the round trip. + scheduler._crawler = fake_crawler(scraper_busy=set()) + assert scheduler.next_request() is None rq.mark_request_as_handled.assert_called_once_with(apify_request) + async_thread.submit_coro.assert_called_once_with(rq.mark_request_as_handled.return_value) + + +def test_has_pending_requests_waits_for_the_non_blocking_updates(scheduler: ApifyScheduler) -> None: + """The queue is asked whether it is finished only after the updates fired off on the hot path have landed.""" + async_thread = cast('mock.MagicMock', scheduler._async_thread) + scheduler._crawler = fake_crawler() + async_thread.run_coro.return_value = True # the queue reports itself finished + assert scheduler.has_pending_requests() is False -def test_close_reclaims_requests_scrapy_never_finished(scheduler: ApifyScheduler) -> None: + # The queue answers from its own bookkeeping, which a pending update has not reached yet. + assert async_thread.mock_calls == [ + mock.call.wait_for_submitted(), + mock.call.run_coro(cast('mock.MagicMock', scheduler._rq).is_finished()), + ] + + +@pytest.mark.parametrize( + 'busy_kwarg', + [ + pytest.param('downloader_busy', id='busy in the downloader'), + pytest.param('scraper_busy', id='busy in the scraper slot'), + ], +) +def test_close_reclaims_requests_scrapy_never_finished(scheduler: ApifyScheduler, busy_kwarg: str) -> None: """Requests still being processed when the scheduler closes go back to the queue instead of being lost.""" rq = cast('mock.MagicMock', scheduler._rq) async_thread = cast('mock.MagicMock', scheduler._async_thread) @@ -256,8 +295,8 @@ def test_close_reclaims_requests_scrapy_never_finished(scheduler: ApifyScheduler async_thread.run_coro.return_value = apify_request scrapy_request = scheduler.next_request() - # Scrapy is still downloading the request when the run is interrupted. - scheduler._crawler = fake_crawler(busy={cast('Request', scrapy_request)}) + # Scrapy is still working on the request when the run is interrupted. + scheduler._crawler = fake_crawler(**{busy_kwarg: {cast('Request', scrapy_request)}}) scheduler.close('shutdown') @@ -265,6 +304,103 @@ def test_close_reclaims_requests_scrapy_never_finished(scheduler: ApifyScheduler rq.mark_request_as_handled.assert_not_called() +def test_close_marks_the_requests_scrapy_finished_as_handled(scheduler: ApifyScheduler) -> None: + """Requests Scrapy drained before the shutdown are marked as handled rather than reclaimed.""" + rq = cast('mock.MagicMock', scheduler._rq) + async_thread = cast('mock.MagicMock', scheduler._async_thread) + + apify_request = ApifyRequest( + url='https://example.com', + method='GET', + unique_key='https://example.com', + user_data={}, + ) + async_thread.run_coro.return_value = apify_request + scheduler.next_request() + + # Scrapy drains its downloader and its scraper before the scheduler is closed. + scheduler._crawler = fake_crawler(scraper_busy=set()) + + scheduler.close('finished') + + rq.mark_request_as_handled.assert_called_once_with(apify_request) + rq.reclaim_request.assert_not_called() + + +def test_close_reclaims_the_other_requests_after_a_failed_reclaim( + scheduler: ApifyScheduler, + caplog: pytest.LogCaptureFixture, +) -> None: + """One failing reclaim does not stop the other in-flight requests from going back to the queue.""" + rq = cast('mock.MagicMock', scheduler._rq) + async_thread = cast('mock.MagicMock', scheduler._async_thread) + + apify_requests = [ + ApifyRequest( + url=f'https://example.com/{index}', + method='GET', + unique_key=f'https://example.com/{index}', + user_data={}, + ) + for index in range(2) + ] + + # The crawler stub keeps a reference to this set, so both requests stay busy as they are handed out. + busy: set[Request] = set() + scheduler._crawler = fake_crawler(downloader_busy=busy) + + async_thread.run_coro.side_effect = apify_requests + for _ in apify_requests: + busy.add(cast('Request', scheduler.next_request())) + + async_thread.run_coro.side_effect = [RuntimeError('boom'), None] + + with caplog.at_level(logging.ERROR, logger='apify.scrapy.scheduler'): + scheduler.close('shutdown') + + assert rq.reclaim_request.call_count == len(apify_requests) + errors = [record for record in caplog.records if record.levelno >= logging.ERROR] + assert len(errors) == 1 + + +def test_close_waits_for_the_non_blocking_updates(scheduler: ApifyScheduler) -> None: + """The event loop is not torn down before the updates fired off on the hot path have landed.""" + async_thread = cast('mock.MagicMock', scheduler._async_thread) + + scheduler.close('finished') + + calls = async_thread.mock_calls + assert calls.index(mock.call.wait_for_submitted()) < calls.index(mock.call.close()) + + +def test_a_failed_mark_keeps_the_request_tracked( + scheduler: ApifyScheduler, + caplog: pytest.LogCaptureFixture, +) -> None: + """A request whose mark-as-handled fails stays tracked, so the next resolution retries it.""" + async_thread = cast('mock.MagicMock', scheduler._async_thread) + + apify_request = ApifyRequest( + url='https://example.com', + method='GET', + unique_key='https://example.com', + user_data={}, + ) + async_thread.run_coro.return_value = apify_request + scheduler.next_request() + + scheduler._crawler = fake_crawler() + # The mark fails, then the queue reports itself unfinished because the request is still in progress. + async_thread.run_coro.side_effect = [RuntimeError('boom'), False] + + with caplog.at_level(logging.ERROR, logger='apify.scrapy.scheduler'): + assert scheduler.has_pending_requests() is True + + assert scheduler._requests_in_flight + errors = [record for record in caplog.records if record.levelno >= logging.ERROR] + assert len(errors) == 1 + + def test_from_crawler_keeps_the_crawler(monkeypatch: pytest.MonkeyPatch) -> None: """`from_crawler` keeps the crawler, which is how the scheduler learns what Scrapy is still working on.""" monkeypatch.setattr('apify.scrapy.scheduler.is_asyncio_reactor_installed', lambda: True) From bc88a719cf4bf73dd4ee1ee2b318bf8a790c3f0c Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Fri, 21 Aug 2026 18:50:12 +0200 Subject: [PATCH 03/17] test(scrapy): cover the unique-key stamp round-trip --- .../unit/scrapy/requests/test_to_apify_request.py | 10 ++++++++++ .../scrapy/requests/test_to_scrapy_request.py | 15 +++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/tests/unit/scrapy/requests/test_to_apify_request.py b/tests/unit/scrapy/requests/test_to_apify_request.py index 1ce15fe9..754e0290 100644 --- a/tests/unit/scrapy/requests/test_to_apify_request.py +++ b/tests/unit/scrapy/requests/test_to_apify_request.py @@ -190,6 +190,16 @@ def test_apify_request_id_in_meta_is_ignored(spider: Spider) -> None: assert apify_request.unique_key == 'https://example.com' +def test_unchanged_request_keeps_the_unique_key_it_was_stamped_with(spider: Spider) -> None: + """A request handed to Scrapy and enqueued again unchanged reuses the unique key it was minted for.""" + scrapy_request = to_scrapy_request(ApifyRequest.from_url('https://example.com'), spider) + + apify_request = to_apify_request(scrapy_request, spider) + + assert apify_request is not None + assert apify_request.unique_key == scrapy_request.meta['apify_request_unique_key'] + + def test_redirected_request_does_not_inherit_the_parents_unique_key(spider: Spider) -> None: """A redirect derived from a fetched request gets its own unique key instead of the parent's stamp.""" parent = to_scrapy_request(ApifyRequest.from_url('https://example.com/redirect'), spider) diff --git a/tests/unit/scrapy/requests/test_to_scrapy_request.py b/tests/unit/scrapy/requests/test_to_scrapy_request.py index 898312f2..c3803d7b 100644 --- a/tests/unit/scrapy/requests/test_to_scrapy_request.py +++ b/tests/unit/scrapy/requests/test_to_scrapy_request.py @@ -68,6 +68,21 @@ def test_without_reconstruction(spider: Spider) -> None: assert apify_request.unique_key == scrapy_request.meta.get('apify_request_unique_key') +def test_unique_key_is_stamped_together_with_its_url(spider: Spider) -> None: + """The queue's unique key is stamped alongside the URL it belongs to, so derived requests can be told apart.""" + apify_request = ApifyRequest( + url='https://example.com', + method='GET', + unique_key='https://example.com', + user_data={}, + ) + + scrapy_request = to_scrapy_request(apify_request, spider) + + assert scrapy_request.meta['apify_request_unique_key'] == apify_request.unique_key + assert scrapy_request.meta['apify_request_url'] == scrapy_request.url + + def test_without_reconstruction_with_optional_fields(spider: Spider) -> None: """The without-reconstruction path also carries optional headers and user data to the Scrapy request.""" apify_request = ApifyRequest( From 4ee5615380dd5d77e199df3ba23178e7c49474ef Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Fri, 21 Aug 2026 18:50:20 +0200 Subject: [PATCH 04/17] docs: explain what happens to Scrapy requests after a migration --- docs/03_guides/06_scrapy.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/03_guides/06_scrapy.mdx b/docs/03_guides/06_scrapy.mdx index 49755de6..53d909e1 100644 --- a/docs/03_guides/06_scrapy.mdx +++ b/docs/03_guides/06_scrapy.mdx @@ -104,7 +104,7 @@ The following example shows a Scrapy Actor that scrapes page titles and enqueues ## Dealing with imminent migration to another host -Under some circumstances, the platform may decide to [migrate your Actor](https://docs.apify.com/academy/expert-scraping-with-apify/migrations-maintaining-state) from one piece of infrastructure to another while it's in progress. While [Crawlee](https://crawlee.dev/python)-based projects can pause and resume their work after a restart, achieving the same with a Scrapy-based project can be challenging. +Under some circumstances, the platform may decide to [migrate your Actor](https://docs.apify.com/academy/expert-scraping-with-apify/migrations-maintaining-state) from one piece of infrastructure to another while it's in progress. Requests that Scrapy hasn't finished when the run stops stay unhandled in the request queue, so the next run picks them up and downloads them from scratch. A Scrapy-based project doesn't resume where it left off the way a [Crawlee](https://crawlee.dev/python)-based one does, so items their callbacks already pushed can land in the dataset twice. As a workaround for this issue (tracked as [apify/actor-templates#303](https://github.com/apify/actor-templates/issues/303)), turn on caching with `HTTPCACHE_ENABLED` and set `HTTPCACHE_EXPIRATION_SECS` to at least a few minutes—the exact value depends on your use case. If your Actor gets migrated and restarted, the subsequent run will hit the cache, making it fast and avoiding unnecessary resource consumption. From 31391ed197b14376ca77a4ea453f9488126869ed Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Fri, 21 Aug 2026 19:15:13 +0200 Subject: [PATCH 05/17] refactor(scrapy): tighten comments and rename SUBMITTED_PRUNE_THRESHOLD --- src/apify/scrapy/_async_thread.py | 16 +++++------ src/apify/scrapy/requests.py | 13 ++++----- src/apify/scrapy/scheduler.py | 38 +++++++++++--------------- tests/unit/scrapy/test_async_thread.py | 4 +-- 4 files changed, 31 insertions(+), 40 deletions(-) diff --git a/src/apify/scrapy/_async_thread.py b/src/apify/scrapy/_async_thread.py index 3e3d57ee..d35dec3a 100644 --- a/src/apify/scrapy/_async_thread.py +++ b/src/apify/scrapy/_async_thread.py @@ -12,7 +12,7 @@ logger = getLogger(__name__) -_SUBMITTED_PRUNE_THRESHOLD = 128 +SUBMITTED_PRUNE_THRESHOLD = 128 """How many `submit_coro` futures may pile up before the finished ones are dropped from the tracking list.""" @@ -84,9 +84,8 @@ def submit_coro(self, coro: Coroutine) -> None: """Schedule a coroutine on the event loop without waiting for its result. Use this for work whose result nothing depends on, so the calling thread is not blocked by the round - trip. Failures are logged, as there is no caller left to propagate them to, and a coroutine still - pending when `close` runs is cancelled along with the rest - call `wait_for_submitted` before anything - that must not see that happen. + trip. Failures are logged, as no caller is left to propagate them to, and `close` cancels whatever is + still pending - call `wait_for_submitted` first if that matters. Args: coro: The coroutine to run. @@ -97,9 +96,9 @@ def submit_coro(self, coro: Coroutine) -> None: if self._eventloop.is_closed(): raise RuntimeError(f'The coroutine {coro} cannot be executed because the event loop is closed.') - # Drop the futures that already finished. `wait_for_submitted` only runs once Scrapy goes idle, so - # without this the list would hold every coroutine the whole crawl ever submitted, with its result. - if len(self._submitted) >= _SUBMITTED_PRUNE_THRESHOLD: + # `wait_for_submitted` only runs once Scrapy goes idle, so without pruning here the list would hold + # every coroutine the whole crawl ever submitted, with its result. + if len(self._submitted) >= SUBMITTED_PRUNE_THRESHOLD: self._submitted = [submitted for submitted in self._submitted if not submitted.done()] future = asyncio.run_coroutine_threadsafe(coro, self._eventloop) @@ -121,8 +120,7 @@ def wait_for_submitted(self, timeout: timedelta | None = None) -> None: self._submitted = list(futures.wait(self._submitted, timeout=timeout.total_seconds()).not_done) - # Returning with coroutines still pending breaks the guarantee the callers rely on, so say so rather - # than letting them act on effects that have not landed. + # Callers rely on the effects having landed, so a timeout has to be visible. if self._submitted: logger.warning(f'{len(self._submitted)} submitted coroutines did not finish within the timeout.') diff --git a/src/apify/scrapy/requests.py b/src/apify/scrapy/requests.py index e8333886..caa62413 100644 --- a/src/apify/scrapy/requests.py +++ b/src/apify/scrapy/requests.py @@ -76,11 +76,10 @@ def to_apify_request(scrapy_request: ScrapyRequest, spider: Spider) -> ApifyRequ try: if scrapy_request.dont_filter: request_kwargs['always_enqueue'] = True - # Reuse the queue's own unique key only while this is still the very request it was minted for. - # Scrapy derives new requests from a fetched one with `Request.replace()` (redirects) and spiders - # often forward `meta` verbatim to another URL; both inherit the stamp, and reusing it there would - # deduplicate the derived request against its parent and silently drop it. A stamp without a URL - # beside it was set by hand rather than by `to_scrapy_request`, so it is taken at face value. + # Reuse the queue's unique key only while this is still the request it was minted for. Redirects + # (`Request.replace()`) and spiders forwarding `meta` to another URL both inherit the stamp, and + # reusing it there deduplicates the derived request against its parent. A stamp without a URL beside + # it was set by hand, so it is taken at face value. elif (unique_key := scrapy_request.meta.get('apify_request_unique_key')) and ( scrapy_request.meta.get('apify_request_url', scrapy_request.url) == scrapy_request.url ): @@ -198,8 +197,8 @@ def to_scrapy_request(apify_request: ApifyRequest, spider: Spider) -> ScrapyRequ else: scrapy_request = ScrapyRequest(url=apify_request.url, method=apify_request.method) - # Stamp the queue's unique key together with the URL it belongs to, so that `to_apify_request` can tell - # this request apart from the ones Scrapy derives from it. + # Stamp the unique key together with the URL it belongs to, so `to_apify_request` can tell this request + # apart from the ones Scrapy derives from it. scrapy_request.meta['apify_request_unique_key'] = apify_request.unique_key scrapy_request.meta['apify_request_url'] = scrapy_request.url diff --git a/src/apify/scrapy/scheduler.py b/src/apify/scrapy/scheduler.py index 94096176..72f5656c 100644 --- a/src/apify/scrapy/scheduler.py +++ b/src/apify/scrapy/scheduler.py @@ -110,9 +110,8 @@ def close(self, reason: str) -> None: except Exception: logger.exception('Failed to resolve the requests still in flight in the request queue.') - # Whatever Scrapy did not finish - an interrupted run, an Actor migration - goes back to the queue - # while the event loop is still around, so the next run gets it as pending. Each request is - # reclaimed on its own, so one failure does not strand the rest. + # Whatever Scrapy did not finish goes back to the queue, so the next run gets it as pending. + # One failed reclaim must not strand the rest. for apify_request, _ in self._requests_in_flight: try: self._async_thread.run_coro(rq.reclaim_request(apify_request)) @@ -121,8 +120,8 @@ def close(self, reason: str) -> None: self._requests_in_flight.clear() - # Let the updates fired off on the hot path finish: closing the event loop cancels them silently, which - # would leave those requests unhandled in the queue. + # Closing the event loop cancels the updates fired off on the hot path silently, leaving those + # requests unhandled. self._async_thread.wait_for_submitted() try: @@ -152,8 +151,7 @@ def has_pending_requests(self) -> bool: # as in flight is provably finished. self._resolve_finished_requests(wait=True) - # The queue answers from its own bookkeeping, so an update still in flight would let it report itself - # finished while a request is unhandled - and closing the crawl would then cancel that update. + # The queue answers from its own bookkeeping, which a pending update has not reached yet. self._async_thread.wait_for_submitted() # Log here before re-raising: this coroutine ran on a separate event-loop thread, and the failure is @@ -207,8 +205,8 @@ def next_request(self) -> Request | None: if not isinstance(self._rq, RequestQueue): raise TypeError('self._rq must be an instance of the RequestQueue class') - # Resolve whatever Scrapy has finished since the last call. The engine polls this method throughout the - # crawl, which keeps the queue's view of progress current without blocking on the round trips. + # The engine polls this method throughout the crawl, so resolving here keeps the queue current + # without blocking on the round trips. self._resolve_finished_requests(wait=False) # Log here before re-raising: this coroutine ran on a separate event-loop thread, and the failure is @@ -225,9 +223,8 @@ def next_request(self) -> Request | None: if not isinstance(self.spider, Spider): raise TypeError('self.spider must be an instance of the Spider class') - # A malformed entry must not crash the whole run, so on failure it is logged and skipped rather than - # propagating. Such an unrecoverable entry (a corrupt or legacy payload) is marked as handled right - # away, otherwise the queue would keep handing it back forever. + # A corrupt or legacy payload must not crash the run, and is marked as handled right away, otherwise + # the queue would keep handing it back forever. try: scrapy_request = to_scrapy_request(apify_request, spider=self.spider) except Exception as exc: @@ -239,8 +236,8 @@ def next_request(self) -> Request | None: raise return None - # The entry stays unresolved in the queue until Scrapy is done with the request, so a run interrupted - # mid-flight leaves it pending instead of silently handled. + # The entry stays unresolved until Scrapy is done with the request, so a run interrupted mid-flight + # leaves it pending instead of silently handled. self._requests_in_flight.append((apify_request, scrapy_request)) return scrapy_request @@ -248,10 +245,9 @@ def next_request(self) -> Request | None: def _requests_busy_in_scrapy(self) -> set[Request]: """Return the requests Scrapy is still working on. - A request handed out by `next_request` joins the downloader's active set before the middleware chain - runs, and leaves the scraper's only once the spider callback and the item pipeline have finished with - it. Absence from both therefore means Scrapy is done with the request, whether it was downloaded, - dropped by a middleware or errored out. + A request joins the downloader's active set before the middleware chain runs and leaves the scraper's + only once the callback and the item pipeline are done, so absence from both means Scrapy has finished + with it - downloaded, dropped by a middleware or errored out alike. """ engine = self._crawler.engine if self._crawler is not None else None if engine is None: @@ -263,10 +259,8 @@ def _requests_busy_in_scrapy(self) -> set[Request]: def _resolve_finished_requests(self, *, wait: bool) -> None: """Mark every request Scrapy has finished processing as handled in the request queue. - Only a failure to dispatch the update - a timed-out or closed event loop - keeps a request tracked for - the next call to retry, without stopping the rest of the list from being resolved. The queue reports - the update's own failures by returning `None`, which is indistinguishable from success here, so such a - request is left unhandled for the next run to pick up. + A request whose update cannot be dispatched stays tracked for the next call to retry, without holding + up the rest of the list. Args: wait: Whether to block until the queue has been updated. Pass False on the crawl's hot path, where diff --git a/tests/unit/scrapy/test_async_thread.py b/tests/unit/scrapy/test_async_thread.py index 994db6e8..7030d51f 100644 --- a/tests/unit/scrapy/test_async_thread.py +++ b/tests/unit/scrapy/test_async_thread.py @@ -11,7 +11,7 @@ import pytest from ..._utils import poll_until_condition -from apify.scrapy._async_thread import _SUBMITTED_PRUNE_THRESHOLD, AsyncThread +from apify.scrapy._async_thread import SUBMITTED_PRUNE_THRESHOLD, AsyncThread async def _return(value: int) -> int: @@ -265,7 +265,7 @@ def test_submit_coro_drops_the_finished_futures() -> None: thread = AsyncThread() _wait_until_running(thread) - for _ in range(_SUBMITTED_PRUNE_THRESHOLD): + for _ in range(SUBMITTED_PRUNE_THRESHOLD): thread.submit_coro(_return(1)) assert futures.wait(list(thread._submitted), timeout=2).not_done == set() From 95c03e05c8dd5d0a2c2b465c5426be82a7e47a4d Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 09:59:55 +0200 Subject: [PATCH 06/17] docs(scrapy): apply review wording suggestion for migration section --- docs/03_guides/06_scrapy.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/03_guides/06_scrapy.mdx b/docs/03_guides/06_scrapy.mdx index 53d909e1..7e40633a 100644 --- a/docs/03_guides/06_scrapy.mdx +++ b/docs/03_guides/06_scrapy.mdx @@ -104,7 +104,7 @@ The following example shows a Scrapy Actor that scrapes page titles and enqueues ## Dealing with imminent migration to another host -Under some circumstances, the platform may decide to [migrate your Actor](https://docs.apify.com/academy/expert-scraping-with-apify/migrations-maintaining-state) from one piece of infrastructure to another while it's in progress. Requests that Scrapy hasn't finished when the run stops stay unhandled in the request queue, so the next run picks them up and downloads them from scratch. A Scrapy-based project doesn't resume where it left off the way a [Crawlee](https://crawlee.dev/python)-based one does, so items their callbacks already pushed can land in the dataset twice. +Under some circumstances, the platform may decide to [migrate your Actor](https://docs.apify.com/academy/expert-scraping-with-apify/migrations-maintaining-state) from one piece of infrastructure to another while the run is in progress. Requests that Scrapy hasn't finished when the run stops stay unhandled in the request queue, so the next run picks them up and downloads them from scratch. A Scrapy-based project doesn't resume where it left off the way a [Crawlee](https://crawlee.dev/python)-based one does, so items that their callbacks already pushed can land in the dataset twice. As a workaround for this issue (tracked as [apify/actor-templates#303](https://github.com/apify/actor-templates/issues/303)), turn on caching with `HTTPCACHE_ENABLED` and set `HTTPCACHE_EXPIRATION_SECS` to at least a few minutes—the exact value depends on your use case. If your Actor gets migrated and restarted, the subsequent run will hit the cache, making it fast and avoiding unnecessary resource consumption. From 4ec5892c3d8bede15cbe31d36a95bcabc3ff20cd Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 10:34:17 +0200 Subject: [PATCH 07/17] fix(scrapy): stop same-URL derived requests from inheriting the parent's unique key --- src/apify/scrapy/requests.py | 46 ++++++++++++++----- .../scrapy/requests/test_to_apify_request.py | 42 ++++++++++++++++- .../scrapy/requests/test_to_scrapy_request.py | 8 ++-- 3 files changed, 79 insertions(+), 17 deletions(-) diff --git a/src/apify/scrapy/requests.py b/src/apify/scrapy/requests.py index caa62413..a57414d8 100644 --- a/src/apify/scrapy/requests.py +++ b/src/apify/scrapy/requests.py @@ -11,7 +11,8 @@ from scrapy.utils.request import request_from_dict from crawlee._request import UserData -from crawlee._types import HttpHeaders +from crawlee._types import HttpHeaders, HttpMethod +from crawlee._utils.requests import compute_unique_key from ._serialization import decode_from_json, encode_to_json from apify import Request as ApifyRequest @@ -47,6 +48,27 @@ def _ensure_known_request_class(request_dict: dict[str, Any]) -> None: ) +def _compute_fingerprint(scrapy_request: ScrapyRequest) -> str: + """Identify the request a queue unique key was minted for. + + `to_scrapy_request` stamps this beside the unique key so `to_apify_request` can tell a request that came out + of the queue from one Scrapy derived from it. It covers the URL, the method and the body, so a redirect, a + method switch and a changed body are all recognized as a different request rather than the parent. + + Headers are deliberately left out even though they are part of the unique key: Scrapy's own downloader + middlewares (`DefaultHeadersMiddleware`, `UserAgentMiddleware`) call `headers.setdefault()` on the request + in place before it is handed back to the scheduler, so a request that is otherwise untouched would no longer + match the stamp it was given. + """ + return compute_unique_key( + url=scrapy_request.url, + method=cast('HttpMethod', scrapy_request.method), + payload=scrapy_request.body, + keep_url_fragment=False, + use_extended_unique_key=True, + ) + + def to_apify_request(scrapy_request: ScrapyRequest, spider: Spider) -> ApifyRequest | None: """Convert a Scrapy request to an Apify request. @@ -76,14 +98,14 @@ def to_apify_request(scrapy_request: ScrapyRequest, spider: Spider) -> ApifyRequ try: if scrapy_request.dont_filter: request_kwargs['always_enqueue'] = True - # Reuse the queue's unique key only while this is still the request it was minted for. Redirects - # (`Request.replace()`) and spiders forwarding `meta` to another URL both inherit the stamp, and - # reusing it there deduplicates the derived request against its parent. A stamp without a URL beside - # it was set by hand, so it is taken at face value. - elif (unique_key := scrapy_request.meta.get('apify_request_unique_key')) and ( - scrapy_request.meta.get('apify_request_url', scrapy_request.url) == scrapy_request.url - ): - request_kwargs['unique_key'] = unique_key + elif unique_key := scrapy_request.meta.get('apify_request_unique_key'): + # Reuse the queue's unique key only while this is still the request it was minted for. Redirects + # (`Request.replace()`) and spiders forwarding `meta` to another URL both inherit the stamp, and + # reusing it there deduplicates the derived request against its parent. A stamp without a + # fingerprint beside it was set by hand, so it is taken at face value. + fingerprint = _compute_fingerprint(scrapy_request) + if scrapy_request.meta.get('apify_request_fingerprint', fingerprint) == fingerprint: + request_kwargs['unique_key'] = unique_key # Serialize the Scrapy request now, before `Request.from_url()` runs below. `from_url()` mutates the # `user_data` dict it receives in place (it injects a live `CrawleeRequestData` under `__crawlee`), and that @@ -197,10 +219,10 @@ def to_scrapy_request(apify_request: ApifyRequest, spider: Spider) -> ScrapyRequ else: scrapy_request = ScrapyRequest(url=apify_request.url, method=apify_request.method) - # Stamp the unique key together with the URL it belongs to, so `to_apify_request` can tell this request - # apart from the ones Scrapy derives from it. + # Stamp the unique key together with a fingerprint of the request it belongs to, so `to_apify_request` can + # tell this request apart from the ones Scrapy derives from it. scrapy_request.meta['apify_request_unique_key'] = apify_request.unique_key - scrapy_request.meta['apify_request_url'] = scrapy_request.url + scrapy_request.meta['apify_request_fingerprint'] = _compute_fingerprint(scrapy_request) # Add optional 'headers' field if apify_request.headers: diff --git a/tests/unit/scrapy/requests/test_to_apify_request.py b/tests/unit/scrapy/requests/test_to_apify_request.py index 754e0290..37a310bc 100644 --- a/tests/unit/scrapy/requests/test_to_apify_request.py +++ b/tests/unit/scrapy/requests/test_to_apify_request.py @@ -2,7 +2,7 @@ import json import logging -from typing import cast +from typing import Any, cast import pytest from scrapy import Request, Spider @@ -212,6 +212,46 @@ def test_redirected_request_does_not_inherit_the_parents_unique_key(spider: Spid assert apify_request.unique_key != parent.meta['apify_request_unique_key'] +@pytest.mark.parametrize( + 'changes', + [ + pytest.param({'method': 'POST', 'body': b'page=2'}, id='method and body'), + pytest.param({'body': b'page=2'}, id='body'), + pytest.param({'method': 'HEAD'}, id='method'), + ], +) +def test_derived_request_on_the_same_url_gets_its_own_unique_key(spider: Spider, changes: dict[str, Any]) -> None: + """A request Scrapy derives without leaving the URL must not be deduplicated against its parent.""" + parent = to_scrapy_request(ApifyRequest.from_url('https://example.com/listing'), spider) + derived = parent.replace(**changes) + + apify_request = to_apify_request(derived, spider) + + assert apify_request is not None + assert apify_request.unique_key != parent.meta['apify_request_unique_key'] + + +def test_stamp_survives_the_headers_scrapy_middlewares_add(spider: Spider) -> None: + """Scrapy's own middlewares add default headers in place, which must not read as a different request.""" + scrapy_request = to_scrapy_request(ApifyRequest.from_url('https://example.com'), spider) + scrapy_request.headers.setdefault(b'User-Agent', b'Scrapy/2.14') + + apify_request = to_apify_request(scrapy_request, spider) + + assert apify_request is not None + assert apify_request.unique_key == scrapy_request.meta['apify_request_unique_key'] + + +def test_hand_set_unique_key_without_a_fingerprint_is_taken_at_face_value(spider: Spider) -> None: + """A unique key put in `meta` by hand carries no fingerprint to check it against, so it is honoured.""" + scrapy_request = Request(url='https://example.com', meta={'apify_request_unique_key': 'my-own-key'}) + + apify_request = to_apify_request(scrapy_request, spider) + + assert apify_request is not None + assert apify_request.unique_key == 'my-own-key' + + def test_follow_up_request_with_propagated_meta_gets_its_own_unique_key(spider: Spider) -> None: """A spider callback forwarding `meta` verbatim to another URL must not reuse the parent's unique key.""" parent = to_scrapy_request(ApifyRequest.from_url('https://example.com/listing'), spider) diff --git a/tests/unit/scrapy/requests/test_to_scrapy_request.py b/tests/unit/scrapy/requests/test_to_scrapy_request.py index c3803d7b..c0b656cf 100644 --- a/tests/unit/scrapy/requests/test_to_scrapy_request.py +++ b/tests/unit/scrapy/requests/test_to_scrapy_request.py @@ -12,7 +12,7 @@ from apify import Request as ApifyRequest from apify.scrapy._serialization import encode_to_json -from apify.scrapy.requests import to_apify_request, to_scrapy_request +from apify.scrapy.requests import _compute_fingerprint, to_apify_request, to_scrapy_request class DummySpider(Spider): @@ -68,8 +68,8 @@ def test_without_reconstruction(spider: Spider) -> None: assert apify_request.unique_key == scrapy_request.meta.get('apify_request_unique_key') -def test_unique_key_is_stamped_together_with_its_url(spider: Spider) -> None: - """The queue's unique key is stamped alongside the URL it belongs to, so derived requests can be told apart.""" +def test_unique_key_is_stamped_together_with_its_fingerprint(spider: Spider) -> None: + """The queue's unique key is stamped alongside a fingerprint of the request it belongs to.""" apify_request = ApifyRequest( url='https://example.com', method='GET', @@ -80,7 +80,7 @@ def test_unique_key_is_stamped_together_with_its_url(spider: Spider) -> None: scrapy_request = to_scrapy_request(apify_request, spider) assert scrapy_request.meta['apify_request_unique_key'] == apify_request.unique_key - assert scrapy_request.meta['apify_request_url'] == scrapy_request.url + assert scrapy_request.meta['apify_request_fingerprint'] == _compute_fingerprint(scrapy_request) def test_without_reconstruction_with_optional_fields(spider: Spider) -> None: From 7cd79e028f030f15c15e6b42590d539e49537145 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 10:34:23 +0200 Subject: [PATCH 08/17] fix(scrapy): recover in-flight requests whose queue update fails after dispatch --- src/apify/scrapy/_async_thread.py | 7 +- src/apify/scrapy/scheduler.py | 162 ++++++++++++-- tests/unit/scrapy/test_scheduler.py | 334 ++++++++++++++++++---------- 3 files changed, 367 insertions(+), 136 deletions(-) diff --git a/src/apify/scrapy/_async_thread.py b/src/apify/scrapy/_async_thread.py index d35dec3a..5e8c72dc 100644 --- a/src/apify/scrapy/_async_thread.py +++ b/src/apify/scrapy/_async_thread.py @@ -80,7 +80,7 @@ def run_coro( future.cancel() raise - def submit_coro(self, coro: Coroutine) -> None: + def submit_coro(self, coro: Coroutine) -> futures.Future: """Schedule a coroutine on the event loop without waiting for its result. Use this for work whose result nothing depends on, so the calling thread is not blocked by the round @@ -90,6 +90,9 @@ def submit_coro(self, coro: Coroutine) -> None: Args: coro: The coroutine to run. + Returns: + The future of the scheduled coroutine, for callers that do want to inspect its outcome later. + Raises: RuntimeError: If the event loop has been closed. """ @@ -105,6 +108,8 @@ def submit_coro(self, coro: Coroutine) -> None: future.add_done_callback(self._log_failure) self._submitted.append(future) + return future + def wait_for_submitted(self, timeout: timedelta | None = None) -> None: """Block until the coroutines submitted via `submit_coro` have finished. diff --git a/src/apify/scrapy/scheduler.py b/src/apify/scrapy/scheduler.py index 72f5656c..cc546a2a 100644 --- a/src/apify/scrapy/scheduler.py +++ b/src/apify/scrapy/scheduler.py @@ -1,10 +1,12 @@ from __future__ import annotations +import asyncio from datetime import timedelta from logging import getLogger -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any from scrapy import Spider +from scrapy import __version__ as scrapy_version from scrapy.core.scheduler import BaseScheduler from scrapy.utils.reactor import is_asyncio_reactor_installed @@ -15,6 +17,9 @@ from apify.storages import RequestQueue if TYPE_CHECKING: + from collections.abc import Coroutine, Iterable + from concurrent.futures import Future + from scrapy.crawler import Crawler from scrapy.http.request import Request from twisted.internet.defer import Deferred @@ -24,6 +29,16 @@ logger = getLogger(__name__) +async def _gather_failures(operations: Iterable[Coroutine[Any, Any, Any]]) -> list[BaseException | None]: + """Run request queue updates concurrently, reporting each one's failure, or `None`, in the order given. + + The updates of a whole batch cost one round trip rather than one each, and one failing update neither + raises nor stops the others. + """ + outcomes = await asyncio.gather(*operations, return_exceptions=True) + return [outcome if isinstance(outcome, BaseException) else None for outcome in outcomes] + + class ApifyScheduler(BaseScheduler): """A Scrapy scheduler that uses the Apify `RequestQueue` to manage requests. @@ -48,6 +63,9 @@ def __init__( self._requests_in_flight: list[tuple[ApifyRequest, Request]] = [] """Requests handed over to Scrapy and not resolved in the request queue yet.""" + self._pending_marks: list[tuple[list[tuple[ApifyRequest, Request]], Future]] = [] + """Batches of mark-as-handled updates dispatched off the hot path, whose outcome is not known yet.""" + # A thread with the asyncio event loop to run coroutines on. self._async_thread = AsyncThread(default_timeout=async_thread_timeout) @@ -69,6 +87,15 @@ def open(self, spider: Spider) -> Deferred[None] | None: """ self.spider = spider + if self._crawler is None: + logger.warning( + f'{ApifyScheduler.__qualname__} was built without a crawler, so it cannot see what Scrapy is ' + 'still working on. Every request is marked as handled the moment it is handed over, and an ' + 'interrupted run loses whatever was in flight. Build it with `from_crawler` to avoid that.' + ) + else: + self._verify_engine_internals() + async def open_rq() -> RequestQueue: configuration = Configuration.get_global_configuration() if configuration.is_at_home: @@ -110,13 +137,24 @@ def close(self, reason: str) -> None: except Exception: logger.exception('Failed to resolve the requests still in flight in the request queue.') - # Whatever Scrapy did not finish goes back to the queue, so the next run gets it as pending. - # One failed reclaim must not strand the rest. - for apify_request, _ in self._requests_in_flight: + # Whatever Scrapy did not finish goes back to the queue, so the next run gets it as pending. The + # reclaims travel together: a migration cuts the shutdown short, and one round trip per request may + # not fit in what is left of it. One failed reclaim must not strand the rest either. + if self._requests_in_flight: + reclaims = _gather_failures( + rq.reclaim_request(apify_request) for apify_request, _ in self._requests_in_flight + ) try: - self._async_thread.run_coro(rq.reclaim_request(apify_request)) + outcomes = self._async_thread.run_coro(reclaims) except Exception: - logger.exception(f'Failed to reclaim the request {apify_request} in the request queue.') + logger.exception('Failed to reclaim the requests still in flight in the request queue.') + else: + for (apify_request, _), outcome in zip(self._requests_in_flight, outcomes, strict=True): + if outcome is not None: + logger.error( + f'Failed to reclaim the request {apify_request} in the request queue.', + exc_info=outcome, + ) self._requests_in_flight.clear() @@ -242,12 +280,34 @@ def next_request(self) -> Request | None: return scrapy_request + def _verify_engine_internals(self) -> None: + """Fail early if Scrapy's engine no longer exposes what the in-flight tracking reads. + + `_requests_busy_in_scrapy` reads engine internals Scrapy does not document as public API, and the hot + path deliberately does not guard them: swallowing an `AttributeError` there would quietly revert to + marking every request as handled the moment it is handed over, which is what the tracking exists to + prevent. Checking once at open time keeps such a breakage loud, early and easy to place. + + Raises: + RuntimeError: If the engine internals the tracking relies on cannot be read. + """ + try: + self._requests_busy_in_scrapy() + except AttributeError as exc: + raise RuntimeError( + f'{ApifyScheduler.__qualname__} cannot tell which requests Scrapy is working on: this Scrapy ' + f'version ({scrapy_version}) does not expose the engine internals it reads. Please report this ' + 'at https://github.com/apify/apify-sdk-python/issues.' + ) from exc + def _requests_busy_in_scrapy(self) -> set[Request]: """Return the requests Scrapy is still working on. A request joins the downloader's active set before the middleware chain runs and leaves the scraper's only once the callback and the item pipeline are done, so absence from both means Scrapy has finished with it - downloaded, dropped by a middleware or errored out alike. + + Without a crawler there is nothing to ask, and every request reads as finished; `open` warns about that. """ engine = self._crawler.engine if self._crawler is not None else None if engine is None: @@ -259,32 +319,98 @@ def _requests_busy_in_scrapy(self) -> set[Request]: def _resolve_finished_requests(self, *, wait: bool) -> None: """Mark every request Scrapy has finished processing as handled in the request queue. - A request whose update cannot be dispatched stays tracked for the next call to retry, without holding - up the rest of the list. + A whole resolution pass reaches the queue in a single round trip. A request whose update cannot be + dispatched, or whose dispatched update turns out to have failed, stays tracked for the next call to + retry, without holding up the rest of the list. Args: wait: Whether to block until the queue has been updated. Pass False on the crawl's hot path, where nothing depends on the result and blocking would stall the Twisted reactor. """ rq = self._rq - if not self._requests_in_flight or not isinstance(rq, RequestQueue): + if not isinstance(rq, RequestQueue) or not (self._requests_in_flight or self._pending_marks): return - busy = self._requests_busy_in_scrapy() + # Updates dispatched by an earlier pass that did not land are marked again by this one. + finished = self._collect_failed_marks(wait=wait) unresolved: list[tuple[ApifyRequest, Request]] = [] - for apify_request, scrapy_request in self._requests_in_flight: - if scrapy_request in busy: - unresolved.append((apify_request, scrapy_request)) - continue + if self._requests_in_flight: + busy = self._requests_busy_in_scrapy() + for apify_request, scrapy_request in self._requests_in_flight: + if scrapy_request in busy: + unresolved.append((apify_request, scrapy_request)) + else: + finished.append((apify_request, scrapy_request)) + if finished: + marks = _gather_failures(rq.mark_request_as_handled(apify_request) for apify_request, _ in finished) try: if wait: - self._async_thread.run_coro(rq.mark_request_as_handled(apify_request)) + unresolved.extend(self._failed_marks(finished, self._async_thread.run_coro(marks))) else: - self._async_thread.submit_coro(rq.mark_request_as_handled(apify_request)) + self._pending_marks.append((finished, self._async_thread.submit_coro(marks))) except Exception: - logger.exception(f'Failed to mark the request {apify_request} as handled in the request queue.') - unresolved.append((apify_request, scrapy_request)) + logger.exception(f'Failed to mark {len(finished)} request(s) as handled in the request queue.') + unresolved.extend(finished) self._requests_in_flight = unresolved + + def _collect_failed_marks(self, *, wait: bool) -> list[tuple[ApifyRequest, Request]]: + """Return the requests whose already dispatched mark-as-handled did not land, so it can be retried. + + `submit_coro` reports nothing back to the reactor thread, so an update that fails after it was + dispatched would otherwise drop its request from the tracking for good and leave it in progress in the + queue forever - the queue would never report itself finished and the crawl would never end. + + Args: + wait: Whether to block until every dispatched update has finished. Updates still running are kept + for the next call. + """ + if not self._pending_marks: + return [] + + if wait: + self._async_thread.wait_for_submitted() + + failed: list[tuple[ApifyRequest, Request]] = [] + pending: list[tuple[list[tuple[ApifyRequest, Request]], Future]] = [] + + for requests, future in self._pending_marks: + if not future.done(): + pending.append((requests, future)) + elif future.cancelled(): + logger.error(f'Marking {len(requests)} request(s) as handled was cancelled before it finished.') + failed.extend(requests) + else: + # `_gather_failures` reports a failed update in its result rather than raising, so an exception + # here means the dispatch itself did not survive - the event loop was torn down under it. + try: + outcomes = future.result() + except Exception: + logger.exception(f'Failed to mark {len(requests)} request(s) as handled in the request queue.') + failed.extend(requests) + else: + failed.extend(self._failed_marks(requests, outcomes)) + + self._pending_marks = pending + + return failed + + @staticmethod + def _failed_marks( + requests: list[tuple[ApifyRequest, Request]], + outcomes: list[BaseException | None], + ) -> list[tuple[ApifyRequest, Request]]: + """Pair a batch of updates back with their requests, returning and logging the ones that failed.""" + failed = [] + + for (apify_request, scrapy_request), outcome in zip(requests, outcomes, strict=True): + if outcome is not None: + logger.error( + f'Failed to mark the request {apify_request} as handled in the request queue.', + exc_info=outcome, + ) + failed.append((apify_request, scrapy_request)) + + return failed diff --git a/tests/unit/scrapy/test_scheduler.py b/tests/unit/scrapy/test_scheduler.py index c4032955..959fd3f4 100644 --- a/tests/unit/scrapy/test_scheduler.py +++ b/tests/unit/scrapy/test_scheduler.py @@ -1,9 +1,11 @@ from __future__ import annotations +import asyncio import logging +from concurrent.futures import Future from datetime import timedelta from types import SimpleNamespace -from typing import Any, cast +from typing import TYPE_CHECKING, Any, cast from unittest import mock import pytest @@ -14,6 +16,9 @@ from apify.scrapy.scheduler import ApifyScheduler from apify.storages import RequestQueue +if TYPE_CHECKING: + from collections.abc import Coroutine + class DummySpider(Spider): name = 'dummy_spider' @@ -39,16 +44,60 @@ def fake_crawler( return SimpleNamespace(engine=engine) +class FakeAsyncThread: + """Stand-in for `AsyncThread` that runs the scheduler's coroutines on a real event loop. + + The scheduler batches the updates of a whole resolution pass into a single coroutine, so a double that + never runs them would leave these tests asserting on the batching instead of on what reaches the queue. + """ + + def __init__(self, default_timeout: timedelta | None = None) -> None: + self.default_timeout = default_timeout + self.calls: list[str] = [] + """The methods called on this thread, in order, for the tests that care about the ordering.""" + + def run_coro(self, coro: Coroutine) -> Any: + self.calls.append('run_coro') + return asyncio.run(coro) + + def submit_coro(self, coro: Coroutine) -> Future: + self.calls.append('submit_coro') + future: Future = Future() + try: + future.set_result(asyncio.run(coro)) + except Exception as exc: + future.set_exception(exc) + return future + + def wait_for_submitted(self) -> None: + self.calls.append('wait_for_submitted') + + def close(self) -> None: + self.calls.append('close') + + +def stub_scheduler_dependencies(monkeypatch: pytest.MonkeyPatch) -> None: + """Stub out the reactor check, the event loop thread and the queue that `open` reaches for.""" + monkeypatch.setattr('apify.scrapy.scheduler.is_asyncio_reactor_installed', lambda: True) + monkeypatch.setattr('apify.scrapy.scheduler.AsyncThread', FakeAsyncThread) + + async def open_rq(*_args: Any, **_kwargs: Any) -> Any: + rq = mock.AsyncMock() + rq.__class__ = RequestQueue + return rq + + monkeypatch.setattr(RequestQueue, 'open', open_rq) + + @pytest.fixture def scheduler(monkeypatch: pytest.MonkeyPatch, spider: DummySpider) -> ApifyScheduler: - """Create a scheduler with its reactor check and async thread stubbed out.""" - monkeypatch.setattr('apify.scrapy.scheduler.is_asyncio_reactor_installed', lambda: True) - monkeypatch.setattr('apify.scrapy.scheduler.AsyncThread', mock.MagicMock()) + """Create a scheduler with its reactor check stubbed out, a fake event loop thread and a mocked queue.""" + stub_scheduler_dependencies(monkeypatch) scheduler = ApifyScheduler() scheduler.spider = spider - rq = mock.MagicMock() + rq = mock.AsyncMock() rq.__class__ = RequestQueue scheduler._rq = rq @@ -57,12 +106,12 @@ def scheduler(monkeypatch: pytest.MonkeyPatch, spider: DummySpider) -> ApifySche def test_has_pending_requests_reflects_queue_state(scheduler: ApifyScheduler) -> None: """`has_pending_requests` is True while the queue is not finished and False once it is.""" - async_thread = cast('mock.MagicMock', scheduler._async_thread) + rq = cast('mock.AsyncMock', scheduler._rq) - async_thread.run_coro.return_value = False # the queue still has work + rq.is_finished.return_value = False # the queue still has work assert scheduler.has_pending_requests() is True - async_thread.run_coro.return_value = True # the queue is drained + rq.is_finished.return_value = True # the queue is drained assert scheduler.has_pending_requests() is False @@ -86,9 +135,8 @@ def test_enqueue_request_skips_non_serializable_request( def test_enqueue_request_enqueues_converted_request(scheduler: ApifyScheduler) -> None: """A convertible request is enqueued and reported as newly added when the queue had not seen it.""" - rq = cast('mock.MagicMock', scheduler._rq) - async_thread = cast('mock.MagicMock', scheduler._async_thread) - async_thread.run_coro.return_value = SimpleNamespace(was_already_present=False) + rq = cast('mock.AsyncMock', scheduler._rq) + rq.add_request.return_value = SimpleNamespace(was_already_present=False) result = scheduler.enqueue_request(Request(url='https://example.com')) @@ -98,8 +146,8 @@ def test_enqueue_request_enqueues_converted_request(scheduler: ApifyScheduler) - def test_enqueue_request_returns_false_for_duplicate(scheduler: ApifyScheduler) -> None: """A request already present in the queue is reported as not newly enqueued (returns False).""" - async_thread = cast('mock.MagicMock', scheduler._async_thread) - async_thread.run_coro.return_value = SimpleNamespace(was_already_present=True) + rq = cast('mock.AsyncMock', scheduler._rq) + rq.add_request.return_value = SimpleNamespace(was_already_present=True) result = scheduler.enqueue_request(Request(url='https://example.com')) @@ -111,8 +159,7 @@ def test_next_request_skips_request_that_fails_to_convert( caplog: pytest.LogCaptureFixture, ) -> None: """A queue entry that fails to reconstruct is skipped and still marked handled, not retried forever.""" - rq = cast('mock.MagicMock', scheduler._rq) - async_thread = cast('mock.MagicMock', scheduler._async_thread) + rq = cast('mock.AsyncMock', scheduler._rq) # A queue entry whose encoded Scrapy request is malformed; `to_scrapy_request` raises on it. malformed_request = ApifyRequest( @@ -122,8 +169,7 @@ def test_next_request_skips_request_that_fails_to_convert( user_data={'scrapy_request': 'this is not a correctly encoded Scrapy request'}, ) - # `run_coro` is called for `fetch_next_request`, then for `mark_request_as_handled`. - async_thread.run_coro.side_effect = [malformed_request, None] + rq.fetch_next_request.return_value = malformed_request with caplog.at_level(logging.WARNING, logger='apify.scrapy.scheduler'): result = scheduler.next_request() @@ -139,8 +185,7 @@ def test_next_request_skips_request_that_fails_to_convert( def test_next_request_returns_converted_request(scheduler: ApifyScheduler) -> None: """A valid queue entry is reconstructed into a Scrapy request and left unhandled until Scrapy is done.""" - rq = cast('mock.MagicMock', scheduler._rq) - async_thread = cast('mock.MagicMock', scheduler._async_thread) + rq = cast('mock.AsyncMock', scheduler._rq) apify_request = ApifyRequest( url='https://example.com', @@ -148,7 +193,7 @@ def test_next_request_returns_converted_request(scheduler: ApifyScheduler) -> No unique_key='https://example.com', user_data={}, ) - async_thread.run_coro.return_value = apify_request + rq.fetch_next_request.return_value = apify_request result = scheduler.next_request() @@ -159,9 +204,8 @@ def test_next_request_returns_converted_request(scheduler: ApifyScheduler) -> No def test_next_request_returns_none_when_queue_empty(scheduler: ApifyScheduler) -> None: """An empty queue makes `next_request` return None and skip marking anything as handled.""" - rq = cast('mock.MagicMock', scheduler._rq) - async_thread = cast('mock.MagicMock', scheduler._async_thread) - async_thread.run_coro.return_value = None + rq = cast('mock.AsyncMock', scheduler._rq) + rq.fetch_next_request.return_value = None result = scheduler.next_request() @@ -174,8 +218,8 @@ def test_next_request_logs_exception_before_propagating( caplog: pytest.LogCaptureFixture, ) -> None: """A failure in the coroutine run is logged with its traceback via `logger.exception` before propagating.""" - async_thread = cast('mock.MagicMock', scheduler._async_thread) - async_thread.run_coro.side_effect = RuntimeError('boom') + rq = cast('mock.AsyncMock', scheduler._rq) + rq.fetch_next_request.side_effect = RuntimeError('boom') with caplog.at_level(logging.ERROR, logger='apify.scrapy.scheduler'), pytest.raises(RuntimeError, match='boom'): scheduler.next_request() @@ -206,72 +250,85 @@ def __init__(self, default_timeout: timedelta | None = None) -> None: assert captured['default_timeout'] == timedelta(seconds=123) -def test_has_pending_requests_marks_finished_requests_as_handled(scheduler: ApifyScheduler) -> None: - """Requests Scrapy has finished with are marked as handled once it goes idle and asks about pending work.""" - rq = cast('mock.MagicMock', scheduler._rq) - async_thread = cast('mock.MagicMock', scheduler._async_thread) - - apify_request = ApifyRequest( - url='https://example.com', +APIFY_REQUESTS = [ + ApifyRequest( + url=f'https://example.com/{index}', method='GET', - unique_key='https://example.com', + unique_key=f'https://example.com/{index}', user_data={}, ) - async_thread.run_coro.return_value = apify_request + for index in range(4) +] + + +def hand_out(scheduler: ApifyScheduler, count: int) -> list[Request]: + """Fetch `count` requests, keeping each busy in Scrapy so it is not resolved as it is handed over. + + Returns the Scrapy requests, and leaves the crawler stub reporting all of them as busy in the downloader. + """ + rq = cast('mock.AsyncMock', scheduler._rq) + rq.fetch_next_request.side_effect = APIFY_REQUESTS[:count] + + busy: set[Request] = set() + scheduler._crawler = fake_crawler(downloader_busy=busy) + + for _ in range(count): + busy.add(cast('Request', scheduler.next_request())) + + rq.fetch_next_request.side_effect = None + rq.fetch_next_request.return_value = None + + return list(busy) + + +def test_has_pending_requests_marks_finished_requests_as_handled(scheduler: ApifyScheduler) -> None: + """Requests Scrapy has finished with are marked as handled once it goes idle and asks about pending work.""" + rq = cast('mock.AsyncMock', scheduler._rq) + + rq.fetch_next_request.return_value = APIFY_REQUESTS[0] scheduler.next_request() rq.mark_request_as_handled.assert_not_called() # Scrapy asks about pending work only once its downloader and its scraper are both idle. scheduler._crawler = fake_crawler(scraper_busy=set()) - async_thread.run_coro.return_value = True # the queue reports itself finished + rq.is_finished.return_value = True assert scheduler.has_pending_requests() is False - rq.mark_request_as_handled.assert_called_once_with(apify_request) + rq.mark_request_as_handled.assert_called_once_with(APIFY_REQUESTS[0]) def test_next_request_marks_finished_requests_without_blocking(scheduler: ApifyScheduler) -> None: """On the crawl's hot path a finished request is marked as handled without blocking the reactor on it.""" - rq = cast('mock.MagicMock', scheduler._rq) - async_thread = cast('mock.MagicMock', scheduler._async_thread) + rq = cast('mock.AsyncMock', scheduler._rq) + async_thread = cast('FakeAsyncThread', scheduler._async_thread) - apify_request = ApifyRequest( - url='https://example.com', - method='GET', - unique_key='https://example.com', - user_data={}, - ) - async_thread.run_coro.return_value = apify_request - scrapy_request = scheduler.next_request() - - # The queue is drained from here on, so no further request is handed out. - async_thread.run_coro.return_value = None + (scrapy_request,) = hand_out(scheduler, 1) # Scrapy is still downloading the request, so it stays unresolved. - scheduler._crawler = fake_crawler(downloader_busy={cast('Request', scrapy_request)}) + scheduler._crawler = fake_crawler(downloader_busy={scrapy_request}) assert scheduler.next_request() is None - async_thread.submit_coro.assert_not_called() + rq.mark_request_as_handled.assert_not_called() # Scrapy is done with it, so it is resolved off the reactor thread instead of blocking on the round trip. scheduler._crawler = fake_crawler(scraper_busy=set()) + async_thread.calls.clear() assert scheduler.next_request() is None - rq.mark_request_as_handled.assert_called_once_with(apify_request) - async_thread.submit_coro.assert_called_once_with(rq.mark_request_as_handled.return_value) + rq.mark_request_as_handled.assert_called_once_with(APIFY_REQUESTS[0]) + assert 'submit_coro' in async_thread.calls def test_has_pending_requests_waits_for_the_non_blocking_updates(scheduler: ApifyScheduler) -> None: """The queue is asked whether it is finished only after the updates fired off on the hot path have landed.""" - async_thread = cast('mock.MagicMock', scheduler._async_thread) + rq = cast('mock.AsyncMock', scheduler._rq) + async_thread = cast('FakeAsyncThread', scheduler._async_thread) scheduler._crawler = fake_crawler() - async_thread.run_coro.return_value = True # the queue reports itself finished + rq.is_finished.return_value = True assert scheduler.has_pending_requests() is False # The queue answers from its own bookkeeping, which a pending update has not reached yet. - assert async_thread.mock_calls == [ - mock.call.wait_for_submitted(), - mock.call.run_coro(cast('mock.MagicMock', scheduler._rq).is_finished()), - ] + assert async_thread.calls == ['wait_for_submitted', 'run_coro'] @pytest.mark.parametrize( @@ -283,47 +340,31 @@ def test_has_pending_requests_waits_for_the_non_blocking_updates(scheduler: Apif ) def test_close_reclaims_requests_scrapy_never_finished(scheduler: ApifyScheduler, busy_kwarg: str) -> None: """Requests still being processed when the scheduler closes go back to the queue instead of being lost.""" - rq = cast('mock.MagicMock', scheduler._rq) - async_thread = cast('mock.MagicMock', scheduler._async_thread) + rq = cast('mock.AsyncMock', scheduler._rq) - apify_request = ApifyRequest( - url='https://example.com', - method='GET', - unique_key='https://example.com', - user_data={}, - ) - async_thread.run_coro.return_value = apify_request - scrapy_request = scheduler.next_request() + (scrapy_request,) = hand_out(scheduler, 1) # Scrapy is still working on the request when the run is interrupted. - scheduler._crawler = fake_crawler(**{busy_kwarg: {cast('Request', scrapy_request)}}) + scheduler._crawler = fake_crawler(**{busy_kwarg: {scrapy_request}}) scheduler.close('shutdown') - rq.reclaim_request.assert_called_once_with(apify_request) + rq.reclaim_request.assert_called_once_with(APIFY_REQUESTS[0]) rq.mark_request_as_handled.assert_not_called() def test_close_marks_the_requests_scrapy_finished_as_handled(scheduler: ApifyScheduler) -> None: """Requests Scrapy drained before the shutdown are marked as handled rather than reclaimed.""" - rq = cast('mock.MagicMock', scheduler._rq) - async_thread = cast('mock.MagicMock', scheduler._async_thread) + rq = cast('mock.AsyncMock', scheduler._rq) - apify_request = ApifyRequest( - url='https://example.com', - method='GET', - unique_key='https://example.com', - user_data={}, - ) - async_thread.run_coro.return_value = apify_request - scheduler.next_request() + hand_out(scheduler, 1) # Scrapy drains its downloader and its scraper before the scheduler is closed. scheduler._crawler = fake_crawler(scraper_busy=set()) scheduler.close('finished') - rq.mark_request_as_handled.assert_called_once_with(apify_request) + rq.mark_request_as_handled.assert_called_once_with(APIFY_REQUESTS[0]) rq.reclaim_request.assert_not_called() @@ -332,45 +373,44 @@ def test_close_reclaims_the_other_requests_after_a_failed_reclaim( caplog: pytest.LogCaptureFixture, ) -> None: """One failing reclaim does not stop the other in-flight requests from going back to the queue.""" - rq = cast('mock.MagicMock', scheduler._rq) - async_thread = cast('mock.MagicMock', scheduler._async_thread) - - apify_requests = [ - ApifyRequest( - url=f'https://example.com/{index}', - method='GET', - unique_key=f'https://example.com/{index}', - user_data={}, - ) - for index in range(2) - ] - - # The crawler stub keeps a reference to this set, so both requests stay busy as they are handed out. - busy: set[Request] = set() - scheduler._crawler = fake_crawler(downloader_busy=busy) + rq = cast('mock.AsyncMock', scheduler._rq) - async_thread.run_coro.side_effect = apify_requests - for _ in apify_requests: - busy.add(cast('Request', scheduler.next_request())) - - async_thread.run_coro.side_effect = [RuntimeError('boom'), None] + hand_out(scheduler, 2) + rq.reclaim_request.side_effect = [RuntimeError('boom'), None] with caplog.at_level(logging.ERROR, logger='apify.scrapy.scheduler'): scheduler.close('shutdown') - assert rq.reclaim_request.call_count == len(apify_requests) + assert rq.reclaim_request.call_count == 2 errors = [record for record in caplog.records if record.levelno >= logging.ERROR] assert len(errors) == 1 +def test_close_reaches_the_queue_in_one_round_trip_per_operation(scheduler: ApifyScheduler) -> None: + """Marks and reclaims each travel together, as a migration may not leave room for one round trip each.""" + rq = cast('mock.AsyncMock', scheduler._rq) + async_thread = cast('FakeAsyncThread', scheduler._async_thread) + + handed_out = hand_out(scheduler, 4) + + # Scrapy finished half of the requests and is still working on the rest when the run is interrupted. + scheduler._crawler = fake_crawler(downloader_busy=set(handed_out[2:])) + + async_thread.calls.clear() + scheduler.close('shutdown') + + assert rq.mark_request_as_handled.await_count == 2 + assert rq.reclaim_request.await_count == 2 + assert async_thread.calls.count('run_coro') == 2 + + def test_close_waits_for_the_non_blocking_updates(scheduler: ApifyScheduler) -> None: """The event loop is not torn down before the updates fired off on the hot path have landed.""" - async_thread = cast('mock.MagicMock', scheduler._async_thread) + async_thread = cast('FakeAsyncThread', scheduler._async_thread) scheduler.close('finished') - calls = async_thread.mock_calls - assert calls.index(mock.call.wait_for_submitted()) < calls.index(mock.call.close()) + assert async_thread.calls.index('wait_for_submitted') < async_thread.calls.index('close') def test_a_failed_mark_keeps_the_request_tracked( @@ -378,20 +418,14 @@ def test_a_failed_mark_keeps_the_request_tracked( caplog: pytest.LogCaptureFixture, ) -> None: """A request whose mark-as-handled fails stays tracked, so the next resolution retries it.""" - async_thread = cast('mock.MagicMock', scheduler._async_thread) + rq = cast('mock.AsyncMock', scheduler._rq) - apify_request = ApifyRequest( - url='https://example.com', - method='GET', - unique_key='https://example.com', - user_data={}, - ) - async_thread.run_coro.return_value = apify_request - scheduler.next_request() + hand_out(scheduler, 1) scheduler._crawler = fake_crawler() # The mark fails, then the queue reports itself unfinished because the request is still in progress. - async_thread.run_coro.side_effect = [RuntimeError('boom'), False] + rq.mark_request_as_handled.side_effect = RuntimeError('boom') + rq.is_finished.return_value = False with caplog.at_level(logging.ERROR, logger='apify.scrapy.scheduler'): assert scheduler.has_pending_requests() is True @@ -401,6 +435,72 @@ def test_a_failed_mark_keeps_the_request_tracked( assert len(errors) == 1 +def test_a_mark_that_fails_after_being_fired_off_is_retried( + scheduler: ApifyScheduler, + caplog: pytest.LogCaptureFixture, +) -> None: + """A hot-path mark that fails on its way to the queue is retried, instead of leaving the request in progress.""" + rq = cast('mock.AsyncMock', scheduler._rq) + + hand_out(scheduler, 1) + + # Scrapy is done with the request, so the hot path fires the mark off - and it fails out of sight. + scheduler._crawler = fake_crawler() + rq.mark_request_as_handled.side_effect = RuntimeError('boom') + assert scheduler.next_request() is None + + # The failure only surfaces on the next pass, which picks the request back up and marks it again. + rq.mark_request_as_handled.side_effect = None + rq.is_finished.return_value = True + + with caplog.at_level(logging.ERROR, logger='apify.scrapy.scheduler'): + assert scheduler.has_pending_requests() is False + + assert rq.mark_request_as_handled.await_count == 2 + assert not scheduler._requests_in_flight + assert not scheduler._pending_marks + + +def test_open_warns_when_built_without_a_crawler( + monkeypatch: pytest.MonkeyPatch, + spider: DummySpider, + caplog: pytest.LogCaptureFixture, +) -> None: + """Without a crawler the scheduler cannot see what Scrapy holds, and that must not pass silently.""" + stub_scheduler_dependencies(monkeypatch) + + with caplog.at_level(logging.WARNING, logger='apify.scrapy.scheduler'): + ApifyScheduler().open(spider) + + assert 'without a crawler' in caplog.text + + +def test_open_is_quiet_with_a_crawler( + monkeypatch: pytest.MonkeyPatch, + spider: DummySpider, + caplog: pytest.LogCaptureFixture, +) -> None: + """A scheduler that can read the engine internals it relies on opens without complaining.""" + stub_scheduler_dependencies(monkeypatch) + + with caplog.at_level(logging.WARNING, logger='apify.scrapy.scheduler'): + ApifyScheduler(crawler=fake_crawler(scraper_busy=set())).open(spider) + + assert not [record for record in caplog.records if record.name == 'apify.scrapy.scheduler'] + + +def test_open_fails_loudly_when_the_scrapy_engine_internals_move( + monkeypatch: pytest.MonkeyPatch, + spider: DummySpider, +) -> None: + """A Scrapy release that moves the engine internals has to break at open, not silently mid-crawl.""" + stub_scheduler_dependencies(monkeypatch) + crawler = SimpleNamespace(engine=SimpleNamespace(downloader=SimpleNamespace(), scraper=SimpleNamespace())) + + with pytest.raises(RuntimeError, match='engine internals'): + ApifyScheduler(crawler=cast('Any', crawler)).open(spider) + + def test_from_crawler_keeps_the_crawler(monkeypatch: pytest.MonkeyPatch) -> None: """`from_crawler` keeps the crawler, which is how the scheduler learns what Scrapy is still working on.""" monkeypatch.setattr('apify.scrapy.scheduler.is_asyncio_reactor_installed', lambda: True) From 098d656f8d7a221e9c6aa3529d26908580316925 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 10:34:28 +0200 Subject: [PATCH 09/17] test(scrapy): guard every async thread test's close with try/finally --- tests/unit/scrapy/test_async_thread.py | 162 +++++++++++++++---------- 1 file changed, 97 insertions(+), 65 deletions(-) diff --git a/tests/unit/scrapy/test_async_thread.py b/tests/unit/scrapy/test_async_thread.py index 7030d51f..948886af 100644 --- a/tests/unit/scrapy/test_async_thread.py +++ b/tests/unit/scrapy/test_async_thread.py @@ -70,9 +70,11 @@ def test_run_coro_raises_after_close() -> None: async_thread.close() coro = _return(42) - with pytest.raises(RuntimeError): - async_thread.run_coro(coro) - coro.close() + try: + with pytest.raises(RuntimeError): + async_thread.run_coro(coro) + finally: + coro.close() def test_run_coro_cancels_the_coroutine_on_timeout() -> None: @@ -91,13 +93,14 @@ async def slow() -> None: cancelled.set() raise - with pytest.raises(futures.TimeoutError): - thread.run_coro(slow(), timeout=timedelta(seconds=0.1)) - - assert started.wait(timeout=2) - assert cancelled.wait(timeout=2), 'the timed-out coroutine was left running instead of being cancelled' + try: + with pytest.raises(futures.TimeoutError): + thread.run_coro(slow(), timeout=timedelta(seconds=0.1)) - thread.close() + assert started.wait(timeout=2) + assert cancelled.wait(timeout=2), 'the timed-out coroutine was left running instead of being cancelled' + finally: + thread.close() def test_run_coro_does_not_log_on_exception(caplog: pytest.LogCaptureFixture) -> None: @@ -108,10 +111,14 @@ def test_run_coro_does_not_log_on_exception(caplog: pytest.LogCaptureFixture) -> async def boom() -> None: raise RuntimeError('boom') - with caplog.at_level(logging.DEBUG, logger='apify.scrapy._async_thread'), pytest.raises(RuntimeError, match='boom'): - thread.run_coro(boom()) - - thread.close() + try: + with ( + caplog.at_level(logging.DEBUG, logger='apify.scrapy._async_thread'), + pytest.raises(RuntimeError, match='boom'), + ): + thread.run_coro(boom()) + finally: + thread.close() assert [record for record in caplog.records if record.levelno >= logging.ERROR] == [] @@ -120,29 +127,36 @@ def test_close_is_idempotent() -> None: """Calling `close` twice is a no-op the second time, not a `RuntimeError` on the closed loop.""" thread = AsyncThread() _wait_until_running(thread) - thread.run_coro(asyncio.sleep(0)) + try: + thread.run_coro(asyncio.sleep(0)) - thread.close() - thread.close() # must not raise + thread.close() + thread.close() # must not raise + finally: + thread.close() def test_close_passes_its_timeout_to_the_shutdown_step(monkeypatch: pytest.MonkeyPatch) -> None: """`close(timeout=...)` honours that timeout for the task-cancellation step, not only the thread join.""" thread = AsyncThread() _wait_until_running(thread) - thread.run_coro(asyncio.sleep(0)) - recorded: list[timedelta | str] = [] - original = thread.run_coro + try: + thread.run_coro(asyncio.sleep(0)) + + recorded: list[timedelta | str] = [] + original = thread.run_coro - def spy(coro: Any, timeout: timedelta | Literal['default'] = 'default') -> Any: - recorded.append(timeout) - return original(coro, timeout=timeout) + def spy(coro: Any, timeout: timedelta | Literal['default'] = 'default') -> Any: + recorded.append(timeout) + return original(coro, timeout=timeout) - monkeypatch.setattr(thread, 'run_coro', spy) - thread.close(timeout=timedelta(seconds=42)) + monkeypatch.setattr(thread, 'run_coro', spy) + thread.close(timeout=timedelta(seconds=42)) - assert recorded == [timedelta(seconds=42)] + assert recorded == [timedelta(seconds=42)] + finally: + thread.close() def test_close_stops_and_joins_thread_even_when_task_cancellation_fails(monkeypatch: pytest.MonkeyPatch) -> None: @@ -155,12 +169,15 @@ async def boom() -> None: monkeypatch.setattr(thread, '_shutdown_tasks', boom) - with pytest.raises(RuntimeError, match='shutdown boom'): - thread.close(timeout=timedelta(seconds=5)) + try: + with pytest.raises(RuntimeError, match='shutdown boom'): + thread.close(timeout=timedelta(seconds=5)) - # The loop was stopped and its thread joined despite the failing cancellation, so nothing is left running. - assert not thread._thread.is_alive() - assert thread._eventloop.is_closed() + # The loop was stopped and its thread joined despite the failing cancellation, so nothing is left running. + assert not thread._thread.is_alive() + assert thread._eventloop.is_closed() + finally: + thread.close() def test_submit_coro_runs_the_coroutine_without_blocking() -> None: @@ -175,15 +192,18 @@ async def gated() -> None: await asyncio.to_thread(release.wait) finished.set() - thread.submit_coro(gated()) - - # The call returned while the coroutine is still parked on the gate. - assert not finished.is_set() + try: + thread.submit_coro(gated()) - release.set() - assert finished.wait(timeout=2) + # The call returned while the coroutine is still parked on the gate. + assert not finished.is_set() - thread.close() + release.set() + assert finished.wait(timeout=2) + finally: + # Open the gate before closing, so a failed assertion above cannot leave `close` waiting on it. + release.set() + thread.close() def test_submit_coro_logs_a_failing_coroutine(caplog: pytest.LogCaptureFixture) -> None: @@ -194,8 +214,11 @@ def test_submit_coro_logs_a_failing_coroutine(caplog: pytest.LogCaptureFixture) async def boom() -> None: raise RuntimeError('boom') - with caplog.at_level(logging.ERROR, logger='apify.scrapy._async_thread'): - thread.submit_coro(boom()) + try: + with caplog.at_level(logging.ERROR, logger='apify.scrapy._async_thread'): + thread.submit_coro(boom()) + thread.close() + finally: thread.close() errors = [record for record in caplog.records if record.levelno >= logging.ERROR] @@ -210,9 +233,11 @@ def test_submit_coro_raises_after_close() -> None: thread.close() coro = _return(42) - with pytest.raises(RuntimeError): - thread.submit_coro(coro) - coro.close() + try: + with pytest.raises(RuntimeError): + thread.submit_coro(coro) + finally: + coro.close() def test_wait_for_submitted_blocks_until_the_coroutines_finish() -> None: @@ -227,13 +252,16 @@ async def gated() -> None: await asyncio.to_thread(release.wait) finished.set() - thread.submit_coro(gated()) - release.set() + try: + thread.submit_coro(gated()) + release.set() - thread.wait_for_submitted() + thread.wait_for_submitted() - assert finished.is_set() - thread.close() + assert finished.is_set() + finally: + release.set() + thread.close() def test_wait_for_submitted_keeps_an_unfinished_coroutine_tracked(caplog: pytest.LogCaptureFixture) -> None: @@ -246,18 +274,21 @@ def test_wait_for_submitted_keeps_an_unfinished_coroutine_tracked(caplog: pytest async def gated() -> None: await asyncio.to_thread(release.wait) - thread.submit_coro(gated()) - - with caplog.at_level(logging.WARNING, logger='apify.scrapy._async_thread'): - thread.wait_for_submitted(timeout=timedelta(seconds=0.01)) - assert len(thread._submitted) == 1 - assert [record for record in caplog.records if record.levelno == logging.WARNING] + try: + thread.submit_coro(gated()) - release.set() - thread.wait_for_submitted() - assert thread._submitted == [] + with caplog.at_level(logging.WARNING, logger='apify.scrapy._async_thread'): + thread.wait_for_submitted(timeout=timedelta(seconds=0.01)) + assert len(thread._submitted) == 1 + assert [record for record in caplog.records if record.levelno == logging.WARNING] - thread.close() + release.set() + thread.wait_for_submitted() + assert thread._submitted == [] + finally: + # Open the gate before closing, so a failed assertion above cannot leave `close` waiting on it. + release.set() + thread.close() def test_submit_coro_drops_the_finished_futures() -> None: @@ -265,13 +296,14 @@ def test_submit_coro_drops_the_finished_futures() -> None: thread = AsyncThread() _wait_until_running(thread) - for _ in range(SUBMITTED_PRUNE_THRESHOLD): - thread.submit_coro(_return(1)) - - assert futures.wait(list(thread._submitted), timeout=2).not_done == set() + try: + for _ in range(SUBMITTED_PRUNE_THRESHOLD): + thread.submit_coro(_return(1)) - # Every tracked coroutine has finished, so this submission drops them instead of growing the list. - thread.submit_coro(_return(1)) - assert len(thread._submitted) == 1 + assert futures.wait(list(thread._submitted), timeout=2).not_done == set() - thread.close() + # Every tracked coroutine has finished, so this submission drops them instead of growing the list. + thread.submit_coro(_return(1)) + assert len(thread._submitted) == 1 + finally: + thread.close() From ee0f86e81d69707927a214a6ea4a7aa91dfa5ad8 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 10:46:33 +0200 Subject: [PATCH 10/17] refactor(scrapy): use the RQ abbreviation and tighten the comments this PR added --- src/apify/scrapy/_async_thread.py | 12 ++--- src/apify/scrapy/requests.py | 25 +++++---- src/apify/scrapy/scheduler.py | 53 +++++++++---------- .../scrapy/requests/test_to_scrapy_request.py | 2 +- tests/unit/scrapy/test_scheduler.py | 26 ++++----- 5 files changed, 56 insertions(+), 62 deletions(-) diff --git a/src/apify/scrapy/_async_thread.py b/src/apify/scrapy/_async_thread.py index 5e8c72dc..b9c0bb31 100644 --- a/src/apify/scrapy/_async_thread.py +++ b/src/apify/scrapy/_async_thread.py @@ -83,15 +83,15 @@ def run_coro( def submit_coro(self, coro: Coroutine) -> futures.Future: """Schedule a coroutine on the event loop without waiting for its result. - Use this for work whose result nothing depends on, so the calling thread is not blocked by the round - trip. Failures are logged, as no caller is left to propagate them to, and `close` cancels whatever is - still pending - call `wait_for_submitted` first if that matters. + Use this for work nothing depends on, so the calling thread is not blocked by the round trip. Failures + are logged, as there is no caller to propagate them to, and `close` cancels whatever is still pending - + call `wait_for_submitted` first if that matters. Args: coro: The coroutine to run. Returns: - The future of the scheduled coroutine, for callers that do want to inspect its outcome later. + The future of the scheduled coroutine, for callers that want to inspect its outcome later. Raises: RuntimeError: If the event loop has been closed. @@ -99,8 +99,8 @@ def submit_coro(self, coro: Coroutine) -> futures.Future: if self._eventloop.is_closed(): raise RuntimeError(f'The coroutine {coro} cannot be executed because the event loop is closed.') - # `wait_for_submitted` only runs once Scrapy goes idle, so without pruning here the list would hold - # every coroutine the whole crawl ever submitted, with its result. + # `wait_for_submitted` only runs once Scrapy goes idle, so without pruning the list would hold every + # coroutine the crawl ever submitted, with its result. if len(self._submitted) >= SUBMITTED_PRUNE_THRESHOLD: self._submitted = [submitted for submitted in self._submitted if not submitted.done()] diff --git a/src/apify/scrapy/requests.py b/src/apify/scrapy/requests.py index a57414d8..1185414b 100644 --- a/src/apify/scrapy/requests.py +++ b/src/apify/scrapy/requests.py @@ -49,16 +49,15 @@ def _ensure_known_request_class(request_dict: dict[str, Any]) -> None: def _compute_fingerprint(scrapy_request: ScrapyRequest) -> str: - """Identify the request a queue unique key was minted for. + """Identify the request an RQ unique key was minted for. `to_scrapy_request` stamps this beside the unique key so `to_apify_request` can tell a request that came out - of the queue from one Scrapy derived from it. It covers the URL, the method and the body, so a redirect, a - method switch and a changed body are all recognized as a different request rather than the parent. + of the RQ from one Scrapy derived from it. It covers the URL, the method and the body, so a redirect, a + method switch and a changed body all read as a different request rather than the parent. - Headers are deliberately left out even though they are part of the unique key: Scrapy's own downloader - middlewares (`DefaultHeadersMiddleware`, `UserAgentMiddleware`) call `headers.setdefault()` on the request - in place before it is handed back to the scheduler, so a request that is otherwise untouched would no longer - match the stamp it was given. + Headers are left out even though the unique key covers them: `DefaultHeadersMiddleware` and + `UserAgentMiddleware` call `headers.setdefault()` on the request in place before the scheduler sees it + again, so an otherwise untouched request would no longer match its own stamp. """ return compute_unique_key( url=scrapy_request.url, @@ -99,10 +98,10 @@ def to_apify_request(scrapy_request: ScrapyRequest, spider: Spider) -> ApifyRequ if scrapy_request.dont_filter: request_kwargs['always_enqueue'] = True elif unique_key := scrapy_request.meta.get('apify_request_unique_key'): - # Reuse the queue's unique key only while this is still the request it was minted for. Redirects - # (`Request.replace()`) and spiders forwarding `meta` to another URL both inherit the stamp, and - # reusing it there deduplicates the derived request against its parent. A stamp without a - # fingerprint beside it was set by hand, so it is taken at face value. + # Reuse the RQ unique key only while this is still the request it was minted for. Redirects + # (`Request.replace()`) and spiders forwarding `meta` to another URL inherit the stamp, and reusing + # it there deduplicates the derived request against its parent. A stamp without a fingerprint was + # set by hand, so it is taken at face value. fingerprint = _compute_fingerprint(scrapy_request) if scrapy_request.meta.get('apify_request_fingerprint', fingerprint) == fingerprint: request_kwargs['unique_key'] = unique_key @@ -219,8 +218,8 @@ def to_scrapy_request(apify_request: ApifyRequest, spider: Spider) -> ScrapyRequ else: scrapy_request = ScrapyRequest(url=apify_request.url, method=apify_request.method) - # Stamp the unique key together with a fingerprint of the request it belongs to, so `to_apify_request` can - # tell this request apart from the ones Scrapy derives from it. + # Stamp the unique key with a fingerprint of the request it belongs to, so `to_apify_request` can tell + # this request apart from the ones Scrapy derives from it. scrapy_request.meta['apify_request_unique_key'] = apify_request.unique_key scrapy_request.meta['apify_request_fingerprint'] = _compute_fingerprint(scrapy_request) diff --git a/src/apify/scrapy/scheduler.py b/src/apify/scrapy/scheduler.py index cc546a2a..9d2a3a12 100644 --- a/src/apify/scrapy/scheduler.py +++ b/src/apify/scrapy/scheduler.py @@ -30,10 +30,9 @@ async def _gather_failures(operations: Iterable[Coroutine[Any, Any, Any]]) -> list[BaseException | None]: - """Run request queue updates concurrently, reporting each one's failure, or `None`, in the order given. + """Run RQ updates concurrently, reporting each one's failure, or `None`, in the order given. - The updates of a whole batch cost one round trip rather than one each, and one failing update neither - raises nor stops the others. + The whole batch costs one round trip, and one failing update neither raises nor stops the others. """ outcomes = await asyncio.gather(*operations, return_exceptions=True) return [outcome if isinstance(outcome, BaseException) else None for outcome in outcomes] @@ -61,7 +60,7 @@ def __init__( self._crawler = crawler self._requests_in_flight: list[tuple[ApifyRequest, Request]] = [] - """Requests handed over to Scrapy and not resolved in the request queue yet.""" + """Requests handed over to Scrapy and not resolved in the RQ yet.""" self._pending_marks: list[tuple[list[tuple[ApifyRequest, Request]], Future]] = [] """Batches of mark-as-handled updates dispatched off the hot path, whose outcome is not known yet.""" @@ -137,9 +136,8 @@ def close(self, reason: str) -> None: except Exception: logger.exception('Failed to resolve the requests still in flight in the request queue.') - # Whatever Scrapy did not finish goes back to the queue, so the next run gets it as pending. The - # reclaims travel together: a migration cuts the shutdown short, and one round trip per request may - # not fit in what is left of it. One failed reclaim must not strand the rest either. + # Whatever Scrapy did not finish goes back to the RQ as pending, in a single round trip: a migration + # cuts the shutdown short. One failed reclaim must not strand the rest either. if self._requests_in_flight: reclaims = _gather_failures( rq.reclaim_request(apify_request) for apify_request, _ in self._requests_in_flight @@ -158,8 +156,7 @@ def close(self, reason: str) -> None: self._requests_in_flight.clear() - # Closing the event loop cancels the updates fired off on the hot path silently, leaving those - # requests unhandled. + # Closing the event loop would silently cancel the updates fired off on the hot path. self._async_thread.wait_for_submitted() try: @@ -189,7 +186,7 @@ def has_pending_requests(self) -> bool: # as in flight is provably finished. self._resolve_finished_requests(wait=True) - # The queue answers from its own bookkeeping, which a pending update has not reached yet. + # The RQ answers from its own bookkeeping, which a pending update has not reached yet. self._async_thread.wait_for_submitted() # Log here before re-raising: this coroutine ran on a separate event-loop thread, and the failure is @@ -243,8 +240,8 @@ def next_request(self) -> Request | None: if not isinstance(self._rq, RequestQueue): raise TypeError('self._rq must be an instance of the RequestQueue class') - # The engine polls this method throughout the crawl, so resolving here keeps the queue current - # without blocking on the round trips. + # The engine polls this method throughout the crawl, so resolving here keeps the RQ current without + # blocking on the round trips. self._resolve_finished_requests(wait=False) # Log here before re-raising: this coroutine ran on a separate event-loop thread, and the failure is @@ -262,7 +259,7 @@ def next_request(self) -> Request | None: raise TypeError('self.spider must be an instance of the Spider class') # A corrupt or legacy payload must not crash the run, and is marked as handled right away, otherwise - # the queue would keep handing it back forever. + # the RQ would keep handing it back forever. try: scrapy_request = to_scrapy_request(apify_request, spider=self.spider) except Exception as exc: @@ -283,10 +280,9 @@ def next_request(self) -> Request | None: def _verify_engine_internals(self) -> None: """Fail early if Scrapy's engine no longer exposes what the in-flight tracking reads. - `_requests_busy_in_scrapy` reads engine internals Scrapy does not document as public API, and the hot - path deliberately does not guard them: swallowing an `AttributeError` there would quietly revert to - marking every request as handled the moment it is handed over, which is what the tracking exists to - prevent. Checking once at open time keeps such a breakage loud, early and easy to place. + The hot path deliberately does not guard those undocumented internals: swallowing an `AttributeError` + there would quietly go back to marking every request as handled the moment it is handed over. Checking + once at open time keeps such a breakage loud and early. Raises: RuntimeError: If the engine internals the tracking relies on cannot be read. @@ -304,10 +300,10 @@ def _requests_busy_in_scrapy(self) -> set[Request]: """Return the requests Scrapy is still working on. A request joins the downloader's active set before the middleware chain runs and leaves the scraper's - only once the callback and the item pipeline are done, so absence from both means Scrapy has finished - with it - downloaded, dropped by a middleware or errored out alike. + only once the callback and the item pipeline are done, so absence from both means Scrapy is done with + it - downloaded, dropped by a middleware or errored out alike. - Without a crawler there is nothing to ask, and every request reads as finished; `open` warns about that. + Without a crawler there is nothing to ask and every request reads as finished; `open` warns about that. """ engine = self._crawler.engine if self._crawler is not None else None if engine is None: @@ -317,14 +313,13 @@ def _requests_busy_in_scrapy(self) -> set[Request]: return engine.downloader.active | (scraper_slot.active if scraper_slot is not None else set()) def _resolve_finished_requests(self, *, wait: bool) -> None: - """Mark every request Scrapy has finished processing as handled in the request queue. + """Mark every request Scrapy has finished processing as handled in the RQ. - A whole resolution pass reaches the queue in a single round trip. A request whose update cannot be - dispatched, or whose dispatched update turns out to have failed, stays tracked for the next call to - retry, without holding up the rest of the list. + A whole pass reaches the RQ in a single round trip. A request whose update cannot be dispatched, or + turns out to have failed, stays tracked for the next call to retry. Args: - wait: Whether to block until the queue has been updated. Pass False on the crawl's hot path, where + wait: Whether to block until the RQ has been updated. Pass False on the crawl's hot path, where nothing depends on the result and blocking would stall the Twisted reactor. """ rq = self._rq @@ -359,9 +354,9 @@ def _resolve_finished_requests(self, *, wait: bool) -> None: def _collect_failed_marks(self, *, wait: bool) -> list[tuple[ApifyRequest, Request]]: """Return the requests whose already dispatched mark-as-handled did not land, so it can be retried. - `submit_coro` reports nothing back to the reactor thread, so an update that fails after it was - dispatched would otherwise drop its request from the tracking for good and leave it in progress in the - queue forever - the queue would never report itself finished and the crawl would never end. + `submit_coro` reports nothing back to the reactor thread, so an update failing after dispatch would + otherwise drop its request from the tracking and leave it in progress in the RQ forever, which would + keep the RQ from ever reporting itself finished. Args: wait: Whether to block until every dispatched update has finished. Updates still running are kept @@ -384,7 +379,7 @@ def _collect_failed_marks(self, *, wait: bool) -> list[tuple[ApifyRequest, Reque failed.extend(requests) else: # `_gather_failures` reports a failed update in its result rather than raising, so an exception - # here means the dispatch itself did not survive - the event loop was torn down under it. + # here means the dispatch itself did not survive the event loop being torn down under it. try: outcomes = future.result() except Exception: diff --git a/tests/unit/scrapy/requests/test_to_scrapy_request.py b/tests/unit/scrapy/requests/test_to_scrapy_request.py index c0b656cf..16c037a1 100644 --- a/tests/unit/scrapy/requests/test_to_scrapy_request.py +++ b/tests/unit/scrapy/requests/test_to_scrapy_request.py @@ -69,7 +69,7 @@ def test_without_reconstruction(spider: Spider) -> None: def test_unique_key_is_stamped_together_with_its_fingerprint(spider: Spider) -> None: - """The queue's unique key is stamped alongside a fingerprint of the request it belongs to.""" + """The RQ unique key is stamped alongside a fingerprint of the request it belongs to.""" apify_request = ApifyRequest( url='https://example.com', method='GET', diff --git a/tests/unit/scrapy/test_scheduler.py b/tests/unit/scrapy/test_scheduler.py index 959fd3f4..67a80951 100644 --- a/tests/unit/scrapy/test_scheduler.py +++ b/tests/unit/scrapy/test_scheduler.py @@ -48,7 +48,7 @@ class FakeAsyncThread: """Stand-in for `AsyncThread` that runs the scheduler's coroutines on a real event loop. The scheduler batches the updates of a whole resolution pass into a single coroutine, so a double that - never runs them would leave these tests asserting on the batching instead of on what reaches the queue. + never runs them would leave these tests asserting on the batching instead of on what reaches the RQ. """ def __init__(self, default_timeout: timedelta | None = None) -> None: @@ -77,7 +77,7 @@ def close(self) -> None: def stub_scheduler_dependencies(monkeypatch: pytest.MonkeyPatch) -> None: - """Stub out the reactor check, the event loop thread and the queue that `open` reaches for.""" + """Stub out the reactor check, the event loop thread and the RQ that `open` reaches for.""" monkeypatch.setattr('apify.scrapy.scheduler.is_asyncio_reactor_installed', lambda: True) monkeypatch.setattr('apify.scrapy.scheduler.AsyncThread', FakeAsyncThread) @@ -91,7 +91,7 @@ async def open_rq(*_args: Any, **_kwargs: Any) -> Any: @pytest.fixture def scheduler(monkeypatch: pytest.MonkeyPatch, spider: DummySpider) -> ApifyScheduler: - """Create a scheduler with its reactor check stubbed out, a fake event loop thread and a mocked queue.""" + """Create a scheduler with its reactor check stubbed out, a fake event loop thread and a mocked RQ.""" stub_scheduler_dependencies(monkeypatch) scheduler = ApifyScheduler() @@ -108,10 +108,10 @@ def test_has_pending_requests_reflects_queue_state(scheduler: ApifyScheduler) -> """`has_pending_requests` is True while the queue is not finished and False once it is.""" rq = cast('mock.AsyncMock', scheduler._rq) - rq.is_finished.return_value = False # the queue still has work + rq.is_finished.return_value = False # the RQ still has work assert scheduler.has_pending_requests() is True - rq.is_finished.return_value = True # the queue is drained + rq.is_finished.return_value = True # the RQ is drained assert scheduler.has_pending_requests() is False @@ -184,7 +184,7 @@ def test_next_request_skips_request_that_fails_to_convert( def test_next_request_returns_converted_request(scheduler: ApifyScheduler) -> None: - """A valid queue entry is reconstructed into a Scrapy request and left unhandled until Scrapy is done.""" + """A valid RQ entry is reconstructed into a Scrapy request and left unhandled until Scrapy is done.""" rq = cast('mock.AsyncMock', scheduler._rq) apify_request = ApifyRequest( @@ -319,7 +319,7 @@ def test_next_request_marks_finished_requests_without_blocking(scheduler: ApifyS def test_has_pending_requests_waits_for_the_non_blocking_updates(scheduler: ApifyScheduler) -> None: - """The queue is asked whether it is finished only after the updates fired off on the hot path have landed.""" + """The RQ is asked whether it is finished only after the updates fired off on the hot path have landed.""" rq = cast('mock.AsyncMock', scheduler._rq) async_thread = cast('FakeAsyncThread', scheduler._async_thread) scheduler._crawler = fake_crawler() @@ -327,7 +327,7 @@ def test_has_pending_requests_waits_for_the_non_blocking_updates(scheduler: Apif rq.is_finished.return_value = True assert scheduler.has_pending_requests() is False - # The queue answers from its own bookkeeping, which a pending update has not reached yet. + # The RQ answers from its own bookkeeping, which a pending update has not reached yet. assert async_thread.calls == ['wait_for_submitted', 'run_coro'] @@ -339,7 +339,7 @@ def test_has_pending_requests_waits_for_the_non_blocking_updates(scheduler: Apif ], ) def test_close_reclaims_requests_scrapy_never_finished(scheduler: ApifyScheduler, busy_kwarg: str) -> None: - """Requests still being processed when the scheduler closes go back to the queue instead of being lost.""" + """Requests still being processed when the scheduler closes go back to the RQ instead of being lost.""" rq = cast('mock.AsyncMock', scheduler._rq) (scrapy_request,) = hand_out(scheduler, 1) @@ -372,7 +372,7 @@ def test_close_reclaims_the_other_requests_after_a_failed_reclaim( scheduler: ApifyScheduler, caplog: pytest.LogCaptureFixture, ) -> None: - """One failing reclaim does not stop the other in-flight requests from going back to the queue.""" + """One failing reclaim does not stop the other in-flight requests from going back to the RQ.""" rq = cast('mock.AsyncMock', scheduler._rq) hand_out(scheduler, 2) @@ -386,7 +386,7 @@ def test_close_reclaims_the_other_requests_after_a_failed_reclaim( assert len(errors) == 1 -def test_close_reaches_the_queue_in_one_round_trip_per_operation(scheduler: ApifyScheduler) -> None: +def test_close_reaches_the_rq_in_one_round_trip_per_operation(scheduler: ApifyScheduler) -> None: """Marks and reclaims each travel together, as a migration may not leave room for one round trip each.""" rq = cast('mock.AsyncMock', scheduler._rq) async_thread = cast('FakeAsyncThread', scheduler._async_thread) @@ -423,7 +423,7 @@ def test_a_failed_mark_keeps_the_request_tracked( hand_out(scheduler, 1) scheduler._crawler = fake_crawler() - # The mark fails, then the queue reports itself unfinished because the request is still in progress. + # The mark fails, then the RQ reports itself unfinished because the request is still in progress. rq.mark_request_as_handled.side_effect = RuntimeError('boom') rq.is_finished.return_value = False @@ -439,7 +439,7 @@ def test_a_mark_that_fails_after_being_fired_off_is_retried( scheduler: ApifyScheduler, caplog: pytest.LogCaptureFixture, ) -> None: - """A hot-path mark that fails on its way to the queue is retried, instead of leaving the request in progress.""" + """A hot-path mark that fails on its way to the RQ is retried, instead of leaving the request in progress.""" rq = cast('mock.AsyncMock', scheduler._rq) hand_out(scheduler, 1) From 56235625b03933f0cd00030a117dc16fd9722bde Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 11:30:52 +0200 Subject: [PATCH 11/17] refactor(scrapy): alias the in-flight tuple and mock the async thread with Mock --- src/apify/scrapy/scheduler.py | 26 ++++++----- tests/unit/scrapy/test_scheduler.py | 69 ++++++++++++----------------- 2 files changed, 44 insertions(+), 51 deletions(-) diff --git a/src/apify/scrapy/scheduler.py b/src/apify/scrapy/scheduler.py index 9d2a3a12..010a2816 100644 --- a/src/apify/scrapy/scheduler.py +++ b/src/apify/scrapy/scheduler.py @@ -26,6 +26,9 @@ from apify import Request as ApifyRequest + InFlightRequest = tuple[ApifyRequest, Request] + """A request handed over to Scrapy, paired with the RQ request it came from.""" + logger = getLogger(__name__) @@ -59,10 +62,10 @@ def __init__( self.spider: Spider | None = None self._crawler = crawler - self._requests_in_flight: list[tuple[ApifyRequest, Request]] = [] + self._requests_in_flight: list[InFlightRequest] = [] """Requests handed over to Scrapy and not resolved in the RQ yet.""" - self._pending_marks: list[tuple[list[tuple[ApifyRequest, Request]], Future]] = [] + self._pending_marks: list[tuple[list[InFlightRequest], Future]] = [] """Batches of mark-as-handled updates dispatched off the hot path, whose outcome is not known yet.""" # A thread with the asyncio event loop to run coroutines on. @@ -278,11 +281,12 @@ def next_request(self) -> Request | None: return scrapy_request def _verify_engine_internals(self) -> None: - """Fail early if Scrapy's engine no longer exposes what the in-flight tracking reads. + """Fail at open time if Scrapy's engine no longer exposes what the in-flight tracking reads. The hot path deliberately does not guard those undocumented internals: swallowing an `AttributeError` - there would quietly go back to marking every request as handled the moment it is handed over. Checking - once at open time keeps such a breakage loud and early. + there would quietly go back to marking every request as handled the moment it is handed over. The + scraper's slot does not exist yet when the scheduler opens, so this only proves `downloader.active` and + `scraper.slot` are there; a missing `slot.active` surfaces from `next_request` on the first request. Raises: RuntimeError: If the engine internals the tracking relies on cannot be read. @@ -328,7 +332,7 @@ def _resolve_finished_requests(self, *, wait: bool) -> None: # Updates dispatched by an earlier pass that did not land are marked again by this one. finished = self._collect_failed_marks(wait=wait) - unresolved: list[tuple[ApifyRequest, Request]] = [] + unresolved: list[InFlightRequest] = [] if self._requests_in_flight: busy = self._requests_busy_in_scrapy() @@ -351,7 +355,7 @@ def _resolve_finished_requests(self, *, wait: bool) -> None: self._requests_in_flight = unresolved - def _collect_failed_marks(self, *, wait: bool) -> list[tuple[ApifyRequest, Request]]: + def _collect_failed_marks(self, *, wait: bool) -> list[InFlightRequest]: """Return the requests whose already dispatched mark-as-handled did not land, so it can be retried. `submit_coro` reports nothing back to the reactor thread, so an update failing after dispatch would @@ -368,8 +372,8 @@ def _collect_failed_marks(self, *, wait: bool) -> list[tuple[ApifyRequest, Reque if wait: self._async_thread.wait_for_submitted() - failed: list[tuple[ApifyRequest, Request]] = [] - pending: list[tuple[list[tuple[ApifyRequest, Request]], Future]] = [] + failed: list[InFlightRequest] = [] + pending: list[tuple[list[InFlightRequest], Future]] = [] for requests, future in self._pending_marks: if not future.done(): @@ -394,9 +398,9 @@ def _collect_failed_marks(self, *, wait: bool) -> list[tuple[ApifyRequest, Reque @staticmethod def _failed_marks( - requests: list[tuple[ApifyRequest, Request]], + requests: list[InFlightRequest], outcomes: list[BaseException | None], - ) -> list[tuple[ApifyRequest, Request]]: + ) -> list[InFlightRequest]: """Pair a batch of updates back with their requests, returning and logging the ones that failed.""" failed = [] diff --git a/tests/unit/scrapy/test_scheduler.py b/tests/unit/scrapy/test_scheduler.py index 67a80951..df7fac9b 100644 --- a/tests/unit/scrapy/test_scheduler.py +++ b/tests/unit/scrapy/test_scheduler.py @@ -13,6 +13,7 @@ from scrapy.settings import Settings from apify import Request as ApifyRequest +from apify.scrapy._async_thread import AsyncThread from apify.scrapy.scheduler import ApifyScheduler from apify.storages import RequestQueue @@ -44,24 +45,14 @@ def fake_crawler( return SimpleNamespace(engine=engine) -class FakeAsyncThread: - """Stand-in for `AsyncThread` that runs the scheduler's coroutines on a real event loop. +def fake_async_thread(default_timeout: timedelta | None = None) -> mock.Mock: # noqa: ARG001 + """Build an `AsyncThread` double that runs the scheduler's coroutines on a real event loop. The scheduler batches the updates of a whole resolution pass into a single coroutine, so a double that never runs them would leave these tests asserting on the batching instead of on what reaches the RQ. """ - def __init__(self, default_timeout: timedelta | None = None) -> None: - self.default_timeout = default_timeout - self.calls: list[str] = [] - """The methods called on this thread, in order, for the tests that care about the ordering.""" - - def run_coro(self, coro: Coroutine) -> Any: - self.calls.append('run_coro') - return asyncio.run(coro) - - def submit_coro(self, coro: Coroutine) -> Future: - self.calls.append('submit_coro') + def submit_coro(coro: Coroutine) -> Future: future: Future = Future() try: future.set_result(asyncio.run(coro)) @@ -69,17 +60,21 @@ def submit_coro(self, coro: Coroutine) -> Future: future.set_exception(exc) return future - def wait_for_submitted(self) -> None: - self.calls.append('wait_for_submitted') + async_thread = mock.Mock(spec=AsyncThread) + async_thread.run_coro.side_effect = asyncio.run + async_thread.submit_coro.side_effect = submit_coro + return async_thread + - def close(self) -> None: - self.calls.append('close') +def called_methods(async_thread: mock.Mock) -> list[str]: + """Name the methods called on an `AsyncThread` double, in order, for the tests that care about the ordering.""" + return [name for name, _args, _kwargs in async_thread.method_calls] def stub_scheduler_dependencies(monkeypatch: pytest.MonkeyPatch) -> None: """Stub out the reactor check, the event loop thread and the RQ that `open` reaches for.""" monkeypatch.setattr('apify.scrapy.scheduler.is_asyncio_reactor_installed', lambda: True) - monkeypatch.setattr('apify.scrapy.scheduler.AsyncThread', FakeAsyncThread) + monkeypatch.setattr('apify.scrapy.scheduler.AsyncThread', mock.Mock(side_effect=fake_async_thread)) async def open_rq(*_args: Any, **_kwargs: Any) -> Any: rq = mock.AsyncMock() @@ -234,20 +229,14 @@ def test_next_request_logs_exception_before_propagating( def test_from_crawler_reads_async_thread_timeout_setting(monkeypatch: pytest.MonkeyPatch) -> None: """`from_crawler` wires the `APIFY_ASYNC_THREAD_TIMEOUT_SECS` setting into the async thread's timeout.""" - monkeypatch.setattr('apify.scrapy.scheduler.is_asyncio_reactor_installed', lambda: True) - - captured: dict[str, Any] = {} - - class _RecordingAsyncThread: - def __init__(self, default_timeout: timedelta | None = None) -> None: - captured['default_timeout'] = default_timeout - - monkeypatch.setattr('apify.scrapy.scheduler.AsyncThread', _RecordingAsyncThread) + stub_scheduler_dependencies(monkeypatch) + async_thread_cls = mock.Mock(side_effect=fake_async_thread) + monkeypatch.setattr('apify.scrapy.scheduler.AsyncThread', async_thread_cls) crawler = SimpleNamespace(settings=Settings({'APIFY_ASYNC_THREAD_TIMEOUT_SECS': 123})) ApifyScheduler.from_crawler(cast('Any', crawler)) - assert captured['default_timeout'] == timedelta(seconds=123) + async_thread_cls.assert_called_once_with(default_timeout=timedelta(seconds=123)) APIFY_REQUESTS = [ @@ -300,7 +289,7 @@ def test_has_pending_requests_marks_finished_requests_as_handled(scheduler: Apif def test_next_request_marks_finished_requests_without_blocking(scheduler: ApifyScheduler) -> None: """On the crawl's hot path a finished request is marked as handled without blocking the reactor on it.""" rq = cast('mock.AsyncMock', scheduler._rq) - async_thread = cast('FakeAsyncThread', scheduler._async_thread) + async_thread = cast('mock.Mock', scheduler._async_thread) (scrapy_request,) = hand_out(scheduler, 1) @@ -311,24 +300,24 @@ def test_next_request_marks_finished_requests_without_blocking(scheduler: ApifyS # Scrapy is done with it, so it is resolved off the reactor thread instead of blocking on the round trip. scheduler._crawler = fake_crawler(scraper_busy=set()) - async_thread.calls.clear() + async_thread.reset_mock() assert scheduler.next_request() is None rq.mark_request_as_handled.assert_called_once_with(APIFY_REQUESTS[0]) - assert 'submit_coro' in async_thread.calls + assert 'submit_coro' in called_methods(async_thread) def test_has_pending_requests_waits_for_the_non_blocking_updates(scheduler: ApifyScheduler) -> None: """The RQ is asked whether it is finished only after the updates fired off on the hot path have landed.""" rq = cast('mock.AsyncMock', scheduler._rq) - async_thread = cast('FakeAsyncThread', scheduler._async_thread) + async_thread = cast('mock.Mock', scheduler._async_thread) scheduler._crawler = fake_crawler() rq.is_finished.return_value = True assert scheduler.has_pending_requests() is False # The RQ answers from its own bookkeeping, which a pending update has not reached yet. - assert async_thread.calls == ['wait_for_submitted', 'run_coro'] + assert called_methods(async_thread) == ['wait_for_submitted', 'run_coro'] @pytest.mark.parametrize( @@ -389,28 +378,29 @@ def test_close_reclaims_the_other_requests_after_a_failed_reclaim( def test_close_reaches_the_rq_in_one_round_trip_per_operation(scheduler: ApifyScheduler) -> None: """Marks and reclaims each travel together, as a migration may not leave room for one round trip each.""" rq = cast('mock.AsyncMock', scheduler._rq) - async_thread = cast('FakeAsyncThread', scheduler._async_thread) + async_thread = cast('mock.Mock', scheduler._async_thread) handed_out = hand_out(scheduler, 4) # Scrapy finished half of the requests and is still working on the rest when the run is interrupted. scheduler._crawler = fake_crawler(downloader_busy=set(handed_out[2:])) - async_thread.calls.clear() + async_thread.reset_mock() scheduler.close('shutdown') assert rq.mark_request_as_handled.await_count == 2 assert rq.reclaim_request.await_count == 2 - assert async_thread.calls.count('run_coro') == 2 + assert called_methods(async_thread).count('run_coro') == 2 def test_close_waits_for_the_non_blocking_updates(scheduler: ApifyScheduler) -> None: """The event loop is not torn down before the updates fired off on the hot path have landed.""" - async_thread = cast('FakeAsyncThread', scheduler._async_thread) + async_thread = cast('mock.Mock', scheduler._async_thread) scheduler.close('finished') - assert async_thread.calls.index('wait_for_submitted') < async_thread.calls.index('close') + methods = called_methods(async_thread) + assert methods.index('wait_for_submitted') < methods.index('close') def test_a_failed_mark_keeps_the_request_tracked( @@ -503,8 +493,7 @@ def test_open_fails_loudly_when_the_scrapy_engine_internals_move( def test_from_crawler_keeps_the_crawler(monkeypatch: pytest.MonkeyPatch) -> None: """`from_crawler` keeps the crawler, which is how the scheduler learns what Scrapy is still working on.""" - monkeypatch.setattr('apify.scrapy.scheduler.is_asyncio_reactor_installed', lambda: True) - monkeypatch.setattr('apify.scrapy.scheduler.AsyncThread', mock.MagicMock()) + stub_scheduler_dependencies(monkeypatch) crawler = SimpleNamespace(settings=Settings()) scheduler = ApifyScheduler.from_crawler(cast('Any', crawler)) From cc7c7c5d6f225992bfbdba1a1c0a50afb0e030cd Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 11:51:01 +0200 Subject: [PATCH 12/17] test(scrapy): add E2E tests for redirects and in-flight requests --- tests/e2e/actor_source_base/server.py | 27 ++++++++++ .../actor_source/spider_in_flight.py | 47 ++++++++++++++++++ .../actor_source/spider_redirect.py | 32 ++++++++++++ .../e2e/test_scrapy/test_in_flight_spider.py | 49 +++++++++++++++++++ tests/e2e/test_scrapy/test_redirect_spider.py | 22 +++++++++ 5 files changed, 177 insertions(+) create mode 100644 tests/e2e/test_scrapy/actor_source/spider_in_flight.py create mode 100644 tests/e2e/test_scrapy/actor_source/spider_redirect.py create mode 100644 tests/e2e/test_scrapy/test_in_flight_spider.py create mode 100644 tests/e2e/test_scrapy/test_redirect_spider.py diff --git a/tests/e2e/actor_source_base/server.py b/tests/e2e/actor_source_base/server.py index fd5d1f38..5299afaa 100644 --- a/tests/e2e/actor_source_base/server.py +++ b/tests/e2e/actor_source_base/server.py @@ -11,6 +11,12 @@ /products/2 (depth 1 or 2) - Widget B /products/3 (depth 1 or 2) - Widget C +Routes not linked from anywhere, for the tests that need a specific response rather than a site to crawl: + + /redirect - Redirects (302) to /redirect-target + /redirect-target - Page the redirect lands on + /slow - Answers only after 10 minutes, to keep a request in flight while a run is interrupted + The homepage includes both direct product links (for Scrapy spiders that look for /products/ links on the start page) and category links (for testing crawl depth with Crawlee crawlers). With max_crawl_depth=2, the crawler reaches all products and categories but does not go beyond /deep/2. @@ -47,6 +53,17 @@ async def _send_html(send: Send, html: str, status: int = 200) -> None: await send({'type': 'http.response.body', 'body': html.encode()}) +async def _send_redirect(send: Send, location: str) -> None: + await send( + { + 'type': 'http.response.start', + 'status': 302, + 'headers': [[b'location', location.encode()]], + } + ) + await send({'type': 'http.response.body', 'body': b''}) + + async def app(scope: dict[str, Any], _receive: Receive, send: Send) -> None: assert scope['type'] == 'http' path = scope['path'] @@ -107,6 +124,16 @@ async def app(scope: dict[str, Any], _receive: Receive, send: Send) -> None: 'Back to Home' '', ) + elif path == '/redirect': + await _send_redirect(send, '/redirect-target') + elif path == '/redirect-target': + await _send_html( + send, + 'Redirect Target

Redirect Target

', + ) + elif path == '/slow': + await asyncio.sleep(600) + await _send_html(send, 'Slow Page

Slow Page

') elif path.startswith('/deep/'): try: n = int(path.split('/')[-1]) diff --git a/tests/e2e/test_scrapy/actor_source/spider_in_flight.py b/tests/e2e/test_scrapy/actor_source/spider_in_flight.py new file mode 100644 index 00000000..a3843c42 --- /dev/null +++ b/tests/e2e/test_scrapy/actor_source/spider_in_flight.py @@ -0,0 +1,47 @@ +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING, Any +from urllib.parse import urljoin + +from scrapy import Request, Spider + +if TYPE_CHECKING: + from collections.abc import AsyncIterator, Generator + + from scrapy.http.response import Response + +logger = logging.getLogger(__name__) + +IN_FLIGHT_LOG_MARKER = 'Handing the request over to the downloader' +"""Logged once Scrapy starts downloading a request, so a test knows the request is in flight.""" + + +class MarkInFlightMiddleware: + """Downloader middleware that logs `IN_FLIGHT_LOG_MARKER` for every request it sees.""" + + def process_request(self, request: Request, spider: Spider) -> None: # noqa: ARG002 + logger.info(f'{IN_FLIGHT_LOG_MARKER}: {request.url}') + + +class InFlightSpider(Spider): + """Request a page that does not answer, so the request stays in flight until the run is interrupted.""" + + name = 'in_flight_spider' + + custom_settings = { # noqa: RUF012 + 'DOWNLOADER_MIDDLEWARES': {'src.spiders.spider.MarkInFlightMiddleware': 543}, + # The page answers after 10 minutes; the download must not time out before the test aborts the run. + 'DOWNLOAD_TIMEOUT': 600, + } + + def __init__(self, start_urls: list[str], *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self.start_urls = start_urls + + async def start(self) -> AsyncIterator[Request]: + for url in self.start_urls: + yield Request(urljoin(url, '/slow'), callback=self.parse) + + def parse(self, response: Response) -> Generator[dict, None, None]: + yield {'url': response.url} diff --git a/tests/e2e/test_scrapy/actor_source/spider_redirect.py b/tests/e2e/test_scrapy/actor_source/spider_redirect.py new file mode 100644 index 00000000..e9743da0 --- /dev/null +++ b/tests/e2e/test_scrapy/actor_source/spider_redirect.py @@ -0,0 +1,32 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING, Any +from urllib.parse import urljoin + +from scrapy import Request, Spider + +if TYPE_CHECKING: + from collections.abc import AsyncIterator, Generator + + from scrapy.http.response import Response + + +class RedirectSpider(Spider): + """Fetch a page that redirects and report where the redirect landed.""" + + name = 'redirect_spider' + + def __init__(self, start_urls: list[str], *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self.start_urls = start_urls + + async def start(self) -> AsyncIterator[Request]: + # Plain requests without `dont_filter`, so the redirected request goes through deduplication. + for url in self.start_urls: + yield Request(urljoin(url, '/redirect'), callback=self.parse) + + def parse(self, response: Response) -> Generator[dict, None, None]: + yield { + 'url': response.url, + 'title': response.css('title::text').get(''), + } diff --git a/tests/e2e/test_scrapy/test_in_flight_spider.py b/tests/e2e/test_scrapy/test_in_flight_spider.py new file mode 100644 index 00000000..09ebb4d5 --- /dev/null +++ b/tests/e2e/test_scrapy/test_in_flight_spider.py @@ -0,0 +1,49 @@ +from __future__ import annotations + +from datetime import timedelta +from typing import TYPE_CHECKING + +from ..._utils import poll_until_condition +from .actor_source.spider_in_flight import IN_FLIGHT_LOG_MARKER +from .conftest import get_scrapy_source_files + +if TYPE_CHECKING: + from apify_client import ApifyClientAsync + + from ..conftest import MakeActorFunction + + +async def test_in_flight_spider(make_actor: MakeActorFunction, apify_client_async: ApifyClientAsync) -> None: + """A request Scrapy is still downloading when the run is aborted stays pending in the request queue.""" + actor = await make_actor( + label='scrapy-in-flight', + source_files=get_scrapy_source_files('spider_in_flight.py', 'InFlightSpider'), + additional_requirements=['scrapy>=2.14.0'], + ) + run = await actor.start() + run_client = apify_client_async.run(run.id) + + # Interrupt the run only once Scrapy holds the request, otherwise there would be nothing to lose. The container + # startup time is highly variable, so poll the log with a growing interval. + log_client = run_client.log() + log = await poll_until_condition( + log_client.get, + lambda log: bool(log and IN_FLIGHT_LOG_MARKER in log), + timeout=300, + poll_interval=2, + backoff_factor=1.2, + ) + assert log is not None + assert IN_FLIGHT_LOG_MARKER in log, f'The run did not log {IN_FLIGHT_LOG_MARKER!r} in time:\n{log}' + + await run_client.abort() + run_result = await run_client.wait_for_finish(wait_duration=timedelta(seconds=600)) + assert run_result is not None + assert run_result.status == 'ABORTED' + + # The queue's request counters are eventually consistent, so read the request itself. + assert run.default_request_queue_id is not None + requests = await apify_client_async.request_queue(run.default_request_queue_id).list_requests() + assert [request.url for request in requests.items] == ['http://localhost:8080/slow'] + (slow_request,) = requests.items + assert slow_request.handled_at is None, f'The interrupted request was marked as handled: {slow_request}' diff --git a/tests/e2e/test_scrapy/test_redirect_spider.py b/tests/e2e/test_scrapy/test_redirect_spider.py new file mode 100644 index 00000000..f8e0e462 --- /dev/null +++ b/tests/e2e/test_scrapy/test_redirect_spider.py @@ -0,0 +1,22 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING + +from .conftest import get_scrapy_source_files + +if TYPE_CHECKING: + from ..conftest import MakeActorFunction, RunActorFunction + + +async def test_redirect_spider(make_actor: MakeActorFunction, run_actor: RunActorFunction) -> None: + """A redirect is followed instead of being deduplicated against the request it was redirected from.""" + actor = await make_actor( + label='scrapy-redirect', + source_files=get_scrapy_source_files('spider_redirect.py', 'RedirectSpider'), + additional_requirements=['scrapy>=2.14.0'], + ) + run_result = await run_actor(actor) + assert run_result.status == 'SUCCEEDED' + + items = await actor.last_run().dataset().list_items() + assert items.items == [{'url': 'http://localhost:8080/redirect-target', 'title': 'Redirect Target'}] From ee4eabc8ff0a0c77a071fed0b99f225a28c1b099 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 13:07:44 +0200 Subject: [PATCH 13/17] test(scrapy): replace the dead start_requests overrides with start in the E2E spiders --- tests/e2e/test_scrapy/actor_source/spider_basic.py | 4 ++-- tests/e2e/test_scrapy/actor_source/spider_cb_kwargs.py | 4 ++-- tests/e2e/test_scrapy/actor_source/spider_custom_pipeline.py | 4 ++-- tests/e2e/test_scrapy/actor_source/spider_itemloader.py | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/e2e/test_scrapy/actor_source/spider_basic.py b/tests/e2e/test_scrapy/actor_source/spider_basic.py index 546a0c34..20430151 100644 --- a/tests/e2e/test_scrapy/actor_source/spider_basic.py +++ b/tests/e2e/test_scrapy/actor_source/spider_basic.py @@ -5,7 +5,7 @@ from scrapy import Request, Spider if TYPE_CHECKING: - from collections.abc import Generator + from collections.abc import AsyncIterator, Generator from scrapy.http.response import Response @@ -17,7 +17,7 @@ def __init__(self, start_urls: list[str], *args: Any, **kwargs: Any) -> None: super().__init__(*args, **kwargs) self.start_urls = start_urls - def start_requests(self) -> Generator[Request, None, None]: + async def start(self) -> AsyncIterator[Request]: for url in self.start_urls: yield Request(url, callback=self.parse) diff --git a/tests/e2e/test_scrapy/actor_source/spider_cb_kwargs.py b/tests/e2e/test_scrapy/actor_source/spider_cb_kwargs.py index c62b105c..c0865b6d 100644 --- a/tests/e2e/test_scrapy/actor_source/spider_cb_kwargs.py +++ b/tests/e2e/test_scrapy/actor_source/spider_cb_kwargs.py @@ -5,7 +5,7 @@ from scrapy import Request, Spider if TYPE_CHECKING: - from collections.abc import Generator + from collections.abc import AsyncIterator, Generator from scrapy.http.response import Response @@ -17,7 +17,7 @@ def __init__(self, start_urls: list[str], *args: Any, **kwargs: Any) -> None: super().__init__(*args, **kwargs) self.start_urls = start_urls - def start_requests(self) -> Generator[Request, None, None]: + async def start(self) -> AsyncIterator[Request]: for url in self.start_urls: yield Request(url, callback=self.parse) diff --git a/tests/e2e/test_scrapy/actor_source/spider_custom_pipeline.py b/tests/e2e/test_scrapy/actor_source/spider_custom_pipeline.py index e16b3f54..9903bdd1 100644 --- a/tests/e2e/test_scrapy/actor_source/spider_custom_pipeline.py +++ b/tests/e2e/test_scrapy/actor_source/spider_custom_pipeline.py @@ -5,7 +5,7 @@ from scrapy import Request, Spider if TYPE_CHECKING: - from collections.abc import Generator + from collections.abc import AsyncIterator, Generator from scrapy.http.response import Response @@ -17,7 +17,7 @@ def __init__(self, start_urls: list[str], *args: Any, **kwargs: Any) -> None: super().__init__(*args, **kwargs) self.start_urls = start_urls - def start_requests(self) -> Generator[Request, None, None]: + async def start(self) -> AsyncIterator[Request]: for url in self.start_urls: yield Request(url, callback=self.parse) diff --git a/tests/e2e/test_scrapy/actor_source/spider_itemloader.py b/tests/e2e/test_scrapy/actor_source/spider_itemloader.py index aeae3090..fc79c588 100644 --- a/tests/e2e/test_scrapy/actor_source/spider_itemloader.py +++ b/tests/e2e/test_scrapy/actor_source/spider_itemloader.py @@ -9,7 +9,7 @@ from src.items import ProductItem # ty: ignore[unresolved-import] if TYPE_CHECKING: - from collections.abc import Generator + from collections.abc import AsyncIterator, Generator from scrapy.http.response import Response @@ -29,7 +29,7 @@ def __init__(self, start_urls: list[str], *args: Any, **kwargs: Any) -> None: super().__init__(*args, **kwargs) self.start_urls = start_urls - def start_requests(self) -> Generator[Request, None, None]: + async def start(self) -> AsyncIterator[Request]: for url in self.start_urls: yield Request(url, callback=self.parse) From 62fbe9fdb672f12ff858c09131d79f31f245b020 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 13:13:17 +0200 Subject: [PATCH 14/17] test(scrapy): drop the deprecated spider argument from the E2E downloader middleware --- tests/e2e/test_scrapy/actor_source/spider_in_flight.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/test_scrapy/actor_source/spider_in_flight.py b/tests/e2e/test_scrapy/actor_source/spider_in_flight.py index a3843c42..d1b1b3e4 100644 --- a/tests/e2e/test_scrapy/actor_source/spider_in_flight.py +++ b/tests/e2e/test_scrapy/actor_source/spider_in_flight.py @@ -20,7 +20,7 @@ class MarkInFlightMiddleware: """Downloader middleware that logs `IN_FLIGHT_LOG_MARKER` for every request it sees.""" - def process_request(self, request: Request, spider: Spider) -> None: # noqa: ARG002 + def process_request(self, request: Request) -> None: logger.info(f'{IN_FLIGHT_LOG_MARKER}: {request.url}') From 4a61562a0b95dea2dd15873e5d4dcaac6ac2dec5 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 13:51:38 +0200 Subject: [PATCH 15/17] refactor(scrapy): replace casts in the request conversion with a checked HTTP method narrowing --- src/apify/scrapy/requests.py | 21 ++++++++++++++++--- .../scrapy/requests/test_to_apify_request.py | 12 +++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/apify/scrapy/requests.py b/src/apify/scrapy/requests.py index 1185414b..ebbcae19 100644 --- a/src/apify/scrapy/requests.py +++ b/src/apify/scrapy/requests.py @@ -2,7 +2,7 @@ from copy import deepcopy from logging import getLogger -from typing import Any, cast +from typing import TYPE_CHECKING, Any, cast, get_args from scrapy import Request as ScrapyRequest from scrapy import Spider @@ -17,6 +17,9 @@ from ._serialization import decode_from_json, encode_to_json from apify import Request as ApifyRequest +if TYPE_CHECKING: + from typing_extensions import TypeIs + logger = getLogger(__name__) @@ -48,6 +51,11 @@ def _ensure_known_request_class(request_dict: dict[str, Any]) -> None: ) +def _is_http_method(method: str) -> TypeIs[HttpMethod]: + """Tell whether `method` is one of the HTTP methods the RQ accepts; Scrapy itself takes any string.""" + return method in get_args(HttpMethod) + + def _compute_fingerprint(scrapy_request: ScrapyRequest) -> str: """Identify the request an RQ unique key was minted for. @@ -59,9 +67,16 @@ def _compute_fingerprint(scrapy_request: ScrapyRequest) -> str: `UserAgentMiddleware` call `headers.setdefault()` on the request in place before the scheduler sees it again, so an otherwise untouched request would no longer match its own stamp. """ + method = scrapy_request.method + if not _is_http_method(method): + raise ValueError( + f'Unsupported HTTP method {method!r} in {scrapy_request}; ' + f'the request queue accepts {", ".join(get_args(HttpMethod))}.' + ) + return compute_unique_key( url=scrapy_request.url, - method=cast('HttpMethod', scrapy_request.method), + method=method, payload=scrapy_request.body, keep_url_fragment=False, use_extended_unique_key=True, @@ -192,7 +207,7 @@ def to_scrapy_request(apify_request: ApifyRequest, spider: Spider) -> ScrapyRequ Returns: The converted Scrapy request. """ - if not isinstance(cast('Any', apify_request), ApifyRequest): + if not isinstance(apify_request, ApifyRequest): raise TypeError('apify_request must be an apify.Request instance') # If the apify_request comes from the Scrapy diff --git a/tests/unit/scrapy/requests/test_to_apify_request.py b/tests/unit/scrapy/requests/test_to_apify_request.py index 37a310bc..9f218db4 100644 --- a/tests/unit/scrapy/requests/test_to_apify_request.py +++ b/tests/unit/scrapy/requests/test_to_apify_request.py @@ -114,6 +114,18 @@ def test_non_json_serializable_meta_is_skipped(spider: Spider, caplog: pytest.Lo assert any('JSON-serializable' in record.getMessage() for record in caplog.records) +def test_unsupported_http_method_is_skipped(spider: Spider, caplog: pytest.LogCaptureFixture) -> None: + """A request with an HTTP method the request queue does not accept is skipped (returns None) and logged.""" + stamped_request = to_scrapy_request(ApifyRequest.from_url('https://example.com'), spider) + scrapy_request = stamped_request.replace(method='PROPFIND') + + with caplog.at_level(logging.ERROR, logger='apify.scrapy.requests'): + apify_request = to_apify_request(scrapy_request, spider) + + assert apify_request is None + assert 'Unsupported HTTP method' in caplog.text + + def test_roundtrip_follow_up_request_with_propagated_userdata(spider: Spider) -> None: """Regression: propagating userData across repeated roundtrips must not fail on `__crawlee` data.""" # Step 1: Initial request -> first roundtrip From bd76a7ad35fa352f7f62794379fcf7fa9d2f7e69 Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 13:55:42 +0200 Subject: [PATCH 16/17] refactor(scrapy): validate Scrapy headers through HttpHeaders instead of a cast --- src/apify/scrapy/requests.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/apify/scrapy/requests.py b/src/apify/scrapy/requests.py index ebbcae19..66894227 100644 --- a/src/apify/scrapy/requests.py +++ b/src/apify/scrapy/requests.py @@ -2,7 +2,7 @@ from copy import deepcopy from logging import getLogger -from typing import TYPE_CHECKING, Any, cast, get_args +from typing import TYPE_CHECKING, Any, get_args from scrapy import Request as ScrapyRequest from scrapy import Spider @@ -155,8 +155,9 @@ def to_apify_request(scrapy_request: ScrapyRequest, spider: Spider) -> ApifyRequ # the old behavior, which dropped such requests entirely. if isinstance(scrapy_request.headers, Headers): try: - headers = cast('dict[str, str]', dict(scrapy_request.headers.to_unicode_dict())) - request_kwargs['headers'] = HttpHeaders(headers) + # `to_unicode_dict()` yields str keys and values, but Scrapy types it as `UserDict[str | bytes, Any]`, + # so the mapping goes through the model's validator instead of a cast. + request_kwargs['headers'] = HttpHeaders.model_validate(scrapy_request.headers.to_unicode_dict()) except UnicodeDecodeError: logger.warning( 'Could not represent Scrapy request headers as Apify request headers (non-UTF-8 values); ' From fb6a4ad35a2b4eebe077212b14f66120b719274d Mon Sep 17 00:00:00 2001 From: Vlada Dusek Date: Tue, 25 Aug 2026 14:14:03 +0200 Subject: [PATCH 17/17] docs(scrapy): describe when the close() reclaim branch runs and generalize the prune comment --- src/apify/scrapy/_async_thread.py | 4 ++-- src/apify/scrapy/scheduler.py | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/apify/scrapy/_async_thread.py b/src/apify/scrapy/_async_thread.py index b9c0bb31..ead5a9d0 100644 --- a/src/apify/scrapy/_async_thread.py +++ b/src/apify/scrapy/_async_thread.py @@ -99,8 +99,8 @@ def submit_coro(self, coro: Coroutine) -> futures.Future: if self._eventloop.is_closed(): raise RuntimeError(f'The coroutine {coro} cannot be executed because the event loop is closed.') - # `wait_for_submitted` only runs once Scrapy goes idle, so without pruning the list would hold every - # coroutine the crawl ever submitted, with its result. + # Callers may go a long time between `wait_for_submitted` calls, so without pruning the list would hold + # every coroutine ever submitted, with its result. if len(self._submitted) >= SUBMITTED_PRUNE_THRESHOLD: self._submitted = [submitted for submitted in self._submitted if not submitted.done()] diff --git a/src/apify/scrapy/scheduler.py b/src/apify/scrapy/scheduler.py index 010a2816..0c4fb396 100644 --- a/src/apify/scrapy/scheduler.py +++ b/src/apify/scrapy/scheduler.py @@ -139,8 +139,9 @@ def close(self, reason: str) -> None: except Exception: logger.exception('Failed to resolve the requests still in flight in the request queue.') - # Whatever Scrapy did not finish goes back to the RQ as pending, in a single round trip: a migration - # cuts the shutdown short. One failed reclaim must not strand the rest either. + # Scrapy drains its in-progress requests before closing the scheduler, so this only runs if a future + # Scrapy closes it earlier; then they go back to the RQ as pending, in a single round trip. One failed + # reclaim must not strand the rest either. if self._requests_in_flight: reclaims = _gather_failures( rq.reclaim_request(apify_request) for apify_request, _ in self._requests_in_flight