-
Notifications
You must be signed in to change notification settings - Fork 55
fix: Surface the error field and request ID on 422 exceptions #1656
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -397,7 +397,7 @@ export class WorkOS { | |
| if (response) { | ||
| const { status, data, headers } = response; | ||
|
|
||
| const requestID = headers['X-Request-ID'] ?? ''; | ||
| const requestID = headers.get('X-Request-ID') ?? ''; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 The bracket-to- The old code Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, and this is intended. Before this change On the downstream-formatting concern:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
ArtifactsRepro: focused Jest timeout repro source
Stack trace captured during the T-Rex run
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/workos.ts
Line: 400
Comment:
**Preserve non-fetch headers**
`handleHttpError` also receives locally-created `HttpClientError`s, such as the timeout path in `FetchHttpClient` that sets `headers: {}` for status `408`. Calling `headers.get(...)` here throws `TypeError: headers.get is not a function` before the error mapper can produce the intended WorkOS exception, so timed-out requests fail with the wrong error.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed. The timeout path in In commit a7f1eb1 the timeout path now uses |
||
| const { | ||
| code, | ||
| error_description: errorDescription, | ||
|
|
@@ -416,6 +416,7 @@ export class WorkOS { | |
| case 422: { | ||
| throw new UnprocessableEntityException({ | ||
| code, | ||
| error, | ||
| errors, | ||
| message, | ||
| requestID, | ||
|
|
||
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.
🔴 Request timeouts crash with a TypeError instead of reporting the timeout error
The request-ID header is read using a method call (
headers.get('X-Request-ID')atsrc/workos.ts:400) that only works on nativeHeadersobjects, but the timeout error path supplies a plain empty object instead, so any timed-out request throws an unrelated TypeError.Impact: When any non-retryable API call times out, the SDK crashes with "headers.get is not a function" instead of surfacing the timeout error.
Timeout error path constructs headers as a plain object
In
src/common/net/fetch-client.ts:306-313, when a request times out (AbortError), anHttpClientErroris thrown withheaders: {}— a plain JavaScript object. This error reacheshandleHttpErrorinsrc/workos.ts:390, whereheaders.get('X-Request-ID')is called at line 400. Since{}doesn't have a.get()method, this throwsTypeError: headers.get is not a function.The old code used bracket access (
headers['X-Request-ID']), which returnsundefinedon both plain objects andHeadersobjects, safely falling back to''via?? ''.The fix should either:
fetch-client.ts:310use a realHeadersobject:headers: new Headers(), orworkos.ts:400:typeof headers.get === 'function' ? headers.get('X-Request-ID') : headers['X-Request-ID']Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Good catch, and confirmed. I reproduced it independently: the AbortError/timeout branch in
fetch-client.tsconstructsHttpClientErrorwithheaders: {}, so oncehandleHttpErrorreads the request ID viaheaders.get('X-Request-ID')the plain object throwsTypeError: headers.get is not a function, masking the real timeout error.Fixed via option 1 — the timeout path now constructs the error with
new Headers()(commit a7f1eb1). I chose fixing it at the source over a defensivetypeof headers.get === 'function'check inhandleHttpErrorbecause it keeps a single, uniform header-access path (.get) — the same one already used forRetry-Afteron 429s — and guarantees everyHttpClientError.response.headersis a fetchHeadersinstance rather than papering over an inconsistent shape at the read site.Added a WorkOS-level regression test (
when the request times out) that drives a timed-out request throughWorkOS.postand asserts it rejects with the 408 timeout error (requestID: '') instead of aTypeError. Verified it fails on the pre-fix code with exactlyheaders.get is not a functionand passes after.