Skip to content

Fix a rare leak in GC - [MOD-11123]#7315

Merged
GuyAv46 merged 1 commit into2.10from
guyav-MOD-11123
Nov 12, 2025
Merged

Fix a rare leak in GC - [MOD-11123]#7315
GuyAv46 merged 1 commit into2.10from
guyav-MOD-11123

Conversation

@GuyAv46
Copy link
Copy Markdown
Collaborator

@GuyAv46 GuyAv46 commented Nov 11, 2025

Fix memory menegment in recvNumIdx and recvCardvals

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

Note

Fixes memory leaks in fork GC by correctly freeing cardinality arrays on error and simplifying recvCardvals allocation/zero-length handling.

  • GC (numeric receive path):
    • recvCardvals:
      • Remove pre-free of *tgt; allocate with array_newlen and read directly; early-return on zero length.
      • Add note that inputs are valid/zeroed.
    • recvNumIdx:
      • On error, free idxbufs and cardValsArr, then zero ninfo to avoid leaks/invalid state.
      • Add note that ninfo is expected zeroed.

Written by Cursor Bugbot for commit fc0c19e. This will update automatically on new commits. Configure here.

Comment thread src/fork_gc.c
return REDISMODULE_OK;
}
if (*tgt) {
rm_free(*tgt);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not needed, and also not the correct free function

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Incomplete Cleanup Path Leaks Memory

When ninfo.node->range is NULL, the code jumps to loop_cleanup without calling resetCardinality, which transfers ownership of ninfo.cardValsArr. This leaves ninfo.cardValsArr allocated but not freed in the cleanup section, causing a memory leak. The cleanup at loop_cleanup needs to free ninfo.cardValsArr when status == FGC_COLLECTED.

src/fork_gc.c#L963-L967

RediSearch/src/fork_gc.c

Lines 963 to 967 in fc0c19e

rt = openNumericKeysDict(sctx->spec, keyName, DONT_CREATE_INDEX);
initialized = true;
}
if (rt->uniqueId != rtUniqueId) {

Fix in Cursor Fix in Web


@codecov
Copy link
Copy Markdown

codecov bot commented Nov 11, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.25%. Comparing base (f884bc3) to head (fc0c19e).
⚠️ Report is 2 commits behind head on 2.10.

Files with missing lines Patch % Lines
src/fork_gc.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             2.10    #7315      +/-   ##
==========================================
- Coverage   89.28%   89.25%   -0.04%     
==========================================
  Files         207      207              
  Lines       35309    35307       -2     
==========================================
- Hits        31525    31512      -13     
- Misses       3784     3795      +11     
Flag Coverage Δ
flow 83.82% <0.00%> (-0.17%) ⬇️
unit 42.47% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@GuyAv46 GuyAv46 requested a review from dor-forer November 12, 2025 11:56
@GuyAv46 GuyAv46 added this pull request to the merge queue Nov 12, 2025
Merged via the queue into 2.10 with commit 81036bb Nov 12, 2025
14 of 15 checks passed
@GuyAv46 GuyAv46 deleted the guyav-MOD-11123 branch November 12, 2025 12:50
redisearch-backport-pull-request bot pushed a commit that referenced this pull request Nov 12, 2025
fix leak

(cherry picked from commit 81036bb)
@redisearch-backport-pull-request
Copy link
Copy Markdown
Contributor

Successfully created backport PR for 2.8:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants