Fix timeout classification and add proxy failure-path tests - #63579
Closed
jswizzle3737 wants to merge 3 commits into
Closed
Fix timeout classification and add proxy failure-path tests#63579jswizzle3737 wants to merge 3 commits into
jswizzle3737 wants to merge 3 commits into
Conversation
- gateway/platforms/bluebubbles.py: _handle_webhook's password check and malformed-body handling had no failing-path coverage (fixture always supplied a correct secret). Add tests for wrong/missing password (401) and unparsable body (400). - hermes_cli/proxy/server.py: the proxy's upstream-unreachable (502) and upstream-timeout (504) error mapping in _open_upstream was never exercised. Add tests covering both via a refused connection and a patched client-timeout.
Mock the connection failure instead of using a real connection to an unused port (port 1 can be silently dropped rather than refused in some CI/container network setups, turning a 502 test into a spurious 504), and scope both mocked exceptions to the fake upstream URL only so the outer test client's own request to the local proxy runner is never accidentally intercepted.
aiohttp's own sock_connect/sock_read timeout errors (ServerTimeoutError and subclasses) inherit from both asyncio.TimeoutError and aiohttp.ClientError. _open_upstream checked ClientError first, so every real upstream timeout was caught there and reported as 502 upstream_unreachable instead of 504 upstream_timeout — the timeout branch was effectively dead code. Reorder the except clauses so the timeout check runs first, and update the 504 test to raise a real aiohttp.ServerTimeoutError instead of a bare asyncio.TimeoutError that doesn't occur in practice, so it actually exercises this path. Found by chatgpt-codex-connector's review of the earlier test-coverage PR.
alt-glitch
added
type/bug
Contributor
|
Thanks for the focused fix and deterministic failure-path coverage. Current main still catches This is an automated hermes-sweeper review. |
teknium1
added
sweeper:risk-compatibility
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes proxy error classification and adds deterministic failure-path tests.
aiohttp.ServerTimeoutErrorinherits from bothasyncio.TimeoutErrorandaiohttp.ClientError. The existing exception order catches it as a generic client error first, returning HTTP 502upstream_unreachableinstead of HTTP 504upstream_timeout.This change checks
asyncio.TimeoutErrorfirst and adds tests covering:upstream_unreachable; andupstream_timeout.The focused patch was reviewed in the fork and CodeRabbit reports success. The broader upstream suite should run in this repository's CI before merge.