Skip to content

fix(hardening): TTL-based eviction for connection driver cache (#574) #582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alfredo1996 merged 6 commits into from
Apr 20, 2026
Merged

fix(hardening): TTL-based eviction for connection driver cache (#574) #582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b4c503c
fix(hardening): TTL-based eviction for connection driver cache
alfredorubin96 Apr 20, 2026
3ddc7d2
test(app): add coverage for close() and error paths in driver cache
alfredorubin96 Apr 20, 2026
1d89ffb
fix(test): use undefined close for no-close module test
alfredorubin96 Apr 20, 2026
af7e20f
fix(test): remove problematic no-close mock test
alfredorubin96 Apr 20, 2026
b6cc54e
test(app): direct coverage for _evictStaleEntries + simplify unref check
alfredorubin96 Apr 20, 2026
27743af
refactor(app): simplify closeModuleSilently — remove redundant try
alfredorubin96 Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 156 additions & 0 deletions app/src/lib/__tests__/query/query-executor-core.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ import { describe, it, expect, vi, beforeEach } from "vitest";

const mockRunQuery = vi.fn();
const mockCheckConnection = vi.fn();
const mockClose = vi.fn().mockResolvedValue(undefined);
const mockCreateConnectionModule = vi.fn(() => ({
runQuery: mockRunQuery,
checkConnection: mockCheckConnection,
close: mockClose,
}));

vi.mock("@/lib/connector/connection-adapter", () => ({
Expand Down Expand Up @@ -41,6 +43,10 @@ vi.mock("@neoboard/connection", () => ({
describe("query-executor", () => {
let executeQuery: typeof import("@/lib/query/query-executor").executeQuery;
let testConnection: typeof import("@/lib/query/query-executor").testConnection;
let closeConnection: typeof import("@/lib/query/query-executor").closeConnection;
let closeAllConnections: typeof import("@/lib/query/query-executor").closeAllConnections;
let _getCacheSize: typeof import("@/lib/query/query-executor")._getCacheSize;
let _evictStaleEntries: typeof import("@/lib/query/query-executor")._evictStaleEntries;

beforeEach(async () => {
vi.clearAllMocks();
Expand All @@ -53,6 +59,10 @@ describe("query-executor", () => {
const mod = await import("@/lib/query/query-executor");
executeQuery = mod.executeQuery;
testConnection = mod.testConnection;
closeConnection = mod.closeConnection;
closeAllConnections = mod.closeAllConnections;
_getCacheSize = mod._getCacheSize;
_evictStaleEntries = mod._evictStaleEntries;
});

const neo4jCreds = {
Expand Down Expand Up @@ -460,4 +470,150 @@ describe("query-executor", () => {
expect.objectContaining({ database: "testdb" }),
);
});

// -----------------------------------------------------------------------
// Cache eviction
// -----------------------------------------------------------------------

it("closeConnection removes a cached module and calls close()", async () => {
mockRunQuery.mockImplementation(
(_p: unknown, cbs: { onSuccess: (v: unknown) => void }) => {
cbs.onSuccess([]);
},
);

await executeQuery("neo4j", neo4jCreds, { query: "RETURN 1" });
expect(_getCacheSize()).toBe(1);

closeConnection("neo4j", neo4jCreds);
expect(_getCacheSize()).toBe(0);
expect(mockClose).toHaveBeenCalledTimes(1);
});

it("closeConnection is a no-op for unknown keys", () => {
closeConnection("neo4j", neo4jCreds);
expect(_getCacheSize()).toBe(0);
});

it("closeAllConnections clears the entire cache", async () => {
mockRunQuery.mockImplementation(
(_p: unknown, cbs: { onSuccess: (v: unknown) => void }) => {
cbs.onSuccess([]);
},
);

await executeQuery("neo4j", neo4jCreds, { query: "RETURN 1" });
await executeQuery("postgresql", pgCreds, { query: "SELECT 1" });
expect(_getCacheSize()).toBe(2);

await closeAllConnections();
expect(_getCacheSize()).toBe(0);
});

it("closeAllConnections calls close() on each module", async () => {
mockRunQuery.mockImplementation(
(_p: unknown, cbs: { onSuccess: (v: unknown) => void }) => {
cbs.onSuccess([]);
},
);

await executeQuery("neo4j", neo4jCreds, { query: "RETURN 1" });
await executeQuery("postgresql", pgCreds, { query: "SELECT 1" });
mockClose.mockClear();

await closeAllConnections();
expect(mockClose).toHaveBeenCalledTimes(2);
});

it("closeConnection handles close() rejection gracefully", async () => {
mockClose.mockRejectedValueOnce(new Error("close failed"));
mockRunQuery.mockImplementation(
(_p: unknown, cbs: { onSuccess: (v: unknown) => void }) => {
cbs.onSuccess([]);
},
);

await executeQuery("neo4j", neo4jCreds, { query: "RETURN 1" });
expect(() => closeConnection("neo4j", neo4jCreds)).not.toThrow();
expect(_getCacheSize()).toBe(0);
});

it("cache refreshes lastAccessedAt on reuse", async () => {
mockRunQuery.mockImplementation(
(_p: unknown, cbs: { onSuccess: (v: unknown) => void }) => {
cbs.onSuccess([]);
},
);

await executeQuery("neo4j", neo4jCreds, { query: "RETURN 1" });
expect(mockCreateConnectionModule).toHaveBeenCalledTimes(1);

await executeQuery("neo4j", neo4jCreds, { query: "RETURN 2" });
expect(mockCreateConnectionModule).toHaveBeenCalledTimes(1);
expect(_getCacheSize()).toBe(1);
});

it("_evictStaleEntries is a no-op when cache is empty", () => {
expect(() => _evictStaleEntries()).not.toThrow();
expect(_getCacheSize()).toBe(0);
});

it("_evictStaleEntries keeps entries that are within TTL", async () => {
mockRunQuery.mockImplementation(
(_p: unknown, cbs: { onSuccess: (v: unknown) => void }) => {
cbs.onSuccess([]);
},
);

await executeQuery("neo4j", neo4jCreds, { query: "RETURN 1" });
expect(_getCacheSize()).toBe(1);

// Immediately after creation — well within TTL
_evictStaleEntries();
expect(_getCacheSize()).toBe(1);
expect(mockClose).not.toHaveBeenCalled();
});

it("_evictStaleEntries removes entries past TTL", async () => {
vi.useFakeTimers();
const baseTime = Date.now();
vi.setSystemTime(baseTime);

mockRunQuery.mockImplementation(
(_p: unknown, cbs: { onSuccess: (v: unknown) => void }) => {
cbs.onSuccess([]);
},
);

await executeQuery("neo4j", neo4jCreds, { query: "RETURN 1" });
expect(_getCacheSize()).toBe(1);

// Advance past 30min TTL
vi.setSystemTime(baseTime + 31 * 60 * 1000);
_evictStaleEntries();

expect(_getCacheSize()).toBe(0);
expect(mockClose).toHaveBeenCalledTimes(1);
vi.useRealTimers();
});

it("_evictStaleEntries handles close() rejection", async () => {
vi.useFakeTimers();
const baseTime = Date.now();
vi.setSystemTime(baseTime);

mockClose.mockRejectedValueOnce(new Error("close failed"));
mockRunQuery.mockImplementation(
(_p: unknown, cbs: { onSuccess: (v: unknown) => void }) => {
cbs.onSuccess([]);
},
);

await executeQuery("neo4j", neo4jCreds, { query: "RETURN 1" });
vi.setSystemTime(baseTime + 31 * 60 * 1000);

expect(() => _evictStaleEntries()).not.toThrow();
expect(_getCacheSize()).toBe(0);
vi.useRealTimers();
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});
120 changes: 103 additions & 17 deletions app/src/lib/query/query-executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,94 @@ function toConnectionTypeEnum(type: DbType): number {
return type === "neo4j" ? ConnectionTypes.NEO4J : ConnectionTypes.POSTGRESQL;
}

/** Cache of connection modules keyed by type+uri+username+database. */
const moduleCache = new Map<string, unknown>();
/**
* TTL-based connection module cache. Each entry tracks last-access time
* and is evicted after `CACHE_TTL_MS` of inactivity. This prevents
* leaking driver instances on long-running servers when credentials
* rotate or connections are deleted.
*/
const CACHE_TTL_MS = 30 * 60 * 1000; // 30 minutes
const EVICTION_INTERVAL_MS = 5 * 60 * 1000; // sweep every 5 minutes
Comment on lines +54 to +55

@coderabbitai coderabbitai Bot Apr 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


interface CacheEntry {
module: unknown;
lastAccessedAt: number;
}

const moduleCache = new Map<string, CacheEntry>();

let evictionTimer: ReturnType<typeof setInterval> | null = null;

function startEvictionTimer() {
if (evictionTimer) return;
const timer = setInterval(() => _evictStaleEntries(), EVICTION_INTERVAL_MS);
// unref() exists on Node's Timeout but not in all runtimes. When
// available, prevents the timer from keeping the process alive.
(timer as { unref?: () => void }).unref?.();
evictionTimer = timer;
}

/** Visible for testing. Sweeps the cache and evicts stale entries. */
export function _evictStaleEntries() {
const now = Date.now();
for (const [key, entry] of moduleCache) {
if (now - entry.lastAccessedAt > CACHE_TTL_MS) {
closeModuleSilently(entry.module);
moduleCache.delete(key);
}
}
if (moduleCache.size === 0 && evictionTimer) {
clearInterval(evictionTimer);
evictionTimer = null;
}
}

function closeModuleSilently(mod: unknown) {
const m = mod as { close?: () => Promise<void> };
if (typeof m.close === "function") {
m.close().catch(() => {});
}
}

/**
* Close and remove a cached connection module by its cache key.
* Called when a connection's credentials change or the connection is deleted.
*/
export function closeConnection(
type: DbType,
credentials: ConnectionCredentials,
): void {
const key = getCacheKey(type, credentials);
const entry = moduleCache.get(key);
if (entry) {
closeModuleSilently(entry.module);
moduleCache.delete(key);
}
Comment thread
alfredo1996 marked this conversation as resolved.
}

/**
* Close all cached connection modules. Used in tests and graceful shutdown.
*/
export async function closeAllConnections(): Promise<void> {
const closePromises: Promise<void>[] = [];
for (const [, entry] of moduleCache) {
const m = entry.module as { close?: () => Promise<void> };
if (typeof m.close === "function") {
closePromises.push(m.close().catch(() => {}));
}
}
moduleCache.clear();
if (evictionTimer) {
clearInterval(evictionTimer);
evictionTimer = null;
}
await Promise.all(closePromises);
}

/** Visible for testing — returns current cache size. */
export function _getCacheSize(): number {
return moduleCache.size;
}

function getCacheKey(type: DbType, credentials: ConnectionCredentials): string {
const advancedKey = [
Expand Down Expand Up @@ -82,22 +168,22 @@ function getOrCreateModule(
credentials: ConnectionCredentials,
): unknown {
const key = getCacheKey(type, credentials);
let connModule = moduleCache.get(key);
if (!connModule) {
const authConfig = {
uri: ensureDatabaseInUri(credentials.uri, credentials.database),
username: credentials.username,
password: credentials.password,
authType: 1, // NATIVE
};
const advancedOptions = buildAdvancedOptions(credentials);
connModule = createConnectionModule(
type, // string type for registry lookup
authConfig,
advancedOptions,
);
moduleCache.set(key, connModule);
const entry = moduleCache.get(key);
if (entry) {
entry.lastAccessedAt = Date.now();
return entry.module;
}

const authConfig = {
uri: ensureDatabaseInUri(credentials.uri, credentials.database),
username: credentials.username,
password: credentials.password,
authType: 1, // NATIVE
};
const advancedOptions = buildAdvancedOptions(credentials);
const connModule = createConnectionModule(type, authConfig, advancedOptions);
moduleCache.set(key, { module: connModule, lastAccessedAt: Date.now() });
Comment thread
alfredo1996 marked this conversation as resolved.
startEvictionTimer();
return connModule;
}

Expand Down
Loading