`StateSnapshot::remaining_burst_capacity()` never "replenishes"

[hgmich]

StateSnapshot::remaining_burst_capacity() does not seem to replenish correctly, despite correct rate limiting behaviour being observed; once the initial burst quota is exceeded, it is permanently set at zero, even after the replenishment time has passed (and the rate limiter is not rejecting requests). Some tracing indicates that the calculation uses tat as if it were relative to the last "replenishment window", but it is always relative to the RateLimiter's start time, leading to the calculation being as if no time had passed since the first construction of the RateLimiter.

I will update this issue shortly with a reproducible test case.

[hgmich]

Test case reproducing/demonstrating issue:

#[test]
fn state_snapshot_tracks_quota_accurately() {
    use crate::clock::FakeRelativeClock;
    use crate::RateLimiter;
    use nonzero_ext::*;

    let clock = FakeRelativeClock::default();

    let lim = RateLimiter::direct_with_clock(Quota::per_minute(nonzero!(5_u32)), &clock)
        .with_middleware::<StateInformationMiddleware>();

    let state = lim.check_n(nonzero!(4_u32)).expect("Should not rate limit");
    assert_eq!(state.remaining_burst_capacity(), 1);

    let state = lim.check().expect("Should not rate limit");
    assert_eq!(state.remaining_burst_capacity(), 0);

    lim.check().expect_err("Should rate limit");

    clock.advance(Duration::from_secs(120));
    let state = lim.check().expect("Should not rate limit");
    assert_eq!(state.remaining_burst_capacity(), 4);
}

The expected behaviour should be that this test passes.

Instead, it fails at the last line as state.remaining_burst_capacity() is 0:

thread 'middleware::test::state_snapshot_tracks_quota_accurately' panicked at 'assertion failed: `(left == right)`
  left: `0`,
 right: `4`'[...]

[antifuchs]

I'm currently really busy with a move, but wanted to say thank you for this super-thorough bug report! I should be able to take a peek at why that is in 2 weeks or so, but if you are feeling inspired until then, feel free to fix the bug yourself & I'll happily merge!

[antifuchs]

Oh, hah, I had some time to look at this and it appears I under-tested StateSnapshot a bit: The structure also needs to store the old tat (or t0 of the measurement), so that it knows from what time on that next tat value is relevant - that allows for a more reasonable computation of remaining burst capacity (something on the order of quota.burst_size - ((next - old_tat) / t)).

Unfortunately, this is an incompatible change, as the From impl for StateSnapshot was not made to accommodate more than that one parameter; on the other hand, I'm not sure why anyone outside this crate ought to be able to construct a StateSnapshot on their own; I'm inclined to drop that From impl, which will make everything a lot easier.

[antifuchs]

Heads-up, I put up a draft fix for this in #108, but I'm not convinced everything's right yet: There seems to be an off-by-one error that I didn't investigate fully yet.

[antifuchs]

@holmesmr sorry this took so long - I finally merged the fix for this, and will hopefully be able to release a fixed version soon!

[artefom]

Experiencing the same issue. Hope to see fix for that soon :)

[antifuchs]

Oh man, I didn't realize I still hadn't released the fix. I'm so sorry for forgetting that. I'll merge the one pending PR that brings master up to date with clippy & will release asap.

[antifuchs]

0.5.0 is released now - sorry for the wait! (It's a minor version bump because there was one incompatible change, dropping a trait)