fix(config-cache): has() no longer inflates hit/miss statistics#498
fix(config-cache): has() no longer inflates hit/miss statistics#498nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
has() delegated to get(), which increments this.hits or this.misses. The common has()+get() pattern double-counted every lookup. Re-implement has() with its own TTL check so cache statistics stay accurate. 35 unit tests added including 3 regression tests for this fix. Closes SynkraAI#497
nikolasdehor
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR fixes a bug in ConfigCache where the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
Fixes ConfigCache.has() so it no longer inflates hit/miss counters by avoiding delegation to get(), and adds regression coverage to prevent the double-counting pattern.
Changes:
- Reimplemented
ConfigCache.has()to check presence/TTL directly without mutating statistics. - Added comprehensive unit tests (including regression tests for #497) covering cache behavior and stats.
- Updated install manifest metadata/hashes to reflect the modified core file(s).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.aios-core/core/config/config-cache.js |
Updates has() to avoid incrementing cache hit/miss stats while still enforcing TTL + cleanup. |
tests/core/config/config-cache.test.js |
Adds unit + regression tests to validate TTL behavior and ensure has() does not affect stats. |
.aios-core/install-manifest.yaml |
Updates generated manifest timestamp and file hashes/sizes for changed artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!this.cache.has(key)) return false; | ||
|
|
||
| const timestamp = this.timestamps.get(key); | ||
| if (Date.now() - timestamp > this.ttl) { | ||
| this.cache.delete(key); | ||
| this.timestamps.delete(key); | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
has() only checks this.cache.has(key) but assumes a corresponding timestamp exists. If the maps ever become out-of-sync (e.g., timestamp missing), Date.now() - timestamp becomes NaN and the entry will be treated as non-expired, returning true. Consider treating a missing timestamp as invalid (cleanup + false) or checking this.timestamps.has(key) alongside this.cache.has(key).
| // Check directly instead of delegating to get(), which would | ||
| // increment hits/misses and inflate cache statistics (fixes #497) | ||
| if (!this.cache.has(key)) return false; | ||
|
|
||
| const timestamp = this.timestamps.get(key); | ||
| if (Date.now() - timestamp > this.ttl) { | ||
| this.cache.delete(key); | ||
| this.timestamps.delete(key); | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
This duplicates expiration/cleanup logic that likely also exists in get(), which can drift over time (e.g., boundary conditions like > vs >=, cleanup behavior, or time source). A concrete way to prevent divergence is to extract a shared internal helper that validates/cleans a key without touching stats, and have both get() and has() rely on it.
| * Fixes #497 — has() was inflating hit/miss stats by calling get() | ||
| */ | ||
|
|
||
| jest.useFakeTimers(); |
There was a problem hiding this comment.
The suite enables fake timers at module scope but never restores real timers. To reduce cross-suite coupling (especially if this file is ever refactored/merged), add an afterAll(() => jest.useRealTimers()) (or equivalent) in this test file.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/config/config-cache.test.js (1)
12-12: Restore real timers after the suite.
jest.useFakeTimers()is module-scoped with no corresponding teardown. If the suite ever runs--runInBand(or in a shared worker), the fake timer state can leak into subsequently loaded modules. A singleafterAllguard is cheap insurance.🛡️ Proposed fix
jest.useFakeTimers(); const { ConfigCache, globalConfigCache } = require('../../../.aios-core/core/config/config-cache'); + +afterAll(() => { + jest.useRealTimers(); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/config/config-cache.test.js` at line 12, Add a teardown to restore real timers after the suite: the test file calls jest.useFakeTimers() but never reverts the timer implementation, so add an afterAll hook that calls jest.useRealTimers() (i.e., include an afterAll(() => jest.useRealTimers()) near the top-level of the test file) to prevent fake timer state leaking across modules or when running with --runInBand.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/config/config-cache.test.js`:
- Line 12: Add a teardown to restore real timers after the suite: the test file
calls jest.useFakeTimers() but never reverts the timer implementation, so add an
afterAll hook that calls jest.useRealTimers() (i.e., include an afterAll(() =>
jest.useRealTimers()) near the top-level of the test file) to prevent fake timer
state leaking across modules or when running with --runInBand.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.aios-core/core/config/config-cache.js.aios-core/install-manifest.yamltests/core/config/config-cache.test.js
Resumo
has()no ConfigCache para não chamarget()internamentehas()delegava paraget(), que incrementava contadores de hits/misses — uma simples verificação de existência inflava as estatísticas do cachehas()verifica diretamente no Map sem tocar nas métricasCloses #497
Plano de teste
has()não altera stats de hits/misseshas()seguido deget()conta exatamente 1 hit