feat: handle rate limiting

[tziemek]

resolves #70

[sourcery-ai]

Reviewer's Guide

Implements rate-limit (HTTP 429) handling across the Fetcher and SendVisitor paths by introducing configurable default Retry-After behavior, centralizing header parsing, wiring CLI options, and adding tests to validate backoff semantics.

Sequence diagram for SendVisitor upload with rate limiting handling

sequenceDiagram
    actor User
    participant Caller
    participant SendVisitor
    participant HttpSender
    participant ReqwestClient as Reqwest_Client
    participant HttpUtils as http_module
    participant Backoff as Backoff_Retry

    User->>Caller: start_upload(name, body, url)
    Caller->>SendVisitor: send(name, body, method, url)
    SendVisitor->>Backoff: create_backoff_with_retries(retries)
    loop each_attempt
        SendVisitor->>SendVisitor: send_once(name, body, method, url)
        SendVisitor->>HttpSender: build_request(method, url, body)
        HttpSender->>ReqwestClient: execute_request(request)
        ReqwestClient-->>SendVisitor: Response
        SendVisitor->>HttpUtils: get_retry_after_from_response_header(response, default_retry_after)
        alt status_is_429
            HttpUtils-->>SendVisitor: Some(retry_after)
            SendVisitor-->>Backoff: return Temporary(SendError_RateLimited(retry_after))
        else status_not_429
            HttpUtils-->>SendVisitor: None
            alt success_status
                SendVisitor-->>Caller: Ok(())
                Caller-->>User: upload_completed
                note over SendVisitor: break
            else client_or_server_error
                SendVisitor-->>Backoff: return Temporary_or_Permanent_error
            end
        end
        Backoff-->>SendVisitor: schedule_next_attempt(dur)
        opt notify_hook
            Backoff->>SendVisitor: notify(err = Temporary_RateLimited, dur)
            alt dur < retry_after
                SendVisitor->>SendVisitor: additional = retry_after - dur
                SendVisitor->>SendVisitor: std_thread_sleep(additional)
            else dur >= retry_after
                SendVisitor->>SendVisitor: wait_dur_only
            end
        end
    end
    Backoff-->>Caller: propagate_final_error
    Caller-->>User: upload_failed

Loading

Class diagram for updated HTTP rate limiting components

classDiagram
    class Fetcher {
        +Client client
        +usize retries
        +Duration default_retry_after
        +new(client: Client, options: FetcherOptions) Fetcher
        +get(url: Url, processor: FetcherResponseProcessor) Result~T, Error~
        +request(method: Method, url: Url, body: Option~Body~, processor: FetcherResponseProcessor) Result~T, Error~
    }

    class FetcherOptions {
        +Duration timeout
        +usize retries
        +Duration default_retry_after
        +new(timeout: Duration) FetcherOptions
        +retries(retries: usize) FetcherOptions
        +default_retry_after(duration: Duration) FetcherOptions
        +Default() FetcherOptions
    }

    class ClientArguments {
        +humantime_Duration timeout
        +usize retries
        +humantime_Duration default_retry_after
        +From_ClientArguments_for_FetcherOptions()
    }

    class FetcherError {
        <<enumeration>>
        +Request(reqwest_Error)
        +RateLimited(Duration)
    }

    class SendVisitor {
        +HttpSender sender
        +usize retries
        +Option~Duration~ min_delay
        +Option~Duration~ max_delay
        +Option~Duration~ default_retry_after
        +new(sender: HttpSender) SendVisitor
        +retries(retries: usize) SendVisitor
        +min_delay(retry_delay: Duration) SendVisitor
        +max_delay(retry_delay: Duration) SendVisitor
        +default_retry_after(duration: Duration) SendVisitor
        +send_once(name: String, body: Bytes, method: Method, url: Url) Result~(), SendOnceError~
        +send(name: String, body: Bytes, method: Method, url: Url) Result~(), SendError~
    }

    class SendError {
        <<enumeration>>
        +Client(StatusCode)
        +Server(StatusCode)
        +UnexpectedStatus(StatusCode)
        +RateLimited(Duration)
    }

    class SendOnceError {
        <<enumeration>>
        +Permanent(SendError)
        +Temporary(SendError)
    }

    class http_module {
        <<module>>
        +get_retry_after_from_response_header(response: Response, default_duration: Duration) Option~Duration~
        -parse_retry_after(value: str) Option~Duration~
    }

    class BackoffPolicy {
        <<external>>
        +notify(callback: fn(err, dur))
        +retry(policy: BackoffPolicy)
    }

    Fetcher --> FetcherOptions : constructs_with
    Fetcher --> FetcherError : returns
    Fetcher --> http_module : uses
    FetcherOptions <.. ClientArguments : From_impl

    SendVisitor --> SendError : returns
    SendVisitor --> SendOnceError : returns
    SendVisitor --> http_module : uses
    SendOnceError --> SendError : wraps

    Fetcher --> BackoffPolicy : uses
    SendVisitor --> BackoffPolicy : uses

