Swartzn/fix/multicursor ring buffer gc#331
Conversation
c441a4a to
f80bd1f
Compare
ea3eb0b to
af0d0d3
Compare
0a27b3a to
138ab96
Compare
cdf3484 to
11640ec
Compare
* Fixed wrapped cursor ordering: getOldestAckCursor() now computes ring-buffer distance correctly when end has wrapped, so GC chooses the actual oldest unacknowledged cursor. * Fixed false dropped-event reporting: Now returns nil when GC drops an event with either no subscribers or there was a race on the oldestAckCursor and no event was actually unacknowledged.
Remove redundant conditionals and unreachable code from MultiCursorRingBuffer's AckEvent and searchIndexOfSeqID. Fix or remove inaccurate or stale comments.
64ef6aa to
6e886f4
Compare
iamjoemccormick
left a comment
iamjoemccormick
left a comment
There was a problem hiding this comment.
Potentially amending the commit message is the main blocker.
| func (b *MultiCursorRingBuffer) getOldestAckCursor() int { | ||
| oldestAckCursor := noAckCursor | ||
|
|
||
| // The index the oldest ackCursor is pointing to. | ||
| // There may be multiple ackCursors pointing to the same index. | ||
| oldestAckCursor := -1 | ||
| // The offset is the distance from the end to a particular ackCursor. | ||
| // Thus the closest offset is zero, indicating an ackCursor is all caught up. | ||
| var oldestOffset int | ||
|
|
||
| for _, c := range b.cursors { | ||
|
|
||
| if oldestAckCursor == -1 { | ||
| oldestAckCursor = c.ackCursor | ||
| oldestOffset = b.end - c.ackCursor | ||
| } else { | ||
| offset := b.end - c.ackCursor | ||
|
|
||
| // The oldest offset is always the highest negative number: | ||
| if offset < 0 { | ||
| if oldestOffset < 0 { | ||
| // If both offsets are negative, the oldest offset will be greater: | ||
| if offset > oldestOffset { | ||
| oldestAckCursor = c.ackCursor | ||
| oldestOffset = offset | ||
| } | ||
| } else { | ||
| // Otherwise if the new offset is negative it will always be older: | ||
| oldestAckCursor = c.ackCursor | ||
| oldestOffset = offset | ||
| } | ||
| } else if oldestOffset >= 0 { // Otherwise the offset is positive. If the oldest offset was already negative it is older. | ||
| // If both the offset and the oldest offset are positive, the oldest offset is whichever one is greater: | ||
| if offset > oldestOffset { | ||
| oldestAckCursor = c.ackCursor | ||
| oldestOffset = offset | ||
| } | ||
| } | ||
| ackCursor := c.ackCursor | ||
| offset := b.getDistance(ackCursor, b.end) | ||
| if oldestAckCursor == noAckCursor || oldestOffset < offset { | ||
| oldestAckCursor = ackCursor | ||
| oldestOffset = offset | ||
| } | ||
| } | ||
| return oldestAckCursor |
There was a problem hiding this comment.
todo: functionally this appears correct and is a good simplification. However I'm not sure this was actually a bug in the original implementation?
- Fixed wrapped cursor ordering: getOldestAckCursor() now computes ring-buffer distance
correctly when end has wrapped, so GC chooses the actual oldest unacknowledged cursor.
While the original getOldestAckCursor() was a bit hard to reason about, I believe it always choose the actual oldest unack'd cursor. Unless I'm missing something and this is not the case, we should just drop that line from the commit message.
Notably wouldn't some test case in TestGetOldestAckCursor() have failed?
| // The caller must ensure targetSeqID is less than or equal to the seqID at endIndex. If targetSeqID | ||
| // is found the index is returned with true. Otherwise it returns the next index with a higher seqID | ||
| // and false. |
There was a problem hiding this comment.
nitpick: while we document it here this could be easy to overlook when making future changes. A defensive check could make future debugging easier.
Removed defensive coverage + silent wrong-return in searchIndexOfSeqID beegfs-go/watch/internal/types/multicursorringbuffer.go:534
The rewritten binary search no longer returns the -1 "not found / out of range" sentinel; if targetSeqID exceeds the seqID at endIndex it silently returns endIndex (a lower, wrong seqID). This is not a live bug — the sole caller (AckEvent) now enforces seqIDToAck <= lastSentSeqID (line 497) before calling, so the precondition always holds. But the two test cases that exercised the old out-of-range behavior were deleted (multicursorringbuffer_test.go:646) with no replacement, so a future change that weakens that guard would fail silently instead of erroring. Consider a cheap if targetSeqID > b.buffer[endIndex].Meta.SeqId { return endIndex, false } defensive check, or restore an assertion covering the contract.
Assisted-by: Claude:claude-opus-4-8
2 participants
@iamjoemccormick I found some issues in Watch and since they were not directly part of your PR, I create this one.
What does this PR do / why do we need it?
Required for all PRs.
Related Issue(s)
Required when applicable.
Where should the reviewer(s) start reviewing this?
Only required for larger PRs when this may not be immediately obvious.
Are there any specific topics we should discuss before merging?
Not required.
What are the next steps after this PR?
Not required.
Checklist before merging:
Required for all PRs.
When creating a PR these are items to keep in mind that cannot be checked by GitHub actions:
For more details refer to the Go coding standards and the pull request process.