Skip to content

fix: validate HTTP status when fetching sitemaps - #2123

Open
anxkhn wants to merge 2 commits into
apify:masterfrom
anxkhn:fix/sitemap-http-status
Open

fix: validate HTTP status when fetching sitemaps#2123
anxkhn wants to merge 2 commits into
apify:masterfrom
anxkhn:fix/sitemap-http-status

Conversation

@anxkhn

@anxkhn anxkhn commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
@vdusek
vdusek requested a review from Mantisus August 6, 2026 08:18
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>

@Mantisus Mantisus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vdusek
vdusek requested a balanced review from Copilot August 7, 2026 10:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants