Skip to content

feat(@cacheable/memory): add hooks like cacheable#1644

Merged
jaredwray merged 1 commit into
mainfrom
claude/friendly-ritchie-sI8gP
May 26, 2026
Merged

feat(@cacheable/memory): add hooks like cacheable#1644
jaredwray merged 1 commit into
mainfrom
claude/friendly-ritchie-sI8gP

Conversation

@jaredwray
Copy link
Copy Markdown
Owner

Summary

  • Adds CacheableMemoryHooks enum with 14 hooks: BEFORE/AFTER for set, setMany, get, getMany, delete, deleteMany, and clear
  • Integrates hookSync() calls into all CacheableMemory operations, matching the hook pattern from the cacheable package
  • BEFORE hooks receive mutable parameters (key, value, ttl) so consumers can intercept and modify operations before they execute

Hook signatures

Hook Parameter
BEFORE_GET key: string
AFTER_GET { key, result }
BEFORE_GET_MANY keys: string[]
AFTER_GET_MANY { keys, result }
BEFORE_SET { key, value, ttl } (mutable)
AFTER_SET { key, value, ttl }
BEFORE_SET_MANY items: CacheableItem[]
AFTER_SET_MANY items: CacheableItem[]
BEFORE_DELETE key: string
AFTER_DELETE key: string
BEFORE_DELETE_MANY keys: string[]
AFTER_DELETE_MANY keys: string[]
BEFORE_CLEAR (none)
AFTER_CLEAR (none)

Test plan

  • BEFORE_SET / AFTER_SET hooks fire and allow value mutation
  • BEFORE_SET hook can modify ttl
  • BEFORE_SET hook can modify the key
  • BEFORE_SET_MANY / AFTER_SET_MANY hooks fire with items array
  • BEFORE_GET / AFTER_GET hooks fire with correct key and result
  • AFTER_GET fires with undefined result on cache miss
  • AFTER_GET fires with undefined result on expired item
  • BEFORE_GET_MANY / AFTER_GET_MANY hooks fire with correct keys and results
  • BEFORE_DELETE / AFTER_DELETE hooks fire
  • BEFORE_DELETE_MANY / AFTER_DELETE_MANY hooks fire
  • BEFORE_CLEAR / AFTER_CLEAR hooks fire
  • All existing tests pass (106 tests, 100% coverage)

https://claude.ai/code/session_016g93vFCCgE3LPqYRYJWzCG


Generated by Claude Code

Add CacheableMemoryHooks enum and hookSync invocations to CacheableMemory
for get, getMany, set, setMany, delete, deleteMany, and clear operations.
BEFORE hooks allow mutation of parameters (key, value, ttl) before the
operation executes, matching the cacheable package's hook pattern.

https://claude.ai/code/session_016g93vFCCgE3LPqYRYJWzCG
@codecov
Copy link
Copy Markdown

codecov Bot commented May 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (bf3ea48) to head (fa65181).

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1644   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines         2608      2644   +36     
  Branches       577       582    +5     
=========================================
+ Hits          2608      2644   +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive hook system (CacheableMemoryHooks) to CacheableMemory, triggering synchronous hooks before and after major cache operations (get, set, delete, clear). It also adds corresponding unit tests to verify hook behaviors. The reviewer feedback recommends wrapping primitive parameters (like keys and arrays) in mutable objects (e.g., { key } or { keys }) across the hook triggers. This ensures consistency with other hooks and enables listeners to mutate keys (e.g., for prefixing or namespacing) during read and delete operations.

Comment thread packages/memory/src/index.ts
Comment thread packages/memory/src/index.ts
Comment thread packages/memory/src/index.ts
Comment thread packages/memory/src/index.ts
Comment thread packages/memory/src/index.ts
Comment thread packages/memory/test/index.test.ts
Comment thread packages/memory/test/index.test.ts
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa651813dc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/memory/src/index.ts
Comment thread packages/memory/src/index.ts
Comment thread packages/memory/src/index.ts
Copy link
Copy Markdown
Owner Author

Code Review Findings