Loading

File-Level Changes

Change	Details	Files
Add shared HTTP rate-limiting helper to extract Retry-After durations from 429 responses.	Introduce http module with parse_retry_after and get_retry_after_from_response_header helpers. Return a duration whenever status is 429, using Retry-After header if parseable, otherwise falling back to a provided default duration. Export the new http module in the common crate root so other modules can use it.	common/src/http.rs common/src/lib.rs
Extend Fetcher to recognize 429 responses, surface a dedicated error, and coordinate backoff with Retry-After.	Add default_retry_after to Fetcher and FetcherOptions with a Default of 10 seconds plus a CLI flag to override. Introduce an Error::RateLimited(Duration) variant and use get_retry_after_from_response_header to return it when a 429 response is received. Hook Fetcher’s retry backoff notify callback to, on rate-limit errors, synchronously sleep long enough to satisfy the Retry-After duration when the exponential backoff delay would otherwise be shorter.	common/src/fetcher/mod.rs common/src/cli/client.rs
Extend SendVisitor to detect 429s, propagate a rate-limit error, and coordinate retry delay with Retry-After.	Introduce SendError::RateLimited(Duration) and a default_retry_after field on SendVisitor with builder configuration and a default of 10 seconds. Use get_retry_after_from_response_header before normal status handling to detect 429 and convert it into a SendOnceError::Temporary(SendError::RateLimited). Adjust the backoff notify callback to, when encountering rate-limit errors, enforce at least the Retry-After wait by sleeping longer if the exponential backoff delay is shorter.	extras/src/visitors/send/mod.rs extras/src/visitors/send/clap.rs
Add integration-style tests around Fetcher rate-limit behavior and backoff semantics using a local hyper-based HTTP server.	Introduce dev-dependencies on hyper, hyper-util, and tokio with server/runtime features to support a mock HTTP server in tests. Add tests covering basic success, 429 with and without Retry-After, exponential backoff on 5xx errors, retry exhaustion, multiple 429s, and custom default_retry_after behavior. Use an atomic counter and Instant-based elapsed timing assertions to verify the number of attempts and that total wait time respects Retry-After or default values.	common/Cargo.toml 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:

• Both Fetcher and SendVisitor use std::thread::sleep inside the notify callback, which will block the async runtime; consider replacing this with a non-blocking tokio::time::sleep-based mechanism (e.g., adjusting the backoff delay itself or using an async-aware hook) so retries do not block the entire executor thread.
• SendVisitor::send_once calls get_retry_after_from_response_header with self.default_retry_after.unwrap_or_default(), which means default_retry_after: None results in a 0-second delay for 429s; if None is intended to mean "no special rate-limit delay" or "use exponential backoff only", consider handling the Option explicitly instead of defaulting to Duration::ZERO.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Both `Fetcher` and `SendVisitor` use `std::thread::sleep` inside the `notify` callback, which will block the async runtime; consider replacing this with a non-blocking `tokio::time::sleep`-based mechanism (e.g., adjusting the backoff delay itself or using an async-aware hook) so retries do not block the entire executor thread.
- `SendVisitor::send_once` calls `get_retry_after_from_response_header` with `self.default_retry_after.unwrap_or_default()`, which means `default_retry_after: None` results in a 0-second delay for 429s; if `None` is intended to mean "no special rate-limit delay" or "use exponential backoff only", consider handling the `Option` explicitly instead of defaulting to `Duration::ZERO`.

