fix: CPU leak from Bottleneck limiter accumulation + per-request optimizations - #2951
Conversation
…ded in-memory caches Root cause: Bottleneck rate limiter instances in rateLimitManager accumulate without cleanup. Each instance runs an internal heartbeat setInterval every 250ms. Under heavy load with many provider:connection:model combinations, hundreds of limiters accumulate causing CPU to grow ~0.1%/min until server collapse (~2% after 5 minutes of intensive use). Changes: - rateLimitManager: Add idle limiter eviction in watchdogTick() using the previously defined but unused INACTIVE_LIMITER_MS threshold. Populate limiterLastUsed on every getLimiter() call. Clean up all 3 Maps (limiters, lastDispatchAt, limiterLastUsed) consistently. - combo.ts: Add size-based FIFO eviction to rrCounters, resetAwareConnectionCache, and resetAwareQuotaCache Maps. Convert per-target log.info calls in combo execution loops to log.debug?. to reduce serialization overhead. - chatCore.ts: Fix double-serialization in estimateTokens(JSON.stringify(x)) calls (estimateTokens already handles objects). Make trace() conditional on OMNIRROUTE_TRACE/DEBUG env vars. Make per-request usage logging conditional. - apiKeyRotator.ts: Add eviction guards to _keyHealth and _connectionExtraKeys Maps (MAX 500 entries each). Ensure removeConnectionIndex cleans all 3 Maps. - codexQuotaFetcher.ts: Add eviction guard to connectionRegistry and quotaCache Maps (MAX 200 entries each).
Extract 3 high-value CPU/RAM optimizations from perf branch: 1. estimateSizeFast() — fast object-tree size estimator replacing JSON.stringify().length in isSmallEnoughForSemanticCache(). Walks object tree with a stack, zero string allocation, early exit at 256KB. 2. Consolidate settings reads — move getCachedSettings() to a single early read in handleChatCore(), eliminating a redundant second read 200 lines later. Also removes the isDetailedLoggingEnabled() wrapper call (reads settings internally) in favor of direct field check. 3. Registry Proxy→direct export — convert 8 registries from lazy Proxy+getOrCreate pattern to simple exported const objects. Eliminates Proxy trap overhead on every provider property access during routing. Affected: audio, embedding, image, moderation, music, rerank, search, video registries (-451 lines of Proxy boilerplate). These changes are independent of the CPU leak fix (limiter eviction) and complement it by reducing per-request CPU overhead.
There was a problem hiding this comment.
Code Review
This pull request simplifies provider registries by replacing dynamic proxies with static exported constants, optimizes performance in chatCore.ts by replacing JSON.stringify with a fast size estimator and direct object references, and introduces eviction limits across various in-memory caches to prevent unbounded memory growth. The review feedback highlights a critical correctness issue across multiple files where cache eviction logic triggers on map size alone, which will prematurely evict valid entries when updating existing keys. Additionally, the feedback identifies a potential TypeError in chatCore.ts if provider is null, and suggests a performance optimization to avoid array allocations in the hot path of the new size estimator.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Files Reviewed (8 files)
Reviewed by laguna-m.1-20260312:free · 2,454,040 tokens |
- Add !has(key) guard before eviction to avoid evicting entries that are about to be updated (combo.ts, apiKeyRotator.ts, codexQuotaFetcher.ts) - Use optional chaining for provider?.toUpperCase() null safety - Replace Object.values() with for-in in estimateSizeFast hot path
- estimateSizeFast: add WeakSet cycle detection to prevent infinite loop on circular object references - trace(): wrap JSON.stringify(extra) in try-catch to handle BigInt, circular refs, or other non-serializable values gracefully - Registry API change (Comment 3): verified all callers already use new getter functions — no broken call sites
5 new test files covering all 13 changed production files: - estimateSizeFast.test.ts: 16 tests for fast size estimator (circular ref protection, early exit, nested structures, Map safety) - eviction-guards-apiKeyRotator.test.ts: 5 tests for Map eviction guards (!has() check prevents evicting existing keys on update) - eviction-guards-codexQuotaFetcher.test.ts: 4 tests for connectionRegistry and quotaCache eviction guards - rateLimitManager-idle-eviction.test.ts: 6 tests for idle limiter cleanup, limiterLastUsed tracking, and shutdown behavior - registry-direct-exports.test.ts: 20 tests verifying all 8 registries export plain objects (no Proxy traps, no lazy getters, mutable entries) Extract estimateSizeFast/isSmallEnoughForSemanticCache into standalone open-sse/utils/estimateSize.ts to make them testable without importing the entire chatCore.ts dependency tree.
|
Thank you for your contribution! This PR has been reviewed and integrated into the upcoming |
- Add !has(key) guard before eviction to avoid evicting entries that are about to be updated (combo.ts, apiKeyRotator.ts, codexQuotaFetcher.ts) - Use optional chaining for provider?.toUpperCase() null safety - Replace Object.values() with for-in in estimateSizeFast hot path
- estimateSizeFast: add WeakSet cycle detection to prevent infinite loop on circular object references - trace(): wrap JSON.stringify(extra) in try-catch to handle BigInt, circular refs, or other non-serializable values gracefully - Registry API change (Comment 3): verified all callers already use new getter functions — no broken call sites
…mizations (diegosouzapw#2951) Integrated into release/v3.8.8
- Add !has(key) guard before eviction to avoid evicting entries that are about to be updated (combo.ts, apiKeyRotator.ts, codexQuotaFetcher.ts) - Use optional chaining for provider?.toUpperCase() null safety - Replace Object.values() with for-in in estimateSizeFast hot path
- estimateSizeFast: add WeakSet cycle detection to prevent infinite loop on circular object references - trace(): wrap JSON.stringify(extra) in try-catch to handle BigInt, circular refs, or other non-serializable values gracefully - Registry API change (Comment 3): verified all callers already use new getter functions — no broken call sites
…mizations (diegosouzapw#2951) Integrated into release/v3.8.8
- Add !has(key) guard before eviction to avoid evicting entries that are about to be updated (combo.ts, apiKeyRotator.ts, codexQuotaFetcher.ts) - Use optional chaining for provider?.toUpperCase() null safety - Replace Object.values() with for-in in estimateSizeFast hot path
- estimateSizeFast: add WeakSet cycle detection to prevent infinite loop on circular object references - trace(): wrap JSON.stringify(extra) in try-catch to handle BigInt, circular refs, or other non-serializable values gracefully - Registry API change (Comment 3): verified all callers already use new getter functions — no broken call sites
…mizations (diegosouzapw#2951) Integrated into release/v3.8.8
Problem
Under heavy load (~5 min of intensive API usage), CPU grows ~0.1%/min until it hits ~2% and the server collapses. Root cause: Bottleneck rate limiter instances accumulate without cleanup — each creates a 250ms heartbeat interval that never stops.
Fix — 2 commits
1. CPU leak fix (+100 lines)
2. Per-request optimizations extracted from perf branch (+97/-451 lines)
Mechanism of collapse
Testing