feat: add response caching for read-only API calls#330
Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Trace
Other
Bug Fixes 🐛Api
Formatters
Setup
Other
Documentation 📚
Internal Changes 🔧Api
Other
🤖 This preview updates automatically when you update the PR. |
Codecov Results 📊✅ 2668 passed | Total: 2668 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 93.36%. Project has 3156 uncovered lines. Files with missing lines (21)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 82.43% 82.69% +0.26%
==========================================
Files 126 127 +1
Lines 17792 18234 +442
Branches 0 0 —
==========================================
+ Hits 14666 15078 +412
- Misses 3126 3156 +30
- Partials 0 0 —Generated by Codecov Action |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
BugBot #9 (corrupted cache entry) — fixed in 85a119d. Wrapped
BugBot #10 (async clearAuth not awaited) — fixed in 85a119d. Added |
|
Re: login.ts — don't mix await with .catch() (thread PRRT_kwDOQm6jAs5yDJFz) Fixed in 2cee148. Both occurrences replaced with |
|
Re: whoami.ts — factory/wrapper for fresh flag boilerplate (thread PRRT_kwDOQm6jAs5yDJvW) Fixed in 2cee148. Exported import { applyFreshFlag, FRESH_ALIASES, FRESH_FLAG } from "../../lib/list-command.js";
// parameters:
flags: { ..., fresh: FRESH_FLAG },
aliases: { ..., ...FRESH_ALIASES },
// func():
applyFreshFlag(flags);
|
|
Re: response-cache.ts — immutable TTL too short (thread PRRT_kwDOQm6jAs5yDLi1) Fixed in 2cee148. Bumped immutable tier from 1 hour → 24 hours. Events and traces are write-once, so caching them for a day is safe. The |
|
Re: response-cache.ts — URL tier patterns restructure (threads PRRT_kwDOQm6jAs5yDMZM, PRRT_kwDOQm6jAs5yDMoU, PRRT_kwDOQm6jAs5yDMyc) All three addressed in 2cee148:
|
|
Re: response-cache.ts — assorted cleanup (threads #7–#11) All addressed in 2cee148:
|
|
Re: response-cache.ts — calculate expiresAt for cheaper cleanup (thread PRRT_kwDOQm6jAs5yDSK3) Fixed in 2cee148. Added Kept the |
|
Re: response-cache.ts — parallelize I/O with concurrency limiter (threads #13–#15) All three addressed in 2cee148. Added a
Used a lightweight inline |
|
Re: issue/list.ts — Does FRESH_ALIASES override f: "follow"? (thread PRRT_kwDOQm6jAs5yD7C7) No conflict. Verified that only |
|
Re: response-cache.ts — cacheHeuristic vs CLEANUP_PROBABILITY (thread PRRT_kwDOQm6jAs5yD94G) These are completely unrelated despite having the same numeric value (0.1):
Same number, different domain — no shared constant makes sense. |
|
Re: response-cache.ts — parallel() simplification & eviction await (threads PRRT_kwDOQm6jAs5yD_wj, PRRT_kwDOQm6jAs5yEAVl) Both addressed in c3c622a: Simplified parallel(): Dropped the custom const cacheIO = pLimit(CACHE_IO_CONCURRENCY);
// At each call site:
await cacheIO.map(items, fn);Best-effort eviction: Both |
|
Re: response-cache.ts — Sentry instrumentation (thread PRRT_kwDOQm6jAs5yEBHK) Added in c3c622a:
This lets us see cache hit/miss rates and latency impact in the Sentry performance dashboard. |
BYK
force-pushed
the
feat/response-cache
branch
2 times, most recently
from
8d13fd1 to
21db140
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Implement RFC 7234/9111-aware HTTP response caching for GET requests to the Sentry API. Caching is transparent and integrated at the fetch level inside createAuthenticatedFetch(). Cache design: - Filesystem-based at ~/.sentry/cache/responses/ with JSON files keyed by SHA-256 hash of normalized URL + Authorization header - Uses http-cache-semantics for standards-compliant freshness evaluation - Tiered TTL system: immutable (1hr), stable (5min), volatile (60sec), no-cache (0sec) based on URL pattern matching - Probabilistic cleanup (10% chance per write) with 500-file LRU cap - Only successful (2xx) responses are cached Integration: - --refresh / -r flag added to all read commands (issue list/view/explain/ plan, project list/view, org list/view, event view, trace list/view/logs, log list/view, auth whoami/status, team list, repo list) - SENTRY_NO_CACHE=1 env var for global cache bypass - Cache cleared on login and logout for security - Autofix/root-cause polling endpoints excluded from caching Testing: - 18 unit tests for cache operations, TTL tiers, cleanup, and edge cases - 15 property-based tests for URL classification, key normalization, TTL bounds, and round-trip invariants - 4 additional integration-style tests Closes #318
…lassification and auth headers - Fix issue detail URLs (/issues/12345/) misclassified as 'stable' (5min) instead of 'volatile' (60sec). Broadened pattern to match all /issues/ URLs. - Fix cache ignoring Authorization headers for Vary-correctness. Thread current auth token into tryCacheHit() and refreshed token into cacheResponse() via new authHeaders() helper. - Extract fetchWithRetry() to keep authenticatedFetch complexity under 15. - Rename --refresh flag to --fresh with -f alias across all 16 commands. - Create FRESH_FLAG constant and applyFreshFlag() helper in list-command.ts as a common primitive, removing direct disableResponseCache imports from all command files. - Add -f alias to buildOrgListCommand and all individual command aliases. Exception: log list uses -f for --follow (no conflict). - Verify logout already clears cache via clearAuth() → clearResponseCache().
- Fix fallback TTL overriding explicitly expired server cache headers. Changed isEntryFresh() to use serverTtl !== 0 (catches both positive and negative values) instead of serverTtl > 0 (missed negative/expired). Same fix in collectEntryMetadata() with explicit < 0 branch. - Fix cache storing stale token after 401 refresh. Use getAuthToken() (reads current DB value) instead of the captured token variable when building cache request headers, so post-refresh tokens are stored.
Make clearAuth() properly async so clearResponseCache() is awaited rather than fire-and-forget. This ensures the filesystem cache is fully removed before the process exits. Fix pre-existing bug in model-based tests: fcAssert(asyncProperty(...)) returns a Promise that was never awaited. When all commands were synchronous this was invisible, but with the async clearAuth() the yield point caused createIsolatedDbContext() cleanups to interleave with subsequent iterations, corrupting the SENTRY_CONFIG_DIR env var. Changes: - src/lib/db/auth.ts: clearAuth() → async, try/await/catch for clearResponseCache() (was .catch() fire-and-forget) - test/lib/db/model-based.test.ts: add async/await to all test functions using asyncProperty; await clearAuth() calls - test/lib/db/pagination.model-based.test.ts: same async/await fix - test/commands/project/list.test.ts: await clearAuth() calls, add required fresh:false to ListFlags objects
BugBot #9: Wrap CachePolicy.fromObject() and related calls in try-catch inside getCachedResponse(). A corrupted or version-incompatible policy object now triggers a cache miss (and best-effort cleanup of the broken entry) instead of crashing the API request. BugBot #10: Add missing `await` to clearAuth() calls in project/list tests at lines 1080 and 1362 to prevent floating promises.
…parallelize cleanup
Review Round 3 — 15 human review comments addressed:
to reduce boilerplate; update 15 command files to use it
never change once created)
combine duplicate regex patterns into single alternations
ASCII URL query param sorting
cache already came from a fetch, always valid)
instead of hardcoded magic number
manual forEach loop
catch block
checks during cleanup (no CachePolicy deserialization needed)
deleteExpiredEntries, evictExcessEntries) using p-limit-style concurrency
limiter (max 8 concurrent)
…pans
Review comments addressed:
has f: 'follow' and it doesn't use FRESH_ALIASES
not shared (RFC heuristic vs probabilistic cleanup trigger)
built-in .map() method (cacheIO.map(items, fn)) at all 3 call sites
Promise.all() since they operate on disjoint file sets
cache.lookup and cache.store spans in response-cache.ts with URL attrs
1 participant
Summary
Implements RFC 7234/9111-aware HTTP response caching for GET requests to the Sentry API, integrated transparently at the fetch level inside
createAuthenticatedFetch().Closes #318
Cache Design
~/.sentry/cache/responses/with JSON files keyed by SHA-256 hash of normalized URL + Authorization headerhttp-cache-semanticsfor freshness evaluation/events/{id}/,/issues/{id}/hashes//organizations/,/projects/,/teams//issues/(lists), issue details/autofix/,/root-cause/(polling)New Flags & Env Vars
--refresh/-rflag on all read commands — bypasses cache for that requestSENTRY_NO_CACHE=1env var — disables caching globallyCommands Updated
All read-only commands now support
--refresh: issue list/view/explain/plan, project list/view, org list/view, event view, trace list/view/logs, log list/view, auth whoami/status, team list, repo list.Testing