## Individual Comments

### Comment 1
<location> `extras/src/visitors/send/mod.rs:190-199` </location>
<code_context>
             }
         })
         .retry(&backoff.with_max_times(retries))
+        .notify(|err, dur| {
+            // If rate limited, ensure we wait at least the Retry-After duration
+            if let Error::RateLimited(retry_after) = err {
+                if dur < *retry_after {
+                    log::info!(
+                        "Rate limited, extending wait from {:?} to {:?}",
+                        dur,
+                        retry_after
+                    );
+                    let additional = *retry_after - dur;
+                    std::thread::sleep(additional);
+                }
+            }
</code_context>

<issue_to_address>
**issue (bug_risk):** The notify closure pattern-matches on a reference and uses blocking sleep in an async retry loop.

Here `err` is almost certainly a reference (e.g. `&SendOnceError`), so `if let Error::RateLimited(retry_after) = err` will never match as written. You likely need to match on `*err` or use a `&Error::RateLimited(...)` pattern so the rate-limit branch can actually execute.

Also, `std::thread::sleep` inside this async retry callback will block the executor thread and undermines the `tokio::time::sleep`-based backoff. To enforce a minimum delay based on `retry_after`, either incorporate it into the backoff configuration or add an extra `tokio::time::sleep(additional)` instead of a blocking sleep.
</issue_to_address>

### Comment 2
<location> `common/src/fetcher/mod.rs:139-148` </location>
<code_context>
             }
         })
         .retry(&backoff.with_max_times(retries))
