dont grow events unbounded when persisting #2560
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe event persistence logic in pkg/wps/wps.go was simplified: the ArrTotalAdds field and threshold-based remanufacture logic were removed. The code now uses a fixed-size recent history capped by numPersist (derived from the event’s Persist and clamped to MaxPersist). persistEventWrap is initialized with an Events slice capacity of numPersist, each new persist appends the event, and when the slice exceeds numPersist it is trimmed to retain only the most recent numPersist entries. The constant ReMakeArrThreshold was removed. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/wps/wps.go (1)
19-19: Remove unused constant.The
ReMakeArrThresholdconstant is no longer used after the windowing logic simplification and should be removed to avoid confusion.const MaxPersist = 4096 -const ReMakeArrThreshold = 10 * 1024
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/wps/wps.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/wps/wps.go (1)
pkg/wps/wpstypes.go (1)
WaveEvent(26-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (2)
pkg/wps/wps.go (2)
36-38: LGTM!The struct simplification by removing the
ArrTotalAddscounter aligns well with the new fixed-size history approach.
216-216: LGTM!Pre-allocating capacity for the slice is a good optimization that reduces allocations during the initial growth phase.
| if len(pe.Events) > numPersist { | ||
| pe.Events = pe.Events[len(pe.Events)-numPersist:] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Slice reslicing causes memory leak.
The slice reslicing operation pe.Events[len(pe.Events)-numPersist:] creates a new slice header that shares the same underlying array. The old events at the beginning of the array remain in memory because the slice still references the underlying array, preventing garbage collection. Over time, this causes unbounded memory growth—the opposite of the PR's intent.
To fix this, allocate a new slice and copy the elements:
pe.Events = append(pe.Events, &event)
if len(pe.Events) > numPersist {
- pe.Events = pe.Events[len(pe.Events)-numPersist:]
+ newEvents := make([]*WaveEvent, numPersist)
+ copy(newEvents, pe.Events[len(pe.Events)-numPersist:])
+ pe.Events = newEvents
}Additional concern: The persistence window size varies based on each event's Persist field value. If different events for the same (Event, Scope) key have different Persist values, the window size will change unpredictably. Consider whether this is intentional or if the window size should be fixed per key.
I need to verify the memory leak claim and the proposed fix. Let me examine the actual code context.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the actual code context around lines 221-223
cat pkg/wps/wps.go | sed -n '210,235p'
Length of output: 603
🏁 Script executed:
#!/bin/bash
# Find where numPersist is defined/set to understand its value
rg -n 'numPersist\s*:=' pkg/wps/wps.go -B5 -A2Length of output: 207
🏁 Script executed:
#!/bin/bash
# Find the type definition for pe.Events to understand the data structure
rg -n 'Events\s*\[\]' pkg/wps/wps.go -B3 -A1Length of output: 71
🏁 Script executed:
#!/bin/bash
# Search for WaveEvent type definition
rg -n 'type.*WaveEvent' pkg/wps/wps.go -A5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are other places where Events is resliced or trimmed
rg -n 'Events\s*=' pkg/wps/wps.go -B2 -A2Length of output: 192
I need to verify the Go slice behavior to confirm the memory leak claim.
---
🌐 Web query:
```web
Go slice reslicing memory leak garbage collection underlying array
💡 Result:
Short answer
- Reslicing a slice keeps a pointer to its entire underlying array, so the GC cannot free parts of that array while any slice points into it — this can look like a memory leak when you keep small subslices of large arrays. [1][2]
Why it happens
- A slice is (pointer, len, cap). The pointer references the underlying array block; as long as any pointer into that block exists, the whole block is reachable to the GC. Reslicing does not copy the array. [1][2]
How to fix / avoid it
- Copy the needed data to a new slice so it points at a freshly allocated, smaller array:
c := make([]byte, len(b)); copy(c, b)
(or use append to allocate-and-copy). [1] - When deleting elements, zero out removed slots so they aren’t kept live by the underlying array:
copy(a[i:], a[j:]); for k := len(a)-j+i; k < len(a); k++ { a[k] = nil }; a = a[:len(a)-j+i]. [3] - If you only need a short window of a large buffer, explicitly allocate a new slice with copy before returning from the function. [1][8]
- Let long-lived slices drop to nil (or reassign) so the underlying array becomes unreachable and can be GC’d.
References
- Go blog — “Go Slices: usage and internals” (explains the gotcha and copy fix). [1]
- Stack Overflow — explanation and examples about slice internals and GC behavior. [2]
- Go Wiki / SliceTricks — recommended patterns for Cut/Delete that zero-out removed elements. [3]
Critical: Slice reslicing causes memory leak—memory leak claim confirmed.
Reslicing a slice keeps a pointer to its entire underlying array, so the GC cannot free parts of that array while any slice points into it. The code at line 222 reslices pe.Events but the underlying array continues to hold references to old events, preventing garbage collection. This contradicts the PR's intent to prevent unbounded growth.
The proposed fix is correct. Copy the needed data to a new slice so it points at a freshly allocated, smaller array:
pe.Events = append(pe.Events, &event)
if len(pe.Events) > numPersist {
- pe.Events = pe.Events[len(pe.Events)-numPersist:]
+ newEvents := make([]*WaveEvent, numPersist)
+ copy(newEvents, pe.Events[len(pe.Events)-numPersist:])
+ pe.Events = newEvents
}Additional concern: The persistence window size varies based on each event's Persist field. Different events for the same (Event, Scope) key will have different window sizes if their Persist values differ, making the window size unpredictable. Clarify whether this behavior is intentional or if the window should be fixed per key.
📝 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.
| if len(pe.Events) > numPersist { | |
| pe.Events = pe.Events[len(pe.Events)-numPersist:] | |
| } | |
| if len(pe.Events) > numPersist { | |
| newEvents := make([]*WaveEvent, numPersist) | |
| copy(newEvents, pe.Events[len(pe.Events)-numPersist:]) | |
| pe.Events = newEvents | |
| } |
🤖 Prompt for AI Agents
In pkg/wps/wps.go around lines 221 to 223, reslicing pe.Events (pe.Events =
pe.Events[len(pe.Events)-numPersist:]) retains a reference to the original
underlying array and prevents GC of older events; replace the reslice with
allocation of a new slice of length numPersist, copy the tail elements into it
(using make + copy) and assign pe.Events to that new slice so the old array can
be freed; also consider zeroing unused references if necessary. Additionally,
verify and document whether the persistence window size should vary per-event
(based on each event's Persist field) or be fixed per (Event, Scope) key and
adjust logic or add a comment accordingly.
There was a problem hiding this comment.
common false positive for go code analysis. because we are constantly appending, eventually when we hit the capacity cap the append will cause go to reallocate the backing array freeing the previously orphaned elements. the unmodified code is actually quite efficient and correct.
There was a problem hiding this comment.
@sawka, you're absolutely right—I apologize for the false positive.
I misanalyzed the slice behavior. When you reslice pe.Events[len(pe.Events)-numPersist:], the resulting slice has both length and capacity equal to numPersist (since it's sliced from the end of the underlying array). On the very next append, since len == cap, Go will allocate a fresh backing array, copy the elements, and the old array with the orphaned elements becomes unreachable for GC.
This is indeed an efficient pattern—you're letting Go's append reallocation mechanism handle the cleanup naturally. Thank you for the clarification!
(\_(\
(^_^) Thanks for teaching me!
(")_(")
✏️ Learnings added
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2560
File: pkg/wps/wps.go:221-223
Timestamp: 2025-11-14T19:05:00.483Z
Learning: In Go, reslicing from the end of a slice (e.g., `slice[len(slice)-n:]`) creates a new slice with both length and capacity equal to n. On the next append, since len == cap, Go will allocate a new backing array and the old array becomes GC-eligible. This is an efficient pattern for maintaining a sliding window and should not be flagged as a memory leak.
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.
No description provided.