fix: validate HTTP status when fetching sitemaps - #2123
Conversation
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Mantisus
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
_fetch_and_process_sitemapnever looked atresponse.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.SitemapRequestLoaderthen 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.SitemapRequestLoader.is_finishednow 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
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.status_codeon the mocked response, since the code now reads it.uv run pytest tests/unit/_utils/test_sitemap.py tests/unit/request_loaders/test_sitemap_request_loader.py.Checklist