[MOD-12419] Track Timeout errors and warnings in info by lerman25 · Pull Request #7414 · RediSearch/RediSearch

lerman25 · 2025-11-19T08:56:54Z

Continuing the work of #7386
This PR add tracking timeout warnings and errors in the global stats.

Note -

For FT.HYBRID there is debug timeout mechanism in cluster
FT.HYBRID SA has a bug in timeout warnings

Note

Track timeout errors and warnings across query paths, aggregate them in global stats, and expose them via INFO (with tests).

Global Stats / API:
- Add timeout counters to QueryErrorsGlobalStats and new QueryWarningGlobalStats with QueryWarningsGlobalStats_UpdateWarning.
- Treat standalone as coordinator for stats; include timeout in QueryErrorsGlobalStats_UpdateError.
- Export stats in INFO MODULES with new fields for shard/coord timeout errors/warnings.
Query Execution:
- Aggregate: update error/timeout handling to increment global error/warning stats; mark timeouts in profile context.
- Hybrid: increment stats on errors/timeouts; track timeout warnings in handleAndReplyWarning; adjust cleanup error accounting.
- Coordinator (module reducer): on shard errors/timeouts, increment coord error counters; parse warning strings and increment warning counters.
Rust/FFI:
- Introduce QueryWarningCode and QueryWarningCode_GetCodeFromMessage in FFI and headers.
Tests/Utils:
- Extend tests to validate timeout error/warning metrics in standalone and cluster (RESP2/RESP3).
- Add helpers to change timeout policy on shards and minor test data adjustments.

^{Written by Cursor Bugbot for commit be69839. This will update automatically on new commits. Configure here.}

src/hybrid/hybrid_exec.c

tests/pytests/test_info_modules.py

enricozb · 2025-11-19T11:16:33Z

src/redisearch_rs/c_entrypoint/query_error_ffi/cbindgen.toml

+[export]
+include = ["QueryWarningCode"]


Since you do a pub use .. QueryWarningCode in src/redisearch_rs/c_entrypoint/query_error_ffi/src/lib.rs, i don't think this line is needed.

The base branch was changed.

(cherry picked from commit fafe1dc)

(cherry picked from commit 8529cb9)

(cherry picked from commit c56a0d9)

(cherry picked from commit a414641)

(cherry picked from commit 6853bb9)

src/module.c

src/aggregate/aggregate_exec.c

codecov · 2025-11-19T16:50:55Z

Codecov Report

❌ Patch coverage is 59.42029% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.94%. Comparing base (e56bf5b) to head (be69839).
⚠️ Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
...isearch_rs/c_entrypoint/query_error_ffi/src/lib.rs	0.00%	14 Missing ⚠️
src/redisearch_rs/query_error/src/lib.rs	0.00%	11 Missing ⚠️
src/module.c	83.33%	2 Missing ⚠️
src/hybrid/hybrid_exec.c	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7414      +/-   ##
==========================================
+ Coverage   84.92%   84.94%   +0.02%     
==========================================
  Files         348      348              
  Lines       53773    53836      +63     
  Branches    14353    14378      +25     
==========================================
+ Hits        45668    45733      +65     
+ Misses       7909     7907       -2     
  Partials      196      196

Flag	Coverage Δ
flow	`84.98% <93.18%> (+0.04%)`	⬆️
unit	`52.36% <0.00%> (-0.07%)`	⬇️

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.

…-metric

cursor · 2025-11-20T12:42:04Z

src/aggregate/aggregate_exec.c


    if (ShouldReplyWithError(QueryError_GetCode(rp->parent->err), req->reqConfig.timeoutPolicy, IsProfile(req))) {
+      // Track errors in global statistics
+      QueryErrorsGlobalStats_UpdateError(QueryError_GetCode(rp->parent->err), 1, !IsInternal(req));


Bug: Inconsistent error tracking between RESP2 and RESP3

In sendChunk_Resp3, the error tracking uses QueryError_GetCode(rp->parent->err) while the error reply uses QueryError_GetUserError(qctx->err). This differs from sendChunk_Resp2 which consistently uses qctx->err for both tracking and replying. If rp->parent->err and qctx->err contain different error codes, the tracked error code won't match the error actually returned to the user, leading to incorrect statistics.

I don't think I should fix misalignment between resp2 and resp3 in this PR.
Using rp->parent->err is aligned with the line above which checks errors in rp->parent->err

…-metric

…afterwards)" This reverts commit 829ac53.

