feat: add caching to getCACertificates#7189
feat: add caching to getCACertificates#7189lohit-bruno wants to merge 2 commits intousebruno:mainfrom
getCACertificates#7189Conversation
Cache CA certificate results using file modification times for invalidation, avoiding redundant file reads and cert merging on repeated calls with unchanged cert files.
lohit-bruno
requested review from
bijin-bruno,
helloanoop and
naman-bruno
as code owners
WalkthroughAdded in-memory caching layer to CA certificate computation with file-timestamp-based cache invalidation. When getCACertificates is called, results are returned from cache if the cache key (derived from file paths, modification times, and settings) matches; otherwise fresh results are computed and stored. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 1
🧹 Nitpick comments (2)
packages/bruno-requests/src/utils/ca-cert.ts (2)
103-113: Twofs.statSynccalls execute on every invocation — including cache hits.
getFileMtimeMscallsfs.statSyncsynchronously at lines 105 and 107 before the cache check. On a hot request path this means two blocking stat syscalls per request even when the cache would have been valid. If this function is called per-request, consider whether the stat overhead is acceptable or if a short-lived TTL (e.g., re-validate at most once per second) would be a better tradeoff.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-requests/src/utils/ca-cert.ts` around lines 103 - 113, The current code computes getFileMtimeMs (which calls fs.statSync) for caCertFilePath and NODE_EXTRA_CA_CERTS on every invocation before checking cachedResult, causing two blocking stat syscalls even on cache hits; change the logic in the get/construct cacheKey path so you only call getFileMtimeMs when necessary — either (A) compute a simple cacheKey that omits mtimes and instead store mtimes inside the cached value and revalidate them only when the cached entry is stale, or (B) add a short TTL (e.g., 1s) on cachedResult and skip calling getFileMtimeMs while the TTL is valid; adjust references to cacheKey, cachedResult and getFileMtimeMs accordingly so fs.statSync is avoided on hot cache hits.
174-177: Cached and returned objects share the samecaCertificatesCountreference — callers can silently corrupt the cache.
resultis stored incachedResultand returned to the caller as the same object.caCertificatesCountis a mutable plain object; any caller mutation (e.g.,result.caCertificatesCount.custom = 0) will corrupt the cached entry for all future hits.The cache-hit path at line 112 has the same exposure —
cachedResult.resultis returned directly.🛡️ Proposed fix — return a shallow copy of the count sub-object
- const result = { caCertificates, caCertificatesCount }; - cachedResult = { key: cacheKey, result }; - return result; + const result: T_CACertificatesResult = { caCertificates, caCertificatesCount }; + cachedResult = { key: cacheKey, result }; + return { caCertificates, caCertificatesCount: { ...caCertificatesCount } };And on the cache-hit return:
- return cachedResult.result; + const { caCertificates, caCertificatesCount } = cachedResult.result; + return { caCertificates, caCertificatesCount: { ...caCertificatesCount } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-requests/src/utils/ca-cert.ts` around lines 174 - 177, The cached and returned objects share the same mutable caCertificatesCount reference which allows callers to mutate the cached entry; to fix, ensure you return copies of the mutable sub-object instead of the original: when creating cachedResult assign cachedResult.result to an object that contains caCertificates and a shallow copy of caCertificatesCount (e.g., {...caCertificatesCount}), and on cache hits return a new object that likewise clones the stored result.caCertificatesCount before returning to callers; update references to result, cachedResult, and caCertificatesCount accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-requests/src/utils/ca-cert.ts`:
- Line 20: cachedResult currently stores a single { key: string; result:
T_CACertificatesResult } entry which will thrash when multiple CA configs are
used; replace it with a Map keyed by the existing cacheKey so multiple
caCertFilePath values can be cached concurrently (e.g., change cachedResult to a
Map<string, T_CACertificatesResult> and update the lookup/insert logic in the
functions that reference cachedResult/cacheKey to use
map.get(cacheKey)/map.set(cacheKey, result)); optionally add a size cap and
evict oldest entries when the map exceeds N to prevent unbounded growth.
---
Nitpick comments:
In `@packages/bruno-requests/src/utils/ca-cert.ts`:
- Around line 103-113: The current code computes getFileMtimeMs (which calls
fs.statSync) for caCertFilePath and NODE_EXTRA_CA_CERTS on every invocation
before checking cachedResult, causing two blocking stat syscalls even on cache
hits; change the logic in the get/construct cacheKey path so you only call
getFileMtimeMs when necessary — either (A) compute a simple cacheKey that omits
mtimes and instead store mtimes inside the cached value and revalidate them only
when the cached entry is stale, or (B) add a short TTL (e.g., 1s) on
cachedResult and skip calling getFileMtimeMs while the TTL is valid; adjust
references to cacheKey, cachedResult and getFileMtimeMs accordingly so
fs.statSync is avoided on hot cache hits.
- Around line 174-177: The cached and returned objects share the same mutable
caCertificatesCount reference which allows callers to mutate the cached entry;
to fix, ensure you return copies of the mutable sub-object instead of the
original: when creating cachedResult assign cachedResult.result to an object
that contains caCertificates and a shallow copy of caCertificatesCount (e.g.,
{...caCertificatesCount}), and on cache hits return a new object that likewise
clones the stored result.caCertificatesCount before returning to callers; update
references to result, cachedResult, and caCertificatesCount accordingly.
There was a problem hiding this comment.
Single-slot cache will thrash when multiple CA configurations are active.
cachedResult holds exactly one entry. Bruno supports per-collection CA cert overrides, so two collections with different caCertFilePath values will evict each other on every call — the cache never hits. Use a small Map keyed by cacheKey instead.
🗂️ Proposed fix — multi-entry cache
- let cachedResult: { key: string; result: T_CACertificatesResult } | undefined;
+ const resultCache = new Map<string, T_CACertificatesResult>();- if (cachedResult && cachedResult.key === cacheKey) {
- return cachedResult.result;
- }
+ if (resultCache.has(cacheKey)) {
+ return resultCache.get(cacheKey)!;
+ }- const result = { caCertificates, caCertificatesCount };
- cachedResult = { key: cacheKey, result };
- return result;
+ const result: T_CACertificatesResult = { caCertificates, caCertificatesCount };
+ resultCache.set(cacheKey, result);
+ return result;Optional: cap the map size (e.g., evict oldest entry when
> N) to avoid unbounded growth if many unique paths are cycled through.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let cachedResult: { key: string; result: T_CACertificatesResult } | undefined; | |
| const resultCache = new Map<string, T_CACertificatesResult>(); | |
| // In the cache lookup section: | |
| if (resultCache.has(cacheKey)) { | |
| return resultCache.get(cacheKey)!; | |
| } | |
| // In the cache storage section: | |
| const result: T_CACertificatesResult = { caCertificates, caCertificatesCount }; | |
| resultCache.set(cacheKey, result); | |
| return result; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-requests/src/utils/ca-cert.ts` at line 20, cachedResult
currently stores a single { key: string; result: T_CACertificatesResult } entry
which will thrash when multiple CA configs are used; replace it with a Map keyed
by the existing cacheKey so multiple caCertFilePath values can be cached
concurrently (e.g., change cachedResult to a Map<string, T_CACertificatesResult>
and update the lookup/insert logic in the functions that reference
cachedResult/cacheKey to use map.get(cacheKey)/map.set(cacheKey, result));
optionally add a size cap and evict oldest entries when the map exceeds N to
prevent unbounded growth.
1 participant
Description
Cache CA certificate results using file modification times for invalidation, avoiding redundant file reads and cert merging on repeated calls with unchanged cert files.
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit