fix: validate HTTP status when fetching sitemaps

[anxkhn]

Description

• _fetch_and_process_sitemap never looked at response.status_code. It opened the stream, picked a parser from the content type, parsed whatever body came back, and then broke out of the retry loop. A 503 with an empty body was therefore accepted as a valid, empty sitemap.
• The practical effect is that a transient outage on a sitemap endpoint silently produces zero URLs. SitemapRequestLoader then marks the sitemap processed and reports itself finished, so the crawl looks successful while nothing was crawled. Per RFC 9110 section 15.6, a 5xx response means the server failed to fulfil the request, not that a representation was returned.
• This change validates the status before a parser is selected. 408, 429 and 5xx go through the retry loop that already exists in this function, and other non-success statuses are raised immediately without parsing the body.
• SitemapRequestLoader.is_finished now retrieves the result of the background loading task, so a load that failed is surfaced to the caller instead of being swallowed as a clean completion. Cancellation is still treated as before.

Behaviour change worth flagging: fetch failures already aborted parse_sitemap (a connection error after the retries are exhausted propagates today), and terminal statuses now join that class. That means a stale 404 entry inside a sitemap index will fail the load rather than being parsed as an empty sitemap. If you would prefer terminal statuses to be logged and skipped instead, I am happy to change it.

Issues

• No related issue filed. Happy to open one if you prefer to track it there.

Testing

• tests/unit/_utils/test_sitemap.py: two 503 responses followed by a valid sitemap are retried and the URLs are returned; a persistent 503 raises once the retries are exhausted; a 404 raises immediately without a retry and without parsing the body it carried.
• tests/unit/request_loaders/test_sitemap_request_loader.py: the loader recovers from transient 503s and loads the URLs, and it surfaces the failure instead of finishing empty when the retries are exhausted.
• The existing stream mocks in these files needed a status_code on the mocked response, since the code now reads it.
• Ran uv run pytest tests/unit/_utils/test_sitemap.py tests/unit/request_loaders/test_sitemap_request_loader.py.

Checklist

• CI passed

[Mantisus]

That means a stale 404 entry inside a sitemap index will fail the load rather than being parsed as an empty sitemap. If you would prefer terminal statuses to be logged and skipped instead, I am happy to change it.

Yes. I suggest:

• Server errors trigger a retry. Once all retries are exhausted, log a warning instead of raising.
• The same applies to 429 and 408.
• Other client errors skip the sitemap, treating it as empty.

Also, updating is_finished interrupts the loader's operation, preventing it from handing out URLs that have already been loaded. Checking url_queue and in_progress before retrieving the task result would be enough.

[Copilot]

Pull request overview

Validates sitemap HTTP responses before parsing and propagates background loader failures, preventing failed sitemap fetches from appearing as successful empty loads.

Changes:

• Retry 408, 429, and 5xx sitemap responses; immediately reject other error statuses.
• Surface completed background loading task exceptions from SitemapRequestLoader.is_finished.
• Add coverage for transient, persistent, and terminal HTTP failures.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
src/crawlee/_utils/sitemap.py	Adds HTTP status validation and retry classification.
src/crawlee/request_loaders/_sitemap_request_loader.py	Propagates background loading failures.
tests/unit/_utils/test_sitemap.py	Tests status retry and rejection behavior.
tests/unit/request_loaders/test_sitemap_request_loader.py	Tests loader recovery and failure propagation.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[anxkhn]

@Mantisus @vdusek addressed the review feedback: retryable failures are contained per sitemap, exhausted and non-retryable responses are skipped with warnings, successful sources continue draining, and partial-failure coverage was added with the requested cleanup. rebased onto the latest master; 119 focused tests and ruff pass. could you please take another look?

[vdusek]

Important: the guard only sees this loader's own buffer, so it does not protect the requests the crawl actually owns. RequestManagerTandem consults the loader first and outside any try - in is_finished (_request_manager_tandem.py:49) and again in fetch_next_request (_request_manager_tandem.py:77) - so once this raises, _read_write_manager is never reached.

With one unreachable sitemap plus two requests seeded via crawler.run([...]), master returns normally and crawls both seeded requests, while this branch raises ConnectionError out of crawler.run() and handles nothing. Same for requests enqueued mid-crawl through context.add_requests().

Either drop the raise and surface the sitemap failure some other way, or have the tandem consult _read_write_manager first so its pending work short-circuits the loader.

[vdusek]

Important: on the other timing path the same failure is swallowed entirely, so the scenario this PR opens by describing is still a clean empty success. When the loading task fails with no worker parked in the poll loop (unreachable sitemap, no other work), the raise leaves via AutoscaledPool._worker_task_orchestrator, whose finally completes run.result anyway - so crawler.run() returns finished=0 failed=0 and only logs, exactly as on master.

So the same failure either aborts the crawl with data loss or reports success, decided purely by timing. Whichever way this is resolved, it needs a BasicCrawler + to_tandem() test - the current loader-in-isolation test cannot see either path.

[vdusek]

