Cached importJwk and data host data to improve cpu usage#1630
Cached importJwk and data host data to improve cpu usage#1630
importJwk and data host data to improve cpu usage#1630Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughTwo modules gain in-memory caching. In src/dispatchers.ts the keypairDispatcher now uses a cryptoKeyCache and MAX_CACHE_SIZE to return cached imported keypairs early and store newly imported keypairs (bounded by the max). The dispatcher factory’s internal initialization was adjusted to return the dispatcher function from an outer scope; the external signature is unchanged. In src/http/host-data-context-loader.ts HostDataContextLoader adds a TTL (60s) and max-size (1000) cache with simple LRU eviction, moving the DB query into a private helper and changing the public method to return a named HostDataResult type. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/http/host-data-context-loader.ts (2)
9-12: Use contextual error objects forHostDataResulterrors.The new union keeps opaque string literals; structured errors are easier to log, extend, and debug safely.
As per coding guidelines, "Prefer error objects with context over string literal errors in Result types".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http/host-data-context-loader.ts` around lines 9 - 12, Replace the opaque string literal union in HostDataResult with structured error objects so callers can access context: change the Result error type from 'site-not-found' | 'account-not-found' | 'multiple-users-for-site' to a discriminated union like { type: 'site-not-found'; host: string } | { type: 'account-not-found'; accountId?: string } | { type: 'multiple-users-for-site'; siteId: string; userIds: string[] } (or equivalent Error subclasses) and update usages of HostDataResult, the Result type instantiations, and any pattern-matching code to read the error.type and context fields instead of comparing string literals.
33-35: Current eviction policy is FIFO, not LRU.A cache hit returns immediately without updating recency, so hot hosts can still be evicted before colder entries.
♻️ Proposed refinement
if (cached && cached.expiry > now) { + // refresh recency for LRU behavior + this.cache.delete(host); + this.cache.set(host, cached); return cached.result; } + + if (cached && cached.expiry <= now) { + this.cache.delete(host); + }Also applies to: 39-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http/host-data-context-loader.ts` around lines 33 - 35, The cache currently uses FIFO because a cache hit simply returns cached.result without updating recency; to implement LRU update the entry on access (e.g., refresh cached.expiry or set/update cached.lastAccess = Date.now()) so eviction logic uses lastAccess (or expiry computed from lastAccess) rather than insertion time; modify the hit paths in host-data-context-loader.ts (the branches where cached && cached.expiry > now and the similar block at 39-40) to update the cached metadata on read and ensure the eviction routine checks lastAccess (or the refreshed expiry) when choosing entries to remove.src/dispatchers.ts (1)
157-160: Deduplicate concurrent cache misses per account.Parallel requests for the same account can both run
importJwkbefore the first write completes; tracking in-flight imports prevents duplicate CPU work.Also applies to: 193-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dispatchers.ts` around lines 157 - 160, Concurrent requests can trigger duplicate importJwk work because you only check cryptoKeyCache (cached) before calling importJwk; introduce an in-flight promise map (e.g., cryptoImportPromises keyed by account.id) to deduplicate: on cache miss, check cryptoImportPromises[account.id] and await it if present; otherwise create and store a promise that performs importJwk, writes the result into cryptoKeyCache, and finally deletes the in-flight entry; apply this pattern in both places where you currently read cryptoKeyCache and call importJwk (the block referencing cryptoKeyCache.get(account.id) and the later importJwk usage) so parallel requests reuse the same import promise instead of running importJwk concurrently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/dispatchers.ts`:
- Around line 108-112: The cryptoKeyCache and MAX_CACHE_SIZE currently allow
entries to live forever and stop caching once size hits 1000; update the cache
to store metadata {publicKey, privateKey, createdAt} and introduce a KEY_TTL_MS
(expiration) and simple LRU/eviction policy so when inserting into
cryptoKeyCache you first remove expired entries, and if cache size >=
MAX_CACHE_SIZE evict the oldest/least-recently-used entry before inserting; also
update any lookup/get code to check createdAt (or lastAccess) against KEY_TTL_MS
and treat expired entries as missing so callers regenerate keys. Ensure you
reference and modify the cryptoKeyCache initialization and all places that
read/write it to enforce TTL and eviction logic.
In `@src/http/host-data-context-loader.ts`:
- Around line 37-46: The current load path calls this.queryDataForHost(host) and
unconditionally evicts/sets the cache (using
HostDataContextLoader.MAX_CACHE_SIZE, CACHE_TTL_MS, and this.cache.set), which
caches failure Results; change it to detect error/failure Results returned from
this.queryDataForHost (e.g. result.error, result.type === 'error', result.status
=== 'error' or known codes like 'site-not-found'/'account-not-found') and skip
caching those failure Results (or alternatively cache them with a very short
TTL) while still performing the normal eviction only when you will actually set
a non-error result.
---
Nitpick comments:
In `@src/dispatchers.ts`:
- Around line 157-160: Concurrent requests can trigger duplicate importJwk work
because you only check cryptoKeyCache (cached) before calling importJwk;
introduce an in-flight promise map (e.g., cryptoImportPromises keyed by
account.id) to deduplicate: on cache miss, check
cryptoImportPromises[account.id] and await it if present; otherwise create and
store a promise that performs importJwk, writes the result into cryptoKeyCache,
and finally deletes the in-flight entry; apply this pattern in both places where
you currently read cryptoKeyCache and call importJwk (the block referencing
cryptoKeyCache.get(account.id) and the later importJwk usage) so parallel
requests reuse the same import promise instead of running importJwk
concurrently.
In `@src/http/host-data-context-loader.ts`:
- Around line 9-12: Replace the opaque string literal union in HostDataResult
with structured error objects so callers can access context: change the Result
error type from 'site-not-found' | 'account-not-found' |
'multiple-users-for-site' to a discriminated union like { type:
'site-not-found'; host: string } | { type: 'account-not-found'; accountId?:
string } | { type: 'multiple-users-for-site'; siteId: string; userIds: string[]
} (or equivalent Error subclasses) and update usages of HostDataResult, the
Result type instantiations, and any pattern-matching code to read the error.type
and context fields instead of comparing string literals.
- Around line 33-35: The cache currently uses FIFO because a cache hit simply
returns cached.result without updating recency; to implement LRU update the
entry on access (e.g., refresh cached.expiry or set/update cached.lastAccess =
Date.now()) so eviction logic uses lastAccess (or expiry computed from
lastAccess) rather than insertion time; modify the hit paths in
host-data-context-loader.ts (the branches where cached && cached.expiry > now
and the similar block at 39-40) to update the cached metadata on read and ensure
the eviction routine checks lastAccess (or the refreshed expiry) when choosing
entries to remove.
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
rmgpinto
force-pushed
the
cache-queue-data
branch
from
3e21ca0 to
0a08fe1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/http/host-data-context-loader.ts (1)
43-52:⚠️ Potential issue | 🟠 MajorAvoid caching error
Results in host cache.This currently caches failures (
site-not-found,account-not-found,multiple-users-for-site) the same as successes, which can serve stale failures during recovery windows.💡 Proposed fix
-import { error, ok } from '@/core/result'; +import { error, isError, ok } from '@/core/result'; const result = await this.queryDataForHost(host); -if (this.cache.size >= HostDataContextLoader.MAX_CACHE_SIZE) { - this.cache.delete(this.cache.keys().next().value!); -} - -this.cache.set(host, { - result, - expiry: now + HostDataContextLoader.CACHE_TTL_MS, -}); +if (!isError(result)) { + if (this.cache.size >= HostDataContextLoader.MAX_CACHE_SIZE) { + this.cache.delete(this.cache.keys().next().value!); + } + + this.cache.set(host, { + result, + expiry: Date.now() + HostDataContextLoader.CACHE_TTL_MS, + }); +} return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http/host-data-context-loader.ts` around lines 43 - 52, The code currently caches the raw result from queryDataForHost even when it represents an error; update the caching logic in HostDataContextLoader so that after calling queryDataForHost(host) you detect error results (e.g., result indicating 'site-not-found', 'account-not-found', 'multiple-users-for-site' or a generic error flag/property on the result) and skip writing them into this.cache; preserve the eviction behavior using this.cache.size >= HostDataContextLoader.MAX_CACHE_SIZE and only call this.cache.set(host, { result, expiry: now + HostDataContextLoader.CACHE_TTL_MS }) for non-error/successful results. Ensure queryDataForHost and the specific error token strings are used to identify failures so only successful responses are cached.src/dispatchers.ts (1)
108-112:⚠️ Potential issue | 🟠 MajorKey cache can go stale indefinitely and stops admitting new entries at capacity.
Current cache entries never expire, and once
MAX_CACHE_SIZEis reached, new accounts are no longer cached. This can keep rotated keys stale and degrade hit quality over time.💡 Proposed fix
const MAX_CACHE_SIZE = 1000; +const KEY_CACHE_TTL_MS = 5 * 60_000; const cryptoKeyCache = new Map< number, - { publicKey: CryptoKey; privateKey: CryptoKey } + { + publicKey: CryptoKey; + privateKey: CryptoKey; + expiresAt: number; + lastAccess: number; + } >(); const cached = cryptoKeyCache.get(account.id); if (cached) { - return [cached]; + if (cached.expiresAt > Date.now()) { + cached.lastAccess = Date.now(); + return [{ publicKey: cached.publicKey, privateKey: cached.privateKey }]; + } + cryptoKeyCache.delete(account.id); } ... - if (cryptoKeyCache.size < MAX_CACHE_SIZE) { - cryptoKeyCache.set(account.id, imported); - } + if (cryptoKeyCache.size >= MAX_CACHE_SIZE) { + const lru = [...cryptoKeyCache.entries()].reduce((a, b) => + a[1].lastAccess <= b[1].lastAccess ? a : b + )[0]; + cryptoKeyCache.delete(lru); + } + cryptoKeyCache.set(account.id, { + ...imported, + expiresAt: Date.now() + KEY_CACHE_TTL_MS, + lastAccess: Date.now(), + });Also applies to: 157-160, 204-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dispatchers.ts` around lines 108 - 112, The cryptoKeyCache currently uses a fixed MAX_CACHE_SIZE and never expires entries; update the cache to include per-entry metadata (e.g., lastAccessed timestamp and createdAt) and enforce both a TTL and an eviction policy when adding or reading entries: on get (lookup using cryptoKeyCache) check createdAt/lastAccessed against TTL and remove/refresh expired entries, and update lastAccessed to implement LRU; on set, if cryptoKeyCache.size >= MAX_CACHE_SIZE evict the least-recently-used (oldest lastAccessed) entry before inserting the new key; ensure these changes touch the symbols MAX_CACHE_SIZE and cryptoKeyCache and apply the same logic where the cache is accessed/modified in the other occurrences referenced (the other cryptoKeyCache get/set sites).
🧹 Nitpick comments (2)
src/http/host-data-context-loader.ts (2)
39-40: If LRU is intended, refresh recency on cache hits.Current behavior evicts by insertion order only; hit entries are not promoted. Consider delete+set on hit to keep hot keys resident.
Also applies to: 45-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http/host-data-context-loader.ts` around lines 39 - 40, The cache hit path in host-data-context-loader.ts currently returns cached.result without updating LRU recency; modify the hit branches (the one around the if (cached && cached.expiry > now) return cached.result; and the similar block at lines 45-47) to refresh entry recency by removing the key and re-inserting it (delete + set) or using the cache API's promote method so hot keys are promoted and not evicted by insertion order; keep the expiry check and returned value logic unchanged when performing the refresh.
9-12: Use contextual error objects forHostDataResulterrors.The string-literal error union limits debugging context (for example, host and query path details).
As per coding guidelines, "Prefer error objects with context over string literal errors in Result types".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http/host-data-context-loader.ts` around lines 9 - 12, Replace the string-literal error union in the HostDataResult type with contextual error object types so callers get diagnostic information; define discriminated error interfaces (e.g., SiteNotFoundError { type: 'site-not-found'; host: string; path?: string; }, AccountNotFoundError { type: 'account-not-found'; host: string; }, MultipleUsersForSiteError { type: 'multiple-users-for-site'; siteId: string; users: string[]; }) and update HostDataResult to Result<{ site: Site; account: Account }, SiteNotFoundError | AccountNotFoundError | MultipleUsersForSiteError>; update any code that constructs or matches on these errors (search for HostDataResult and places that return or pattern-match on the old string literals) to populate and inspect the new error object fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/dispatchers.ts`:
- Around line 108-112: The cryptoKeyCache currently uses a fixed MAX_CACHE_SIZE
and never expires entries; update the cache to include per-entry metadata (e.g.,
lastAccessed timestamp and createdAt) and enforce both a TTL and an eviction
policy when adding or reading entries: on get (lookup using cryptoKeyCache)
check createdAt/lastAccessed against TTL and remove/refresh expired entries, and
update lastAccessed to implement LRU; on set, if cryptoKeyCache.size >=
MAX_CACHE_SIZE evict the least-recently-used (oldest lastAccessed) entry before
inserting the new key; ensure these changes touch the symbols MAX_CACHE_SIZE and
cryptoKeyCache and apply the same logic where the cache is accessed/modified in
the other occurrences referenced (the other cryptoKeyCache get/set sites).
In `@src/http/host-data-context-loader.ts`:
- Around line 43-52: The code currently caches the raw result from
queryDataForHost even when it represents an error; update the caching logic in
HostDataContextLoader so that after calling queryDataForHost(host) you detect
error results (e.g., result indicating 'site-not-found', 'account-not-found',
'multiple-users-for-site' or a generic error flag/property on the result) and
skip writing them into this.cache; preserve the eviction behavior using
this.cache.size >= HostDataContextLoader.MAX_CACHE_SIZE and only call
this.cache.set(host, { result, expiry: now + HostDataContextLoader.CACHE_TTL_MS
}) for non-error/successful results. Ensure queryDataForHost and the specific
error token strings are used to identify failures so only successful responses
are cached.
---
Nitpick comments:
In `@src/http/host-data-context-loader.ts`:
- Around line 39-40: The cache hit path in host-data-context-loader.ts currently
returns cached.result without updating LRU recency; modify the hit branches (the
one around the if (cached && cached.expiry > now) return cached.result; and the
similar block at lines 45-47) to refresh entry recency by removing the key and
re-inserting it (delete + set) or using the cache API's promote method so hot
keys are promoted and not evicted by insertion order; keep the expiry check and
returned value logic unchanged when performing the refresh.
- Around line 9-12: Replace the string-literal error union in the HostDataResult
type with contextual error object types so callers get diagnostic information;
define discriminated error interfaces (e.g., SiteNotFoundError { type:
'site-not-found'; host: string; path?: string; }, AccountNotFoundError { type:
'account-not-found'; host: string; }, MultipleUsersForSiteError { type:
'multiple-users-for-site'; siteId: string; users: string[]; }) and update
HostDataResult to Result<{ site: Site; account: Account }, SiteNotFoundError |
AccountNotFoundError | MultipleUsersForSiteError>; update any code that
constructs or matches on these errors (search for HostDataResult and places that
return or pattern-match on the old string literals) to populate and inspect the
new error object fields.
rmgpinto
force-pushed
the
cache-queue-data
branch
from
0a08fe1 to
f4b664b
Compare
rmgpinto
force-pushed
the
cache-queue-data
branch
from
f4b664b to
eb9db35
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces in-memory caching in two hot paths (host-to-site/account lookup and JWK-to-CryptoKey import) to reduce repeated DB queries and key-import CPU overhead.
Changes:
- Added an in-memory TTL cache to
HostDataContextLoader.loadDataForHost()(production-only). - Added an in-memory TTL/LRU-ish cache for imported
CryptoKeypairs inkeypairDispatcher()to avoid repeatedimportJwk()work.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/http/host-data-context-loader.ts |
Adds a bounded TTL cache around the DB join query for host → site/account resolution. |
src/dispatchers.ts |
Adds a bounded TTL cache for imported keypairs to avoid repeated JSON parsing + importJwk() calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const now = Date.now(); | ||
| const cached = this.cache.get(host); | ||
|
|
||
| if (cached && cached.expiry > now) { | ||
| return cached.result; | ||
| } | ||
|
|
||
| const result = await this.queryDataForHost(host); | ||
|
|
||
| if (!isError(result)) { | ||
| if (this.cache.size >= HostDataContextLoader.MAX_CACHE_SIZE) { | ||
| this.cache.delete(this.cache.keys().next().value!); | ||
| } |
There was a problem hiding this comment.
When a cached entry is expired, it is left in the Map (you don’t delete it on expiry). Additionally, eviction runs whenever cache.size >= MAX_CACHE_SIZE even if you’re overwriting an existing host key (which doesn’t increase size), causing unnecessary eviction/churn and potentially dropping a valid entry while keeping expired ones. Consider deleting expired entries on lookup, sweeping expired entries before size-based eviction, and only evicting when inserting a new host key (e.g., !cache.has(host)).
There was a problem hiding this comment.
suggestion: @rmgpinto think this is good feedback, I'd also suggest deleting the expired key as soon as we know it has expired (e.g. on line 42), so that we don't evict more keys than necessary
| async loadDataForHost(host: string): Promise<HostDataResult> { | ||
| if (!HostDataContextLoader.CACHE_ENABLED) { | ||
| return this.queryDataForHost(host); | ||
| } | ||
|
|
There was a problem hiding this comment.
The new caching path (enabled in production) isn’t covered by the existing HostDataContextLoader tests. Adding a test that forces caching on (e.g., temporarily setting NODE_ENV/exposing a flag and using fake timers) would help ensure cache-hit behavior and eviction/expiry logic stay correct.
| const cached = cryptoKeyCache.get(account.id); | ||
| if (cached) { | ||
| if (Date.now() - cached.createdAt < KEY_TTL_MS) { | ||
| return [cached]; |
There was a problem hiding this comment.
On cache hits you return [cached], where cached includes createdAt, but on cache misses you return [imported] without createdAt. This makes the dispatcher’s returned element shape inconsistent and also exposes the internal cache entry object by reference. Consider returning only { publicKey, privateKey } (and keeping createdAt internal) and/or returning a shallow copy so callers can’t mutate cache state.
| return [cached]; | |
| return [ | |
| { | |
| publicKey: cached.publicKey, | |
| privateKey: cached.privateKey, | |
| }, | |
| ]; |
| const cached = cryptoKeyCache.get(account.id); | ||
| if (cached) { | ||
| if (Date.now() - cached.createdAt < KEY_TTL_MS) { | ||
| return [cached]; | ||
| } |
There was a problem hiding this comment.
The new CryptoKey caching/TTL behavior in keypairDispatcher isn’t exercised by the current unit tests (they only cover the non-cached success/error paths). Adding a test that calls the dispatcher twice and asserts the second call avoids accountService.getKeyPair/importJwk, plus a TTL-expiry test, would help prevent regressions.
sagzy
left a comment
sagzy
left a comment
There was a problem hiding this comment.
Generally adding a caching layer for these repeated queries makes sense, but I do have a few concerns around stale cache entries:
- What happens if the key pair is rotated (e.g. Ghost site is migrated to a new domain)? Will outbound ActivityPub signatures fail for x minutes?
- What happens when a site is deleted (e.g. Ghost user disables Network in Admin)? Do we continue serving stale site/account information for x minutes?
We can lower the risk by making the TTL smaller, but then the caching benefits also become smaller.
I'm curious what @mike182uk thinks too!
| const now = Date.now(); | ||
| const cached = this.cache.get(host); | ||
|
|
||
| if (cached && cached.expiry > now) { | ||
| return cached.result; | ||
| } | ||
|
|
||
| const result = await this.queryDataForHost(host); | ||
|
|
||
| if (!isError(result)) { | ||
| if (this.cache.size >= HostDataContextLoader.MAX_CACHE_SIZE) { | ||
| this.cache.delete(this.cache.keys().next().value!); | ||
| } |
There was a problem hiding this comment.
suggestion: @rmgpinto think this is good feedback, I'd also suggest deleting the expired key as soon as we know it has expired (e.g. on line 42), so that we don't evict more keys than necessary
| async function keypairDispatcher(ctx: FedifyContext, identifier: string) { | ||
| ) => { | ||
| const MAX_CACHE_SIZE = 1000; | ||
| const KEY_TTL_MS = 30 * 60 * 1000; // 30 minutes |
There was a problem hiding this comment.
question: What's the reasoning behind 30 min? It that an arbitrary limit?
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Think we should cover the cache read/delete/set behaviour in automated tests, e.g. first call makes the database query and set cache, second one reads from cache, one after x min deletes key and re-fetches
ref https://linear.app/ghost/issue/BER-3241