meiravgri

Nice and clean
I agree
with the SA definiton change

src/hybrid/hybrid_exec.c

meiravgri · 2025-11-23T05:00:29Z

src/aggregate/aggregate_exec.c


    if (ShouldReplyWithError(QueryError_GetCode(rp->parent->err), req->reqConfig.timeoutPolicy, IsProfile(req))) {
+      // Track errors in global statistics
+      QueryErrorsGlobalStats_UpdateError(QueryError_GetCode(rp->parent->err), 1, !IsInternal(req));


meiravgri · 2025-11-23T05:02:58Z

src/aggregate/aggregate_exec.c

-  // Update global query errors statistics before freeing the request.
-  // Both SA and internal are considered shards.
-  QueryErrorsGlobalStats_UpdateError(QueryError_GetCode(&status), 1, SHARD_ERR_WARN);
+  // Update global query errors statistics


wonder why you remove " before freeing the request."

Because the request is no longer relevant if I use numShards

src/aggregate/aggregate_exec.c

src/module.c

src/redisearch_rs/query_error/src/lib.rs

src/aggregate/aggregate_exec.c

src/info/global_stats.h

cursor · 2025-11-23T08:20:35Z

src/aggregate/aggregate_exec.c

    // If an error occurred, or a timeout in strict mode - return a simple error
    if (ShouldReplyWithError(QueryError_GetCode(rp->parent->err), req->reqConfig.timeoutPolicy, IsProfile(req))) {
+      // Track errors in global statistics
+      QueryErrorsGlobalStats_UpdateError(QueryError_GetCode(qctx->err), 1, !IsInternal(req));


Bug: Inconsistent error source in RESP2 error tracking

The error tracking uses qctx->err while the error check on line 401 uses rp->parent->err. This inconsistency means the tracked error code may differ from the error that triggered the tracking. The RESP3 version at line 532 correctly uses rp->parent->err for both operations, confirming this is a bug in the RESP2 path.

src/aggregate/aggregate_exec.c

meiravgri · 2025-11-23T09:13:52Z

tests/pytests/test_info_modules.py

+    for shardId in range(1, self.env.shardsCount + 1):
+      info_dict = info_modules_to_dict(self.env.getConnection(shardId))
+      self.env.assertEqual(info_dict[WARN_ERR_SECTION][TIMEOUT_WARNING_SHARD_METRIC], str(base_warn_shards[shardId] + 3),
+                           message=f"Shard {shardId} AGG coordinator timeout warning should be +3 total")


This one can be flaky
the coordinator might return before the shards reach to the point where they update the timeout wraning

* Track timeout in sendchunk resp2 * Track timeout warning in sendSearchResults * Track timeout error in searchResultReducer (cherry picked from commit fafe1dc) * Track timeout in sendChunk_hybrid (cherry picked from commit 8529cb9) * test timeout metrics (cherry picked from commit c56a0d9) * fix isCoord check * Add query warning code and add function and fields needed to track (cherry picked from commit a414641) * Track timeout in sendchunk resp3 (cherry picked from commit 6853bb9) * readd skip * Update syntax and args error to new SA as cluster * format and enrico comment * Change test to N=0 with Internal only (not working so revert afterwards) * Revert "Change test to N=0 with Internal only (not working so revert afterwards)" This reverts commit 829ac53. * meirav comments * Stablize tests * Add resp3 test * _disable_ hybrid sa timeout * Make test robust * fixup! Make test robust * remove limits (cherry picked from commit 6eee260)

* [MOD-12419] Track Timeout errors and warnings in info (#7414) * Track timeout in sendchunk resp2 * Track timeout warning in sendSearchResults * Track timeout error in searchResultReducer (cherry picked from commit fafe1dc) * Track timeout in sendChunk_hybrid (cherry picked from commit 8529cb9) * test timeout metrics (cherry picked from commit c56a0d9) * fix isCoord check * Add query warning code and add function and fields needed to track (cherry picked from commit a414641) * Track timeout in sendchunk resp3 (cherry picked from commit 6853bb9) * readd skip * Update syntax and args error to new SA as cluster * format and enrico comment * Change test to N=0 with Internal only (not working so revert afterwards) * Revert "Change test to N=0 with Internal only (not working so revert afterwards)" This reverts commit 829ac53. * meirav comments * Stablize tests * Add resp3 test * _disable_ hybrid sa timeout * Make test robust * fixup! Make test robust * remove limits (cherry picked from commit 6eee260) * Align to 8.4

* [MOD-12419] Track Timeout errors and warnings in info ([#7414](#7414)) * Track timeout in sendchunk resp2 * Track timeout warning in sendSearchResults * Track timeout error in searchResultReducer (cherry picked from commit fafe1dc) * Track timeout in sendChunk_hybrid (cherry picked from commit 8529cb9) * test timeout metrics (cherry picked from commit c56a0d9) * fix isCoord check * Add query warning code and add function and fields needed to track (cherry picked from commit a414641) * Track timeout in sendchunk resp3 (cherry picked from commit 6853bb9) * readd skip * Update syntax and args error to new SA as cluster * format and enrico comment * Change test to N=0 with Internal only (not working so revert afterwards) * Revert "Change test to N=0 with Internal only (not working so revert afterwards)" This reverts commit 829ac53. * meirav comments * Stablize tests * Add resp3 test * _disable_ hybrid sa timeout * Make test robust * fixup! Make test robust * remove limits (cherry picked from commit 6eee260) * Align to 8.4 (cherry picked from commit ac92116)

* [MOD-12419] Track Timeout errors and warnings in info ([#7507](#7507)) * [MOD-12419] Track Timeout errors and warnings in info ([#7414](#7414)) * Track timeout in sendchunk resp2 * Track timeout warning in sendSearchResults * Track timeout error in searchResultReducer (cherry picked from commit fafe1dc) * Track timeout in sendChunk_hybrid (cherry picked from commit 8529cb9) * test timeout metrics (cherry picked from commit c56a0d9) * fix isCoord check * Add query warning code and add function and fields needed to track (cherry picked from commit a414641) * Track timeout in sendchunk resp3 (cherry picked from commit 6853bb9) * readd skip * Update syntax and args error to new SA as cluster * format and enrico comment * Change test to N=0 with Internal only (not working so revert afterwards) * Revert "Change test to N=0 with Internal only (not working so revert afterwards)" This reverts commit 829ac53. * meirav comments * Stablize tests * Add resp3 test * _disable_ hybrid sa timeout * Make test robust * fixup! Make test robust * remove limits (cherry picked from commit 6eee260) * Align to 8.4 (cherry picked from commit ac92116) * Fixes * Backport more logic

cursor bot reviewed Nov 19, 2025

View reviewed changes

src/hybrid/hybrid_exec.c Outdated Show resolved Hide resolved

github-actions bot added the size:M label Nov 19, 2025

cursor bot reviewed Nov 19, 2025

View reviewed changes

tests/pytests/test_info_modules.py Outdated Show resolved Hide resolved

enricozb previously approved these changes Nov 19, 2025

View reviewed changes

Base automatically changed from omer-query-error-metrics-base to master November 19, 2025 13:39

lerman25 added 8 commits November 19, 2025 16:14

Track timeout in sendchunk resp2

b5fe6ab

Track timeout warning in sendSearchResults

e054f26

Track timeout error in searchResultReducer

5eed5b0

(cherry picked from commit fafe1dc)

Track timeout in sendChunk_hybrid

88e94c7

(cherry picked from commit 8529cb9)

test timeout metrics

9db717e

(cherry picked from commit c56a0d9)

fix isCoord check

c40f778

Add query warning code and add function and fields needed to track

949f3f3

(cherry picked from commit a414641)

Track timeout in sendchunk resp3

7f330c3

(cherry picked from commit 6853bb9)

lerman25 marked this pull request as draft November 19, 2025 14:44

lerman25 marked this pull request as ready for review November 19, 2025 14:44

lerman25 force-pushed the omerl-track-timeout-metric branch from 38967ea to 7f330c3 Compare November 19, 2025 14:46

readd skip

bcb882b

cursor bot reviewed Nov 19, 2025

View reviewed changes

src/module.c Show resolved Hide resolved

cursor bot reviewed Nov 19, 2025

View reviewed changes

src/aggregate/aggregate_exec.c Outdated Show resolved Hide resolved

lerman25 added 2 commits November 19, 2025 17:24

Update syntax and args error to new SA as cluster

12e143c

format and enrico comment

aefb759

Merge remote-tracking branch 'origin/master' into omerl-track-timeout…

ab22697

…-metric

cursor bot reviewed Nov 20, 2025

View reviewed changes

lerman25 added 3 commits November 22, 2025 16:40

Merge remote-tracking branch 'origin/master' into omerl-track-timeout…

1999f7e

…-metric

Change test to N=0 with Internal only (not working so revert afterwards)

829ac53

Revert "Change test to N=0 with Internal only (not working so revert …

408e765

…afterwards)" This reverts commit 829ac53.

meiravgri reviewed Nov 23, 2025

View reviewed changes

meirav comments

fdb2014

cursor bot reviewed Nov 23, 2025

View reviewed changes

meiravgri reviewed Nov 23, 2025

View reviewed changes

lerman25 added 2 commits November 23, 2025 13:49

Stablize tests

d1024ea

Add resp3 test

aee6f16

github-actions bot added the size:L label Nov 23, 2025

_disable_ hybrid sa timeout

abf4d19

meiravgri previously approved these changes Nov 23, 2025

View reviewed changes

lerman25 added this pull request to the merge queue Nov 23, 2025

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

Make test robust

404c0fc

lerman25 dismissed meiravgri’s stale review via 404c0fc November 24, 2025 07:31

lerman25 added 2 commits November 24, 2025 15:09

fixup! Make test robust

860367e

remove limits

be69839

meiravgri approved these changes Nov 24, 2025

View reviewed changes

lerman25 enabled auto-merge November 24, 2025 13:23

lerman25 added this pull request to the merge queue Nov 24, 2025

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

lerman25 added this pull request to the merge queue Nov 24, 2025

Merged via the queue into master with commit 6eee260 Nov 24, 2025
25 of 26 checks passed

lerman25 deleted the omerl-track-timeout-metric branch November 24, 2025 21:41

lerman25 mentioned this pull request Nov 25, 2025

[8.4] [MOD-12419] Track Timeout errors and warnings in info #7507

Merged

lerman25 mentioned this pull request Dec 2, 2025

[8.2] [MOD-12419] Track Timeout errors and warnings in info #7614

Merged

		[export]
		include = ["QueryWarningCode"]

Conversation

lerman25 commented Nov 19, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enricozb Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

cursor bot Nov 20, 2025

Choose a reason for hiding this comment

Bug: Inconsistent error tracking between RESP2 and RESP3

Uh oh!

meiravgri Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

lerman25 Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

meiravgri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

meiravgri Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

meiravgri Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

lerman25 Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor bot Nov 23, 2025

Choose a reason for hiding this comment

Bug: Inconsistent error source in RESP2 error tracking

Uh oh!

Uh oh!

meiravgri Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lerman25 commented Nov 19, 2025 •

edited by cursor bot

Loading

codecov bot commented Nov 19, 2025 •

edited

Loading