fix(client): keep the HTTP status and describe the failure on DuffelError - #1192
fix(client): keep the HTTP status and describe the failure on DuffelError#1192todor-a wants to merge 1 commit into
Conversation
igorp1
left a comment
There was a problem hiding this comment.
Hey Todor, thanks for putting this together. I agree the current behaviour is pretty opaque, and keeping the HTTP status the SDK actually received would be useful.
One thing we need to be careful about, though, is what that status represents: when the response comes from a proxy or gateway rather than Duffel, it doesn’t necessarily tell you the final state of the request. A gateway could time out while Duffel still completes the request successfully, so consumers shouldn’t treat that status as the authoritative Duffel outcome or use it alone to decide whether to retry.
I’m also a little concerned about changing message and name here, since those are observable runtime changes and could break consumers relying on existing logging, error grouping, tests, or serialization.
Would you be open to narrowing this down to surfacing the received HTTP status, with the distinction above made clear?
Client.request builds errors as new DuffelError({ ...responseBody, status, headers }), but the
constructor only reads meta, errors and headers, so the status it is handed is dropped. meta is
populated only when the API returns a JSON body, and responseBody is a string otherwise, so
spreading it yields index keys and leaves meta undefined. Whenever a proxy or gateway answers on
Duffel's behalf the status is therefore unrecoverable by the caller.
Store the status on DuffelError, falling back to meta.status. It records the response that arrived,
which is not necessarily the outcome of the request at Duffel, so it is documented as diagnostic
information rather than a retry signal.
meta, errors, headers, message and name are unchanged, so this is additive for existing consumers.
77bb19a to
e870965
Compare
|
Hey, Igor. Thank you for taking the time to take a look at this. I agree, the initial diff was a bit too aggressive. I've slimmed it down. |
igorp1
left a comment
There was a problem hiding this comment.
Thank you for considering my input!
Summary
When the API responds with something other than JSON — a gateway or proxy answering instead of the API — the resulting
DuffelErrorcarries no status, no message and no error details. There is nothing for a caller to branch on and nothing useful to log.We hit this in production and could not tell from the error whether to retry, back off, or treat it as a bad request. Tracing it back gave three causes, all in
Client.ts.Observed
2026-07-24T14:01:34.278ZPOST air/offer_requests@duffel/api4.28.0, Nodex-request-idThe missing
x-request-idis the useful part: the response never reached a point where one was issued, which points at the edge rather than the API — and is also why we cannot give you an identifier to trace. Recovering the status is the only thing that would have let us classify it.This is the occurrence we captured end to end. We have seen the same signature since, but this is the one we can evidence precisely.
1. The status is passed to the constructor and then dropped
meta.statuscovers this whenever the API returns JSON, so it usually goes unnoticed. ButresponseBodyis only an object in the JSON branch:Otherwise it is a string, and spreading a string yields index keys — so
metaanderrorsboth come outundefined, and the status the constructor was handed is lost.That is exactly the gateway-error case: a 502 or 503 HTML page from something sitting in front of the API. It is where a caller most wants the status code, and the only place it is unavailable.
2. Every
DuffelErrorhas an emptymessagesuper()is called with no argument, somessageis''on every instance — including well-formed API errors that already carry atitleandmessageinerrors[0]. Anything reportingerr.messageshows nothing.3.
nameis never setDuffelError extends Errorwithout settingthis.name, soerr.nameis'Error'. After bundling the class name is minified too, so there is no reliable way to identify the error type from a serialised log.The change
statustoDuffelError, from the value the constructor already receives, falling back tometa?.status.messagefrom the first API error (title: message), or from the status when there is no error payload —Duffel responded with HTTP 503 and no error details.this.name = 'DuffelError'.meta,errorsandheadersare untouched andstatusis additive, so existing consumers are unaffected. The observable difference is thatmessageandnamenow carry content where they previously carried none.Tests
Adds
src/Client.spec.ts— there wasn't one — covering a JSON 404 with an error payload and a 503 with an HTML body. Both fail onmainand pass with this change.Full suite: 141 passed across 38 suites, no regressions.
tsc --noEmit, ESLint and Prettier clean.