Skip to content

feat(cacheable, memory): add maxTtl option to cap maximum time-to-live#1645

Merged
jaredwray merged 2 commits into
mainfrom
claude/upbeat-shannon-0P0py
May 27, 2026
Merged

feat(cacheable, memory): add maxTtl option to cap maximum time-to-live#1645
jaredwray merged 2 commits into
mainfrom
claude/upbeat-shannon-0P0py

Conversation

@jaredwray
Copy link
Copy Markdown
Owner

Summary

Adds a maxTtl option to both Cacheable and CacheableMemory that enforces an upper bound on any TTL in the cache. When set, per-entry TTLs exceeding maxTtl are capped, and entries with no TTL (that would otherwise live indefinitely) are also bounded. This is useful for guaranteeing that no cache entry outlives a maximum duration regardless of what TTL is passed to individual set() calls.

Changes

  • Add maxTtl option to CacheableMemoryOptions type and CacheableMemory class with getter/setter, constructor handling, and enforcement in set()
  • Add maxTtl option to CacheableOptions type and Cacheable class with getter/setter, constructor handling, and enforcement in set() and setMany() (both primary and secondary stores)
  • Add comprehensive test suites for both packages (100% coverage on memory package)
  • Update README documentation for both @cacheable/memory and cacheable packages with usage examples and API reference

Verification

  • pnpm build passes
  • All memory package tests pass (113/113, 100% coverage)
  • All non-Redis cacheable tests pass (120/120)
  • Lint passes (biome check --error-on-warnings)

Test plan

cd packages/memory && pnpm test   # 113 tests, 100% coverage
cd packages/cacheable && npx vitest run test/index.test.ts  # 120 pass (12 Redis-dependent skip without Redis)

Key scenarios tested:

  • maxTtl defaults to undefined
  • Setting via constructor (number and string shorthand)
  • Setting via property setter
  • Disabling by setting to undefined, 0, or negative
  • Capping TTL when it exceeds maxTtl
  • Not capping when TTL is within maxTtl
  • Enforcing on entries with no TTL
  • Enforcing when default TTL exceeds maxTtl
  • Working with shorthand strings (e.g., '1s', '1h')
  • Actual expiration at maxTtl boundary
  • Enforcement on setMany()
  • Enforcement on secondary store
  • Enforcement with SetOptions object (both ttl and expire fields)

https://claude.ai/code/session_01Gdiqk2rKNc1Vji61zm3qmy


Generated by Claude Code

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 new maxTtl option to both Cacheable and CacheableMemory to enforce an upper bound on cache entry lifetimes. The feedback highlights a performance optimization opportunity: rather than parsing shorthand TTL strings (using regex and date instantiations) on every set or setMany operation, the maxTtl value should be pre-parsed into milliseconds and stored as a private field (_maxTtlMs) when initialized or updated. This avoids overhead in the hot path of the cache operations.

Comment thread packages/cacheable/src/index.ts
Comment thread packages/cacheable/src/index.ts
Comment thread packages/cacheable/src/index.ts
Comment thread packages/cacheable/src/index.ts
Comment thread packages/memory/src/index.ts
Comment thread packages/memory/src/index.ts
Comment thread packages/memory/src/index.ts
@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 (1ff149d) to head (d777d3c).

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1645   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines         2644      2688   +44     
  Branches       581       594   +13     
=========================================
+ Hits          2644      2688   +44     

☔ 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

@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: c087656249

ℹ️ 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/cacheable/src/index.ts
Comment thread packages/cacheable/src/index.ts
Copy link
Copy Markdown
Owner Author

Code Review Response

An internal staff-engineer-grade code review was run against this branch. Here's each finding and the action taken:


⚠️ Major Findings

1. BEFORE_SET hook can bypass maxTtl on primary store (packages/cacheable/src/index.ts)

The BEFORE_SET hook could mutate item.ttl after the maxTtl cap was applied, and the uncapped value would be written to the primary store. The secondary store was correctly re-capped, making behavior inconsistent.

✅ Fixed in 3e14a6f. Added item.ttl = this.capTtl(item.ttl, maxTtlMs) after the hook call, before writing to the primary store. Added a test (should re-cap ttl after BEFORE_SET hook overrides it) confirming maxTtl holds even when hooks override the TTL.


2. Sync subscriber bypasses maxTtl (packages/cacheable/src/sync.ts)

Sync writes directly to Keyv via storage.set() without passing through Cacheable.set(), so a remote instance's TTL isn't re-capped locally.

⏭️ No change. The originating instance already caps the TTL via maxTtl before publishing the sync event, so the published TTL is already capped. If instances have different maxTtl configs, that's an intentional operational choice. Changing sync to route through Cacheable.set() would be architecturally significant (re-triggering hooks, risk of sync loops) and warrants a separate discussion if needed.


3. Commit hygiene — branch mixes unrelated changes

⏭️ False positive. The NodeCacheStore overhaul, tsdown migration, and CI changes are pre-existing commits on main (bf3ea48, 1508695) that appear in the three-dot diff. This branch contains only 2 commits: the maxTtl feature and the hook-bypass fix.


4. Extra get() on every set() for stats in NodeCacheStore

⏭️ Out of scope. This is pre-existing code from PR #1643 (NodeCacheStore enhancement), not part of the maxTtl feature.


💡 Minor Findings

5–6. shorthandToMilliseconds / shorthandToTime called on every set()

Regex parsing happens on each call when maxTtl is a string like '1h'.

⏭️ No change. The regex parse is a single match against a short pattern — negligible compared to the async store operations that follow. Pre-resolving would add complexity (shadow _maxTtlMs field kept in sync) for a micro-optimization unlikely to show in benchmarks. This is consistent with how the existing ttl property works throughout the codebase.


7. mset fans out 2N network calls in NodeCacheStore

⏭️ Out of scope. Pre-existing code from a different PR, not part of the maxTtl feature.


Coverage

Codecov confirms 100% project coverage maintained — all modified and coverable lines are covered by tests (+43 lines, +15 branches).


Generated by Claude Code

claude added 2 commits May 26, 2026 23:27
Adds a `maxTtl` option to both `Cacheable` and `CacheableMemory` that
enforces an upper bound on any TTL in the cache. When set, per-entry
TTLs exceeding maxTtl are capped, and entries with no TTL are also
bounded. Default is `undefined` (no maximum). Includes full test
coverage and documentation updates.

https://claude.ai/code/session_01Gdiqk2rKNc1Vji61zm3qmy
The BEFORE_SET hook can mutate item.ttl, which could bypass the maxTtl
enforcement. Now the TTL is re-capped after the hook runs, ensuring
maxTtl is always enforced regardless of hook modifications.

https://claude.ai/code/session_01Gdiqk2rKNc1Vji61zm3qmy
@jaredwray jaredwray force-pushed the claude/upbeat-shannon-0P0py branch from 3e14a6f to d777d3c Compare May 26, 2026 23:28
@jaredwray jaredwray merged commit 948234a into main May 27, 2026
12 checks passed
@jaredwray jaredwray deleted the claude/upbeat-shannon-0P0py branch May 27, 2026 00:37
@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