fix(lid): bound LIDMappingStore cache to stop unbounded timer/memory growth#2640
fix(lid): bound LIDMappingStore cache to stop unbounded timer/memory growth#2640lopesclayton wants to merge 1 commit into
Conversation
…growth Add `max` and disable `ttlAutopurge` so each mapping no longer holds a 3-day timer; evicted entries fall back to the DB.
📝 WalkthroughWalkthroughThe ChangesLID Mapping Cache Configuration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Signal/lid-mapping.ts (1)
7-8: 💤 Low valueConsider removing the "PATCH:" prefix from the comment.
The "PATCH:" prefix suggests a temporary workaround, but this appears to be the permanent solution to the timer accumulation issue. Consider rephrasing to a more standard explanatory comment, e.g., "Bounds the cache and disables auto-purge timers; TTL is checked lazily on get to avoid per-entry timers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Signal/lid-mapping.ts` around lines 7 - 8, Remove the "PATCH:" prefix from the comment block in the lid-mapping.ts file (around line 7-8) and rephrase it as a standard explanatory comment. Instead of "// PATCH: `max` bounds the cache and `ttlAutopurge: false` drops the per-entry...", change it to describe this as the permanent solution rather than a temporary workaround. The comment should explain that the cache bounds and disabled auto-purge timers with lazy TTL checking are the intended approach to avoid per-entry timer accumulation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Signal/lid-mapping.ts`:
- Around line 7-8: Remove the "PATCH:" prefix from the comment block in the
lid-mapping.ts file (around line 7-8) and rephrase it as a standard explanatory
comment. Instead of "// PATCH: `max` bounds the cache and `ttlAutopurge: false`
drops the per-entry...", change it to describe this as the permanent solution
rather than a temporary workaround. The comment should explain that the cache
bounds and disabled auto-purge timers with lazy TTL checking are the intended
approach to avoid per-entry timer accumulation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d7f17093-accf-4515-b6e8-6051ce9d0dfe
📒 Files selected for processing (1)
src/Signal/lid-mapping.ts
|
This PR is stale because it has been open for 14 days with no activity. Remove the stale label or comment or this will be closed in 14 days |
Problem
LIDMappingStore.mappingCachewas created as:With no
maxandttlAutopurge: true, the cache grows without bound and arms one 3-daysetTimeoutper entry. Under a flood of@lidmappings this accumulates tens of thousands of timers, leaking memory until OOM (heap dumps show a largeTimersListof259_200_000mstimers).Change
max: 10000— bounds the cache; evicted entries are simply re-fetched from the key store on the next miss.ttlAutopurge: false— removes the per-entry timer. TTL is still honored, just evaluated lazily ongetinstead of via a background timer.Also fixed a stale
// 7 dayscomment (the value has always been 3 days).Notes
Summary by cubic
Bound the
LIDMappingStorecache and removed per-entry timers to stop unbounded memory/timer growth, preventing OOM under heavy@lidmapping traffic.LRUCachetomax: 10000andttlAutopurge: false; TTL stays 3 days and is checked on get.Written for commit a6e43e3. Summary will update on new commits.
Summary by CodeRabbit