Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the in-repo Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2762 +/- ##
==========================================
+ Coverage 53.64% 53.96% +0.32%
==========================================
Files 1019 1020 +1
Lines 23591 23608 +17
Branches 5754 5771 +17
==========================================
+ Hits 12655 12741 +86
+ Misses 9531 9476 -55
+ Partials 1405 1391 -14
🚀 New features to boost your workflow:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/caching/memoryCacheEngine.ts`:
- Around line 3-4: Replace the explicit any used in the LRUCache instantiation
with unknown and remove the eslint-disable comment: change the cache declaration
from LRUCache<string, any> to LRUCache<string, unknown> so the MemoryCacheEngine
wrapper’s generic get/set methods maintain type safety without suppressing the
`@typescript-eslint/no-explicit-any` rule (look for the cache variable and
MemoryCacheEngine usages to update).
- Around line 25-26: The TTL unit is mismatched: make storeInCache treat its
duration parameter as seconds (to match cacheResponse callers) and convert to
milliseconds before passing to lru-cache; inside storeInCache(key: string, data:
T, duration?: number) compute const ttl = duration ? duration * 1000 : undefined
and call cache.set(key, data, ttl ? { ttl } : undefined). Update any references
to storeInCache (and leave cacheResponse(seconds) calls unchanged) so callers
can continue passing seconds without breaking behavior.
|
although this is not introducing radical changes I think the cache engine should be reworked as an interface utilising various storages. E.g. in local dev LRUCache is totally find, in prod we should use a distributed storage like redis. @stalniy wdyt? |
I actually like this one because it's minus 1 dependency. And this is the thing which I wanted to do by myself for a long time :) I think that we should make a discussion for this and consider other options like https://bentocache.dev/docs/introduction |
The healthz service was returning unhealthy immediately on the first failed check after startup (when isFailed was null), bypassing the failure tolerance logic. This could cause liveness probe failures during pod startup when DB connections aren't yet established. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
baktun14
force-pushed
the
fix/api-memory-leaks
branch
from
f9d164b to
238392c
Compare
Replace `any` with `NonNullable<unknown>` cache value type, change `getFromCache` sentinel from `false` to `undefined`, and convert TTL duration from seconds to milliseconds for lru-cache compatibility.
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 `@apps/api/src/caching/memoryCacheEngine.spec.ts`:
- Around line 169-173: The setup function currently has no parameters; change
its signature to accept a single destructured empty object with an inline type
to satisfy the test guideline (e.g., function setup({}: {}) { ... }), keep the
body as-is (new MemoryCacheEngine(), engine.clearAllKeyInCache(), return {
engine }) and ensure this setup remains at the bottom of the root describe
block; reference: setup, MemoryCacheEngine, engine.clearAllKeyInCache.
Track whether a healthcheck has succeeded at least once before tolerating failures, preventing false-positive liveness on cold start.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/healthz/services/healthz/healthz.service.ts (1)
62-99:⚠️ Potential issue | 🟠 MajorClear
checkedAton first-time failure to prevent false-positive liveness.The
check()method always updatescheckedAtin itsfinallyblock (line 105), even when the ping fails. This means a failed first call still caches a timestamp; subsequentisHealthy()calls withincacheTTLskip the check and returntrue, yielding false-positive liveness during cold start. ThehasSucceededguard on line 95 correctly returnsfalseon initial failure, but the cached timestamp bypasses this guard on subsequent calls within the TTL window.🛠️ Minimal fix to avoid caching failures before first success
const prevIsFailed = this.isFailed; + if (!this.hasSucceeded) { + this.checkedAt = null; + return false; + } - if (prevIsFailed || !this.hasSucceeded || options?.ignoreCache) return false; + if (prevIsFailed || options?.ignoreCache) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/healthz/services/healthz/healthz.service.ts` around lines 62 - 99, The Healthcheck's checkedAt is being set even when check() fails, causing a failed first ping to be cached and producing false-positive liveness; update the logic so checkedAt is only recorded after a successful ping (or explicitly clear checkedAt on first-time failure). Specifically, modify Healthcheck.check()/isHealthy() behavior so that checkedAt is not updated in the finally block when the ping throws, and if a ping fails while hasSucceeded is false then set checkedAt = null (or move the checkedAt update into the success path) so subsequent isHealthy() calls within cacheTTL will re-run check() instead of returning a cached true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/src/healthz/services/healthz/healthz.service.ts`:
- Around line 62-99: The Healthcheck's checkedAt is being set even when check()
fails, causing a failed first ping to be cached and producing false-positive
liveness; update the logic so checkedAt is only recorded after a successful ping
(or explicitly clear checkedAt on first-time failure). Specifically, modify
Healthcheck.check()/isHealthy() behavior so that checkedAt is not updated in the
finally block when the ping throws, and if a ping fails while hasSucceeded is
false then set checkedAt = null (or move the checkedAt update into the success
path) so subsequent isHealthy() calls within cacheTTL will re-run check()
instead of returning a cached true.
3 participants
Why
The memory-cache doesn't have any limit
What
Replace with LRUCache
Summary by CodeRabbit
Improvements
Chores
Tests