From 74ba399e04c9eb0c883b768301ae493363bd5d0e Mon Sep 17 00:00:00 2001 From: soyelmismo Date: Sat, 30 May 2026 22:13:59 -0500 Subject: [PATCH] fix: address Gemini code review on OOM fix PR #2965 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. check-permissions.sh: swap NODE_OPTIONS flag order so runtime OMNIROUTE_MEMORY_MB override wins (last flag takes precedence) 2. comboMetrics.ts: evictOldestMetric(targetMap) now accepts the target map as parameter; cleanup timer iterates BOTH metrics AND shadowMetrics; null lastUsedAt falls back to Date.now() instead of epoch 0 (prevents premature eviction of intent-only entries) 3. providerRegistry.ts: remove Proxy wrapper — adds CPU/complexity overhead with zero memory savings since _REGISTRY_EAGER is already fully allocated and generator functions iterate all entries at startup. Simple re-export instead. 4. usage.ts: use per-cache TTL constants in cleanup timer (ANTIGRAVITY_MODELS_CACHE_TTL_MS=1min, others=5min) instead of single 10min SUB_CACHE_TTL_MS for all caches. 5. Add 18 unit tests for comboMetrics memory management covering: basic CRUD, shadow metrics, intent tracking, eviction at capacity, strategy tracking, and reset operations. --- open-sse/config/providerRegistry.ts | 45 +-- open-sse/services/comboMetrics.ts | 27 +- open-sse/services/usage.ts | 9 +- scripts/check-permissions.sh | 2 +- .../services/combo-metrics-memory.test.ts | 276 ++++++++++++++++++ 5 files changed, 302 insertions(+), 57 deletions(-) create mode 100644 tests/unit/services/combo-metrics-memory.test.ts diff --git a/open-sse/config/providerRegistry.ts b/open-sse/config/providerRegistry.ts index e49b117ee8..793cc6f58f 100644 --- a/open-sse/config/providerRegistry.ts +++ b/open-sse/config/providerRegistry.ts @@ -4227,48 +4227,7 @@ const _REGISTRY_EAGER: Record = { }, }; -/** - * Lazy registry proxy — entries are only materialized on first access. - * Reduces memory pressure when most providers are unused. - */ -const _registryCache = new Map(); -const _registryKeys: string[] = Object.keys(_REGISTRY_EAGER); - -export const REGISTRY: Record = new Proxy( - {} as Record, - { - get(_target, prop: string) { - if (typeof prop === "symbol") return undefined; - if (_registryCache.has(prop)) { - return _registryCache.get(prop); - } - if (prop in _REGISTRY_EAGER) { - const entry = _REGISTRY_EAGER[prop]; - _registryCache.set(prop, entry); - return entry; - } - return undefined; - }, - has(_target, prop: string) { - if (typeof prop === "symbol") return false; - return prop in _REGISTRY_EAGER; - }, - ownKeys() { - return _registryKeys; - }, - getOwnPropertyDescriptor(_target, prop: string) { - if (typeof prop === "string" && prop in _REGISTRY_EAGER) { - return { - configurable: true, - enumerable: true, - value: _REGISTRY_EAGER[prop], - writable: false, - }; - } - return undefined; - }, - }, -); +export const REGISTRY: Record = _REGISTRY_EAGER; // ── Generator Functions ─────────────────────────────────────────────────── @@ -4425,7 +4384,7 @@ export function getRegistryEntry(provider: string): RegistryEntry | null { /** Get all registered provider IDs */ export function getRegisteredProviders(): string[] { - return _registryKeys; + return Object.keys(REGISTRY); } // Precomputed map: modelId → unsupportedParams (O(1) lookup instead of O(N×M) scan). diff --git a/open-sse/services/comboMetrics.ts b/open-sse/services/comboMetrics.ts index 3d4ab79918..6c6e8e71da 100644 --- a/open-sse/services/comboMetrics.ts +++ b/open-sse/services/comboMetrics.ts @@ -191,20 +191,31 @@ const shadowMetrics = new Map(); const MAX_METRICS_ENTRIES = 500; const METRICS_TTL_MS = 60 * 60 * 1000; // 1 hour -function evictOldestMetric(): void { +function evictOldestMetric(targetMap: Map): void { let oldest: string | null = null; let oldestTime = Infinity; - for (const [name, entry] of metrics) { - const t = new Date(entry.lastUsedAt ?? 0).getTime(); + for (const [name, entry] of targetMap) { + const t = entry.lastUsedAt ? new Date(entry.lastUsedAt).getTime() : Date.now(); if (t < oldestTime) { oldestTime = t; oldest = name; } } - if (oldest) { metrics.delete(oldest); shadowMetrics.delete(oldest); } + if (oldest) { + metrics.delete(oldest); + shadowMetrics.delete(oldest); + } } const _metricsCleanupTimer = setInterval(() => { const now = Date.now(); for (const [name, entry] of metrics) { - if (now - new Date(entry.lastUsedAt ?? 0).getTime() > METRICS_TTL_MS) { + const lastUsed = entry.lastUsedAt ? new Date(entry.lastUsedAt).getTime() : now; + if (now - lastUsed > METRICS_TTL_MS) { + metrics.delete(name); + shadowMetrics.delete(name); + } + } + for (const [name, entry] of shadowMetrics) { + const lastUsed = entry.lastUsedAt ? new Date(entry.lastUsedAt).getTime() : now; + if (now - lastUsed > METRICS_TTL_MS) { metrics.delete(name); shadowMetrics.delete(name); } @@ -241,7 +252,7 @@ export function recordComboRequest( } ): void { if (!metrics.has(comboName) && metrics.size >= MAX_METRICS_ENTRIES) { - evictOldestMetric(); + evictOldestMetric(metrics); } if (!metrics.has(comboName)) { metrics.set(comboName, createComboEntry(strategy)); @@ -310,7 +321,7 @@ export function recordComboShadowRequest( } ): void { if (!shadowMetrics.has(comboName) && shadowMetrics.size >= MAX_METRICS_ENTRIES) { - evictOldestMetric(); + evictOldestMetric(shadowMetrics); } if (!shadowMetrics.has(comboName)) { shadowMetrics.set(comboName, createShadowEntry()); @@ -426,7 +437,7 @@ export function getAllComboMetrics(): Record { */ export function recordComboIntent(comboName: string, intent: string): void { if (!metrics.has(comboName) && metrics.size >= MAX_METRICS_ENTRIES) { - evictOldestMetric(); + evictOldestMetric(metrics); } if (!metrics.has(comboName)) { metrics.set(comboName, createComboEntry("priority")); diff --git a/open-sse/services/usage.ts b/open-sse/services/usage.ts index f784c89005..1e8401c7a4 100644 --- a/open-sse/services/usage.ts +++ b/open-sse/services/usage.ts @@ -1826,20 +1826,19 @@ const _antigravityCreditProbeInflight = new Map>( // purges stale entries so keys accessed once and never again don't leak memory. // The 2 inflight Maps (availableModelsInflight, creditProbeInflight) self-clean // when the Promise resolves/rejects, so they are NOT touched here. -const SUB_CACHE_TTL_MS = 10 * 60 * 1000; // 10 minutes const _usageCacheCleanupTimer = setInterval(() => { const now = Date.now(); for (const [key, entry] of _geminiCliSubCache) { - if (now - entry.fetchedAt > SUB_CACHE_TTL_MS) _geminiCliSubCache.delete(key); + if (now - entry.fetchedAt > GEMINI_CLI_CACHE_TTL_MS) _geminiCliSubCache.delete(key); } for (const [key, entry] of _antigravitySubCache) { - if (now - entry.fetchedAt > SUB_CACHE_TTL_MS) _antigravitySubCache.delete(key); + if (now - entry.fetchedAt > ANTIGRAVITY_CACHE_TTL_MS) _antigravitySubCache.delete(key); } for (const [key, entry] of _antigravityAvailableModelsCache) { - if (now - entry.fetchedAt > SUB_CACHE_TTL_MS) _antigravityAvailableModelsCache.delete(key); + if (now - entry.fetchedAt > ANTIGRAVITY_MODELS_CACHE_TTL_MS) _antigravityAvailableModelsCache.delete(key); } for (const [key, entry] of _antigravityCreditProbeCache) { - if (now - entry.fetchedAt > SUB_CACHE_TTL_MS) _antigravityCreditProbeCache.delete(key); + if (now - entry.fetchedAt > ANTIGRAVITY_CREDIT_PROBE_TTL_MS) _antigravityCreditProbeCache.delete(key); } }, 5 * 60 * 1000); // every 5 minutes _usageCacheCleanupTimer.unref?.(); // Don't prevent process exit diff --git a/scripts/check-permissions.sh b/scripts/check-permissions.sh index 45846806ca..88e2dba4bc 100755 --- a/scripts/check-permissions.sh +++ b/scripts/check-permissions.sh @@ -5,7 +5,7 @@ set -e # If OMNIROUTE_MEMORY_MB is set, build NODE_OPTIONS dynamically so the # user can tune heap size via environment without editing the Dockerfile. if [ -n "$OMNIROUTE_MEMORY_MB" ]; then - export NODE_OPTIONS="--max-old-space-size=${OMNIROUTE_MEMORY_MB} ${NODE_OPTIONS:-}" + export NODE_OPTIONS="${NODE_OPTIONS:-} --max-old-space-size=${OMNIROUTE_MEMORY_MB}" fi if [ -d "/app/data" ] && [ ! -w "/app/data" ]; then diff --git a/tests/unit/services/combo-metrics-memory.test.ts b/tests/unit/services/combo-metrics-memory.test.ts new file mode 100644 index 0000000000..bc6b2390f9 --- /dev/null +++ b/tests/unit/services/combo-metrics-memory.test.ts @@ -0,0 +1,276 @@ +/** + * Tests for comboMetrics memory management — eviction and TTL behavior. + * + * Verifies that: + * - Recording and retrieval work correctly + * - MAX_METRICS_ENTRIES (500) cap is enforced via eviction of oldest entries + * - Shadow metrics are isolated from production metrics + * - resetComboMetrics and resetAllComboMetrics clear state correctly + * - getAllComboMetrics returns all entries + * - recordComboIntent tracks intent counts + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +const mod = await import("../../../open-sse/services/comboMetrics.ts"); +const { + recordComboRequest, + recordComboShadowRequest, + getComboMetrics, + getAllComboMetrics, + resetComboMetrics, + resetAllComboMetrics, + recordComboIntent, +} = mod; + +// Reset all state before each test group to avoid cross-test pollution. +resetAllComboMetrics(); + +// ─── Basic record + retrieve ──────────────────────────────────────────────── + +test("recordComboRequest: basic recording and retrieval", () => { + resetAllComboMetrics(); + recordComboRequest("test-combo", "gpt-4", { + success: true, + latencyMs: 100, + }); + const metrics = getComboMetrics("test-combo"); + assert.ok(metrics, "metrics should exist for recorded combo"); + assert.equal(metrics.totalRequests, 1, "should have 1 total request"); + assert.equal(metrics.totalSuccesses, 1, "should have 1 success"); + assert.equal(metrics.totalFailures, 0, "should have 0 failures"); + assert.equal(metrics.successRate, 100, "success rate should be 100%"); + assert.equal(metrics.avgLatencyMs, 100, "avg latency should be 100ms"); +}); + +test("recordComboRequest: tracks per-model metrics", () => { + resetAllComboMetrics(); + recordComboRequest("combo-1", "gpt-4", { success: true, latencyMs: 100 }); + recordComboRequest("combo-1", "claude-3", { success: false, latencyMs: 200 }); + const metrics = getComboMetrics("combo-1"); + assert.ok(metrics); + assert.equal(metrics.totalRequests, 2, "should have 2 total requests"); + assert.equal(metrics.successRate, 50, "success rate should be 50%"); + assert.ok(metrics.byModel["gpt-4"], "should have gpt-4 model metrics"); + assert.ok(metrics.byModel["claude-3"], "should have claude-3 model metrics"); + assert.equal(metrics.byModel["gpt-4"].successRate, 100); + assert.equal(metrics.byModel["claude-3"].successRate, 0); +}); + +test("recordComboRequest: handles null model string gracefully", () => { + resetAllComboMetrics(); + recordComboRequest("combo-null", null, { success: true, latencyMs: 50 }); + const metrics = getComboMetrics("combo-null"); + assert.ok(metrics, "metrics should exist even with null model"); + assert.equal(metrics.totalRequests, 1); + // null model should not create per-model entries + assert.equal(Object.keys(metrics.byModel).length, 0, "should have no model entries"); +}); + +test("recordComboRequest: tracks fallback count", () => { + resetAllComboMetrics(); + recordComboRequest("combo-fb", "gpt-4", { + success: true, + latencyMs: 200, + fallbackCount: 2, + }); + const metrics = getComboMetrics("combo-fb"); + assert.ok(metrics); + assert.equal(metrics.totalFallbacks, 2, "should track 2 fallbacks"); + // fallbackRate = (totalFallbacks / totalRequests) * 100 = (2/1) * 100 = 200 + assert.equal(metrics.fallbackRate, 200, "fallback rate should be 200% (2 fallbacks on 1 request)"); +}); + +// ─── getComboMetrics returns null for unknown combos ───────────────────────── + +test("getComboMetrics: returns null for unknown combo", () => { + const metrics = getComboMetrics("nonexistent-combo-xyz"); + assert.equal(metrics, null, "unknown combo should return null"); +}); + +// ─── resetComboMetrics ─────────────────────────────────────────────────────── + +test("resetComboMetrics: clears a specific combo", () => { + resetAllComboMetrics(); + recordComboRequest("to-keep", "gpt-4", { success: true, latencyMs: 100 }); + recordComboRequest("to-reset", "gpt-4", { success: true, latencyMs: 100 }); + resetComboMetrics("to-reset"); + assert.ok(getComboMetrics("to-keep"), "to-keep should still exist"); + assert.equal(getComboMetrics("to-reset"), null, "to-reset should be cleared"); +}); + +// ─── resetAllComboMetrics ──────────────────────────────────────────────────── + +test("resetAllComboMetrics: clears all production and shadow metrics", () => { + recordComboRequest("combo-a", "gpt-4", { success: true, latencyMs: 100 }); + recordComboShadowRequest("shadow-a", "gpt-4", { success: true, latencyMs: 100 }); + assert.ok(getComboMetrics("combo-a"), "combo-a should exist before reset"); + assert.ok(getComboMetrics("shadow-a"), "shadow-a should exist before reset"); + resetAllComboMetrics(); + assert.equal(getComboMetrics("combo-a"), null, "combo-a should be cleared"); + assert.equal(getComboMetrics("shadow-a"), null, "shadow-a should be cleared"); +}); + +// ─── getAllComboMetrics ─────────────────────────────────────────────────────── + +test("getAllComboMetrics: returns all production and shadow entries", () => { + resetAllComboMetrics(); + recordComboRequest("combo-x", "gpt-4", { success: true, latencyMs: 100 }); + recordComboRequest("combo-y", "claude-3", { success: false, latencyMs: 200 }); + recordComboShadowRequest("shadow-x", "gpt-4", { success: true, latencyMs: 150 }); + const all = getAllComboMetrics(); + assert.ok(all["combo-x"], "should include combo-x"); + assert.ok(all["combo-y"], "should include combo-y"); + assert.ok(all["shadow-x"], "should include shadow-x from shadow metrics"); +}); + +// ─── Shadow metrics ────────────────────────────────────────────────────────── + +test("recordComboShadowRequest: tracks shadow metrics separately from production", () => { + resetAllComboMetrics(); + recordComboRequest("prod-combo", "gpt-4", { success: true, latencyMs: 100 }); + recordComboShadowRequest("prod-combo", "claude-3", { success: false, latencyMs: 200 }); + const metrics = getComboMetrics("prod-combo"); + assert.ok(metrics); + // Production sees only gpt-4 + assert.equal(metrics.totalRequests, 1, "production should have 1 request"); + assert.equal(metrics.successRate, 100, "production success rate should be 100%"); + // Shadow is separate + assert.ok(metrics.shadow, "shadow metrics should be present"); + assert.equal(metrics.shadow.totalRequests, 1, "shadow should have 1 request"); + assert.equal(metrics.shadow.successRate, 0, "shadow success rate should be 0%"); +}); + +test("recordComboShadowRequest: shadow-only combo returns in getAllComboMetrics", () => { + resetAllComboMetrics(); + recordComboShadowRequest("shadow-only", "gpt-4", { success: true, latencyMs: 50 }); + const all = getAllComboMetrics(); + assert.ok(all["shadow-only"], "shadow-only combo should appear in getAllComboMetrics"); +}); + +// ─── recordComboIntent ─────────────────────────────────────────────────────── + +test("recordComboIntent: tracks intent counts on existing combo", () => { + resetAllComboMetrics(); + recordComboRequest("intent-combo", "gpt-4", { success: true, latencyMs: 100 }); + recordComboIntent("intent-combo", "chat"); + recordComboIntent("intent-combo", "chat"); + recordComboIntent("intent-combo", "code"); + const metrics = getComboMetrics("intent-combo"); + assert.ok(metrics); + assert.equal(metrics.intentCounts["chat"], 2, "should count 2 chat intents"); + assert.equal(metrics.intentCounts["code"], 1, "should count 1 code intent"); +}); + +test("recordComboIntent: creates combo entry if not yet recorded", () => { + resetAllComboMetrics(); + recordComboIntent("new-intent-combo", "search"); + const metrics = getComboMetrics("new-intent-combo"); + assert.ok(metrics, "combo should be created by recordComboIntent"); + assert.equal(metrics.intentCounts["search"], 1); +}); + +// ─── Eviction: MAX_METRICS_ENTRIES cap ─────────────────────────────────────── + +test("eviction: inserting a new combo at capacity evicts the oldest entry", () => { + resetAllComboMetrics(); + const MAX = 500; + // Fill to capacity with unique combos + for (let i = 0; i < MAX; i++) { + recordComboRequest(`fill-${i}`, "gpt-4", { success: true, latencyMs: 10 }); + } + // All 500 should be present + assert.ok(getComboMetrics("fill-0"), "first inserted combo should exist at capacity"); + assert.ok(getComboMetrics(`fill-${MAX - 1}`), "last inserted combo should exist at capacity"); + + // Insert one more — should trigger eviction of the oldest entry + recordComboRequest("new-after-capacity", "gpt-4", { success: true, latencyMs: 50 }); + const all = getAllComboMetrics(); + const totalProduction = Object.keys(all).filter((k) => { + const m = all[k]; + return m && m.totalRequests > 0; + }).length; + // Map size should not exceed MAX (the new one replaced the oldest) + assert.ok(totalProduction <= MAX, `production combos (${totalProduction}) should not exceed cap (${MAX})`); + assert.ok( + getComboMetrics("new-after-capacity"), + "newly inserted combo should exist after eviction" + ); +}); + +test("eviction: shadow metrics respect their own MAX_METRICS_ENTRIES cap", () => { + resetAllComboMetrics(); + const MAX = 500; + // Fill shadow metrics to capacity + for (let i = 0; i < MAX; i++) { + recordComboShadowRequest(`shadow-fill-${i}`, "gpt-4", { + success: true, + latencyMs: 10, + }); + } + assert.ok( + getComboMetrics("shadow-fill-0"), + "first shadow combo should exist at capacity" + ); + + // Insert one more shadow — should trigger eviction + recordComboShadowRequest("shadow-after-capacity", "gpt-4", { + success: true, + latencyMs: 50, + }); + assert.ok( + getComboMetrics("shadow-after-capacity"), + "newly inserted shadow combo should exist after eviction" + ); +}); + +test("eviction: recordComboIntent respects MAX_METRICS_ENTRIES cap", () => { + resetAllComboMetrics(); + const MAX = 500; + // Fill production metrics to capacity + for (let i = 0; i < MAX; i++) { + recordComboRequest(`intent-fill-${i}`, "gpt-4", { success: true, latencyMs: 10 }); + } + // Adding intent for a NEW combo should trigger eviction + recordComboIntent("intent-after-capacity", "chat"); + assert.ok( + getComboMetrics("intent-after-capacity"), + "newly created intent combo should exist after eviction" + ); +}); + +test("eviction: updating an existing combo at capacity does NOT evict", () => { + resetAllComboMetrics(); + const MAX = 500; + // Fill to capacity + for (let i = 0; i < MAX; i++) { + recordComboRequest(`update-${i}`, "gpt-4", { success: true, latencyMs: 10 }); + } + // Update an existing combo — should NOT trigger eviction since it's not a new key + recordComboRequest("update-0", "gpt-4", { success: false, latencyMs: 50 }); + const metrics = getComboMetrics("update-0"); + assert.ok(metrics, "updated combo should still exist"); + assert.equal(metrics.totalRequests, 2, "updated combo should have 2 requests"); +}); + +// ─── Strategy tracking ─────────────────────────────────────────────────────── + +test("recordComboRequest: stores the routing strategy", () => { + resetAllComboMetrics(); + recordComboRequest("strat-combo", "gpt-4", { + success: true, + latencyMs: 100, + strategy: "weighted", + }); + const metrics = getComboMetrics("strat-combo"); + assert.ok(metrics); + assert.equal(metrics.strategy, "weighted", "should store the strategy"); +}); + +test("recordComboRequest: defaults strategy to 'priority'", () => { + resetAllComboMetrics(); + recordComboRequest("default-strat", "gpt-4", { success: true, latencyMs: 100 }); + const metrics = getComboMetrics("default-strat"); + assert.ok(metrics); + assert.equal(metrics.strategy, "priority", "default strategy should be 'priority'"); +});