Skip to content

Fix GC regression - [MOD-12538]#7441

Merged
GuyAv46 merged 1 commit intomasterfrom
guyav-MOD-12538
Nov 20, 2025
Merged

Fix GC regression - [MOD-12538]#7441
GuyAv46 merged 1 commit intomasterfrom
guyav-MOD-12538

Conversation

@GuyAv46
Copy link
Collaborator

@GuyAv46 GuyAv46 commented Nov 20, 2025

Describe the changes in the pull request

We added RedisModule_SendChildHeartbeat calls #5503, after every inverted index scanned (even if it wasn't changed).
This was recently proved to slow the process down significantly - sub-second tasks take minutes to complete.

The goal was to get a print for the CoW info before we exit, so as we expect GC runs to be short, we will now call the heartbeat API once, right before finishing.
We add another terminal marker to the protocol, so the parent will wait until this info is logged before attempting to kill the child.

Mark if applicable

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

Note

Consolidates ForkGC child heartbeats to a single final heartbeat and adds a terminal marker so the parent waits before termination, removing progress tracking.

  • ForkGC child flow:
    • Remove per-scan progress heartbeats and related calls.
    • Send a single final RedisModule_SendChildHeartbeat(1.0) after all scans complete.
    • Emit an extra final FGC_sendTerminator to mark post-processing completion.
  • Parent/child protocol:
    • Parent now waits for and verifies the final terminator (SIZE_MAX) before finishing, ensuring the heartbeat is logged.
  • Cleanup:
    • Delete progress reporting helpers and the ForkGC.progress field.

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

@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.87%. Comparing base (f5d6289) to head (42b8090).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
src/fork_gc.c 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7441      +/-   ##
==========================================
+ Coverage   84.80%   84.87%   +0.07%     
==========================================
  Files         346      347       +1     
  Lines       53426    53675     +249     
  Branches    14042    14292     +250     
==========================================
+ Hits        45307    45559     +252     
+ Misses       7923     7920       -3     
  Partials      196      196              
Flag Coverage Δ
flow 84.81% <80.00%> (-0.12%) ⬇️
unit 52.35% <80.00%> (+0.21%) ⬆️

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.

Copy link
Collaborator

@alonre24 alonre24 left a comment

Choose a reason for hiding this comment

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

Can we add your micro benchmark to this PR?

@GuyAv46 GuyAv46 requested a review from alonre24 November 20, 2025 13:30
@GuyAv46 GuyAv46 added this pull request to the merge queue Nov 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2025
@GuyAv46 GuyAv46 added this pull request to the merge queue Nov 20, 2025
Merged via the queue into master with commit a904cf5 Nov 20, 2025
34 checks passed
@GuyAv46 GuyAv46 deleted the guyav-MOD-12538 branch November 20, 2025 21:11
@redisearch-backport-pull-request
Copy link
Contributor

Backport failed for 8.2, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 8.2
git worktree add -d .worktree/backport-7441-to-8.2 origin/8.2
cd .worktree/backport-7441-to-8.2
git switch --create backport-7441-to-8.2
git cherry-pick -x a904cf5a1c5cc1d0b6e6d220ea853d45006c5c9d

redisearch-backport-pull-request bot pushed a commit that referenced this pull request Nov 20, 2025
send heartbeat once, and make the parent wait for it

(cherry picked from commit a904cf5)
@redisearch-backport-pull-request
Copy link
Contributor

Successfully created backport PR for 8.4:

GuyAv46 added a commit that referenced this pull request Nov 21, 2025
send heartbeat once, and make the parent wait for it

(cherry picked from commit a904cf5)
github-merge-queue bot pushed a commit that referenced this pull request Nov 21, 2025
Fix GC regression - [MOD-12538] (#7441)

send heartbeat once, and make the parent wait for it

(cherry picked from commit a904cf5)

Co-authored-by: GuyAv46 <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Nov 21, 2025
Fix GC regression - [MOD-12538] (#7441)

send heartbeat once, and make the parent wait for it

(cherry picked from commit a904cf5)
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