feat: expire FUP entries as soon as the limited periods allow - #11
Open
janvrska wants to merge 1 commit into
Open
feat: expire FUP entries as soon as the limited periods allow#11janvrska wants to merge 1 commit into
janvrska wants to merge 1 commit into
Conversation
FUP entries were kept for 35 days of inactivity - the longest period the
package supports - regardless of what the scope actually limits by. For a key
with an unbounded key space that is the difference between holding a key for
two days and for five weeks: a per-IP scope of {"minutely": 60, "daily": 5000}
needs two days, and every source IP that is seen once mints its own key.
The TTL is now derived from the scope being enforced and handed to the driver
through contract.FUPTTLCacheDriverInterface, an optional addition to
CacheDriverInterface - a driver that doesn't implement it keeps being used
through IncrementFUPEntry, with constants.FUPEntryTTL as before, so this is not
a breaking change. Both bundled drivers implement it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #10, which gave FUP entries an expiration for the first time (they never expired before). The value there is
constants.FUPEntryTTL, 35 days — slightly more than the longest period the package supports — and it is used for every entry regardless of what the scope actually limits by.That is more than most scopes need. The expiration is refreshed on every increment, so an entry only has to outlive the gap between two requests that share a period: a scope of
{"per-ip": {"minutely": 60, "daily": 5000}}needs two days, not five weeks. It matters because #10 also made the anonymous FUP key a per-IP one, i.e. a key space bounded by the number of source IPs rather than by the number of clients — in our deployment, one unauthenticated source was measured at 490 requests/minute, where the difference is roughly 5 GB and 300 MB of Redis on an instance shared with ten other services.What changed
constants.Period.GetEntryTTL()— how long an entry has to survive inactivity to keep counting that period correctly (the period plus a margin; monthly stays atFUPEntryTTL).contract.FUPScope.GetEntryTTL(path)— the longest such TTL the given scope path needs. A path the scope doesn't limit falls back toFUPEntryTTL, which covers every period.contract.FUPTTLCacheDriverInterface— an optional addition toCacheDriverInterfacewith one method,IncrementFUPEntryWithTTL(key, ttl).fup.checkLimitsuses it when the driver implements it and falls back toIncrementFUPEntryotherwise.IncrementFUPEntrynow delegates withconstants.FUPEntryTTL, so its behaviour is unchanged.Not a breaking change
A driver written against v3.0.0 keeps working through
IncrementFUPEntryand keeps 35 days for every entry — correct, just less frugal. I deliberately did not extendCacheDriverInterfaceitself: v3.0.0 already cost consumers a/v2→/v3import rewrite, and this doesn't justify another major.One sharp edge worth knowing about, documented at the interface, in the README and in the changelog: a custom driver that wraps a bundled one has to override both increments, or the embedded implementation stays in use for the TTL-aware one. The test suite here caught exactly that —
brokenFUPCacheDriverembeds*MemoryCacheDriverand had silently kept counting.Tests
Period.GetEntryTTLfor every period, each asserted to outlive the period it counts, and an unknown period falling back toFUPEntryTTL.FUPScope.GetEntryTTL: longest period wins, negative limits (no limitation) are not counted, another path doesn't extend this one, unlimited path and empty scope fall back.IncrementFUPEntrystill getsFUPEntryTTL.checkLimits: the TTL handed to the cache comes from the scope, and a driver that only implementsCacheDriverInterfaceis still used and still enforces its limits.Full suite green under
-race,go vetclean,gofmtclean.