fix: concurrent cache misses share one backing request by lucasmcdonald3 · Pull Request #1711 · aws/aws-encryption-sdk-javascript

lucasmcdonald3 · 2026-10-06T20:07:23Z

Issue #, if available: #1663, #1665. Supersedes #1664.

Description of changes:

Concurrent cache misses could result in request spam to the upstream data source:

In the hierarchical keyring, N concurrent misses for one branch key made N keystore calls (Hierarchical Keyring: cold-cache stampede — N concurrent decrypts → N DynamoDB/KMS calls #1663).
In the caching CMM, N concurrent encrypts made N data keys instead of reusing one (Data key caching breaks under concurrency: N parallel encrypts → N data keys #1665).

Now, concurrent misses now share the same request to upstream.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

When many encrypt/decrypt operations run concurrently against a cold cache for the same branch key, each cache miss independently queried the keystore, firing N DynamoDB GetItem and N KMS Decrypt calls instead of one. getBranchKeyMaterials now shares a single in-flight request per cache entry id, evicting it on settle so the cryptographic materials cache keeps ownership of caching and TTL. A rejected request is evicted too, so the next call retries rather than sharing the failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Track in-flight branch key requests per cache, so keyrings sharing a cache share requests. Coalesce concurrent caching CMM misses the same way; waiting encrypts read through the cache so each use counts against the entry's limits. Co-authored-by: Yosef Bensimchon <yosef.bensimchon@xero.com>

Copy branch key materials before sharing them with waiting callers, so a concurrent eviction cannot zero them first. In the caching CMM, let waiters request in parallel when the response cannot serve them.

Port the MPL's StormTracker to the hierarchical keyring: refresh entries in their grace period, let another caller fetch after graceInterval, fail waiters after inFlightTTL, and limit concurrent fetches to fanOut. In the caching CMM, callers join an in-flight request only while it has room in its limits, so a burst runs its requests in parallel. Waiters time out after inFlightTTL and retry a failed request once.

…uest

Early refresh of cached branch keys is now an opt-in gracePeriod keyring option (seconds, default 0). With the default, a cached branch key is used until it expires and the keystore is not called, matching the behavior before storm tracking. When a keystore fetch fails, callers waiting on it now fail right away with the keystore's error instead of timing out after inFlightTTL with a generic error. Failed fetches no longer count toward fanOut, so failing branch keys cannot block fetches for other branch keys.

Remove a cache key's in-flight list once it is empty, and drop stuck requests nobody can join, so the map does not grow with every cache key ever used. Move SHARED_REQUESTS and the in-flight map to an internal module that the package index does not export.

Concurrent cache misses still share one backing request by default, but sharing no longer adds new failures or waits: - No timeout for waiting callers and no limit on requests in flight. - If the shared request fails, each waiting caller makes its own request, as it would without sharing. The new stormTracking option (graceInterval, fanOut, inFlightTTL, in milliseconds) opts in to MPL StormTracking-style behavior. A caller that times out is removed from the shared request, so the request no longer clones the data key for it or counts its message.

Concurrent misses for the same branch key share one keystore fetch by default. Waiters are notified when the fetch settles; on failure each waiter makes its own fetch. There is no fanOut limit, no waiter timeout, and no early refresh by default. Replace the unreleased top-level gracePeriod option with stormTracking (gracePeriod, graceInterval, fanOut, inFlightTTL, in seconds), which ports the MPL StormTracker with its defaults and validation rules. Also: - Track each fetch by identity or token, so a late fetch for an entry can't clear or fail a newer fetch for the same entry. - If an early refresh fails, use the still-valid cached branch key.

Make the stormTracking option docs JSDoc so they show up in editors, and fix a stale comment on the cached-entry lookup.

…ng type The 3000-caller hierarchical keyring tests can block the event loop past the shortened test graceInterval on slower CI hosts, which starts a second fetch. Run them with the default timings. Also skip fanOut counting in default mode, and export CachingMaterialsManagerStormTracking from the caching CMM packages.

A slow first call delays callers that joined it within graceInterval.

Concurrent cache misses still share one backing request, which fixes #1663 and #1665. The opt-in MPL StormTracking settings (gracePeriod, graceInterval, fanOut, inFlightTTL) are removed, so this change adds no public API. Storm tracking can come back as a cache option later.

If the backend returned a malformed response, the caller making the shared request threw before releasing the callers waiting on it, and they hung forever. Release them in a finally block so they make their own request, as they would without sharing.

Drop tests that duplicate other tests or guard storm tracking behavior that no longer exists. Add coverage for counting waiting callers against the cache entry and for the starting manager's limits, which no test caught when broken.

Waiting callers each made their own request when the shared one failed, so a throttled backend got one call per waiter. Waiters now coalesce into one retry; if it also fails, they get its error. Also give the 3000-operation keyring tests a long grace interval so a slow runner can't start a second fetch and flake the assertion.

Cut comments that restate the code, state each invariant once, and fix mid-line periods and hidden actors.

Move request sharing into cache-material's shared_requests so the caching CMM and the hierarchical keyring use one implementation. Rename graceInterval to inFlightTTL to match the MPL's StormTracker naming. The keyring now re-checks the cache before a shared retry.

This reverts commit 61f4442.

Yosef Bensimchon and others added 7 commits October 6, 2026 19:53

fix: share branch key copies and skip coalescing uncacheable responses

9fa9480

Copy branch key materials before sharing them with waiting callers, so a concurrent eviction cannot zero them first. In the caching CMM, let waiters request in parallel when the response cannot serve them.

fix(cache-material): coalesce only when an entry can serve two callers

f77502a

docs: clarify cache coalescing comments

84158b4

docs: clarify storm tracking and batching comments

866bb30

lucasmcdonald3 changed the title ~~fix: coalesce concurrent cache misses~~ fix: dedupe concurrent cache misses Oct 9, 2026

refactor: describe concurrent cache misses as sharing one backing req…

cefbdf9

…uest

lucasmcdonald3 changed the title ~~fix: dedupe concurrent cache misses~~ fix: concurrent cache misses share one backing request Oct 9, 2026

Lucas McDonald added 20 commits October 9, 2026 13:49

m

e207f7d

docs: clarify shared request comments

f3a8760

m

13ba7fa

docs: place shared request comments next to the code they describe

f5fcf98

docs: trim shared request comments

5ac6d25

docs: tighten shared request and stormTracking comments

4737df1

Make the stormTracking option docs JSDoc so they show up in editors, and fix a stale comment on the cached-entry lookup.

docs: drop the no-new-waits claim from stormTracking docs

f32dba0

A slow first call delays callers that joined it within graceInterval.

docs: state the no-added-failures rule once in shared request comments

f1e95f5

docs: drop the findBatch comment

712eb9a

docs: rewrite shared request comments one by one

2c9d878

docs: say what the over-limits check is for

ee71bcf

docs: rewrite shared request comments for a first-time reader

2ec4d0f

test: keep only tests that fail when sharing breaks

fc4dad0

Drop tests that duplicate other tests or guard storm tracking behavior that no longer exists. Add coverage for counting waiting callers against the cache entry and for the starting manager's limits, which no test caught when broken.

Lucas McDonald added 8 commits October 12, 2026 15:03

docs: trim shared_requests comments

22417fb

docs: rewrite shared request comments

8758dbb

Cut comments that restate the code, state each invariant once, and fix mid-line periods and hidden actors.

docs: state the retry rule instead of the flag

0941412

docs: say what releasing waiting callers does on failure

c0f1dec

docs: explain when the batch can exceed its limits

41814da

docs: replace jargon in shared request comments

4843825

docs: drop comment on the shared timing object

8eb86f1

lucasmcdonald3 marked this pull request as ready for review October 13, 2026 15:47

lucasmcdonald3 requested a review from a team as a code owner October 13, 2026 15:47

Lucas McDonald added 2 commits October 13, 2026 09:06

Revert "refactor: share one in-flight request helper"

b8547a6

This reverts commit 61f4442.

Navigation Menu

Repository navigation

fix: concurrent cache misses share one backing request - #1711

fix: concurrent cache misses share one backing request#1711
lucasmcdonald3 wants to merge 38 commits into
masterfrom
lucmcdon/coalesce-cache-misses

lucasmcdonald3 commented Oct 6, 2026 •

edited

Loading

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

lucasmcdonald3 commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Check any applicable:

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

lucasmcdonald3 commented Oct 6, 2026 •

edited

Loading