Important: successful_sources counts "did not raise", so a skipped sibling silences a real failure. Terminal 4xx and retry-exhausted statuses break out of _fetch_and_process_sitemap rather than raising, and raw sources can never fail at all, so all of them count as successes here:

Sitemap.load(['broken.xml'])                -> RAISED ConnectionError
Sitemap.load(['broken.xml', 'missing.xml']) -> NO RAISE, urls=[]    # missing.xml = 404

The same unreachable sitemap raises or not depending on its siblings, so callers cannot program against it. Sitemap.try_common_names hits this directly. Counting only a real 2xx parse as a success - letting the skip path be neither a success nor a source_error - keeps a lone 404 quiet while 404 + ConnectionError still raises.

[vdusek]

Nit: this discards every other failure and flips which source raises - before this PR the first failing source propagated.

Suggested change

	raise source_errors[-1]
	raise source_errors[0]

[vdusek]

Note: two things worth pinning down here. sitemap.py does a plain import asyncio with no module-local alias (crawlee._utils.sitemap.asyncio is asyncio is True), so patching crawlee._utils.sitemap.asyncio.sleep replaces asyncio.sleep process-wide for the test - it also collapses fetch_next_request's 0.1s poll and poll_until_condition's interval into busy-spins.

And this only terminates because it calls the import-time alias asyncio_sleep; a later tidy-up to the obvious asyncio.sleep(0) would recurse forever. Giving the retry pause a named constant in sitemap.py and patching that instead would scope the patch to exactly the delay under test and remove the trap.

[vdusek]

Note: this asserts pytest.raises around a bare is_finished() and reaches into the private loader._loading_task, so it only pins the loader in isolation - where the behaviour is as designed. The failures show up one layer out, in RequestManagerTandem and AutoscaledPool, which is why neither is visible to the suite today.

[vdusek]

Note: the retained first sentence is now false rather than merely incomplete - "return None if and only if is_finished would return True" no longer holds, because there is a third state where is_finished neither returns True nor False. The appended sentence papers over the contradiction instead of resolving it. SitemapRequestLoader.is_finished also still documents itself as just "Check if all URLs have been processed.", and the while not await loader.is_finished(): pattern in docs/guides/request_loaders.mdx can now throw from the loop condition.

[vdusek]

Note: retryability is settled, so this is just the cost - all three clients follow redirects by default and raise TooManyRedirects past the limit, so a 3xx reaching here is a 300, a 304, or a redirect with no Location, none of which a retry fixes. That is 2 wasted requests and 2s per such sitemap. Separately, discover_valid_sitemaps still gates on is_status_code_successful, which is 2xx or 3xx, so discovery can hand this function a URL it will retry three times and then skip.

[vdusek]

Nit: this runtime import sits below the TYPE_CHECKING block instead of with the other imports at the top of the file.

[vdusek]

Nit: there are now four near-identical mock HTTP client factories - this one, _make_flaky_stream_client and _make_stream_client in test_sitemap.py, and the inline one in test_sitemap_request_loader.py. The PR added status_code = 200 to each instead of folding them into this shared helper.

[vdusek]

Note: _raise_for_sitemap_status raises an exception that only its own function catches, 85 lines later, in a handler that then has to re-discriminate it from genuine transport errors via isinstance - twice. The status decision needs no exception at all; it is a local branch inside the stream block. That round trip is also why three of this handler's four outcomes return normally, leaving the caller unable to tell "parsed 0 URLs" from "skipped" - which is the mechanical root cause of the successful_sources accounting further down.

[vdusek]

Nit: "4xx is terminal, everything else retries" is a policy hardcoded where ParseSitemapOptions exists for exactly this - it already carries sitemap_retries, max_depth, timeout and enqueue_strategy, and BasicCrawler already has the vocabulary (additional_http_error_status_codes / ignore_http_error_status_codes). ParseSitemapOptions is total=False, so this is not lock-in and a key can be added later without breaking anyone - worth a deliberate "not now" rather than arriving there by omission.

[vdusek]

Note: 429 now reaches this flat 1s sleep. It is in Session._DEFAULT_BLOCKED_STATUS_CODES (_session.py:33) and the crawler path routes it through parse_retry_after_header plus session rotation, but sitemap.py takes no session at all - so a rate-limiting sitemap endpoint now gets 3 hits at 1s intervals from an unrotated identity, where master sent one. Either honour Retry-After here, or leave 429 out of the retryable set on this session-less path.

[vdusek]

Question: this reinstates the silent-empty path that #1943 (76927d74, "retry sitemap fetching on error and raise when retries are exhausted") explicitly removed - its own summary was that a sitemap which keeps failing after all retries now raises to the caller instead of silently producing empty results. That is a partial revert of a shipped fix, and the PR body does not mention it. Worth settling fail-vs-skip once and stating the decision, especially since crawlee JS went the other way at all three layers and stayed there: sitemap.ts:325-345 warns and gives up without throwing, Sitemap.parse (:485-494) catches everything and returns new Sitemap([]), and sitemap_request_list.ts:412-414 catches per sitemap URL and continues, keeping isFinished() a pure predicate. As written this is a fourth semantic, matching neither #1943 nor JS.