Skip to content

fix(limits): Fixes an attempt to subtract with overflow panic in Rate::maybe_reset() during concurrent access when overflow checks are enabled - #1107

Closed
deepak-pandey2005 wants to merge 1 commit into
cloudflare:mainfrom
deepak-pandey2005:test/rate-concurrent-reset
Closed

deepak-pandey2005 wants to merge 1 commit into
cloudflare:mainfrom
deepak-pandey2005:test/rate-concurrent-reset

Conversation

@deepak-pandey2005

Copy link
Copy Markdown

Summary

  • Prevent integer underflow panics in Rate::maybe_reset() during concurrent access when overflow checks are enabled (e.g. in debug builds and test profiles).
  • Use saturating_sub for elapsed time calculations in maybe_reset to safely handle timestamp differences across racing threads.
  • Add a multithreaded regression test (test_concurrent_reset_does_not_panic) in pingora-limits.

Motivation

In Rate::maybe_reset():

let now = Instant::now().duration_since(self.start).as_millis() as u64;
let last_reset = self.last_reset_time.load(Ordering::SeqCst);
let past_ms = now - last_reset;

When multiple threads concurrently call observe() or rate():

  1. Thread A samples now.
  2. Thread B wins compare_exchange and advances self.last_reset_time to its newer timestamp.
  3. Thread A then loads last_reset, which is now strictly greater than its sampled now (now < last_reset).
  4. Evaluating now - last_reset panics with:
    thread '<unnamed>' panicked at pingora-limits/src/rate.rs:147:23:
    attempt to subtract with overflow
    

Additionally:

  • Line 167: if now - last_reset >= self.reset_interval_ms * 2 can underflow for the same reason.
  • Line 174: assert!(new >= now - 1000) panics with underflow if now < 1000 ms (within the first second of process startup).

Design

  • saturating_sub for elapsed time: Replacing now - last_reset with now.saturating_sub(last_reset). When another thread has already won CAS and advanced last_reset_time past now, past_ms safely evaluates to 0, which is < reset_interval_ms. The calling thread correctly treats the reset as already performed and continues without panicking.
  • Bounded CAS check: Changing the assertion to assert!(new >= now.saturating_sub(1000)) avoids underflowing during early process uptime.
  • No API or structural changes: Keeps the existing lock-free CAS state machine unchanged; only replaces bare subtractions with their saturating counterparts.

Cost

  • Zero overhead: compiling to standard saturating subtraction instructions; no locking or allocations added.

Tests

  • Added test_concurrent_reset_does_not_panic: spawns 8 threads continuously hammering observe() and rate() concurrently for 500ms over 1ms reset intervals.
  • Without the fix, this test reliably triggers the panic within ~10ms.
  • With the fix, all 11 unit tests in pingora-limits pass cleanly:
    cargo test -p pingora-limits
    cargo clippy -p pingora-limits --tests

When multiple threads concurrently call maybe_reset, last_reset_time
can be updated by another thread after now is sampled, causing now < last_reset.
In debug builds or with overflow checks enabled, now - last_reset panics with
'attempt to subtract with overflow'. Similarly, now - 1000 can underflow when
now < 1000ms.

Use saturating_sub to safely handle timestamp differences without panicking,
and add a concurrent regression test.
@deepak-pandey2005
deepak-pandey2005 deleted the test/rate-concurrent-reset branch November 4, 2026 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant