Skip to content

dont grow events unbounded when persisting #2560

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 14, 2025
Merged

dont grow events unbounded when persisting #2560

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6d246e
dont grow events unbounded when persisting
sawka Nov 14, 2025
a55ea49
remove unused cost
sawka Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions pkg/wps/wps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
// strong typing and event types can be defined elsewhere

const MaxPersist = 4096
const ReMakeArrThreshold = 10 * 1024

type Client interface {
SendEvent(routeId string, event WaveEvent)
Expand All @@ -34,8 +33,7 @@ type persistKey struct {
}

type persistEventWrap struct {
ArrTotalAdds int
Events []*WaveEvent
Events []*WaveEvent
}

type BrokerType struct {
Expand Down Expand Up @@ -214,16 +212,13 @@ func (b *BrokerType) persistEvent(event WaveEvent) {
pe := b.PersistMap[key]
if pe == nil {
pe = &persistEventWrap{
ArrTotalAdds: 0,
Events: make([]*WaveEvent, 0, event.Persist),
Events: make([]*WaveEvent, 0, numPersist),
}
b.PersistMap[key] = pe
}
pe.Events = append(pe.Events, &event)
pe.ArrTotalAdds++
if pe.ArrTotalAdds > ReMakeArrThreshold {
pe.Events = append([]*WaveEvent{}, pe.Events...)
pe.ArrTotalAdds = len(pe.Events)
if len(pe.Events) > numPersist {
pe.Events = pe.Events[len(pe.Events)-numPersist:]
}
Comment on lines +220 to 222
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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 -A2

Length 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 -A1

Length of output: 71

🏁 Script executed:

#!/bin/bash
# Search for WaveEvent type definition
rg -n 'type.*WaveEvent' pkg/wps/wps.go -A5

Length 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 -A2

Length 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.

Suggested change
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.

Copy link
Member Author

@sawka sawka Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
}
Expand Down
Loading