fix: classify request timeouts distinctly instead of unknown error - #239
Open
reeshika-h wants to merge 2 commits into
Open
fix: classify request timeouts distinctly instead of unknown error#239reeshika-h wants to merge 2 commits into
reeshika-h wants to merge 2 commits into
Conversation
retryResponseErrorHandler threw a plain object literal (error_message/error_code fields, no .message) for axios ECONNABORTED (client-side timeout) errors. APIError.fromAxiosError only recognizes .response.data, .request, or .message, so the plain object matched none of those and collapsed to a generic UNKNOWN_ERROR/status:0 - indistinguishable from a true network-layer failure, with no indication the request had actually timed out. Throw a real Error with .message and .code set instead, so it flows through the existing err.message branch in fromAxiosError and surfaces as a distinct, diagnosable TIMEOUT (408) error. DX-9991
No longer needed now that the ECONNABORTED branch throws a real Error instead of a plain object literal. DX-9991
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
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.
Problem
Under
retryResponseErrorHandler, an axios client-side timeout (error.code === 'ECONNABORTED') was converted into a plain object literal —{ error_message, error_code, errors }— instead of a realError. That object has no.message,.request, or.response.APIError.fromAxiosErroronly classifies errors by checkingerr.response?.data, thenerr.request != null && response == null, thenerr.message. The plain object matches none of those, so it fell through to the final catch-all and surfaced to SDK consumers as:This is indistinguishable from a genuine network-layer failure (DNS resolution failure, connection reset, etc.) — a legitimate 30s request timeout gave consumers zero information that it was, in fact, a timeout.
Confirmed via git blame this was an unintended regression from a well-intentioned change (commit
feb5bfb, Jun 2024) that tried to give timeouts a distinct408/TIMEOUTidentity, but never updatedfromAxiosErrorto recognize the shape it produced.Fix
Throw a real
Errorwith.messageset to the existing timeout message and.codeset to theTIMEOUTconstant, instead of a plain object literal. No changes needed inapi-error.ts— its existingerr.messagebranch already produces a correct, distinctAPIErroronce the thrown value is a properError.Also removed the
/* eslint-disable @typescript-eslint/no-throw-literal */at the top of the file — nothing throws a literal anymore.Scope
This fix only corrects error classification. It does not change whether a timed-out request is retried (unchanged:
ECONNABORTEDis not retried unless a customretryConditionmatches it) and does not touch connection pooling/keepAlive defaults. Those are tracked separately as follow-up SDK-side improvements under the same ticket, to be reviewed and shipped independently given they change default behavior for all consumers.Tests
ECONNABORTEDerror through the actualretryResponseErrorHandler→APIError.fromAxiosErrorpipeline (no mocking either side) — reproduces the reported bug before the fix (error_code: 'UNKNOWN_ERROR'instead of408).dist-build check, confirmed viagit stashto predate this change).Test plan
npm test)