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 2 commits intoSynkraAI:mainfrom
Conversation
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. |
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a comprehensive Jest test suite for ConfigCache and fixes ConfigCache.has() to check existence/expiration directly instead of calling get(), preventing unintended increments to hit/miss statistics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
nikolasdehor
force-pushed
the
fix/config-cache-has-stats-inflation
branch
from
f9fa146 to
e5d620e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aiox-core/core/config/config-cache.js:
- Around line 79-91: The get() method must mirror the new invalidation rule in
has(): if a key exists in this.cache but has no corresponding timestamp in
this.timestamps it should be treated as invalid — delete the stale cache entry
and return undefined. Update the get(key) implementation to first check the
timestamp presence/age (reuse or call has(key)) and on missing/expired timestamp
perform this.cache.delete(key) and this.timestamps.delete(key) as appropriate
before returning; also add a regression unit test that inserts a value into
cache without a timestamp and asserts get(key) returns undefined and the cache
entry is removed, keeping behavior unchanged for valid entries (respect this.ttl
and backwards compatibility).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5493c279-28e1-4d4f-800b-159fcd62d39f
📒 Files selected for processing (3)
.aiox-core/core/config/config-cache.js.aiox-core/install-manifest.yamltests/core/config/config-cache.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/config/config-cache.test.js
| // If timestamp is missing, maps are out-of-sync — treat as invalid | ||
| if (!this.timestamps.has(key)) { | ||
| this.cache.delete(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.
Keep get() aligned with the new invalidation rule.
Line 79 now treats a missing timestamp as an invalid entry, but get() still returns the cached value in that same out-of-sync state because Date.now() - undefined > this.ttl is false. That makes has(key) and get(key) disagree on validity and can still record a hit for corrupted entries.
Suggested fix
get(key) {
if (!this.cache.has(key)) {
this.misses++;
return null;
}
+ if (!this.timestamps.has(key)) {
+ this.cache.delete(key);
+ this.misses++;
+ return null;
+ }
+
const timestamp = this.timestamps.get(key);
const now = Date.now();
// Check if expired
if (now - timestamp > this.ttl) {
this.cache.delete(key);
this.timestamps.delete(key);
this.misses++;
return null;
}Please add a matching regression for get() with an out-of-sync cache/timestamp pair as well.
As per coding guidelines, Ensure backwards compatibility — core modules are consumed by all agents.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/config/config-cache.js around lines 79 - 91, The get()
method must mirror the new invalidation rule in has(): if a key exists in
this.cache but has no corresponding timestamp in this.timestamps it should be
treated as invalid — delete the stale cache entry and return undefined. Update
the get(key) implementation to first check the timestamp presence/age (reuse or
call has(key)) and on missing/expired timestamp perform this.cache.delete(key)
and this.timestamps.delete(key) as appropriate before returning; also add a
regression unit test that inserts a value into cache without a timestamp and
asserts get(key) returns undefined and the cache entry is removed, keeping
behavior unchanged for valid entries (respect this.ttl and backwards
compatibility).
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
…ra timers - has() agora verifica this.timestamps.has(key) além de this.cache.has(key) - Timestamp ausente trata como inválido (cleanup + return false) - Adiciona afterAll com jest.useRealTimers() para evitar vazamento entre suites - Adiciona teste para cenário de mapas dessincronizados - Corrige caminho de import .aios-core → .aiox-core no teste
nikolasdehor
force-pushed
the
fix/config-cache-has-stats-inflation
branch
from
1f7063e to
68f2f76
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@Pedrovaleriolopez @oalanicolas, esse fix do ConfigCache.has() tá aberto desde 24/fev. Ontem o #581 do @rafaelscosta foi mergeado em menos de 24h com fixes que eu já tinha pronto há semanas (#500, #514, #523). Esse aqui (#498) é outro fix original nosso que ainda tá esperando. Qualquer feedback, estou pronto pra ajustar. |
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 hitSummary by CodeRabbit
Bug Fixes
Tests