Merge pull request #56 from semiotic-ai/fix/rate-limit-zero-validation

fix: reject zero arguments in RateLimitLayer constructors

Author: suchapalaver

CHANGELOG.md

@@ -45,6 +45,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- `RateLimitLayer::new`, `::per_second`, and `::with_min_delay` now panic
+  at construction when given zero arguments, instead of silently building
+  a degenerate layer. Previously `RateLimitLayer::per_second(0)` produced
+  a token bucket with capacity zero and a refill rate of zero, so the
+  first acquire computed `wait_nanos = 1.0 / 0.0 = +inf` and the service
+  stalled indefinitely on the first request; `with_min_delay(Duration::
+  ZERO)` produced one whose `refill_rate` was `+inf` and whose refill
+  arithmetic tainted the token count with `NaN`, leaving every subsequent
+  acquire returning implementation-defined wait times. An operator who
+  loaded a per-chain pacing config from a TOML/YAML file where
+  `min_delay_ms` defaulted to `0` — or who wrote `with_rate_limit(0)`
+  thinking it disabled the rate-limit budget — would see flaky throughput
+  or a hung provider with no error message pointing at the misconfigured
+  layer. The constructors now reject both shapes with a panic that names
+  the invalid axis and points operators at the documented "leave the axis
+  unset on `ProviderConfig` to disable" idiom, so misconfiguration
+  surfaces at construction time rather than as runtime pathology. Closes
+  #53.
 - Provider pools built through `ProviderPoolBuilder::with_rpc_policy`
   now honour the policy's per-chain rate-limit delay. Operators who
   configured `SemioscanConfigBuilder::chain_rate_limit(...)` or

src/provider/factory.rs

@@ -30,6 +30,7 @@ use super::AnyHttpProvider;
 /// Precedence: when both axes are set, `rate_limit_per_second` wins and
 /// `min_delay` is dropped with a warn. This matches the documented
 /// `ProviderPoolBuilder` precedence and the historical HTTP behaviour.
+#[track_caller]
 pub(super) fn rate_limit_layer_for(
     rate_limit_per_second: Option<u32>,
     min_delay: Option<Duration>,

src/transport/rate_limit.rs

@@ -51,6 +51,17 @@ impl RateLimitLayer {
     /// * `requests` - Maximum number of requests allowed in the given period
     /// * `period` - The time period for the rate limit
     ///
+    /// # Panics
+    ///
+    /// Panics if `requests` is `0` or `period` is [`Duration::ZERO`]. Either
+    /// value would produce a degenerate token bucket whose math cannot
+    /// represent a finite rate: a zero budget would stall every request
+    /// indefinitely on the first acquire, and a zero refill period would
+    /// taint the refill rate with `inf`/`NaN` so subsequent acquires return
+    /// implementation-defined wait times. If you want to disable pacing,
+    /// leave the relevant axis on [`ProviderConfig`](crate::provider::ProviderConfig)
+    /// unset (i.e. `None`) instead of passing zero.
+    ///
     /// # Example
     ///
     /// ```rust
@@ -63,7 +74,18 @@ impl RateLimitLayer {
     /// // Allow 100 requests per minute
     /// let layer = RateLimitLayer::new(100, Duration::from_secs(60));
     /// ```
+    #[track_caller]
     pub fn new(requests: u32, period: Duration) -> Self {
+        assert!(
+            requests > 0,
+            "RateLimitLayer requires requests > 0; got 0. \
+             To disable rate limiting, leave the axis unset on ProviderConfig instead of passing zero."
+        );
+        assert!(
+            !period.is_zero(),
+            "RateLimitLayer requires period > 0; got Duration::ZERO. \
+             To disable pacing, leave min_delay unset on ProviderConfig instead of passing Duration::ZERO."
+        );
         Self {
             state: Arc::new(Mutex::new(RateLimitState::new(requests, period))),
         }
@@ -73,6 +95,11 @@ impl RateLimitLayer {
     ///
     /// This is a convenience constructor for common rate limiting scenarios.
     ///
+    /// # Panics
+    ///
+    /// Panics if `requests` is `0`. See [`RateLimitLayer::new`] for the
+    /// reasoning and how to express "no rate limit" instead.
+    ///
     /// # Example
     ///
     /// ```rust
@@ -81,6 +108,7 @@ impl RateLimitLayer {
     /// // 25 requests per second
     /// let layer = RateLimitLayer::per_second(25);
     /// ```
+    #[track_caller]
     pub fn per_second(requests: u32) -> Self {
         Self::new(requests, Duration::from_secs(1))
     }
@@ -90,6 +118,11 @@ impl RateLimitLayer {
     /// This is useful when you want to ensure a fixed delay between
     /// consecutive requests rather than allowing bursts.
     ///
+    /// # Panics
+    ///
+    /// Panics if `delay` is [`Duration::ZERO`]. See [`RateLimitLayer::new`]
+    /// for the reasoning and how to express "no pacing" instead.
+    ///
     /// # Example
     ///
     /// ```rust
@@ -99,6 +132,7 @@ impl RateLimitLayer {
     /// // At least 100ms between requests (max 10 req/s)
     /// let layer = RateLimitLayer::with_min_delay(Duration::from_millis(100));
     /// ```
+    #[track_caller]
     pub fn with_min_delay(delay: Duration) -> Self {
         Self::new(1, delay)
     }
@@ -269,6 +303,40 @@ mod tests {
         assert!(layer.state.lock().await.capacity == 25);
     }
 
+    // The three panic tests below pin the constructor contract documented in
+    // each `# Panics` section. Without them, `RateLimitLayer::per_second(0)`
+    // builds a layer that stalls every request indefinitely on the first
+    // acquire (capacity = 0, refill_rate = 0 ⇒ wait_nanos = +inf), and
+    // `RateLimitLayer::with_min_delay(Duration::ZERO)` builds one whose
+    // refill math taints `tokens` with `NaN` so every subsequent acquire
+    // returns implementation-defined wait times. Catching this at
+    // construction makes the misconfiguration visible immediately instead
+    // of as flaky throughput or a hung provider in production.
+
+    #[test]
+    #[should_panic(expected = "requests > 0")]
+    fn test_rate_limit_layer_new_rejects_zero_requests() {
+        let _ = RateLimitLayer::new(0, Duration::from_secs(1));
+    }
+
+    #[test]
+    #[should_panic(expected = "period > 0")]
+    fn test_rate_limit_layer_new_rejects_zero_period() {
+        let _ = RateLimitLayer::new(10, Duration::ZERO);
+    }
+
+    #[test]
+    #[should_panic(expected = "requests > 0")]
+    fn test_rate_limit_per_second_rejects_zero() {
+        let _ = RateLimitLayer::per_second(0);
+    }
+
+    #[test]
+    #[should_panic(expected = "period > 0")]
+    fn test_rate_limit_with_min_delay_rejects_zero() {
+        let _ = RateLimitLayer::with_min_delay(Duration::ZERO);
+    }
+
     #[tokio::test]
     async fn test_rate_limit_enforces_rate() {
         // Service that returns immediately