ACM-34279 fix compression dictionary memory leak#6305
Conversation
Assisted-by: Cursor (Claude Opus 4.6 High) Signed-off-by: Kevin Cormier <kcormier@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR enhances resource compression by detecting ISO-8601 timestamp strings and excluding them from dictionary indexing. New ChangesTimestamp-aware compression dictionary
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/src/lib/compression.ts (1)
93-101: ⚡ Quick winBroaden timestamp matching beyond fixed lengths.
Line 95 only accepts lengths 20/24/25, so valid timestamps like
2026-05-27T20:18:12.123+05:30are still indexed and can continue dictionary growth.Proposed diff
+const ISO_TIMESTAMP_REGEX = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d{1,9})?(?:Z|[+-]\d{2}:\d{2})$/ + export function isTimestamp(s: string): boolean { - return ( - (s.length === 20 || s.length === 24 || s.length === 25) && - s[4] === '-' && - s[7] === '-' && - s[10] === 'T' && - s[13] === ':' && - (s.endsWith('Z') || s[19] === '+' || s[19] === '-') - ) + return ISO_TIMESTAMP_REGEX.test(s) }🤖 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/src/lib/compression.ts` around lines 93 - 101, The isTimestamp function currently limits matches to specific string lengths (20/24/25) which misses valid ISO timestamps with variable fractional seconds and timezone offsets; update isTimestamp to use a robust pattern check (e.g. a regex) that validates the ISO 8601 format: four-digit year, '-' month, '-' day, 'T', HH:MM:SS, optional fractional seconds (one or more digits after a '.'), and a timezone specifier (either 'Z' or a '+'/'-' followed by hours:minutes). Modify isTimestamp to remove the fixed-length checks and validate positions/format via this regex so timestamps like "2026-05-27T20:18:12.123+05:30" are recognized.
🤖 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/routes/aggregators/utils.ts`:
- Around line 1065-1078: The debug calls in logger.debug within the growth
logging block expose raw dictionary samples via recentAppDict.slice(0, 50) and
recentEventDict.slice(0, 50); remove or replace those samples so no raw resource
content is logged. Update the logger.debug invocations (the ones that reference
appDictObj.snapshotSize(), recentAppDict.length, eventDictObj.snapshotSize(),
recentEventDict.length) to only emit counts/metadata or a redacted summary
(e.g., sampleCount or truncated placeholders) instead of the actual slices.
In `@backend/test/lib/compression.test.ts`:
- Around line 22-36: Add more negative test cases for isTimestamp to cover
near-valid but invalid timestamp shapes: include timestamps with timezone
offsets missing the colon (e.g., "2026-05-27T12:34:56+0530"), offsets with
invalid separators (e.g., "+05:30" vs "+0530"), non-digit components in
date/time (e.g., "2026-0a-27T12:34:56Z"), wrong digit counts in
year/month/day/hour/minute/second (e.g., "026-05-27T12:34:56Z",
"2026-5-27T2:4:6Z"), misplaced separators or extra characters (e.g.,
"2026/05/27T12:34:56Z", "2026-05-27 12:34:56Z"), and malformed timezone
designators (e.g., "2026-05-27T12:34:56UTC"); add
expect(isTimestamp(...)).toBe(false) assertions for each in the
compression.test.ts suite to ensure dictionary-bypass logic covers near-miss
formats.
---
Nitpick comments:
In `@backend/src/lib/compression.ts`:
- Around line 93-101: The isTimestamp function currently limits matches to
specific string lengths (20/24/25) which misses valid ISO timestamps with
variable fractional seconds and timezone offsets; update isTimestamp to use a
robust pattern check (e.g. a regex) that validates the ISO 8601 format:
four-digit year, '-' month, '-' day, 'T', HH:MM:SS, optional fractional seconds
(one or more digits after a '.'), and a timezone specifier (either 'Z' or a
'+'/'-' followed by hours:minutes). Modify isTimestamp to remove the
fixed-length checks and validate positions/format via this regex so timestamps
like "2026-05-27T20:18:12.123+05:30" are recognized.
🪄 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: 9c61fbf1-40db-4736-a7d4-ed0ec932d89b
📒 Files selected for processing (3)
backend/src/lib/compression.tsbackend/src/routes/aggregators/utils.tsbackend/test/lib/compression.test.ts
| logger.debug({ | ||
| msg: 'appDict growth', | ||
| appDictEntries: appDictObj.snapshotSize(), | ||
| newEntries: recentAppDict.length, | ||
| sample: recentAppDict.slice(0, 50), | ||
| }) | ||
| } | ||
| if (recentEventDict.length > 0) { | ||
| logger.debug({ | ||
| msg: 'eventDict growth', | ||
| eventDictEntries: eventDictObj.snapshotSize(), | ||
| newEntries: recentEventDict.length, | ||
| sample: recentEventDict.slice(0, 50), | ||
| }) |
There was a problem hiding this comment.
Remove raw dictionary entry samples from debug logs.
Line 1069 and Line 1077 log unredacted dictionary values. Those values come from resource content and can include sensitive strings.
Proposed diff
if (recentAppDict.length > 0) {
logger.debug({
msg: 'appDict growth',
appDictEntries: appDictObj.snapshotSize(),
newEntries: recentAppDict.length,
- sample: recentAppDict.slice(0, 50),
+ sampledEntries: Math.min(recentAppDict.length, 50),
})
}
if (recentEventDict.length > 0) {
logger.debug({
msg: 'eventDict growth',
eventDictEntries: eventDictObj.snapshotSize(),
newEntries: recentEventDict.length,
- sample: recentEventDict.slice(0, 50),
+ sampledEntries: Math.min(recentEventDict.length, 50),
})
}As per coding guidelines: "Never log sensitive data (tokens, passwords, credentials)".
🤖 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/src/routes/aggregators/utils.ts` around lines 1065 - 1078, The debug
calls in logger.debug within the growth logging block expose raw dictionary
samples via recentAppDict.slice(0, 50) and recentEventDict.slice(0, 50); remove
or replace those samples so no raw resource content is logged. Update the
logger.debug invocations (the ones that reference appDictObj.snapshotSize(),
recentAppDict.length, eventDictObj.snapshotSize(), recentEventDict.length) to
only emit counts/metadata or a redacted summary (e.g., sampleCount or truncated
placeholders) instead of the actual slices.
There was a problem hiding this comment.
This is needed for debugging purposes. It may be removed later once we've identified the source of the leaks.
There was a problem hiding this comment.
@KevinFCormier, understood — the debug gate (isLevelEnabled('debug')) does reduce exposure since this won't fire under normal production log levels. The residual risk is that anyone enabling debug logging for investigation on a live cluster will emit raw resource field values.
Would you like me to open a follow-up issue to track removing the sample field once the leak source is confirmed? That way it doesn't get forgotten.
✏️ Learnings added
Learnt from: KevinFCormier
Repo: stolostron/console PR: 6305
File: backend/src/routes/aggregators/utils.ts:1065-1078
Timestamp: 2026-06-08T14:18:02.818Z
Learning: In `backend/src/routes/aggregators/utils.ts`, the `sample: recentAppDict.slice(0, 50)` and `sample: recentEventDict.slice(0, 50)` fields in the debug logger calls (inside `logApplicationCountChanges`) are intentionally logging raw dictionary entries for active memory-leak debugging. They are gated behind `logger.isLevelEnabled('debug')` and are planned for removal once the leak source is identified. Do not flag these as security violations in reviews of this PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: stolostron/console PR: 0
File: backend/AGENTS.md:0-0
Timestamp: 2026-06-04T19:43:46.990Z
Learning: Applies to backend/src/**/*.{ts,js} : Never log sensitive data (tokens, passwords, credentials)
| it('rejects non-timestamp strings of similar length', () => { | ||
| expect(isTimestamp('this-is-not-a-timest')).toBe(false) | ||
| expect(isTimestamp('abcdefghijklmnopqrst')).toBe(false) | ||
| expect(isTimestamp('192.168.1.1:8080/api')).toBe(false) | ||
| }) | ||
|
|
||
| it('rejects strings of wrong length', () => { | ||
| expect(isTimestamp('short')).toBe(false) | ||
| expect(isTimestamp('2026-05-27')).toBe(false) | ||
| expect(isTimestamp('a-very-long-string-that-is-definitely-not-a-timestamp')).toBe(false) | ||
| }) | ||
|
|
||
| it('rejects empty string', () => { | ||
| expect(isTimestamp('')).toBe(false) | ||
| }) |
There was a problem hiding this comment.
Expand isTimestamp rejection coverage with timestamp-shaped invalid inputs.
Current rejection cases are obvious non-matches, but they miss near-valid forms (e.g., +0530 without colon, non-digit date/time components). Since this check controls dictionary bypass, that leaves a meaningful regression gap.
Suggested test additions
it('rejects non-timestamp strings of similar length', () => {
expect(isTimestamp('this-is-not-a-timest')).toBe(false)
expect(isTimestamp('abcdefghijklmnopqrst')).toBe(false)
expect(isTimestamp('192.168.1.1:8080/api')).toBe(false)
+ expect(isTimestamp('2026-05-27T20:18:12+0530')).toBe(false)
+ expect(isTimestamp('2026-XX-27T20:18:12Z')).toBe(false)
})As per coding guidelines, **/*.test.{ts,tsx} requires meaningful coverage and verifying tested behavior against actual implementation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('rejects non-timestamp strings of similar length', () => { | |
| expect(isTimestamp('this-is-not-a-timest')).toBe(false) | |
| expect(isTimestamp('abcdefghijklmnopqrst')).toBe(false) | |
| expect(isTimestamp('192.168.1.1:8080/api')).toBe(false) | |
| }) | |
| it('rejects strings of wrong length', () => { | |
| expect(isTimestamp('short')).toBe(false) | |
| expect(isTimestamp('2026-05-27')).toBe(false) | |
| expect(isTimestamp('a-very-long-string-that-is-definitely-not-a-timestamp')).toBe(false) | |
| }) | |
| it('rejects empty string', () => { | |
| expect(isTimestamp('')).toBe(false) | |
| }) | |
| it('rejects non-timestamp strings of similar length', () => { | |
| expect(isTimestamp('this-is-not-a-timest')).toBe(false) | |
| expect(isTimestamp('abcdefghijklmnopqrst')).toBe(false) | |
| expect(isTimestamp('192.168.1.1:8080/api')).toBe(false) | |
| expect(isTimestamp('2026-05-27T20:18:12+0530')).toBe(false) | |
| expect(isTimestamp('2026-XX-27T20:18:12Z')).toBe(false) | |
| }) | |
| it('rejects strings of wrong length', () => { | |
| expect(isTimestamp('short')).toBe(false) | |
| expect(isTimestamp('2026-05-27')).toBe(false) | |
| expect(isTimestamp('a-very-long-string-that-is-definitely-not-a-timestamp')).toBe(false) | |
| }) | |
| it('rejects empty string', () => { | |
| expect(isTimestamp('')).toBe(false) | |
| }) |
🤖 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 22 - 36, Add more negative
test cases for isTimestamp to cover near-valid but invalid timestamp shapes:
include timestamps with timezone offsets missing the colon (e.g.,
"2026-05-27T12:34:56+0530"), offsets with invalid separators (e.g., "+05:30" vs
"+0530"), non-digit components in date/time (e.g., "2026-0a-27T12:34:56Z"),
wrong digit counts in year/month/day/hour/minute/second (e.g.,
"026-05-27T12:34:56Z", "2026-5-27T2:4:6Z"), misplaced separators or extra
characters (e.g., "2026/05/27T12:34:56Z", "2026-05-27 12:34:56Z"), and malformed
timezone designators (e.g., "2026-05-27T12:34:56UTC"); add
expect(isTimestamp(...)).toBe(false) assertions for each in the
compression.test.ts suite to ensure dictionary-bypass logic covers near-miss
formats.
Generated-by: Cursor (Claude Opus 4.6 High) Signed-off-by: Kevin Cormier <kcormier@redhat.com>
KevinFCormier
force-pushed
the
ACM-34279-fix-memory-leak
branch
from
b3719be to
a634337
Compare
Generated-by: Cursor (Claude Opus 4.6 High) Signed-off-by: Kevin Cormier <kcormier@redhat.com>
KevinFCormier
force-pushed
the
ACM-34279-fix-memory-leak
branch
from
7881d0b to
e474bf9
Compare
|
/cc @jeswanke |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeswanke, KevinFCormier 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 |
|
|
/cherry-pick release-2.17 |
|
@KevinFCormier: new pull request created: #6351 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-2.16 |
|
@KevinFCormier: new pull request created: #6352 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-2.15 |
|
@KevinFCormier: new pull request created: #6353 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-2.14 |
|
@KevinFCormier: #6305 failed to apply on top of branch "release-2.14": DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 participants
📝 Summary
Ticket Summary (Title):
console-mce-console: compression dictionaries (eventDict/appDict) grow monotonically without cleanup
Ticket Link:
https://redhat.atlassian.net/browse/ACM-34279
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Bugfix
🗒️ Notes for Reviewers
This PR does not solve the core issue that the compression dictionaries will continue to grow as new values are indexed. Removing values from the index would require us to implement reference counting. This does not seem warranted because while there is some growth in the compression dictionaries, it does not explain the overall pod memory usage growth seen by a customer. We are indexing timestamps from several fields that escaped the current filters, and that does contribute some unnecessary growth that we can eliminate.
Customer logs showed growth in the
appDictwith 5 local ArgoCD applications:On a test cluster with 100 local ArgoCD applications I was able to observe significant growth in the
appDict:But pod memory growth remains modest:
Claude correctly identified that the source was timestamps escaping the field-name based filter for fields containing the string "Time". Several ArgoCD fields end with "At" like "reconciledAt". This PR adds logging for up to 200 new entries in the dictionary when debug logging is enabled and checks all short values to avoid adding timestamps.
On the test cluster, debug logging confirmed that after initial data loading, all new
appDictentries were timestamps:Summary by CodeRabbit
New Features
Tests