Ran a max-effort code review (5 angles + verification + sweep). 6 findings survived verification. Ranked by severity:


1. onceHook / prependOnceHook are broken with hookSync (High)

File: packages/memory/src/index.ts (all hookSync call sites)

Hookified's onceHook() wraps the user's handler in an async function internally. hookSync() checks handler.constructor.name === "AsyncFunction" and silently skips it. This means:

  • cache.onceHook(CacheableMemoryHooks.BEFORE_SET, handler) — handler never fires
  • The handler is never removed from the hooks map (since it never executes the removal logic)
  • This is a permanent silent memory leak

This is a Hookified-level issue but directly affects anyone using CacheableMemory hooks. Worth documenting that onceHook should not be used with this class, or switching the affected methods to use async hook().


2. typeof null === "object" — hook setting ttl = null crashes set() (High)

File: packages/memory/src/index.ts, line 426

If a BEFORE_SET hook sets hookItem.ttl = null:

  1. null !== undefinedtrue, outer guard passes
  2. typeof null === "object"true, enters SetOptions branch
  3. effectiveTtl.expireTypeError: Cannot read properties of null

The typeof check was pre-existing, but hooks make this path newly reachable from user-supplied code. A null guard before the object branch would fix this:

if (effectiveTtl !== null && typeof effectiveTtl === "object") {

3. BEFORE_SET hook cannot suppress instance-level default TTL (Medium)

File: packages/memory/src/index.ts, line 442

If a cache has { ttl: '1h' } and a BEFORE_SET hook sets hookItem.ttl = undefined to make an item permanent:

const effectiveTtl = hookItem.ttl; // undefined
// undefined !== undefined || this._ttl !== undefined → true (second clause)
shorthandToTime(effectiveTtl ?? this._ttl); // shorthandToTime('1h')

The ?? operator falls through to this._ttl, overriding the hook's intent. This differs from the cacheable package where BEFORE_SET receives the already-resolved TTL, so setting it to undefined actually works.


4. AFTER_SET_MANY payload doesn't reflect per-item BEFORE_SET mutations (Medium)

File: packages/memory/src/index.ts, lines 478–484

setMany() passes the original items array to both BEFORE_SET_MANY and AFTER_SET_MANY. But each this.set() call creates a new hookItem object from the item's properties — so BEFORE_SET mutations (key prefixing, value transforms) apply to what's stored but are invisible in the items array passed to AFTER_SET_MANY. Observers of AFTER_SET_MANY see stale data.


5. AFTER_SET hook receives raw {key, value, ttl} without the resolved expires timestamp (Medium)

File: packages/memory/src/index.ts, line 470

The AFTER_SET hook receives hookItem = { key, value, ttl } where ttl is the raw input (e.g., "1h"). The resolved expires epoch timestamp is computed locally and written to the store item but never exposed to the hook. An AFTER_SET observer that needs to know when the item actually expires has no access to this information without re-parsing the TTL.


6. LRU eviction inside set() fires DELETE hooks between BEFORE_SET and AFTER_SET (Low)

File: packages/memory/src/index.ts, line 461

When LRU eviction triggers during set(), the hook sequence is:

BEFORE_SET → BEFORE_DELETE(evicted) → AFTER_DELETE(evicted) → AFTER_SET

A BEFORE_DELETE hook has no way to distinguish an LRU eviction from an explicit delete() call. This could cause false audit entries, spurious side effects (replication, metrics), or re-entrant state issues if a DELETE hook interacts with the cache mid-SET.


Notes

  • Findings 1 and 2 are actionable bugs worth fixing before merge.
  • Finding 3 is a behavioral divergence from cacheable that could be addressed by resolving TTL before creating hookItem.
  • Findings 4–6 are design trade-offs worth documenting but not necessarily blocking.

Generated by Claude Code

@jaredwray jaredwray merged commit 1ff149d into main May 26, 2026
12 checks passed
@jaredwray jaredwray deleted the claude/friendly-ritchie-sI8gP branch May 26, 2026 23:25
@jaredwray jaredwray mentioned this pull request May 27, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants