feat: add retry logic for 5xx gateway errors in async HTTP client - #397
Conversation
📝 WalkthroughWalkthroughAdds retry handling to ChangesHTTP Retry Logic & Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds retry behavior to the async Hathor Core HTTP client to avoid noisy/log-spammy failures when upstream gateways return transient 5xx (502/503/504) HTML responses, and introduces unit tests covering the new retry scenarios.
Changes:
- Implemented bounded retry logic in
HathorCoreAsyncClient.get()for{502, 503, 504}with a fixed delay. - Ensured retryable 5xx responses consume the body and avoid JSON parsing exceptions, reducing duplicate logging.
- Added async unit tests validating success, retry-then-success, retry-exhausted, non-retryable 4xx, and exception paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| gateways/clients/hathor_core_client.py | Adds retry loop/constants to suppress repeated logs/JSON parsing failures on transient gateway 5xx responses. |
| tests/unit/gateways/clients/test_hathor_core_client.py | Adds async unit tests to validate retry behavior and logging expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/gateways/clients/test_hathor_core_client.py (1)
62-97: Optional: tighten return-value assertions.Two minor gaps in the new tests — behavior is still well-covered, but these would pin down the contract more precisely:
- Line 75: consider asserting the exact shape documented in the PR description, e.g.
assert result == {"error": "status 502"}, so a future change to the error dict format is caught here.- Lines 86-97 (
test_get_no_retry_on_4xx): the returned value is never asserted. Since the 4xx path still callsresponse.json(...)and returns its result (per the file under review, line 100), asserting the returned dict (or at least that it matches the mockedjson_data) would document that non-retryable 4xx responses still bypass the error-dict path.📝 Proposed assertions
- assert "error" in result + assert result == {"error": "status 502"}client = HathorCoreAsyncClient("http://test.node") with patch.object(client, "log") as mock_log: - await client.get("/v1a/missing") + result = await client.get("/v1a/missing") mock_sleep.assert_not_called() mock_log.warning.assert_called_once() + assert result is None # or whatever the 4xx contract is🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/gateways/clients/test_hathor_core_client.py` around lines 62 - 97, Update the two tests to tighten return-value assertions: in test_get_logs_warning_after_all_retries_exhausted, assert the exact returned error dict (e.g. assert result == {"error": "status 502"}) rather than only checking "error" in result; in test_get_no_retry_on_4xx, assert the value returned by calling HathorCoreAsyncClient.get (the mocked response.json payload) so the 4xx path that returns the response.json is validated (use the same json_data supplied to _make_response). Ensure you reference the existing test functions test_get_logs_warning_after_all_retries_exhausted and test_get_no_retry_on_4xx and the HathorCoreAsyncClient.get behavior when adding these assertions.gateways/clients/hathor_core_client.py (1)
75-103: Retry loop LGTM; a couple of observations on cadence & symmetry.Control flow is correct: up to 3 attempts, 2 sleeps on persistent failure, a single consolidated WARNING on exhaustion, and the body is drained each attempt as stated in the PR description. Two minor things to consider (neither blocking):
Polling cadence impact.
CollectNodesStatuses.collectiteratesHATHOR_NODESsequentially on a 1s cycle. WithMAX_RETRIES=2andRETRY_DELAY=1.0, a single persistently-failing node adds up to ~2s to each cycle (andNflaky nodes compound linearly). That may be acceptable here — it still reduces log spam from 3×N/cycle to 1×N/cycle — but worth confirming this is within the tolerance of the polling interval and any downstream alerting. Consider a shorterRETRY_DELAY(e.g. 0.2–0.5s) or a budget-based cap if cycle time matters.Retry symmetry with network exceptions. The
except Exceptionarm returns immediately, so transient connection errors (aiohttp.ClientConnectionError, timeouts, DNS blips) — which in practice are as recoverable as 502/503/504 — are not retried. If the goal is "reduce transient noise", retrying on a narrow set of network exceptions would be a natural extension. Explicitly out of scope for this PR per the description; just flagging for a possible follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateways/clients/hathor_core_client.py` around lines 75 - 103, The loop currently treats HTTP retryable statuses but returns immediately on any exception, which prevents retry symmetry; change the except block in the GET loop so it only treats network/timeout exceptions (e.g., aiohttp.ClientError, asyncio.TimeoutError) as retryable and follows the same MAX_RETRIES/RETRY_DELAY logic (sleep and continue while attempt < MAX_RETRIES, log a single warning on exhaustion using the same "hathor_core_error" semantics), and consider reducing the default RETRY_DELAY used by MAX_RETRIES (or make it configurable) so CollectNodesStatuses.collect (which iterates HATHOR_NODES) doesn't incur multi-second stalls per node.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gateways/clients/hathor_core_client.py`:
- Around line 75-103: The loop currently treats HTTP retryable statuses but
returns immediately on any exception, which prevents retry symmetry; change the
except block in the GET loop so it only treats network/timeout exceptions (e.g.,
aiohttp.ClientError, asyncio.TimeoutError) as retryable and follows the same
MAX_RETRIES/RETRY_DELAY logic (sleep and continue while attempt < MAX_RETRIES,
log a single warning on exhaustion using the same "hathor_core_error"
semantics), and consider reducing the default RETRY_DELAY used by MAX_RETRIES
(or make it configurable) so CollectNodesStatuses.collect (which iterates
HATHOR_NODES) doesn't incur multi-second stalls per node.
In `@tests/unit/gateways/clients/test_hathor_core_client.py`:
- Around line 62-97: Update the two tests to tighten return-value assertions: in
test_get_logs_warning_after_all_retries_exhausted, assert the exact returned
error dict (e.g. assert result == {"error": "status 502"}) rather than only
checking "error" in result; in test_get_no_retry_on_4xx, assert the value
returned by calling HathorCoreAsyncClient.get (the mocked response.json payload)
so the 4xx path that returns the response.json is validated (use the same
json_data supplied to _make_response). Ensure you reference the existing test
functions test_get_logs_warning_after_all_retries_exhausted and
test_get_no_retry_on_4xx and the HathorCoreAsyncClient.get behavior when adding
these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69fcaa89-8b69-47fc-a44a-d152f2cd3cca
📒 Files selected for processing (2)
gateways/clients/hathor_core_client.pytests/unit/gateways/clients/test_hathor_core_client.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateways/clients/hathor_core_client.py`:
- Around line 82-99: The current error branch returns a synthetic {"error":
"status X"} for any response.status > 299, dropping JSON error payloads; update
the logic in the error handling around RETRYABLE_STATUS_CODES / MAX_RETRIES so
that for non-retryable statuses you parse and return the original JSON body via
response.json(content_type=content_type) (and still log via self.log.warning
with path/status/body), while for retryable statuses (checked against
self.RETRYABLE_STATUS_CODES and attempt < self.MAX_RETRIES) you keep the
sleep/continue behavior and only synthesize the fallback {"error": f"status
{response.status}"} after retries are exhausted; adjust uses of response.text()
vs response.json(...) accordingly so callers receive server-provided JSON when
available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef8bda0d-4075-461c-9339-14c7e3414494
📒 Files selected for processing (2)
gateways/clients/hathor_core_client.pytests/unit/gateways/clients/test_hathor_core_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/gateways/clients/test_hathor_core_client.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateways/clients/hathor_core_client.py (1)
80-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
return {"error": "max retries exceeded"}at Line 102 is unreachable.The loop runs
attemptover0..MAX_RETRIES. On the final iterationattempt == MAX_RETRIES, soattempt < self.MAX_RETRIESisFalseand the retryable branch can'tcontinue; that iteration always falls through to thereturn self._decode_error_body(...)at Line 97 (and success returns at Line 83). Therefore theforloop never exits without returning, making Line 102 dead code. This also makes its"max retries exceeded"message misleading, since exhausted retries actually return the decoded upstream body, not this payload.The retry/decode logic itself is correct.
♻️ Remove the unreachable fallback
except Exception as e: self.log.error("hathor_core_error", path=path, error=repr(e)) return {"error": repr(e)} - - return {"error": "max retries exceeded"}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateways/clients/hathor_core_client.py` around lines 80 - 102, The fallback return at the end of the retry loop in hathor_core_client.py is unreachable because all paths in the session.get loop already return either on success, on a non-retryable failure, or on the final exhausted retry. Remove the dead `return {"error": "max retries exceeded"}` from the retry handling in the client method that wraps `session.get`, keeping the existing success, warning, and exception paths unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gateways/clients/hathor_core_client.py`:
- Around line 80-102: The fallback return at the end of the retry loop in
hathor_core_client.py is unreachable because all paths in the session.get loop
already return either on success, on a non-retryable failure, or on the final
exhausted retry. Remove the dead `return {"error": "max retries exceeded"}` from
the retry handling in the client method that wraps `session.get`, keeping the
existing success, warning, and exception paths unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e7187f8-657f-45b0-b67c-caa82e5ce0ef
📒 Files selected for processing (2)
gateways/clients/hathor_core_client.pytests/unit/gateways/clients/test_hathor_core_client.py
On transient fullnode unreachability (502/503/504), the daemon was
generating three log entries per node per 1-second polling cycle:
1. WARNING hathor_core_error (status=502, body=html)
2. ERROR hathor_core_error (ContentTypeError from response.json())
3. WARNING collect_status_error
Root cause: HathorCoreAsyncClient.get() immediately logged a warning on
any non-2xx, then called response.json() on an HTML body which raised a
ContentTypeError, triggering a second log in the except branch, which
in turn caused a third log in CollectNodesStatuses._send().
Fix: wrap the request in a retry loop (MAX_RETRIES=2, RETRY_DELAY=1s)
for RETRYABLE_STATUS_CODES={502,503,504}. Only log after all retries are
exhausted, and return {"error": "status 502"} directly instead of
letting the JSON parse exception propagate.
Result:
- A transient 502 that recovers within one retry window => zero logs
- A persistent 502 after all retries => exactly one WARNING
Tests: add TestHathorCoreAsyncClientGet covering success, 502->200
retry (no warning), exhausted retries (single warning), no-retry on
4xx, and no-retry on network exception.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
For error responses we now return the server-provided JSON body when the
body really is JSON, and otherwise return the status plus the raw body
(GCP load balancer 5xx can come back as plain text/HTML). This avoids
overriding error responses with a synthetic {"error": "status X"} and
keeps the original error details for callers.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The retry logic targets transient gateway errors (502/503/504). The fullnode health endpoint, however, returns 503 as a legitimate answer (the body still carries the 'status'), so retrying it only wastes the tight healthcheck timeout budget. Add a 'retry' flag to HathorCoreAsyncClient.get (default True) and disable it in the healthcheck gateway. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
475b645 to
b63e5a5
Compare
github-project-automation
Bot
moved this from In Review (WIP)
to Waiting to be deployed
in Hathor Network
Problem
When a fullnode is temporarily unreachable (e.g. nginx returns
502 Bad Gateway), the daemon was generating three log entries per node per 1-second polling cycle, triggering spurious alerts:Root cause
HathorCoreAsyncClient.get()had no retry mechanism, andresponse.json()was called unconditionally:WARNINGwith the raw body.response.json()was then called on a 502 response whose body is nginx HTML — this raised aContentTypeError, caught by theexceptbranch, which logged anERRORand returned{"error": repr(exc)}(the original body lost)._send()inCollectNodesStatusessaw the"error"key in the returned dict and logged a thirdWARNING.Solution
1. Retry transient gateway errors
HathorCoreAsyncClient.get()now retries retryable gateway errors (502,503,504):MAX_RETRIES = 2— up to 2 retries before giving upRETRY_DELAY = 1.0s— wait 1 second between attemptsRETRYABLE_STATUS_CODES = {502, 503, 504}— only gateway-level transient errors are retriedAll attempts share a single
aiohttp.ClientSession. On a retryable status the body is read (consuming the connection cleanly) and the loop either sleeps + retries or, once retries are exhausted, logs a singleWARNINGand returns the decoded body (see below). Non-retryable 4xx errors are never retried. Theget()docstring now also clarifies thattimeoutis per-attempt, so total elapsed time may exceed it when retries happen.2. Decode error bodies instead of unconditionally calling
response.json()For any error response (
status > 299),_decode_error_bodyreturns:main's behavior for JSON error bodies);{"error": "status X", "body": <raw body>}when the body is not JSON (e.g. GCP load balancer / nginx 5xx that come back as HTML/plain text).This is the key fix versus
main: a non-JSON error body no longer reachesresponse.json(), so it no longer raises aContentTypeError(eliminating the spuriousERRORlog) and the raw error text is preserved instead of being replaced byrepr(exc). Successful (<= 299) responses are unchanged: the JSON body is returned verbatim.3. Retry opt-out for the health endpoint
Because a
503from/v1a/healthis a legitimate answer (unhealthy fullnode) rather than a transient gateway failure, retrying it would only burn the tight healthcheck timeout budget (5s client / 6s lambda).get()takes aretryflag (defaultTrue) andHealthcheckGatewaypassesretry=False, so genuine gateway errors are still retried everywhere else.Outcome (per failing poll cycle, vs
main)mainhathor_core_errorWARNING +collect_status_errorWARNING; the spuriousERRORis goneWhy this fits the two current callers
This client is consumed in only two places, and neither relies on the HTTP status code to detect errors — which matches hathor-core's convention of frequently returning errors with HTTP 200 and signalling them in the body (many resources
return {'success': False, ...}without ever setting a response code). Error detection here is intentionally body-based, not status-based.CollectNodesStatuses(usecases/collect_nodes_statuses.py)/v1a/statusStatusResource.render_GETalways returns HTTP 200 with the status dump — no error branch, never sets a response code. The only failures observable here are transport-level (timeout / connection refused) →{"error": repr(e)}, plus upstream gateway 5xx from nginx/LB.if "error" in data, otherwise parses the body withNode.from_status_dict. Since the endpoint itself never returns an error status, it only reaches_decode_error_bodyfor upstream gateway errors (non-JSON →{"error": ..., "body": ...}, still caught by the"error"check).GetHealthcheck(usecases/get_healthcheck.pyviaHealthcheckGateway)/v1a/healthHealthcheckResourcereturns 200 when healthy and 503 when unhealthy, but the verdict is always in the body (status: pass/fail). It also acceptsstrict_status_code=1to force 200 even when failing.health_response["status"]for thepass/warn/failnuance, so it needs the 503 body returned as-is — which_decode_error_bodydoes (the body is a JSON object). Retries are disabled here so the legitimate 503 is returned immediately.Changes
gateways/clients/hathor_core_client.py— retry loop inHathorCoreAsyncClient.get();MAX_RETRIES/RETRY_DELAY/RETRYABLE_STATUS_CODESconstants;_decode_error_body;retryopt-out flag; per-attempttimeoutdocstringgateways/healthcheck_gateway.py— passretry=Falsefor the health endpointtests/unit/gateways/clients/test_hathor_core_client.py— newTestHathorCoreAsyncClientGetcovering success, retry-then-success, single session reuse, retry exhaustion, non-retryable 4xx, JSON/non-JSON error bodies, theretry=Falseopt-out, and network exceptionstests/unit/gateways/test_healthcheck_gateway.py— assertretry=Falseis passed for the health callSummary by CodeRabbit