perf: sort engines once and bound the MountResolver cache (LRU) (#108)#116
Merged
Conversation
Two hot-path issues hit on every read/write/list/delete/update: - __enginesOverrideLookup re-sorted Object.entries(engines) on each resolve() although the map never changes after construction. The entries are now normalized and sorted once in the constructor (the map is snapshotted; post-construction mutation no longer affects resolution). - The mount cache was unbounded and linearly scanned with startsWith per resolve. It is now a bounded LRU (default 500 entries, opts.maxCacheSize) probed by segment-boundary prefix as exact keys, O(path depth) per lookup. Vault forbids nested mounts, so at most one cached mount can prefix a path and deepest-first probing is behavior-equivalent to the old scan. Hits refresh recency; inserts evict the oldest entries beyond the cap. The recursive collision-fallback logic is untouched, per the issue. Measured (200k worst-case resolves): engines lookup with 50 entries 1191ms -> 34ms; cache lookup with 500 cached mounts 1019ms -> 149ms. Tests: LRU eviction past the cap, recency refresh on hit, in-flight dedup with a bounded cache, engines-snapshot semantics, and the previously untested empty/null detection-response VaultError. Also verified end-to-end against a real Vault dev server (autoDetect v2, v1 passthrough, engines override) and confirmed regional STS signing works on the pinned aws4@1.8.0 (no update needed). Closes #108 Signed-off-by: Yuriy R <22548029+kurok@users.noreply.github.com>
89812ae to
fd6361e
Compare
m2broth
reviewed
Jul 2, 2026
Review follow-up: a negative opts.maxCacheSize was truthy, so it bypassed the || default and reached __cacheStore unchanged, where the eviction loop (size > cap) can never terminate once the map is empty — delete(undefined) cannot shrink it. Non-integer caps likewise made the bound meaningless. Reject anything but a positive integer at construction with InvalidArgumentsError; omitted still falls back to the default (500). Reproduced the hang (bounded repro: eviction loop ran 1000+ iterations on maxCacheSize=-1) before the fix; covered negative/zero/fractional/ NaN/Infinity/string caps plus the default-fallback path with tests. Signed-off-by: Yuriy R <22548029+kurok@users.noreply.github.com>
m2broth
approved these changes
Jul 2, 2026
9 tasks
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.
Summary
Fixes #108 (
priority: medium, performance). Two hot-path issues inMountResolver, both hit on every read/write/list/delete/update:__enginesOverrideLookupre-sortedObject.entries(engines)on eachresolve()although the map is immutable after construction.startsWithover every cached mount) per resolve — monotonic memory + scan growth in long-lived services against many/dynamic mounts.Measured (200k worst-case resolves, old vs new):
Changes
VaultClientconstruction no longer affects resolution.opts.maxCacheSize), per the issue's "a few hundred". Hits refresh recency; inserts evict the oldest beyond the cap. Sizing rationale (the issue's open question): typical deployments talk to a handful of static mounts, so 500 is generous headroom while still bounding the multi-tenant/dynamic-mount worst case.Map.getkeys: O(path depth) instead of O(cache size). Vault forbids nested mounts, so at most one cached mount can prefix a given path, making deepest-first probing behavior-equivalent to the old insertion-order scan.Tests (written red-first where behavior changed)
Unexpected empty detection responsethrow (empty and null response variants) — closing the issue's coverage gapMountResolver suite: 21 passing; full unit suite: 260 passing; lint clean.
Verification beyond unit tests
VaultClientend-to-end — autoDetect on a KV v2 mount, passthrough detection of a KV v1 mount on the same server, and anengines-override client. All round-trips correct.aws4@1.8.0; the existing IAM unit tests assert the regional STS signing behavior from 2.0.0 (regional Host header +/eu-central-1/sts/aws4_requestcredential scope) and pass on exactly that version — nonpm update aws4needed.Type of change
Checklist
npm run lint && npm testpasses locally (unit: 260 passing; e2e suites additionally verified against local dev servers in the prior PRs' setup)# UnreleasedinCHANGELOG.md(no version bump here to avoid colliding with open ci: enforce coverage thresholds and lint test files (#105) #114, which claims 2.0.4 — fold into whichever release ships next)Signed-off-by:trailer (git commit -s)