Fix defrag issues for stream defrag and HFE #14323
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a crash during stream defragmentation and HFE defragmentation that was introduced by a previous issue. The crash occurs due to a mismatch between consumer group references after defragmentation operations.
- Fixed consumer group reference updates during stream defragmentation
- Added manual defragmentation for consumer groups with proper reference tracking
- Corrected the order of operations in HFE defragmentation to prevent race conditions
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/ebuckets.c
Outdated
| if (newSegHdr) { | ||
| /* Update next segment's prev to point to the defragmented segment. */ | ||
| nextSegHdr->prevSeg = newSegHdr; | ||
| nextSegHdr->firstSeg = (FirstSegHdr *)firstSegHdr; |
There was a problem hiding this comment.
don't we need to update that on all segments? (not just when newSegHdr is non NULL)
i.e. don't depend on newSegHdr, but rather add a var newFirstSegHdr? or just update unconditionally?
|
LGTM. looks like an unrelated threshold issue in the test, please confirm. |
|
@oranagra it's a known issue that doesn't relate to this PR. |
This reverts commit d649637.
This PR fixes three defrag issues. 1. Fix the issue that forget to update cgroup_ref_node when the consume group was reallocated. This crash was introduced by redis#14130 In this PR, when performing defragmentation on `s->cgroups` using `defragRadixTree()`, we no longer rely on the automatic data defragmentation of `defragRadixTree()`. Instead, we manually defragment the consumer group and then update its reference in `s->cgroups`. 2. Fix a use-after-free issue caused by updating dictionary keys after HFE key is reallocated. This issue was introduced by redis#13842 3. Fix the issue that forgot to be updated NextSegHdr->firstSeg when the first segment was reallocated. This issue was introduced by redis#13842 --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR fixes three defrag issues.
Fix the issue that forget to update cgroup_ref_node when the consume group was reallocated.
This crash was introduced by Add XDELEX and XACKDEL commands for stream #14130
In this PR, when performing defragmentation on
s->cgroupsusingdefragRadixTree(), we no longer rely on the automatic data defragmentation ofdefragRadixTree(). Instead, we manually defragment the consumer group and then update its reference ins->cgroups.Fix a use-after-free issue caused by updating dictionary keys after HFE key is reallocated.
This issue was introduced by Add support to defrag ebuckets incrementally #13842
Fix the issue that forgot to be updated NextSegHdr->firstSeg when the first segment was reallocated.
This issue was introduced by Add support to defrag ebuckets incrementally #13842
Note that both of these issues were introduced since 8.2, so we can fix them together.