Map reqwest errors to semantically correct Twirp error codes

[tclem]

The From<reqwest::Error> impl was mapping all reqwest errors to InvalidArgument, which is wrong for transport failures like connection refused, DNS resolution failures, and timeouts. Downstream consumers (e.g. retry logic) couldn't distinguish transient infrastructure failures from actual bad input.

Approach

Replace the blanket mapping with variant-specific logic:

• is_builder() → InvalidArgument — actually malformed input (bad URL, etc.)
• is_redirect() / is_body() / is_decode() → Internal — unexpected server behavior or response parsing failure
• everything else (connect, timeout, request) → Unavailable — transient and retryable

The default is Unavailable rather than Internal because it's safer to assume a transport error is transient and retryable.

The .with_rust_error(e) pattern is preserved so the original reqwest error is available for debugging.

_{Generated via Copilot (Claude Opus 4.6) on behalf of @tclem}

[Copilot]

Pull request overview

Updates Twirp’s From<reqwest::Error> conversion to map reqwest error variants to more semantically appropriate Twirp error codes so downstream consumers can distinguish bad input from transient transport failures.

Changes:

• Replace blanket InvalidArgument mapping for reqwest::Error with variant-based mapping (InvalidArgument / Internal / Unavailable).
• Preserve .with_rust_error(e) to retain underlying error details for debugging.
• Add tests covering timeout and request-builder reqwest errors.

Show a summary per file

File	Description
crates/twirp/src/error.rs	Refines reqwest→Twirp error-code mapping and adds targeted tests for the new behavior.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 1/1 changed files
• Comments generated: 3

[jorendorff]

Back when we started all this, should have made the generated trait generic, like

pub trait HaberdasherApi<E>: Send + Sync {
    async fn make_hat(
        &self,
        req: twirp::Request<MakeHatRequest>,
    ) -> Result<twirp::Response<MakeHatResponse>, E>;
    ...
}

Clients would be HaberdasherApi<twirp::ClientError> and servers would be HaberdasherApi<TwirpErrorResponse>.

The ClientError could straightforwardly say what it means, no need to pretend twirp is the world's only source of errors:

enum ClientError {
    Io(std::io::Error),
    BadResponseBody(serde_json::Error),
    // ... timeout, etc.
    Twirp(TwirpErrorResponse),
}

Using the same error type for both, like we do now, would also be bad if we have a Twirp service that talks to another backend Twirp service; someone might naively ? that error up the stack. Then the backend service's 404s become our Rust service's 404s, its 429s become our 429s; and any error meta data it gives us gets forwarded to our user.

But as you said in chat, maybe it's too late to redesign now.

Please bump the 0.x version number (this is an API breaking change) and republish.

[jorendorff]

i just hope we don't get into a situation later where we need to distinguish a real decode error from a twirp 500, or a connect from a timeout.

[tclem]

Yeah, what do you think. Should we really break things and refactor properly? Maybe as a follow up?

[jorendorff]

Yeah, what do you think. Should we really break things and refactor properly? Maybe as a follow up?

I might give it a shot after this.