fix: complete SessionError lifecycle and honor reclaim forefront - #2105
fix: complete SessionError lifecycle and honor reclaim forefront#2105Ayush7614 wants to merge 1 commit into
Conversation
Honor error_handler replacement requests for SessionError, retire blocked sessions when rotations are exhausted, propagate AdaptivePlaywright static SessionError for rotation instead of browser fallback, and reclaim retries with request.forefront for tiered-proxy priority.
There was a problem hiding this comment.
🟡 Not ready to approve
The SessionError path currently leaves requests in an inconsistent lifecycle state and misses retry/error tracking in one replacement branch, which can lead to incorrect persisted request metadata and statistics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR completes the SessionError lifecycle in BasicCrawler/AdaptivePlaywrightCrawler and ensures retry reclaiming respects request queue priority (forefront) so tiered-proxy retries stay at the front of the queue.
Changes:
- Honor
error_handlerreturn values forSessionError, wrap handler exceptions consistently, and retire sessions when rotations are exhausted. - Re-raise
SessionErrorfrom the adaptive crawler’s static path to trigger session rotation instead of falling through to the browser. - Pass
forefront=request.forefrontintoreclaim_requestso priority retries preserve queue ordering.
File summaries
| File | Description |
|---|---|
src/crawlee/crawlers/_basic/_basic_crawler.py |
Updates retry reclaim behavior to honor forefront, and refines SessionError handling (rotation/retire + error_handler honoring). |
src/crawlee/crawlers/_adaptive_playwright/_adaptive_playwright_crawler.py |
Ensures static-path SessionError propagates to enable session rotation rather than browser fallback with the same session. |
tests/unit/crawlers/_basic/test_basic_crawler.py |
Adds unit tests covering SessionError error_handler replacement, session retirement on exhausted rotations, and forefront reclaim behavior. |
tests/unit/crawlers/_adaptive_playwright/test_adaptive_playwright_crawler.py |
Adds a unit test asserting static SessionError propagation triggers session rotation and prevents browser fallback. |
Review details
Suppressed comments (1)
src/crawlee/crawlers/_basic/_basic_crawler.py:1504
- When session rotations are exhausted, the request is marked as handled without setting its final state to ERROR. This leaves failed requests in REQUEST_HANDLER state in storage, which diverges from the normal error path (where the request is set to RequestState.ERROR before marking handled).
else:
# Exhausted rotations: retire the blocked session so it is not reused from the pool.
session.retire()
await self._mark_request_as_handled(request)
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if not session: | ||
| raise RuntimeError('SessionError raised in a crawling context without a session') from session_error | ||
|
|
||
| new_request = None |
| if new_request is not None and new_request != request: | ||
| await request_manager.add_request(new_request) | ||
| await self._mark_request_as_handled(request) | ||
| session.retire() | ||
| return |
Mantisus
left a comment
There was a problem hiding this comment.
Hey, @Ayush7614. Thank you for your contribution!
| if isinstance(static_run.exception, SessionError): | ||
| raise static_run.exception |
There was a problem hiding this comment.
The old behavior is correct here. A site can block the HTTP client even when the same session still works in a browser, for example, by requiring JavaScript execution before granting access.
| session.retire() | ||
| return |
There was a problem hiding this comment.
Replacement with new_request should only apply in the retry branch. Please drop the early return here because it also triggers when session rotations are exhausted, so the request gets replaced instead of failed and failed_request_handler never runs.
| # Exhausted rotations: retire the blocked session so it is not reused from the pool. | ||
| session.retire() |
There was a problem hiding this comment.
Please keep session.retire() only in the _should_retry_request branch. Otherwise, it breaks the long-lived session use case when max_session_rotations=0.
| async def test_reclaim_uses_request_forefront_flag() -> None: | ||
| """Retries must reclaim with `request.forefront` so tiered-proxy priority retries stay at the front.""" | ||
| queue = await RequestQueue.open() | ||
| reclaim_calls: list[bool] = [] | ||
| original_reclaim = queue.reclaim_request | ||
|
|
||
| async def tracking_reclaim(request: Request, *, forefront: bool = False) -> Any: | ||
| reclaim_calls.append(forefront) | ||
| return await original_reclaim(request, forefront=forefront) | ||
|
|
||
| queue.reclaim_request = tracking_reclaim # type: ignore[method-assign] | ||
|
|
||
| crawler = BasicCrawler(request_manager=queue, max_request_retries=1) | ||
|
|
||
| @crawler.router.default_handler | ||
| async def handler(context: BasicCrawlingContext) -> None: | ||
| context.request.forefront = True | ||
| raise RuntimeError('retry me') | ||
|
|
||
| await crawler.run([Request.from_url('https://a.placeholder.com')]) | ||
|
|
||
| assert reclaim_calls == [True] | ||
|
|
||
| await queue.drop() |
There was a problem hiding this comment.
Let's use the mock
| async def test_reclaim_uses_request_forefront_flag() -> None: | |
| """Retries must reclaim with `request.forefront` so tiered-proxy priority retries stay at the front.""" | |
| queue = await RequestQueue.open() | |
| reclaim_calls: list[bool] = [] | |
| original_reclaim = queue.reclaim_request | |
| async def tracking_reclaim(request: Request, *, forefront: bool = False) -> Any: | |
| reclaim_calls.append(forefront) | |
| return await original_reclaim(request, forefront=forefront) | |
| queue.reclaim_request = tracking_reclaim # type: ignore[method-assign] | |
| crawler = BasicCrawler(request_manager=queue, max_request_retries=1) | |
| @crawler.router.default_handler | |
| async def handler(context: BasicCrawlingContext) -> None: | |
| context.request.forefront = True | |
| raise RuntimeError('retry me') | |
| await crawler.run([Request.from_url('https://a.placeholder.com')]) | |
| assert reclaim_calls == [True] | |
| await queue.drop() | |
| async def test_reclaim_uses_request_forefront_flag() -> None: | |
| """Retries must reclaim with `request.forefront` so tiered-proxy priority retries stay at the front.""" | |
| queue = await RequestQueue.open() | |
| crawler = BasicCrawler(request_manager=queue, max_request_retries=1) | |
| @crawler.router.default_handler | |
| async def handler(context: BasicCrawlingContext) -> None: | |
| context.request.forefront = True | |
| raise RuntimeError('Arbitrary crash for testing purposes') | |
| with patch.object(queue, 'reclaim_request', wraps=queue.reclaim_request) as reclaim_mock: | |
| await crawler.run(['https://a.placeholder.com']) | |
| reclaim_mock.assert_awaited_once() | |
| (reclaimed_request,), reclaim_kwargs = reclaim_mock.await_args_list[0] | |
| assert reclaimed_request.url == 'https://a.placeholder.com' | |
| assert reclaim_kwargs == {'forefront': True} | |
| await queue.drop() |
Summary
SessionErrorpath: honorerror_handlerreplacement requests (same as regular errors), wrap handler exceptions inUserDefinedErrorHandlerError, andretire()the blocked session when rotations are exhausted.SessionErrorfrom the static sub-crawler instead of falling through to the browser with the same blocked session.reclaim_requestnow passesforefront=request.forefrontso tiered-proxy priority retries actually stay at the front of the queue.Why
error_handlercould replace a request on normal failures but its return value was discarded forSessionError. Exhausted rotations left blocked sessions usable in the pool. Adaptive staticSessionErrorwas only logged, so a 403/blocked session could be reused for the browser fallback. Tiered proxies setrequest.forefront = Trueon retry, but reclaim always used the defaultforefront=False.Test plan
test_session_error_handler_can_replace_requesttest_session_retired_when_rotations_exhaustedtest_reclaim_uses_request_forefront_flagtest_static_session_error_propagates_for_rotation