[MOD-9023] Replacing freeFlag with a reference counter by meiravgri · Pull Request #5736 · RediSearch/RediSearch

meiravgri · 2025-03-09T16:48:01Z

This PR fixes a race condition on freeFlag in the coordinator mechanism.
The issue occurred because freeFlag was expected to be reset by the uv thread when the coordinator reached MRIterator_Release but since the flag is reset from the a BG thread, this was not always the case.
This led to an incorrect sequence of operations, ultimately causing an assertion failure.
To resolve this, freeFlag has been replaced with a reference counter, which:

Increases before pushing a new job to the RQ.
Decreases when the coordinator no longer requires more replies.
Decreases again when all shards have finished pushing replies to the channel.

Logging improvements:

Added RS_DEBUG_LOG_FMT and RS_DEBUG_LOG macros for debug logging in deps/rmutil/rm_assert.h.
when ENABLE_ASSERT=1 it will call RedisModule_Log with "debug" level

codecov · 2025-03-10T16:33:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.11%. Comparing base (d45701e) to head (48e3fa4).
Report is 15 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5736      +/-   ##
==========================================
- Coverage   88.13%   88.11%   -0.03%     
==========================================
  Files         198      200       +2     
  Lines       35999    36050      +51     
==========================================
+ Hits        31727    31764      +37     
- Misses       4272     4286      +14

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

alonre24 · 2025-03-11T09:40:14Z

