Fix SSE events map memory leak#6524
Conversation
The cleanup loop that removes stale resources after a re-LIST was not using batchPromiseAll, so it blocked the event loop for the entire duration—preventing liveness probe responses. Additionally, the items.find() inside the loop was O(n²). With 22K+ Group resources this caused probe timeouts and CrashLoopBackOff. - Convert the for...in cleanup loop to use batchPromiseAll, yielding the event loop every 100 entries via setImmediate - Replace O(n²) items.find() with a Set<string> for O(1) UID lookups Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: fxiang1 <fxiang@redhat.com>
Cache entries can be deleted by a concurrent watch event during the setImmediate yield between batches. Skip UIDs whose cache entry was removed to prevent a TypeError on undefined.compressed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: fxiang1 <fxiang@redhat.com>
Signed-off-by: fxiang1 <fxiang@redhat.com>
Signed-off-by: fxiang1 <fxiang@redhat.com>
Signed-off-by: fxiang1 <fxiang@redhat.com>
Signed-off-by: fxiang1 <fxiang@redhat.com>
Signed-off-by: fxiang1 <fxiang@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesServer-side event storage
Kubernetes cache reconciliation
Compression dictionary promotion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant cacheResource
participant deleteResource
participant ResourceCache
participant ServerSideEvents
cacheResource->>deleteResource: stale resource and expected cache entry
deleteResource->>ResourceCache: verify expected cache entry
deleteResource->>ServerSideEvents: broadcast DELETED event
deleteResource->>ResourceCache: verify expected cache entry again
deleteResource->>ResourceCache: delete cache entry
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Each browser connection triggered two JSON.stringify calls over ALL cached events (compressed buffers + fully inflated resources) just to compute a compression ratio for a log message. For 10,000 resources this created ~500 MB of temporary strings that V8 promoted to old generation, causing RSS to ratchet up with each connection and never come back down. Remove both sizeOf calls and the unused import. The event count is still logged. Signed-off-by: fxiang1 <fxiang@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@backend/src/lib/compression.ts`:
- Around line 206-226: Move the bigStrings “seen twice” promotion logic inside
the resource.length < 32 && !resource.endsWith('=') guard, after the timestamp
and numeric-string handling. Return the raw resource immediately for its first
short-string occurrence, and ensure long or “=”-terminated strings bypass
bigStrings and dictionary.add entirely.
In `@backend/test/lib/compression.test.ts`:
- Line 109: Update the test case “indexes non-timestamp short strings only after
they are seen twice” to avoid shared compression module state by using unique
input strings that cannot have been cached by other tests; keep the assertions
aligned with those renamed values.
- Around line 129-135: Extend the compression test around deflateResource to
include a string of at least 32 characters in resource. Assert that dict.arr
does not contain it after the first compression and remains absent after
repeated compressions, while preserving the existing short-string promotion
assertions.
🪄 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: Enterprise
Run ID: 9d3acdfd-bd72-4403-b5e7-ae7c1984ab93
📒 Files selected for processing (3)
backend/src/lib/compression.tsbackend/src/lib/server-side-events.tsbackend/test/lib/compression.test.ts
💤 Files with no reviewable changes (1)
- backend/src/lib/server-side-events.ts
| }) | ||
|
|
||
| it('still adds non-timestamp short strings to the dictionary', async () => { | ||
| it('indexes non-timestamp short strings only after they are seen twice', async () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ensure test isolation against shared module state.
The bigStrings cache is a module-level global in compression.ts and is not cleared between tests. If other tests in this suite happen to compress the strings 'Healthy' or 'Synced' before this test runs, those strings will already be in bigStrings. This would cause them to be incorrectly promoted to the dictionary on the first deflateResource call, making this test fail unpredictably.
As per coding guidelines, tests must properly isolate dependencies. Ensure these test strings are unique (e.g., 'Healthy-123', 'Synced-123'), or provide a test-only export in compression.ts to reset bigStrings in a beforeEach hook.
🤖 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 `@backend/test/lib/compression.test.ts` at line 109, Update the test case
“indexes non-timestamp short strings only after they are seen twice” to avoid
shared compression module state by using unique input strings that cannot have
been cached by other tests; keep the assertions aligned with those renamed
values.
Source: Coding guidelines
| expect(dict.arr).not.toContain('Healthy') | ||
| expect(dict.arr).not.toContain('Synced') | ||
|
|
||
| expect(dictEntries).toContain('Healthy') | ||
| expect(dictEntries).toContain('Synced') | ||
| // Second encounter promotes them to the dictionary | ||
| await deflateResource(resource, dict) | ||
| expect(dict.arr).toContain('Healthy') | ||
| expect(dict.arr).toContain('Synced') |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add test coverage to verify long strings are not indexed.
The test name claims the code indexes "short strings" only after they are seen twice, but there is no assertion verifying the behavior for long strings (>= 32 characters). Because of the logic error in compression.ts, long strings are currently also being inadvertently indexed.
As per path instructions, do not infer behavior from test names or comments alone; confirm it from the actual implementation. Additionally, as per coding guidelines, tests should meaningfully cover behavior. Please add an assertion or a separate test case to confirm that strings >= 32 characters are never added to the dictionary, even after being compressed multiple times. This will catch the source bug and prevent regressions.
🤖 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 `@backend/test/lib/compression.test.ts` around lines 129 - 135, Extend the
compression test around deflateResource to include a string of at least 32
characters in resource. Assert that dict.arr does not contain it after the first
compression and remains absent after repeated compressions, while preserving the
existing short-string promotion assertions.
Sources: Coding guidelines, Path instructions
Short non-numeric strings were unconditionally indexed on first encounter, permanently occupying dictionary space even for one-off values like changing label or annotation values. Apply the same 'seen twice' gate (via bigStrings FifoSet) that long strings already use. Numeric-looking strings are still indexed immediately because the decompressor would misinterpret them as dictionary indices otherwise. Non-numeric short strings are stored as-is on first encounter and only promoted to the dictionary when seen a second time. Signed-off-by: fxiang1 <fxiang@redhat.com>
7a70047 to
c8ddfb5
Compare
|
1 participant
📝 Summary
Ticket Summary (Title):
Fix SSE memory leaks: events map fragmentation, handleRequest sizeOf allocations, and eventDict dictionary growth
Type of Change:
Problem
Three memory issues in the backend SSE layer cause pod memory to rise monotonically and never reclaim:
1.
ServerSideEvents.events— V8 hash table fragmentationthis.eventswas a plainRecord<number, ServerSideEvent>with monotonically increasing integer keys. Every resource modification callspushEvent(incrementing the key by 2) andremoveEvent(deleteon the old key). V8 uses "dictionary elements" mode for sparse integer-keyed objects, anddeletemarks slots as holes without shrinking the backing hash table. With thousands of resources under constant updates, the internal hash table grows to accommodate ever-larger keys but never compacts — causing RSS to rise permanently.Fix: Replace the plain object with a
Map<number, ServerSideEvent>.Mapproperly releases memory when entries are deleted.2.
handleRequest— expensivesizeOfcalls per browser connectionEvery time a browser connects to the SSE endpoint,
handleRequestcalledsizeOf()twice:sizeOf(values)—JSON.stringifyof ALL compressed event BufferssizeOf(sending)—JSON.stringifyof ALL fully-inflated resource objectsFor an environment with 10,000 resources, this created ~500 MB of temporary strings per connection — just to compute a compression ratio for a single
logger.infoline. These large allocations get promoted to V8's old generation before the young-gen scavenger can collect them, and Node.js almost never returns freed old-gen memory to the OS. Each browser connection ratchets RSS up, and it stays there.Fix: Remove both
sizeOfcalls and the unused import. The event count is still logged.3.
eventDictcompression dictionary — unbounded growthThe shared compression dictionary (
eventDict) only grows viaadd()— there is no eviction or cleanup path. Short non-numeric strings (< 32 chars) were unconditionally indexed on first encounter, permanently occupying dictionary space even for one-off values like changing label or annotation values. With constant label updates producing new unique values, the dictionary grows without bound.Longer strings (≥ 32 chars) already had a "seen twice" gate via the
bigStringsFifoSet — a string must appear at least twice before being promoted to the dictionary. Short strings bypassed this gate entirely.Fix: Apply the same "seen twice" gate to short strings. Numeric-looking strings (e.g.
"42") are still indexed immediately because the decompressor would misinterpret them as dictionary indices otherwise (Number.isInteger(Number("42"))istrue). Non-numeric short strings are stored as-is on first encounter and only promoted to the dictionary when seen a second time. This is backward-compatible: existing compressed Buffers still decompress correctly since all previously-indexed entries remain in the dictionary.Changes
backend/src/lib/server-side-events.tseventsfromRecord<number, ServerSideEvent>→Map<number, ServerSideEvent>; updatedset(),delete(),Array.from(.values())at all internal access sites; removedsizeOfimport and both calls inhandleRequestbackend/src/lib/compression.tsbigStrings"seen twice" gatebackend/test/routes/events.test.tsgetEvents()consumers:Object.keys()→.keys()/.size;events[id]→events.get(id);for..in+delete→for..of+.delete()backend/test/routes/aggregators/utils.test.tsfor..in+delete→for..of+.delete()in beforeEach/afterEach cleanupbackend/test/lib/compression.test.ts✅ Checklist
General
If Bugfix
🗒️ Notes for Reviewers
sizeOffunction itself (inaggregators/utils.ts) is NOT removed — it's still used by other callers. Only the import and usage fromserver-side-events.tsis removed.compressionfield is dropped from the log line. If the compression ratio is needed, it could be computed cheaply by summing Buffer byte lengths instead of doing full JSON serialization.Summary by CodeRabbit