+        .notify(|err, dur| {
+            // If rate limited, ensure we wait at least the Retry-After duration
+            if let Error::RateLimited(retry_after) = err {
+                if dur < *retry_after {
+                    log::info!(
+                        "Rate limited, extending wait from {:?} to {:?}",
+                        dur,
+                        retry_after
+                    );
+                    let additional = *retry_after - dur;
+                    std::thread::sleep(additional);
+                }
+            }
</code_context>

<issue_to_address>
**issue (bug_risk):** Same notify closure issues: matching on a reference and blocking the async runtime with std::thread::sleep.

Here `err` is almost certainly `&Error`, so `if let Error::RateLimited(retry_after) = err` will never match. You likely want to destructure the inner value instead, e.g. `if let Error::RateLimited(retry_after) = *err` (or `&*err`).

Also, `std::thread::sleep` will block a worker thread in this async path. Since the retry logic already uses async backoff, please enforce `retry_after` using `tokio::time::sleep(additional)` (or by adjusting the backoff) so it cooperates with the async runtime instead of blocking it.
</issue_to_address>

### Comment 3
<location> `extras/src/visitors/send/mod.rs:138` </location>
<code_context>
             .await
             .map_err(|err| SendOnceError::Temporary(err.into()))?;

+        if let Some(retry_after) = get_retry_after_from_response_header(
+            &response,
+            self.default_retry_after.unwrap_or_default(),
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Using unwrap_or_default() for default_retry_after makes None mean 0 seconds, which may not match the documented semantics.

Because `default_retry_after` is an `Option<Duration>`, passing `self.default_retry_after.unwrap_or_default()` into `get_retry_after_from_response_header` makes `None` behave as `Duration::default()` (0s). That changes the meaning of `None` from "no default" to "retry immediately". In particular, `SendArguments` sets `default_retry_after: None`, so the CLI path effectively disables the 10s default and uses 0s instead.

If `None` is meant to represent "no default retry-after" (or to keep the constructor’s 10s default), consider either:
- not overriding the 10s default in `SendArguments` when the user doesn’t provide a value, or
- changing `get_retry_after_from_response_header` to take `Option<Duration>` and only using a fallback when `Some(default)`.

Otherwise, document clearly that `None` is intended to mean "retry immediately" on 429 without a header.

Suggested implementation:

```rust
        if let Some(retry_after) = get_retry_after_from_response_header(
            &response,
            self.default_retry_after,
        ) {

```

To fully implement the semantics you described, you will also need to:

1. Update the signature of `get_retry_after_from_response_header` (likely in the same module or a nearby one) from something like:
   ```rust
   fn get_retry_after_from_response_header(
       response: &Response<Body>,
       default_retry_after: Duration,
   ) -> Option<Duration> { ... }
   ```
   to:
   ```rust
   fn get_retry_after_from_response_header(
       response: &Response<Body>,
       default_retry_after: Option<Duration>,
   ) -> Option<Duration> { ... }
   ```

2. Inside `get_retry_after_from_response_header`, implement the intended behavior for `None`, for example:
   - If the `Retry-After` header is present and valid, use it.
   - Else, if `default_retry_after` is `Some(d)`, use `d`.
   - Else (when `None`), either:
     - Use the constructor’s built-in default (e.g. 10s), or
     - Return `None` to mean “no retry-after default”.

3. Adjust any other call sites of `get_retry_after_from_response_header` to pass an `Option<Duration>` instead of a bare `Duration`, or wrap existing `Duration` arguments in `Some(...)`.

These changes will ensure that `None` for `default_retry_after` does not silently become "retry immediately", and instead preserves the documented semantics (“no default” or “use constructor default”).
</issue_to_address>

### Comment 4
<location> `common/tests/fetcher.rs:46-55` </location>
<code_context>
+#[tokio::test]
</code_context>

<issue_to_address>
**suggestion (testing):** Tests depend on real-time sleeps (including a 10s minimum), which will make the test suite very slow and brittle.

In `test_rate_limit_without_retry_after` (and the other timing-based tests), asserting an elapsed time of at least 10 seconds means the test always sleeps that long because it uses real `std::thread::sleep` in the retry hook. Please either reduce the configured wait durations to a few milliseconds or refactor the retry/backoff logic to be injectable so you can mock time instead of relying on wall-clock delays.
</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]

Thank you so much for this addition. There are some issues to be sorted out though, and room for improvement (which I don't consider blocking). But we have to sort out the sleep, max duration and http date header value.

[ctron]

I think, in addition to the default value, we should also have a "max" value. Otherwise it might happen, malicious or not, that someone asks for "wait 1 year", and we would do.

[tziemek]

But we wouldn't expose the "max" value to the user. What could be a reasonable value for "max"? Or better a maximum value for this limit? 5 minutes? Would be long enough as I don't think any API should have a much longer rate limiting. But if it has there are other problems.

[ctron]

I think we should. You never know what a user wants. But I also think we should offer a reasonable default, like you suggested, 5 mins.

[tziemek]

If we expose MAX to the user, we have to have a limit for that and document it. Otherwise we will have the same problem if the user sets default and max to 1year.

[ctron]

No, I'd say we leave this unlimited. If the user puts in 1 year, that's the user's choice. However, the user is not able to influence the source. So this enables the user to take control of limits.

[tziemek]

Ok, let's leave it open with 5min as default. I'll add that.

[tziemek]

@ctron Do you have any ideas how to handle the sleep issue?
At first I tried to do a tokio::sleep if the response is an error (without notify), but that results in having the delay from the exponential backoff and doing the manual sleep. Then I moved it in the notify, because that kind of works and I can react to the delay in regards to the backoff. And we must do it dynamically.
Another approach could be to recreate the backoff, or do the first call without a backoff, if it gets rate limited, we do it with the fixed backoff. But a Retry-After header shouldn't be cached :(

Another option would be to leave out the backon library and do it manually.

I think I found a way with a custom backoff strategy (wrapper).

[ctron]

What speaks against using https://docs.rs/backon/latest/backon/struct.Retry.html#method.adjust ?

[tziemek]

What speaks against using https://docs.rs/backon/latest/backon/struct.Retry.html#method.adjust ?

Apparently I am blind, looks like this is what we are looking for, I'll try it. Thanks

[ctron]

This looks great. Thanks!

[ctron]

This may be a breaking change, and the CI might complain about this. Just uptick the version number, it's worth it.

[tziemek]

This may be a breaking change, and the CI might complain about this. Just uptick the version number, it's worth it.

Happy to help :)