Retry Squidrouter route requests that fail at the gateway - #1383
Merged
Merged
Conversation
A non-JSON error response from Squidrouter (a Cloudflare or load-balancer
error page) was collapsed to `{}` and the HTTP status was not logged, so
production logs showed "Error fetching route from Squidrouter API: {}"
with no way to tell what the upstream actually returned.
The nightly e2e smoke test failed because production's POST /v1/quotes answered 500: Squidrouter's /route endpoint returned a non-JSON 5xx from the gateway in front of it. Production logs show this ~1-2 times per hour, each one a user-facing 500, while the same request succeeds a second later. Extend the existing retry-once path (previously only for rate limits) to a 5xx whose body is not Squid's JSON error shape. Squid's own deterministic errors, such as low liquidity, keep failing fast without a retry.
✅ Deploy Preview for vortexfi canceled.
|
✅ Deploy Preview for vortex-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vrtx-dashboard canceled.
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Body-read failures bypass the retry path, and the normative security specification remains inconsistent with the new behavior.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Improves Squidrouter route resilience and diagnostics for transient gateway failures.
Changes:
- Preserves non-JSON error bodies and logs HTTP status.
- Retries non-JSON 5xx route failures once.
- Adds regression coverage for gateway and Squid JSON errors.
| File | Description |
|---|---|
packages/shared/src/services/squidrouter/route.ts |
Adds error diagnostics and transient retry handling. |
packages/shared/src/services/squidrouter/route.test.ts |
Tests retry and non-retry behavior. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…e read `response.text()` rejects when the gateway truncates the error stream. That escaped as a bare TypeError, losing the HTTP status and skipping the gateway retry, whereas the previous `.json().catch()` still produced an HttpError. Treat an unreadable body as empty so the status is logged and a 5xx is still retried.
…y spec The Squid integration spec said only 429s are retried and every other error fails fast. It also described the 429 handling as exponential backoff, whereas `getRoute` retries once after the advertised retryAfter.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The security specification’s audit checklist still incorrectly describes 429 handling as exponential backoff.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Resolved since last review (2)
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
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.
Why
The nightly e2e run 35485932144 failed in the live quote smoke step: production
POST /v1/quotes(BRL → USDT on Polygon) answered500 Internal server error. Prod logs for that request:Squidrouter's
/routeendpoint (behind Cloudflare and a load balancer) returned a non-JSON 5xx.squidFetchcollapsed the body to{}and the status was not logged, so the log could not say what Squid actually returned. The SELL quote one second later succeeded, and the same BUY works now.This is not a one-off: production logged 518 of these unparseable Squid errors in the last 14 days (~37/day, ~1.5/hour, evenly spread), each one a user-facing 500. The nightly probe simply caught one.
What
squidFetchkeeps a non-JSON error body as text, and the route error log now includes the HTTP status, so the next occurrence showsHTTP 502 "<html>…"instead of{}.getRouteInternalWithRetryreuses the existing retry-once path (previously only for 429 rate limits) for a 5xx with a non-JSON body, after 1s. Squid's own deterministic JSON errors (e.g. the ~1000/day "Low liquidity" responses) are not retried.Tests
packages/shared: 176 pass. Biome clean.Notes
mainwith the next release.