-
-
Notifications
You must be signed in to change notification settings - Fork 28
Cached importJwk and data host data to improve cpu usage
#1630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -104,8 +104,22 @@ export const actorDispatcher = (hostDataContextLoader: HostDataContextLoader) => | |||||||||||||||
| export const keypairDispatcher = ( | ||||||||||||||||
| accountService: AccountService, | ||||||||||||||||
| hostDataContextLoader: HostDataContextLoader, | ||||||||||||||||
| ) => | ||||||||||||||||
| async function keypairDispatcher(ctx: FedifyContext, identifier: string) { | ||||||||||||||||
| ) => { | ||||||||||||||||
| const MAX_CACHE_SIZE = 1000; | ||||||||||||||||
| const KEY_TTL_MS = 30 * 60 * 1000; // 30 minutes | ||||||||||||||||
| const cryptoKeyCache = new Map< | ||||||||||||||||
| number, | ||||||||||||||||
| { | ||||||||||||||||
| publicKey: CryptoKey; | ||||||||||||||||
| privateKey: CryptoKey; | ||||||||||||||||
| createdAt: number; | ||||||||||||||||
| } | ||||||||||||||||
| >(); | ||||||||||||||||
|
|
||||||||||||||||
| return async function keypairDispatcher( | ||||||||||||||||
| ctx: FedifyContext, | ||||||||||||||||
| identifier: string, | ||||||||||||||||
| ) { | ||||||||||||||||
| const hostData = await hostDataContextLoader.loadDataForHost(ctx.host); | ||||||||||||||||
|
|
||||||||||||||||
| if (isError(hostData)) { | ||||||||||||||||
|
|
@@ -145,6 +159,14 @@ export const keypairDispatcher = ( | |||||||||||||||
|
|
||||||||||||||||
| const { account } = getValue(hostData); | ||||||||||||||||
|
|
||||||||||||||||
| const cached = cryptoKeyCache.get(account.id); | ||||||||||||||||
| if (cached) { | ||||||||||||||||
| if (Date.now() - cached.createdAt < KEY_TTL_MS) { | ||||||||||||||||
| return [cached]; | ||||||||||||||||
|
||||||||||||||||
| return [cached]; | |
| return [ | |
| { | |
| publicKey: cached.publicKey, | |
| privateKey: cached.privateKey, | |
| }, | |
| ]; |
Copilot
AI
Mar 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,23 +3,60 @@ import type { Knex } from 'knex'; | |
| import type { Account } from '@/account/account.entity'; | ||
| import type { KnexAccountRepository } from '@/account/account.repository.knex'; | ||
| import type { Result } from '@/core/result'; | ||
| import { error, ok } from '@/core/result'; | ||
| import { error, isError, ok } from '@/core/result'; | ||
| import type { Site } from '@/site/site.service'; | ||
|
|
||
| type HostDataResult = Result< | ||
| { site: Site; account: Account }, | ||
| 'site-not-found' | 'account-not-found' | 'multiple-users-for-site' | ||
| >; | ||
|
|
||
| interface CacheEntry { | ||
| result: HostDataResult; | ||
| expiry: number; | ||
| } | ||
|
|
||
| export class HostDataContextLoader { | ||
| private static readonly CACHE_TTL_MS = 60_000; | ||
| private static readonly MAX_CACHE_SIZE = 1000; | ||
| private static readonly CACHE_ENABLED = | ||
| process.env.NODE_ENV === 'production'; | ||
| private readonly cache = new Map<string, CacheEntry>(); | ||
|
|
||
| constructor( | ||
| readonly db: Knex, | ||
| readonly accountRepository: KnexAccountRepository, | ||
| ) {} | ||
|
|
||
| async loadDataForHost( | ||
| host: string, | ||
| ): Promise< | ||
| Result< | ||
| { site: Site; account: Account }, | ||
| 'site-not-found' | 'account-not-found' | 'multiple-users-for-site' | ||
| > | ||
| > { | ||
| async loadDataForHost(host: string): Promise<HostDataResult> { | ||
| if (!HostDataContextLoader.CACHE_ENABLED) { | ||
| return this.queryDataForHost(host); | ||
| } | ||
|
|
||
|
Comment on lines
+31
to
+35
|
||
| 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!); | ||
| } | ||
|
Comment on lines
+36
to
+48
|
||
|
|
||
| this.cache.set(host, { | ||
| result, | ||
| expiry: now + HostDataContextLoader.CACHE_TTL_MS, | ||
| }); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| private async queryDataForHost(host: string): Promise<HostDataResult> { | ||
| const results = await this.db('sites') | ||
| .leftJoin('users', 'users.site_id', 'sites.id') | ||
| .leftJoin('accounts', 'accounts.id', 'users.account_id') | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: What's the reasoning behind 30 min? It that an arbitrary limit?