feat(cache): add optional maxEntries LRU eviction to InMemoryCacheAdapter#410
Merged
Merged
Conversation
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.
Closes #386.
This is less a new feature than a documentation promise being made true.
docs/cache-adapters.mdhas been telling users that attl: undefinedentry is stored"until explicitly deleted or evicted by LRU" — but
InMemoryCacheAdapterhad no evictionof any kind, and no cap. An unbounded
Mapin a long-lived process whose keys areper-wallet or per-guild grows with the user base; that is a slow memory leak wearing a
cache's clothes.
The trap this issue is really about
In JavaScript,
map.set(k, v)on a key that already exists keeps its original insertionposition. So the obvious way to refresh recency — just
setit again — leaves the iterationorder as FIFO, and the simple test cases still pass. The eviction only misbehaves in the
case that actually matters: reading an old key and expecting it to survive.
I measured both implementations against the same scenario (
maxEntries: 2; seta, setb, geta, setc):refreshes recency on read, so a re-read entry outlives a newer oneis the test thatseparates them; every other eviction test passes under both. Worth looking at that one
first.
A second ordering detail: inside
get, the TTL sweep runs before the recency refresh,so an expired entry is dropped rather than promoted to most-recently-used.
Design decisions worth reviewing
The recency refresh in
getis gated onmaxEntries !== undefined. Without a cap,mutating the
Mapon every read is pure cost for no benefit. More importantly it makes the"identical behaviour when
maxEntriesis omitted" criterion demonstrable rather thanapproximate — the bookkeeping does not execute at all on that path.
setre-inserts unconditionally, cap or no cap: overwriting a key counts as recent use,and keeping the invariant in one place is cheaper than branching.
evictIfNeedednever throws — it runs on the write path, where a failure would break anAPI call the cache exists only to accelerate. The constructor does throw on an invalid
maxEntries: that is user configuration, not the eviction path, and swallowingmaxEntries: -5silently would be worse. This follows the existing convention athttpClient.ts:114, which throwsGuildPassConfigError/INVALID_CONFIGfor an invalidretryableStatuses.size()counts entries whose TTL has elapsed but which have not been swept yet. Expiryis lazy and happens on read, so an untouched expired entry keeps its slot until it is read,
overwritten, or evicted. Calling this out explicitly because it looks like a bug if you do
not know the adapter is lazily expired — it is documented on the method and in
docs/cache-adapters.md.Notes on the issue text and the file layout
The issue refers to
src/cache/InMemoryCacheAdapter.ts. The class actually lives insrc/cache/cache.types.ts:79— there is no separate file, andsrc/cache/contains onlythat module. The issue hedges with "or equivalent", so this is a heads-up rather than a
correction.
InMemoryCacheAdapterOptionsis exported so consumers can type their options. No change tosrc/index.tswas needed: line 6 already doesexport * from './cache/cache.types'.CacheAdapteritself is untouched.size()is on the class, not the interface — addingit to the interface would break every third-party adapter.
API report
api-report/guildpass-sdk.api.mdwas regenerated withpnpm build && pnpm api-report, nothand-edited. The diff is exactly the three additions:
export class InMemoryCacheAdapter implements CacheAdapter { + constructor(options?: InMemoryCacheAdapterOptions); clear(): Promise<void>; ... set<T>(key: string, value: T, ttl?: number): Promise<void>; + size(): number; +} + +// @public +export interface InMemoryCacheAdapterOptions { + maxEntries?: number; }I deliberately left
temp/guildpass-sdk.api.mdout of this commit.pnpm api-reportregenerates it too, but the committed copy is already stale against upstream's own source,
so regenerating produces ~450 lines of drift unrelated to this change. The gated artifact is
api-report/guildpass-sdk.api.md—scripts/check-api-diff.mjs:18names it explicitly andnothing in the tooling reads
temp/.Tests
+8, in a sibling
describenext to the existingInMemoryCacheAdapter specific detailsblock. No existing test modified.
Eviction order and
size(); recency on read (the FIFO discriminator above); recency onoverwrite; unbounded behaviour with 1000 entries when
maxEntriesis omitted; TTL × LRUinteraction — an expired entry is swept rather than promoted,
size()drops, and the freedslot is reused instead of evicting a live sibling; the
maxEntries: 1edge case, where awrite must not evict the entry it just made; constructor rejection of
0,-5and1.5;and
deleteByPrefixstill working with a cap active, since that is the pathGuildPassClient.invalidateWalletCachedepends on and a regression there would silentlystop cache invalidation.
Verification
pnpm buildexits 0. ESLint reports 0 errors on the touched files (src/cache/cache.types.tsis warning-free; the 27
no-explicit-anywarnings intests/cache.test.tsare allpre-existing, on lines well below the added block).
Pre-existing failures, not attributable to this PR
Identical before and after:
tests/api-report-diff.test.ts— collection error: it imports../scripts/lib/api-report-diff.mjs, which does not exist in the repo.tests/compat/edge/edge.test.ts— 4 assertions under@edge-runtime/vm.Con esto cierras los tres. Dime el número del PR cuando lo abras.
✻ Baked for 1m 19s
❯ $ git push -u origin feat/386-lru-eviction
Enumerating objects: 23, done.
Counting objects: 100% (23/23), done.
Delta compression using up to 12 threads
Compressing objects: 100% (10/10), done.
Writing objects: 100% (12/12), 5.14 KiB | 1.71 MiB/s, done.
Total 12 (delta 9), reused 0 (delta 0), pack-reused 0 (from 0)
remote: Resolving deltas: 100% (9/9), completed with 9 local objects.
remote:
remote: Create a pull request for 'feat/386-lru-eviction' on GitHub by visiting:
remote: https://github.com/XxHugheadxX/guildpass-sdk/pull/new/feat/386-lru-eviction
remote:
To https://github.com/XxHugheadxX/guildpass-sdk
branch 'feat/386-lru-eviction' set up to track 'origin/feat/386-lru-eviction'.