Skip to content

Commit 6fa2c7d

Browse files
tclemCopilot
andauthored
Map reqwest errors to semantically correct Twirp error codes (#314)
* Map reqwest errors to semantically correct Twirp error codes Replace the blanket From<reqwest::Error> → InvalidArgument mapping with variant-specific logic: - is_builder() → InvalidArgument (actually bad input) - is_redirect() / is_body() / is_decode() → Internal (unexpected behavior) - everything else (connect, timeout, request) → Unavailable (transient) This lets downstream consumers distinguish transport failures from application errors, enabling correct retry decisions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Hoist .with_rust_error(e) out of branches Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback on reqwest error mapping tests - Use localhost TcpListener instead of non-routable IP for timeout test - Fix misleading comment on builder error test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 5b756cd commit 6fa2c7d

1 file changed

Lines changed: 50 additions & 2 deletions

File tree

crates/twirp/src/error.rs

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,10 +258,22 @@ impl From<serde_json::Error> for TwirpErrorResponse {
258258
}
259259
}
260260

261-
// unable to build the request
261+
// Map reqwest errors to semantically appropriate Twirp error codes.
262+
// Transport failures (connect, timeout) → Unavailable (retryable).
263+
// Request-building errors → InvalidArgument (caller's fault).
264+
// Response-parsing errors → Internal (unexpected server behavior).
262265
impl From<reqwest::Error> for TwirpErrorResponse {
263266
fn from(e: reqwest::Error) -> Self {
264-
invalid_argument(e.to_string()).with_rust_error(e)
267+
let msg = e.to_string();
268+
let resp = if e.is_builder() {
269+
invalid_argument(msg)
270+
} else if e.is_redirect() || e.is_body() || e.is_decode() {
271+
internal(msg)
272+
} else {
273+
// connect, timeout, request, and anything else — treat as transient
274+
unavailable(msg)
275+
};
276+
resp.with_rust_error(e)
265277
}
266278
}
267279

@@ -393,6 +405,42 @@ mod test {
393405
assert_eq!(response, result);
394406
}
395407

408+
#[tokio::test]
409+
async fn reqwest_timeout_error_maps_to_unavailable() {
410+
// Bind a listener that accepts but never responds, guaranteeing a timeout.
411+
let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
412+
let addr = listener.local_addr().unwrap();
413+
let _accept_thread = std::thread::spawn(move || {
414+
let (_stream, _) = listener.accept().unwrap();
415+
std::thread::sleep(std::time::Duration::from_secs(60));
416+
});
417+
418+
let client = reqwest::Client::builder()
419+
.timeout(std::time::Duration::from_millis(1))
420+
.build()
421+
.unwrap();
422+
let err = client
423+
.get(format!("http://{addr}"))
424+
.send()
425+
.await
426+
.unwrap_err();
427+
let twirp_err: TwirpErrorResponse = err.into();
428+
assert_eq!(twirp_err.code, TwirpErrorCode::Unavailable);
429+
}
430+
431+
#[test]
432+
fn reqwest_builder_error_maps_to_invalid_argument() {
433+
// An empty URL string triggers a builder error
434+
let err = reqwest::Client::builder()
435+
.build()
436+
.unwrap()
437+
.get("")
438+
.build()
439+
.unwrap_err();
440+
let twirp_err: TwirpErrorResponse = err.into();
441+
assert_eq!(twirp_err.code, TwirpErrorCode::InvalidArgument);
442+
}
443+
396444
#[test]
397445
fn twirp_error_response_serialization_skips_fields() {
398446
let response = TwirpErrorResponse {

0 commit comments

Comments
 (0)