src/coord/dist_aggregate.c

  if (timedout && !cmd->forCursor) {
    newCmd = MR_NewCommand(4, "_FT.CURSOR", "DEL", idx, buf);
-    newCmd.depleted = true;
+    // newCmd.depleted = true;


why commenting out?

This is wrong. Here the cmd is essentially not depleted as we are going to send the shard another request...

alonre24 · 2025-03-11T09:42:07Z

src/coord/rmr/rmr.c

+  RS_LOG_ASSERT_FMT(pending >= 0, "Pending (%d) should not reach a negative value. inproc: %d, freeflag: %d, channel size: %d, shard_slot: %d",
+  pending, ctx->it->ctx.inProcess, ctx->it->ctx.freeFlag, MRChannel_Size(ctx->it->ctx.chan), ctx->cmd.targetSlot);


Can we always print it in debug mode rather than only when the assertion fails? perhaps it can bring more information when time comes.

…e one side is changing it twice. The refcount is increased when a new job is pushed to the queue, and decreased when the coordinator doesn't need any replies, or when all readers have done executing the job. introduce logging capabilities when ENABLE_ASSERT=1 : RS_DEBUG_LOG and RS_DEBUG_LOG_FMT to log in debug level

Copilot

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

deps/rmutil/rm_assert.h

src/coord/rmr/rmr.c

GuyAv46

What a catch and what a solution 🎣 🥇

raz-mon · 2025-03-13T09:08:02Z

deps/rmutil/rm_assert.h

+#define RS_DEBUG_LOG_FMT(fmt, ...) // NOP
+#define RS_DEBUG_LOG(str) // NOP


Why have the debug logs depend on ENABLE_ASSERT?
Since they are in debug level I think we should always have them

Their purpose is to be used in a testing env, not in production (even if it's only enabled with debug loglevel)

Yes, why not have it in production?

We don't know the impact on performance of calling RedisModule_log (even if it's not the configured log level, it still executes some code), so i prefer not to compile it at all.
We can open a ticket to measure that

src/coord/dist_aggregate.c

redisearch-backport-pull-request · 2025-03-13T11:58:18Z

Only merged pull requests can be backported.

meiravgri · 2025-03-13T15:12:41Z

/backport

redisearch-backport-pull-request · 2025-03-13T15:13:02Z

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

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

git fetch origin 8.0
git worktree add -d .worktree/backport-5736-to-8.0 origin/8.0
cd .worktree/backport-5736-to-8.0
git switch --create backport-5736-to-8.0
git cherry-pick -x 3420e93a87a6fbd4b8880075b557cf8e5d4a4ab0

* add prints * fix * no depletion in getCursorCommand * add info to assert * free flag is now a ref counter to avoid potential race condition where one side is changing it twice. The refcount is increased when a new job is pushed to the queue, and decreased when the coordinator doesn't need any replies, or when all readers have done executing the job. introduce logging capabilities when ENABLE_ASSERT=1 : RS_DEBUG_LOG and RS_DEBUG_LOG_FMT to log in debug level * cleanups * cr (cherry picked from commit 3420e93)

[MOD-9023] Replacing freeFlag with a reference counter (#5736) * add prints * fix * no depletion in getCursorCommand * add info to assert * free flag is now a ref counter to avoid potential race condition where one side is changing it twice. The refcount is increased when a new job is pushed to the queue, and decreased when the coordinator doesn't need any replies, or when all readers have done executing the job. introduce logging capabilities when ENABLE_ASSERT=1 : RS_DEBUG_LOG and RS_DEBUG_LOG_FMT to log in debug level * cleanups * cr (cherry picked from commit 3420e93)

meiravgri · 2025-03-18T05:46:13Z

/backport

redisearch-backport-pull-request · 2025-03-18T05:46:31Z

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

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

git fetch origin 2.10
git worktree add -d .worktree/backport-5736-to-2.10 origin/2.10
cd .worktree/backport-5736-to-2.10
git switch --create backport-5736-to-2.10
git cherry-pick -x 3420e93a87a6fbd4b8880075b557cf8e5d4a4ab0

redisearch-backport-pull-request · 2025-03-18T05:46:32Z

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

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

git fetch origin 8.0
git worktree add -d .worktree/backport-5736-to-8.0 origin/8.0
cd .worktree/backport-5736-to-8.0
git switch --create backport-5736-to-8.0
git cherry-pick -x 3420e93a87a6fbd4b8880075b557cf8e5d4a4ab0

meiravgri · 2025-03-18T05:51:19Z

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

Please cherry-pick the changes locally and resolve any conflicts.
git fetch origin 8.0
git worktree add -d .worktree/backport-5736-to-8.0 origin/8.0
cd .worktree/backport-5736-to-8.0
git switch --create backport-5736-to-8.0
git cherry-pick -x 3420e93a87a6fbd4b8880075b557cf8e5d4a4ab0

Already backported

* add prints * fix * no depletion in getCursorCommand * add info to assert * free flag is now a ref counter to avoid potential race condition where one side is changing it twice. The refcount is increased when a new job is pushed to the queue, and decreased when the coordinator doesn't need any replies, or when all readers have done executing the job. introduce logging capabilities when ENABLE_ASSERT=1 : RS_DEBUG_LOG and RS_DEBUG_LOG_FMT to log in debug level * cleanups * cr (cherry picked from commit 3420e93)

[MOD-9023] Replacing freeFlag with a reference counter (#5736) * add prints * fix * no depletion in getCursorCommand * add info to assert * free flag is now a ref counter to avoid potential race condition where one side is changing it twice. The refcount is increased when a new job is pushed to the queue, and decreased when the coordinator doesn't need any replies, or when all readers have done executing the job. introduce logging capabilities when ENABLE_ASSERT=1 : RS_DEBUG_LOG and RS_DEBUG_LOG_FMT to log in debug level * cleanups * cr (cherry picked from commit 3420e93)

[MOD-9023] Replacing freeFlag with a reference counter (#5736) * add prints * fix * no depletion in getCursorCommand * add info to assert * free flag is now a ref counter to avoid potential race condition where one side is changing it twice. The refcount is increased when a new job is pushed to the queue, and decreased when the coordinator doesn't need any replies, or when all readers have done executing the job. introduce logging capabilities when ENABLE_ASSERT=1 : RS_DEBUG_LOG and RS_DEBUG_LOG_FMT to log in debug level * cleanups * cr (cherry picked from commit 3420e93) (cherry picked from commit 607a118)

[2.10] [MOD-9023] Replacing freeFlag with a reference counter (#5784) [MOD-9023] Replacing freeFlag with a reference counter (#5736) * add prints * fix * no depletion in getCursorCommand * add info to assert * free flag is now a ref counter to avoid potential race condition where one side is changing it twice. The refcount is increased when a new job is pushed to the queue, and decreased when the coordinator doesn't need any replies, or when all readers have done executing the job. introduce logging capabilities when ENABLE_ASSERT=1 : RS_DEBUG_LOG and RS_DEBUG_LOG_FMT to log in debug level * cleanups * cr (cherry picked from commit 3420e93) (cherry picked from commit 607a118)

[2.10] [MOD-9023] Replacing freeFlag with a reference counter (#5784) [MOD-9023] Replacing freeFlag with a reference counter (#5736) * add prints * fix * no depletion in getCursorCommand * add info to assert * free flag is now a ref counter to avoid potential race condition where one side is changing it twice. The refcount is increased when a new job is pushed to the queue, and decreased when the coordinator doesn't need any replies, or when all readers have done executing the job. introduce logging capabilities when ENABLE_ASSERT=1 : RS_DEBUG_LOG and RS_DEBUG_LOG_FMT to log in debug level * cleanups * cr (cherry picked from commit 3420e93) (cherry picked from commit 607a118) (cherry picked from commit d01e71d)

meiravgri added 4 commits March 9, 2025 16:47

add prints

a60bb62

fix

3d97405

no depletion in getCursorCommand

b3e731e

add info to assert

78d0022

meiravgri requested review from GuyAv46 and alonre24 March 10, 2025 16:22

alonre24 reviewed Mar 11, 2025

View reviewed changes

meiravgri added 2 commits March 12, 2025 09:01

cleanups

684b7b6

meiravgri changed the title ~~add prints~~ [MOD-9023] Replacing freeFlag with a reference counter Mar 12, 2025

meiravgri requested review from alonre24 and Copilot March 12, 2025 16:08

Copilot AI reviewed Mar 12, 2025

View reviewed changes

GuyAv46 reviewed Mar 12, 2025

View reviewed changes

cr

48e3fa4

meiravgri requested a review from GuyAv46 March 12, 2025 17:04

alonre24 reviewed Mar 12, 2025

View reviewed changes

src/coord/rmr/rmr.c Show resolved Hide resolved

alonre24 requested a review from raz-mon March 12, 2025 17:30

GuyAv46 approved these changes Mar 13, 2025

View reviewed changes

raz-mon approved these changes Mar 13, 2025

View reviewed changes

meiravgri added this pull request to the merge queue Mar 13, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 13, 2025

meiravgri added this pull request to the merge queue Mar 13, 2025

Merged via the queue into master with commit 3420e93 Mar 13, 2025
11 checks passed

meiravgri deleted the meiravg_add_prints_to_test_mod6287 branch March 13, 2025 15:05

meiravgri added the backport 8.0 label Mar 13, 2025

meiravgri mentioned this pull request Mar 13, 2025

[8.0] [MOD-9023] Replacing freeFlag with a reference counter #5762

Merged

meiravgri added the backport 2.10 label Mar 18, 2025

meiravgri mentioned this pull request Mar 18, 2025

[2.10] [MOD-9023] Replacing freeFlag with a reference counter #5784

Merged

		RS_LOG_ASSERT_FMT(pending >= 0, "Pending (%d) should not reach a negative value. inproc: %d, freeflag: %d, channel size: %d, shard_slot: %d",
		pending, ctx->it->ctx.inProcess, ctx->it->ctx.freeFlag, MRChannel_Size(ctx->it->ctx.chan), ctx->cmd.targetSlot);

		#define RS_DEBUG_LOG_FMT(fmt, ...) // NOP
		#define RS_DEBUG_LOG(str) // NOP

Conversation

meiravgri commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Logging improvements:

Uh oh!

codecov bot commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alonre24 Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

meiravgri Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

alonre24 Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuyAv46 left a comment

Choose a reason for hiding this comment

Uh oh!

raz-mon Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

meiravgri Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

raz-mon Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

meiravgri Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented Mar 13, 2025

Uh oh!

Uh oh!

Uh oh!

meiravgri commented Mar 13, 2025

Uh oh!

redisearch-backport-pull-request bot commented Mar 13, 2025

Uh oh!

meiravgri commented Mar 18, 2025

Uh oh!

redisearch-backport-pull-request bot commented Mar 18, 2025

Uh oh!

redisearch-backport-pull-request bot commented Mar 18, 2025

Uh oh!

meiravgri commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

meiravgri commented Mar 9, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading