Skip to content

fix(mcp): fail fast on non-MCP HTTP content type instead of waiting 60s - #36064

Closed
uzunkuyruk wants to merge 4 commits into
NousResearch:mainfrom
uzunkuyruk:fix/mcp-html-response-fast-fail
Closed

fix(mcp): fail fast on non-MCP HTTP content type instead of waiting 60s#36064
uzunkuyruk wants to merge 4 commits into
NousResearch:mainfrom
uzunkuyruk:fix/mcp-html-response-fast-fail

Conversation

@uzunkuyruk

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a 60-second startup hang when a misconfigured HTTP MCP server URL returns text/html instead of a valid MCP response.

Before: The SDK logged the content type error immediately, but Hermes waited the full _DEFAULT_CONNECT_TIMEOUT (60s) before surfacing a CancelledError with no actionable context. With 3 retries + backoff, a single bad URL could block startup for the full timeout.

After: Fails in <1s with a clear, actionable message.

Three-part fix:

  • _MCPInvalidContentTypeError — new non-retryable exception class for non-MCP content type responses
  • _detect_non_mcp_content_type — httpx response event hook that raises immediately when a 2xx response has a non-MCP content type (application/json or text/event-stream are the only valid types), cancelling the transport before the timeout expires
  • Non-retryable guard — treats _MCPInvalidContentTypeError like an auth error, skipping the 3x retry + backoff loop entirely

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

References

Fixes #36052

Checklist

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • My PR contains only changes related to this fix

When a misconfigured HTTP MCP server URL returns text/html (e.g. pointing
at a web app root instead of an MCP endpoint), the SDK logs the content
type mismatch immediately but Hermes waited the full 60s connect_timeout
before surfacing a CancelledError with no actionable context.

Three-part fix:
- Add _MCPInvalidContentTypeError for non-MCP content type responses
- Add _detect_non_mcp_content_type httpx response event hook that raises
  immediately when a 2xx response has a non-MCP content type (not
  application/json or text/event-stream), cancelling the transport
  before the timeout expires
- Treat _MCPInvalidContentTypeError as non-retryable (like auth errors)
  so the 3x retry + backoff loop is skipped entirely

Result: a bad HTTP MCP URL fails in <1s with a clear message instead of
blocking startup for 60s and reporting only CancelledError.

Fixes NousResearch#36052
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/mcp MCP client and OAuth labels May 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing fix with #36058 — both address #36052 (MCP HTML content-type blocking discovery for 60s). This PR uses an httpx event hook approach; #36058 uses a HEAD preflight probe and includes tests. Maintainer should pick one approach.

@mxnstrexgl mxnstrexgl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — automated review passed. No security, quality, or test coverage issues detected.

Add 5 tests for the non-MCP content type detection introduced in the
companion fix:

- _MCPInvalidContentTypeError is instantiable with an actionable message
- text/html on a 200 response raises _MCPInvalidContentTypeError
- Valid MCP content types (application/json, text/event-stream) do not raise
- 4xx/5xx responses are exempt — HTTP error handling covers those
@uzunkuyruk

Copy link
Copy Markdown
Contributor Author

Added test coverage in the latest commit:

  • _MCPInvalidContentTypeError instantiation and message format
  • text/html on 200 raises the error
  • Valid MCP content types (application/json, text/event-stream) pass through
  • 4xx/5xx responses are exempt from content-type checking

@liuhao1024

Copy link
Copy Markdown
Contributor

The test file has class TestMCPInvalidContentTypeError defined twice (line 11 and line 98 of the diff), with identical method names. In Python, the second class definition shadows the first — the first class's 5 test methods become dead code.

Since both classes have the same 5 methods (test_invalid_content_type_error_is_non_retryable, test_invalid_content_type_error_message_is_actionable, test_html_response_raises_invalid_content_type_error, test_valid_mcp_content_types_do_not_raise, test_4xx_responses_do_not_raise_content_type_error), pytest still collects and runs 5 tests from the second class. So the coverage is there, but the first class is wasted code.

Fix: remove the first class definition (lines 11–95 of the new file) and keep only the second.

Additionally, the async tests (test_html_response_raises_invalid_content_type_error, test_valid_mcp_content_types_do_not_raise, test_4xx_responses_do_not_raise_content_type_error) use a simulate_hook helper that duplicates the production hook logic inline rather than testing the actual _detect_non_mcp_content_type hook. This means the tests validate the simulation, not the real code — if the production hook's logic diverges from the simulation, the tests still pass. Consider testing the actual hook via the httpx client's event_hooks mechanism instead.

@dsameer0-code

dsameer0-code commented Jun 1, 2026 via email

Copy link
Copy Markdown

…nition

The first class definition (lines 11-95 of the test diff) was dead code —
Python silently shadowed it with the second identical definition.
Remove the first occurrence so pytest runs exactly one class with 5 tests.

Reported by @liuhao1024 in code review.
Per @liuhao1024 review: the async tests were duplicating the production
hook logic via a simulate_hook helper instead of exercising the actual
httpx event_hooks mechanism.

Replace all three async tests with real httpx.AsyncBaseTransport mocks
that fire the hook via event_hooks={'response': [...]}, matching exactly
how the production code registers _detect_non_mcp_content_type.

This ensures the tests validate the real hook dispatch path, not a
hand-rolled simulation.
@uzunkuyruk

Copy link
Copy Markdown
Contributor Author

Second point also addressed in the latest commit replaced all three simulate_hook async tests with real httpx.AsyncBaseTransport mocks that fire the hook via the actual event_hooks={'response': [...]} mechanism, matching exactly how production code registers _detect_non_mcp_content_type.

@teknium1

teknium1 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Merged via #37133 (#37133) — same root cause as your PR, you and @liuhao1024 diagnosed it independently within ~30 min of each other.

Heads-up on the in-SDK event-hook approach for future reference: the hook raises inside the MCP SDK's anyio task group, so the error reaches the retry loop wrapped in an ExceptionGroup rather than as a bare _MCPInvalidContentTypeError — the isinstance guard never matched, so it still burned all 3 retries (verified live against mcp 1.26.0). The shipped fix uses a preflight probe outside the task group, which avoids the wrapping entirely. You're credited as a co-author on the merged commit. Thanks for the report and the work!

@teknium1 teknium1 closed this Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bad HTTP MCP endpoint returning HTML blocks discovery for 60s before CancelledError

6 participants