feat: expose client errors in fetcher and don't retry client errors

[tziemek]

This is the first step towards #69 , more small PRs will follow.

Summary by Sourcery

Expose HTTP client errors from the fetcher and avoid retrying requests that fail with 4xx responses.

New Features:

• Add a dedicated ClientError variant to the fetcher error type to surface HTTP 4xx responses to callers.

Bug Fixes:

• Prevent the fetcher from retrying requests that return HTTP 4xx client errors.

Enhancements:

• Introduce a helper for detecting client error status codes from HTTP responses and integrate it into fetcher response handling.

Tests:

• Add a test verifying that HTTP 404 responses are surfaced as client errors and are not retried.

[sourcery-ai]

Reviewer's Guide

Adds explicit handling of HTTP 4xx responses in the fetcher by introducing a new ClientError error variant, preventing retries for client errors, and covering the behavior with a dedicated test and a small HTTP utility helper.

Sequence diagram for Fetcher client error handling and retry behavior

sequenceDiagram
    participant Caller
    participant Fetcher
    participant RetryPolicy
    participant HttpClient
    participant http

    Caller->>Fetcher: fetch(url, processor)
    Fetcher->>RetryPolicy: configure retry
    loop retry attempts
        RetryPolicy->>Fetcher: call fetch_once(url, processor)
        Fetcher->>HttpClient: send request
        HttpClient-->>Fetcher: Response
        Fetcher->>http: get_client_error(response)
        http-->>Fetcher: Option_StatusCode
        alt client error
            Fetcher-->>RetryPolicy: Error::ClientError
            RetryPolicy->>RetryPolicy: when(e) skip retry on ClientError
            RetryPolicy-->>Caller: return Error::ClientError
        else rate limited 429
            Fetcher-->>RetryPolicy: Error::RateLimited
            RetryPolicy->>RetryPolicy: schedule retry after delay
        else success
            Fetcher->>processor: process(response)
            processor-->>Fetcher: Ok(result)
            Fetcher-->>RetryPolicy: Ok(result)
            RetryPolicy-->>Caller: Ok(result)
        end
    end

Loading

Class diagram for updated error handling in Fetcher

classDiagram
    class http {
        <<module>>
        get_client_error(response) Option_StatusCode
    }

    class Response {
        status() StatusCode
    }

    class StatusCode {
        is_client_error() bool
    }

    class Error {
        <<enum>>
        Request
        RateLimited
        ClientError
    }

    class Fetcher {
        fetch(url, processor)
        fetch_once(url, processor) Result
    }

    class Processor {
        process(response) Result
    }

    Error <|-- Request
    Error <|-- RateLimited
    Error <|-- ClientError

    Fetcher --> Error
    Fetcher --> Processor
    Fetcher --> http : uses
    http --> Response
    Response --> StatusCode

Loading

File-Level Changes

Change	Details	Files
Introduce a ClientError variant to the fetcher error type and ensure the retry logic skips retries on client errors.	Extend the fetcher Error enum with a ClientError(StatusCode) variant and display message. Update the retry chain to only retry when the error is not a ClientError using a when predicate. Maintain existing special handling and backoff adjustment for RateLimited errors.	common/src/fetcher/mod.rs
Detect HTTP client errors in responses and propagate them as fetcher errors instead of retrying.	Add an HTTP helper that returns the status code when a response is a client error (4xx). Invoke the helper in fetch_once to log and return a ClientError when a client error status is detected before processing the response body.	common/src/http.rs common/src/fetcher/mod.rs
Add a regression test asserting that 4xx responses are surfaced as errors and not retried.	Import the new Error type into the fetcher tests. Add an async test that serves a 404 response, asserts the fetch result is a ClientError with 404, and verifies only a single request attempt is made despite configured retries.	common/tests/fetcher.rs

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• In test_404_should_not_retry, the mock body string "Rate limited" is misleading for a 404 case and makes the test harder to read; consider changing it to something like "Not found" to better reflect the scenario being tested.
• In test_404_should_not_retry, instead of assert_eq!(result.is_err(), true) followed by a manual if let, you can use assert!(matches!(result, Err(Error::ClientError(StatusCode::NOT_FOUND)))) to make the expectation about the exact error type and status code more explicit and concise.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `test_404_should_not_retry`, the mock body string "Rate limited" is misleading for a 404 case and makes the test harder to read; consider changing it to something like "Not found" to better reflect the scenario being tested.
- In `test_404_should_not_retry`, instead of `assert_eq!(result.is_err(), true)` followed by a manual `if let`, you can use `assert!(matches!(result, Err(Error::ClientError(StatusCode::NOT_FOUND))))` to make the expectation about the exact error type and status code more explicit and concise.

## Individual Comments

### Comment 1
<location> `common/tests/fetcher.rs:64-73` </location>
<code_context>
 }

+#[tokio::test]
+async fn test_404_should_not_retry() {
+    let attempt_count = Arc::new(AtomicUsize::new(0));
+    let attempt_count_clone = attempt_count.clone();
+
+    let server = start_mock_server(move |_req| {
+        attempt_count_clone.fetch_add(1, Ordering::SeqCst);
+        let builder = hyper::Response::builder().status(StatusCode::NOT_FOUND);
+        builder.body("Rate limited".to_string()).unwrap()
+    })
+    .await;
+
+    let fetcher = Fetcher::new(FetcherOptions::new().retries(2))
+        .await
+        .unwrap();
+
+    let result: Result<String, Error> = fetcher.fetch(&server).await;
+    assert_eq!(result.is_err(), true);
+    if let Error::ClientError(status_code) = result.unwrap_err() {
+        assert_eq!(status_code, StatusCode::NOT_FOUND);
+    }
+    assert_eq!(attempt_count.load(Ordering::SeqCst), 1);
+}
+
</code_context>

<issue_to_address>
**issue (testing):** Strengthen the assertion to ensure the error is specifically `Error::ClientError(StatusCode::NOT_FOUND)` rather than any error

Currently the test only asserts `is_err()` and then conditionally inspects the error with `if let`, so any non-`ClientError` variant would still let the test pass. Instead, unwrap as an error and assert the exact variant, e.g.:

```rust
let err = result.expect_err("expected client error");
assert!(matches!(err, Error::ClientError(StatusCode::NOT_FOUND)));
```

or:

```rust
match result {
    Err(Error::ClientError(code)) => assert_eq!(code, StatusCode::NOT_FOUND),
    other => panic!("expected ClientError(404), got {other:?}"),
}
```

so the test fails if the error type regresses.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[ctron]

Small nit. But otherwise, looks great!

[ctron]

Ha, I could update that myself 😬