feat: improve error messages for unclear HTTP error responses#443
Open
LiteSun wants to merge 4 commits into
Open
feat: improve error messages for unclear HTTP error responses#443LiteSun wants to merge 4 commits into
LiteSun wants to merge 4 commits into
Conversation
When a timeout error occurs, the error message now includes the HTTP method, URL, timeout duration, and a hint to use the --timeout flag. This applies to all backends (API7, APISIX, APISIX Standalone). Before: AxiosError: timeout of 10000ms exceeded After: Request "GET https://example.com/api/version" timed out after 10000ms. Consider increasing the timeout with the --timeout flag. Closes #442 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an SDK helper ChangesTimeout Error Message Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Add integration-level tests using a local HTTP server with delayed responses to verify that timeout errors include clear, actionable messages across all BackendAPI7 operations: ping, version, dump, sync. Also enhance SDK unit test to verify stack trace is updated alongside the error message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@libs/sdk/src/utils.ts`:
- Around line 35-38: The rewritten timeout message builds an invalid endpoint
when error.config.url is absolute and shows "undefinedms" when timeout is
missing; update the logic that constructs url and newMessage: derive url by
using error.config.url unmodified if it looks like an absolute URL (e.g., starts
with "http://" "https://" or "//"), otherwise join baseURL and url with a single
separator to avoid double slashes, and set timeout to a safe fallback (e.g.,
"unknown" or "not set") before interpolating so newMessage never contains
"undefinedms". Reference error.config, method, url, timeout and newMessage when
making this defensive change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa42f622-64a3-4df8-82da-18ce7fdab8fd
📒 Files selected for processing (5)
libs/backend-api7/src/index.tslibs/backend-apisix-standalone/src/index.tslibs/backend-apisix/src/index.tslibs/sdk/src/utils.spec.tslibs/sdk/src/utils.ts
Address CodeRabbit review feedback: - Detect absolute URLs (http(s)://) and avoid duplicating baseURL - Handle missing timeout value gracefully (show 'an unknown duration' instead of 'undefinedms') - Add test cases for both edge cases Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/backend-api7/test/timeout.spec.ts (1)
17-21: ⚡ Quick winReduce artificial delay to keep this suite fast.
Using a 5s delayed response can keep timers alive long after each timeout assertion already passed, which slows CI unnecessarily. A much smaller delay (still >10ms) is enough for the same behavior.
Proposed change
describe('BackendAPI7 timeout error message', () => { + const RESPONSE_DELAY_MS = 200; // Create a local HTTP server that delays responses to trigger timeouts let server: ReturnType<typeof createServer>; @@ server = createServer((_, res) => { - // Delay response by 5 seconds to guarantee timeout - setTimeout(() => { + // Delay response to guarantee timeout + const timer = setTimeout(() => { res.writeHead(200, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ value: {} })); - }, 5000); + }, RESPONSE_DELAY_MS); + timer.unref(); });🤖 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 `@libs/backend-api7/test/timeout.spec.ts` around lines 17 - 21, The test's artificial response delay in timeout.spec.ts uses setTimeout with 5000ms which slows CI; change the delay in the anonymous response callback (the setTimeout in the test server handler) to a much smaller value that is still greater than the client timeout threshold (e.g., 20ms or similar >10ms) so the test behavior remains the same but the suite runs faster and timers don't linger.
🤖 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 `@libs/backend-api7/test/timeout.spec.ts`:
- Around line 17-21: The test's artificial response delay in timeout.spec.ts
uses setTimeout with 5000ms which slows CI; change the delay in the anonymous
response callback (the setTimeout in the test server handler) to a much smaller
value that is still greater than the client timeout threshold (e.g., 20ms or
similar >10ms) so the test behavior remains the same but the suite runs faster
and timers don't linger.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7723d4a-e9cd-4d93-b7f7-0d52b045cb72
📒 Files selected for processing (3)
libs/backend-api7/test/timeout.spec.tslibs/sdk/src/utils.spec.tslibs/sdk/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sdk/src/utils.spec.ts
- libs/sdk/src/utils.ts
When the API7 Dashboard returns non-2xx responses with empty body
(e.g., HTTP 500 under high concurrency), the error message was
displayed as 'Error: ""' which provides no useful diagnostic info.
The root cause is that JSON.stringify('') produces '""', and when
error.response.data is an empty string, error_msg is undefined,
falling through to JSON.stringify.
This commit adds formatAxiosErrorMessage() to the SDK utils that
always includes HTTP method, URL, status code, and status text in
error messages. When error_msg is available, it is included; when
the response body is empty or lacks error_msg, the status code and
URL still provide useful diagnostic context.
Applied to all three backends: api7, apisix, apisix-standalone.
Closes #442
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
LiteSun
changed the title
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
1 participant
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 #442
When the API7 Dashboard returns non-2xx HTTP responses with empty body (e.g., HTTP 500 under high concurrency), ADC displayed
Error: ""which provides no useful diagnostic information.Root Cause Analysis
The bug is in the
catchErrorhandler inoperator.tsacross all three backends:When the Dashboard returns HTTP 500 with empty body:
error.response.data=""(empty string)"".error_msg=undefinedJSON.stringify("")='""'(the string"")Error: ""This happens because:
??(nullish coalescing) only checksnull/undefined, not empty stringJSON.stringify("")produces the misleading""outputWhy 499 appears in Dashboard logs
When ADC sync runs with high concurrency (e.g.,
--request-concurrent 1000) and one request fails:exitOnFailure: truecausesthrowError()in the rxjs chainmergeMaptears down all inner observablesThe 499s are a side effect of the first error, not the cause.
Changes
1.
formatAxiosErrorMessage()in SDK utils (libs/sdk/src/utils.ts)New shared function that formats axios error responses with:
error_msgfrom response body (if available)error_msgfield exists)Before:
Error: ""After:
Error: PUT https://dashboard.example.com/apisix/admin/routes/123, responded with status 500 Internal Server Error2. Applied to all three backends
Updated
catchErrorinoperator.tsfor:libs/backend-api7/src/operator.tslibs/backend-apisix/src/operator.tslibs/backend-apisix-standalone/src/operator.tsBoth
exitOnFailureand non-exitOnFailurepaths are fixed.3. Timeout error improvement (previous commits)
Added
registerTimeoutInterceptorthat enhances timeout errors:Before:
AxiosError: timeout of 5000ms exceeded(no URL/method info)After:
Request "GET https://example.com/api/version" timed out after 5000ms. Consider increasing the timeout with the --timeout flag.Tests
formatAxiosErrorMessagecovering:error_msgError: ""scenario)error_msgregisterTimeoutInterceptorTest coverage note: The
Error: ""scenario requires the Dashboard to return HTTP 500 with empty body, which occurs under specific high-concurrency conditions on the same resource. This was verified through real API calls (300 concurrent PUTs to the same route ID produced 63% HTTP 500 + empty body responses). The fix is defensive and handles all response body formats correctly.Summary by CodeRabbit
Release Notes
Improvements
Tests