Skip to content

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

Merged
Sammyjo20 merged 3 commits intosaloonphp:v2from
evan-burrell:fix/exceeded-fallback-to-release-interval
Mar 25, 2026
Merged

Fix exceeded() to fall back to configured interval when Retry-After is missing#28
Sammyjo20 merged 3 commits intosaloonphp:v2from
evan-burrell:fix/exceeded-fallback-to-release-interval

Conversation

@evan-burrell
Copy link
Contributor

@evan-burrell evan-burrell commented Mar 19, 2026

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
  • tests/Pest.php: Added toLookLike custom Pest expectation that tolerates ±1 second timestamp drift
  • tests/Feature/HasRateLimitsTest.php: Replaced exact timestamp assertions with toLookLike to fix flaky CI failures on slow runners

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
  • Flaky timestamp assertion on Windows CI fixed with tolerant toLookLike expectation

…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.
Copy link
Contributor

@JonPurvis JonPurvis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for this @evan-burrell

Copy link
Member

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Add a `toLookLike` custom expectation that tolerates ±1 second drift
on timestamps, preventing flaky failures when a second boundary is
crossed between capturing the expected time and executing the request.
@evan-burrell evan-burrell requested a review from Sammyjo20 March 25, 2026 10:11
@evan-burrell
Copy link
Contributor Author

@Sammyjo20 Just added an extended expectation for clock based tests that might be flaky

Copy link
Member

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@Sammyjo20 Sammyjo20 merged commit f3e7700 into saloonphp:v2 Mar 25, 2026
26 checks passed
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.

3 participants