[Detail Bug] Retryable HTTP errors with non-JSON bodies are misclassified as non-retryable (decode error → ClientError::Others)

[detail-app]

Detail Bug Report

https://app.detail.dev/org_89d327b3-b883-4365-b6a3-46b6701342a9/bugs/bug_b35409cd-d5e1-4572-b5f9-e4ee0e4f7298

Summary

• Context: The retry logic in src/api.rs determines whether to retry failed requests based on HTTP status codes and error classification.
• Bug: When a retryable HTTP error (500, 502, 503, 504, etc.) returns a non-JSON response body, the JSON parsing failure is misclassified as a non-retryable ClientError::Others, preventing retry attempts.
• Actual vs. expected: Retryable HTTP errors with non-JSON bodies are not retried; they should be retried regardless of response body format.
• Impact: Transient server errors that return HTML error pages (common for 500/502/504) are never retried, causing unnecessary failures for clients when the server could recover on retry.

Code with bug

impl From<reqwest::Error> for ClientError {
    fn from(err: reqwest::Error) -> Self {
        let err_msg = err.to_string();
        if err.is_connect() {
            Self::Connect(err_msg)
        } else if err.is_timeout() {
            Self::Timeout(err_msg)
        } else if err.is_request() {
            if let Some(hyper_err) = source_err::<hyper::Error>(&err) {
                let hyper_err_msg = format!("{hyper_err} -> {err_msg}");
                if hyper_err.is_incomplete_message() {
                    Self::ConnectionClosedEarly(hyper_err_msg)
                } else if hyper_err.is_canceled() {
                    Self::RequestCanceled(hyper_err_msg)
                } else {
                    Self::Others(hyper_err_msg)
                }
            } else {
                Self::Others(err_msg)
            }
        } else if let Some(io_err) = source_err::<std::io::Error>(&err) {
            let io_err_msg = format!("{io_err} -> {err_msg}");
            if io_err.kind() == std::io::ErrorKind::UnexpectedEof {
                Self::UnexpectedEof(io_err_msg)
            } else if io_err.kind() == std::io::ErrorKind::ConnectionReset {
                Self::ConnectionReset(io_err_msg)
            } else if io_err.kind() == std::io::ErrorKind::ConnectionAborted {
                Self::ConnectionAborted(io_err_msg)
            } else if io_err.kind() == std::io::ErrorKind::ConnectionRefused {
                Self::ConnectionRefused(io_err_msg)
            } else {
                Self::Others(io_err_msg)
            }
        } else {
            Self::Others(err_msg)  // <-- BUG 🔴 Decode errors fall through here
        }
    }
}

async fn into_result(self) -> Result<Response, ApiError> {
    let status = self.status();
    if status.is_success() {
        Ok(self)
    } else {
        Err(ApiError::Server(
            status,
            self.json::<ApiErrorResponse>().await?,  // <-- BUG 🔴 If this fails to parse JSON
        ))                                            //           it returns reqwest::Error
    }                                                 //           which becomes ClientError::Others
}

pub fn is_retryable(&self) -> bool {
    !matches!(self, ClientError::Others(_))  // <-- BUG 🔴 Others is not retryable
}

Example

• Server returns HTTP 500 with an HTML error page (non-JSON body).
• Response handling calls into_result(); since the status is not success, it attempts self.json::<ApiErrorResponse>().await?.
• JSON parsing fails; reqwest returns an error with is_decode() == true.
• The error is converted via From<reqwest::Error> for ClientError and falls through to ClientError::Others.
• Retry logic checks err.is_retryable(); ClientError::Others is not retryable, so no retry occurs even though HTTP 500 is designated as retryable.

Result: Decode errors mask retryable HTTP statuses, preventing retries for transient server errors with non-JSON bodies.

Recommended fix

Change the error handling flow to extract the status code before attempting JSON parsing, and make retry decisions based on status code first, before considering the error body. This separates concerns: retry decisions should be based on HTTP semantics (status codes), not on the ability to parse the response body.

[quettabit]

i can see this argument, but we always expect error responses to be in JSON format, so i think the current behavior is reasonable.