Skip to content

Fix exceeded() to fall back to configured interval when Retry-After is missing#28

Open
evan-burrell wants to merge 1 commit intosaloonphp:v2from
evan-burrell:fix/exceeded-fallback-to-release-interval
Open

Fix exceeded() to fall back to configured interval when Retry-After is missing#28
evan-burrell wants to merge 1 commit intosaloonphp:v2from
evan-burrell:fix/exceeded-fallback-to-release-interval

Conversation

@evan-burrell
Copy link

Summary

When Limit::exceeded() is called without a releaseInSeconds value (e.g. when a 429 response has no Retry-After header), the expiryTimestamp was not set explicitly. This caused it to be lazily recalculated from "now" on each call to getExpiryTimestamp(), which can drift and produce inconsistent state depending on when it's accessed.

This change makes exceeded() fall back to the limit's configured releaseInSeconds interval when no explicit value is provided, ensuring the expiry timestamp is always set deterministically at the moment the limit is exceeded.

Before

// Retry-After header missing → releaseInSeconds is null
$limit->exceeded(releaseInSeconds: null);

// expiryTimestamp is NOT set — lazy calculation from "now" on each access
$limit->getExpiryTimestamp(); // recalculates each time

After

// Retry-After header missing → releaseInSeconds is null
$limit->exceeded(releaseInSeconds: null);

// Falls back to the limit's configured interval (e.g. 60s for custom limiters)
// expiryTimestamp is set once, deterministically
$limit->getExpiryTimestamp(); // stable value

Changes

  • src/Limit.php: In exceeded(), fall back to $this->releaseInSeconds when $releaseInSeconds is null
  • tests/Unit/LimitTest.php: Added 3 tests covering the fallback behavior for regular limits, explicit values, and custom limiters

Test plan

  • All existing tests pass (84 passed, 0 failures)
  • New tests verify fallback to configured interval when releaseInSeconds is null
  • New tests verify explicit releaseInSeconds still takes precedence
  • New tests verify Limit::custom() defaults to 60s fallback

…igured interval

When `exceeded()` is called without a `releaseInSeconds` value (e.g. when
a 429 response has no Retry-After header), the expiry timestamp was not
set explicitly. This caused it to be lazily calculated from "now" on each
call to `getExpiryTimestamp()`, which can drift and lead to inconsistent
state.

Now `exceeded()` falls back to the limit's configured `releaseInSeconds`
when no explicit value is provided, ensuring the expiry timestamp is
always set deterministically at the moment the limit is exceeded.
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