fix(hardening): TTL-based eviction for connection driver cache (#574) - #582
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 34 seconds. ⌛ 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 (2)
WalkthroughThe PR implements TTL-based cache eviction in the query-executor module, replacing the unbounded moduleCache with one that tracks last access time, evicts stale entries after a configurable idle period, and provides explicit lifecycle management APIs ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/lib/__tests__/query/query-executor-core.test.ts`:
- Around line 556-569: The test currently only proves cache reuse but not TTL
refresh; update the test to use fake timers (jest.useFakeTimers /
jest.setSystemTime or jest.advanceTimersByTime) and a known TTL value to
exercise the lastAccessedAt refresh path: create the cache entry by calling
executeQuery("neo4j", neo4jCreds, { query: "RETURN 1" }), advance the fake clock
past the configured TTL expiry, then call executeQuery again for "RETURN 2" and
assert the entry was not evicted (expect(_getCacheSize()).toBe(1) and
mockCreateConnectionModule called once). Ensure you restore real timers at the
end and reference the cache entry behavior (entry.lastAccessedAt) by advancing
time before/after the second access so the test fails if lastAccessedAt is not
updated.
In `@app/src/lib/query/query-executor.ts`:
- Around line 54-55: Replace the hard-coded CACHE_TTL_MS and
EVICTION_INTERVAL_MS constants with values read from validated server-side
configuration (e.g., process.env or your config loader) while keeping the
current numeric expressions as fallbacks; locate the constants named
CACHE_TTL_MS and EVICTION_INTERVAL_MS in query-executor.ts and change them to
parse/validate the configured TTL and eviction interval (ensure numeric,
positive, and clamp to sensible min/max) before using them so the idle TTL and
sweep cadence become configurable but safe.
- Around line 103-112: closeConnection currently returns void, deletes the cache
entry and fire-and-forgets closeModuleSilently, so change closeConnection to
return Promise<void> and perform an awaited best-effort close before deleting
the cache entry: use getCacheKey to locate the entry in moduleCache, if found
call an async helper (implement closeModuleBestEffort similar to the example
that checks for a .close function and awaits it inside try/catch) and await it,
then delete the key from moduleCache; after deletion, if moduleCache.size === 0
also stop the background sweep timer (clear the interval variable you use for
sweeping) to avoid leaving it running. Ensure callers can await
closeConnection's returned promise.
- Around line 172-187: The cache is keyed without the password so
rotated/changed credentials reuse a stale driver in moduleCache; update the
PATCH handler that performs credential updates to call closeConnection(type,
newCredentials) immediately after the credentials are saved (so
createConnectionModule will rebuild with new creds next time), and update the
DELETE handler to call closeConnection(type, decryptedCredentials) before
returning confirmation so the removed connection's module is torn down; also
ensure prefetchSchema (which currently runs without invalidating moduleCache)
either calls closeConnection(type, credentials) when it detects credential
changes or explicitly invalidates the moduleCache entry keyed by
getCacheKey(type, credentials) so stale modules are not reused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f9e42f4-edcd-4639-b916-69a409a59760
📒 Files selected for processing (2)
app/src/lib/__tests__/query/query-executor-core.test.tsapp/src/lib/query/query-executor.ts
Comment on lines
+54
to
+55
| const CACHE_TTL_MS = 30 * 60 * 1000; // 30 minutes | ||
| const EVICTION_INTERVAL_MS = 5 * 60 * 1000; // sweep every 5 minutes |
There was a problem hiding this comment.
Make the TTL configurable, not just defaulted.
The issue objective calls for a configurable idle TTL, but these constants hard-code both TTL and sweep cadence. Consider reading validated server-side env/config values with these as fallbacks.
Example direction
+function readPositiveDurationMs(name: string, fallbackMs: number): number {
+ const value = Number(process.env[name]);
+ return Number.isFinite(value) && value > 0 ? value : fallbackMs;
+}
+
-const CACHE_TTL_MS = 30 * 60 * 1000; // 30 minutes
-const EVICTION_INTERVAL_MS = 5 * 60 * 1000; // sweep every 5 minutes
+const CACHE_TTL_MS = readPositiveDurationMs(
+ "QUERY_EXECUTOR_CACHE_TTL_MS",
+ 30 * 60 * 1000,
+);
+const EVICTION_INTERVAL_MS = readPositiveDurationMs(
+ "QUERY_EXECUTOR_EVICTION_INTERVAL_MS",
+ 5 * 60 * 1000,
+);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/query/query-executor.ts` around lines 54 - 55, Replace the
hard-coded CACHE_TTL_MS and EVICTION_INTERVAL_MS constants with values read from
validated server-side configuration (e.g., process.env or your config loader)
while keeping the current numeric expressions as fallbacks; locate the constants
named CACHE_TTL_MS and EVICTION_INTERVAL_MS in query-executor.ts and change them
to parse/validate the configured TTL and eviction interval (ensure numeric,
positive, and clamp to sensible min/max) before using them so the idle TTL and
sweep cadence become configurable but safe.
The moduleCache Map had no eviction — every unique credential set created a driver instance that was never removed. Long-running servers leaked Neo4j/PG connections on credential rotation or deletion. Adds TTL-based eviction (30 min idle), a periodic sweep (5 min), and closeConnection()/closeAllConnections() for explicit cleanup. Closes #574 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verifies close() is called on eviction/cleanup, handles modules without close(), and handles close() rejection gracefully. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
force-pushed
the
fix/issue-574-driver-cache-eviction
branch
from
April 20, 2026 13:01
d2b9432 to
1d89ffb
Compare
The mock factory's inferred type requires close. Coverage of the close-less path is redundant with the no-op test above. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Exports evictStaleEntries as _evictStaleEntries and adds 4 direct tests using vi.useFakeTimers: empty cache no-op, entries within TTL preserved, entries past TTL evicted with close(), close() rejection handled. Simplifies startEvictionTimer's unref() detection via optional chaining. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SonarCloud C Reliability: the try/catch is redundant since close() returns a Promise, and .catch() already handles rejection. Synchronous throw from close() would only happen if the module is malformed — callers are already defensive. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
4 tasks
alfredo1996
added a commit
that referenced
this pull request
Apr 22, 2026
…#594) Security: enforce canWrite on reassign route, fix invalid 'owner' share role in SQL, wrap jsonb_agg with COALESCE for NULL safety, add WITH ORDINALITY to preserve page order, sanitize plugin codegen inputs. Bugs: extract render helpers in widget-preview-panel to reduce cognitive complexity from 50 to within limits (S3358 nested ternaries). Code smells: mark props Readonly (S6759), flip negated condition (S7735), remove unnecessary type assertions (S4325), evict cached connections on credential update/delete (#582). Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
query-executor.tscloseConnection()for explicit cleanup on credential rotation/deletioncloseAllConnections()for tests and graceful shutdownunref()so it doesn't prevent Node.js process exitTest plan
Closes #574
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests