Skip to content

fix(docker): preserve single-url crawl failure details - #2117

Merged
ntohidi merged 2 commits into
unclecode:developfrom
nightcityblade:fix/issue-2116
Aug 12, 2026
Merged

fix(docker): preserve single-url crawl failure details#2117
ntohidi merged 2 commits into
unclecode:developfrom
nightcityblade:fix/issue-2116

Conversation

@nightcityblade

@nightcityblade nightcityblade commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #2116

Treat unsuccessful crawls from the single-URL /md and /llm/{url} handlers as upstream failures (502 Bad Gateway) so the existing exception handler preserves error_message instead of replacing it with an opaque internal-server-error response. The LLM handler now also preserves deliberate HTTPException statuses. This changes those two failure responses from HTTP 500 to HTTP 502.

List of files changed and why

  • deploy/docker/api.py - centralize unsuccessful crawl handling and return a detail-preserving 502 from both single-URL handlers.
  • deploy/docker/tests/test_api_crawl_failures.py - verify the helper mapping and exercise authenticated /md and /llm/{url} requests through the real FastAPI app, with a mocked failing crawler.

How Has This Been Tested?

  • /tmp/crawl4ai-followup-venv/bin/python -m pytest deploy/docker/tests/test_api_crawl_failures.py deploy/docker/tests/test_security_headers_xss.py::TestErrorSanitization -q — 5 passed, 4 existing warnings.
  • /tmp/crawl4ai-followup-venv/bin/ruff check --isolated --select E9,F63,F7,F82 deploy/docker/api.py deploy/docker/tests/test_api_crawl_failures.py — passed.
  • /tmp/crawl4ai-followup-venv/bin/python -m compileall -q deploy/docker/api.py deploy/docker/tests/test_api_crawl_failures.py — passed.
  • git diff --check — passed.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas (N/A: no complex logic was introduced)
  • I have made corresponding changes to the documentation (N/A: the behavioral status change is called out in this PR description)
  • I have added/updated unit tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@nightcityblade

Copy link
Copy Markdown
Contributor Author

CI note: the code-related posture-gate and security-offline checks both pass. The only failing job is the Discord notification workflow; its log shows an empty DISCORD_WEBHOOK and exits before exercising this change. This is repository/fork-secret configuration rather than a patch failure.

@bong-u

bong-u commented Aug 4, 2026

Copy link
Copy Markdown

Nice work.
I hit the same 500 from a client and found one more spot the issue didn't cover.

deploy/docker/server.py:911:

if all(not result["success"] for result in results["results"]):
    raise HTTPException(500, f"Crawl request failed: {results['results'][0]['error_message']}")

For a single-URL request "all failed" is just "it failed", so the detail gets logged and dropped by the same 500 branch. Stock 0.9.2:

POST /crawl {"urls":["https://www.sagaftra.org/"]}
→ HTTP 500
→ {"error":"Internal server error","correlation_id":"844da2acf429"}

That's the same body the issue reports for /md, from the endpoint it lists as unaffected.
server.py:911 is byte-identical on v0.9.2, main and this PR's head.

The detail is already interpolated here, so switching that 500 to 502 is enough to make it survive.
If you'd rather reuse _raise_for_crawl_failure, it won't drop in as-is: handle_crawl_request returns model_dump() dicts (api.py:751), so it's result["success"].

@nightcityblade

Copy link
Copy Markdown
Contributor Author

Thanks for the concrete reproduction. You're right: the all-results-failed branch in /crawl also raises 500, so the issue's statement that /crawl is unaffected is too broad. This PR is scoped to the two single-URL handlers from #2116, and I don't want to expand it without maintainer direction. I'm happy to include that adjacent branch here if the maintainers prefer, or it can be tracked separately.

@bong-u

bong-u commented Aug 5, 2026

Copy link
Copy Markdown

That makes sense.
My main goal was to make sure the /crawl case doesn't get overlooked.
I agree that maintainer preference should decide whether it belongs here or in a follow-up.
Thanks for confirming the behavior.

@ntohidi

ntohidi commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

@nightcityblade
The fix looks right. 502 works because the handler at server.py:509 only strips the body on 500s — anything else keeps its detail. And the except HTTPException: raise you added is needed, otherwise the except Exception below it turns your 502 back into a 500.

One problem with the tests. test_single_url_handlers_use_crawl_failure_mapping just checks that a line of text appears in the source code. That'll pass even if the endpoint is broken. So nothing here actually tests the bug — that a blocked page now comes back with its reason instead of an empty 500.

Take a look at test_crawl_failure_response.py in the same folder. It sends a real request and checks the response. Can you redo that test the same way? The other one's fine, leave it.

Also worth mentioning in the description that /md and /llm/{url} now return 502 instead of 500, since that affects anyone already calling them.

@nightcityblade

Copy link
Copy Markdown
Contributor Author

Thanks — addressed in 4fc16ca. I replaced the source-inspection assertion with authenticated requests through the real FastAPI app for both /md and /llm/{url}. Each test supplies a mocked failing crawler and verifies that the client receives HTTP 502 with the original anti-bot detail.

I also updated the PR description to call out the 500-to-502 behavior change explicitly. The focused endpoint and exception-handler suite passes (5 tests).

@ntohidi

ntohidi commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Tests look good now — thanks. Covering both endpoints and asserting the detail body is exactly right.

@bong-u on the /crawl branch you found: that's already fixed on develop. 130a382 removed those lines a few days ago. You were testing 0.9.2, where it's still there — it'll go away with the next release. Nothing to follow up on, and thanks for chasing it.

Merging this.

@ntohidi
ntohidi merged commit 3cc28d8 into unclecode:develop Aug 12, 2026
2 checks passed
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.

[Bug]: /md and /llm/{url} return HTTP 500 when anti-bot detection marks a result failed

3 participants