-
Notifications
You must be signed in to change notification settings - Fork 388
fix: Transform network failures into specific TLS timeout #2659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Transforms raw network errors (ECONNRESET, ETIMEDOUT, timed out, and TLS handshake) into a specific ApiError (code 408) with a descriptive message regarding potential CPU starvation. This prevents misleading error propagation from the underlying request library.
|
General comment but this logic will need to be ported to Gaxios in the future. |
Splits network error handling: uses 408 for timeouts (timed out, ETIMEDOUT, TLS handshake) and 503 for connection resets (ECONNRESET) to improve retry logic accuracy.
src/nodejs-common/util.ts
Outdated
| let message: string; | ||
| if (err.message.toLowerCase().includes('econnreset')) { | ||
| // ECONNRESET (Connection reset by peer) implies temporary service unavailability | ||
| code = 503; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be forcing these to have an HTTP error code. Things like ECONNRESET are usually indicative of an underlying TCP / socket level issue. They are not necessarily an error returned from the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an insightful point! You're right, TCP/socket errors like ECONNRESET are fundamental network issues and shouldn't be incorrectly treated as HTTP status codes. They reflect a connection failure, not necessarily an application-level server error.
I will check on it and come back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I was using ApiError to throw errors, it was necessary to print the error code. I understand your point, so I have replaced ApiError with the Error function.
Converts raw ECONNRESET, ETIMEDOUT, and TLS handshake failures into a standard Error object with an informative message. This helps diagnose CPU starvation or misleading 401 errors.
Replaces repetitive test cases in `makeAuthenticatedRequest` and `makeRequest` with a single, data-driven test loop. This verifies all conditions (ECONNRESET, ETIMEDOUT, "timed out", "TLS handshake") with reduced code duplication and improved maintenance. ```
|
This is a gentle reminder to please take a look when you have a moment. |
|
I'm not really sure why we are forcing things such as |
Description
Impact
Testing
Additional Information
Checklist
Fixes #