feat(hooks): add TTL HookCache and persistent hook schema store - #655
feat(hooks): add TTL HookCache and persistent hook schema store#655gemammercado wants to merge 5 commits into
Conversation
| export const PersistedStores: ReadonlyArray<StoreName> = [ | ||
| StoreName.public_schemas, | ||
| StoreName.sam_schemas, | ||
| StoreName.hook_schemas, |
There was a problem hiding this comment.
Should this be persisted? We don't persist private customer data on disk. Will it change with credentials/regions?
There was a problem hiding this comment.
Moved to Persistence.memory and reverted the PersistedStores change.
On credentials/regions: yes, this was a bug. Records were keyed hook:${typeName} with no account or region qualifier, so switching profile or region would read back the previous account's schema under the same type name. On disk that survived restarts, and the staleness threshold was in days.
On privacy: CfnService.listHooks() filters Visibility: PRIVATE, so this store was fed almost entirely by hooks the customer registered into their own account. Same category as private_schemas, which is already memory-only here.
| } | ||
|
|
||
| export class TtlCache<T> { | ||
| private readonly entries = new Map<string, CacheEntry<T>>(); |
There was a problem hiding this comment.
How large can these caches get?
There was a problem hiding this comment.
Unbounded as written, added a 100-entry cap per cache and oldest-first eviction
| get size(): number { | ||
| let count = 0; | ||
| for (const entry of this.entries.values()) { | ||
| if (entry.expiresAt > this.now()) { |
There was a problem hiding this comment.
If the whole map is being iterated, could just prune here as well
There was a problem hiding this comment.
size now calls prune()
| expiresAt: number; | ||
| } | ||
|
|
||
| export class TtlCache<T> { |
There was a problem hiding this comment.
Doesn't need to be exported
There was a problem hiding this comment.
un-exported. It was only exported so the unit tests could drive it directly
Keep private hook schemas in memory rather than on disk. listHooks filters to PRIVATE visibility, so these records are customer-authored type names and config fields, matching the existing treatment of private_schemas. The records were also keyed by type name alone with no account or region qualifier, so a persisted record could be served after a profile or region switch. Bound both hook caches. Keys derive from activated hook count rather than user activity, but nothing in TtlCache enforced that, and rule bodies fetched from S3 are unbounded in size. Entries are now capped and the oldest is evicted at the cap. Prune expired entries while reporting size rather than counting them, stop exporting TtlCache now that no consumer outside this module uses it, and drop its unused set method. Tests exercise the cache through HookCache. Add HookSchemaStore unit tests, including a guard that the store is requested with memory persistence.
Two callers asking for the same key before the first load resolves share one call. That depends on routing loads through fetch with a fetchMethod rather than a get and set pair, so it is a property of this wrapper and not of lru-cache alone.
d32c558 to
7a3d54c
Compare
PR 1 of 7 — foundation (stacked)
This is the first slice of the CloudFormation Hooks feature, split into 7 stacked PRs for reviewability. It adds the low-level building blocks that every later slice depends on. Nothing is wired into the server yet — this PR is pure foundation.
What this adds
HookCache. Memoises hook configurations and Guard rule bodies, so a featurethat repeatedly inspects the same hook doesn't re-issue a CloudFormation or S3 call
each time. Configurations and rule bodies live in separate namespaces, each backed
by
lru-cachewith a TTL and a maximumentry count. Concurrent loads of the same key are coalesced into a single call.
Params/Result/Requestdefinitions for everyhook-related LSP message. This is the contract the handlers and client use in later
slices.
What changed in this revision
Addressing @satyakigh's review. Three of the four comments are resolved by removing
code rather than changing it, so the corresponding inline comments will show as
outdated.
Persisting hook schemas. Correct on both counts, and the second was a latent bug.
CfnService.listHooks()filters toVisibility: PRIVATE, so those records are hooktype names and configuration fields authored inside the customer's own account —
the same category as
private_schemas, which is already memory-only here. They werealso keyed by type name alone with no account or region qualifier, so a persisted
record could be served after a profile or region switch, and on disk that survived
restarts with a staleness window measured in days.
Rather than move it to memory,
HookSchemaStoreis removed entirely. Oncepersistence went away, its only distinguishing feature over the schema cache
HooksManageralready keeps in memory was a staleness window, which an lru-cachettlexpresses directly. The schema cache is therefore better introduced alongsideHooksManagerthan as a separate store here. That also drops thehook_schemasstore, so
DataStore.tsis no longer touched by this PR at all.Cache size. Unbounded as written. In practice neither cache was keyed by user activity — configuration keys come from the hooks activated in the account, and rule keys from the rule URIs those configurations reference, so the key count tracked hook count rather than session length. But that was a property of the callers, not of the cache, and rule bodies read from S3 have no inherent size limit. Both namespaces are
now capped (100 entries each by default) with LRU eviction.
Pruning while iterating in
size. Thesizeaccessor no longer exists. lru-cache owns staleness now, so there is no second traversal to fold in.TtlCachedidn't need exporting. Correct — nothing outside the module used it.The hand-rolled
TtlCacheis deleted outright in favour of lru-cache, which suppliesTTL expiry, the entry bound, LRU eviction and request coalescing.
HookCacheis theonly export.
New dependency
lru-cachebecomes a direct dependency, pinned exactly to match the other productiondependencies. It was already in the tree transitively via
path-scurryand isrecorded in
THIRD-PARTY-LICENSES.txtandsbom/sbom.csv.BlueOak-1.0.0, where the transitivev10.4.3 was ISC — Isaac relicensed at v11. Blue Oak is permissive and OSI-approved,
but it falls outside the set
ATTRIBUTION_README.mdtreats as routine(
MIT, Apache-2.0, BSD-2-Clause, BSD-3-Clause, ISC, 0BSD), so it will surface in thelicense report and wants a conscious sign-off. v10.4.3 is the ISC alternative, but it
lacks the
perfoption this PR uses to inject a clock in tests. SBOM and attributionstill need regenerating.
Testing
Tests cover what this wrapper contributes rather than re-testing lru-cache: separate
namespaces for configurations and rules, invalidation across both, that the
ttlandmaxEntriesoptions reach each namespace, that each namespace is boundedindependently rather than sharing one budget, that rejected loads stay uncached, and
that concurrent loads of one key are coalesced. Build, lint and unit tests pass.