fix: prevent crash on KEYLOCK_ACQUIRED check for NO_KEY_TRANSACTIONAL commands #5185
Conversation
tests/dragonfly/search_test.py
Outdated
| finally: | ||
| client.close() | ||
|
|
||
| def writer2_thread(): |
There was a problem hiding this comment.
you do not need to clone the same function twice, you can just launch a second thread with the same function.
tests/dragonfly/search_test.py
Outdated
| stop_threads.set() | ||
|
|
||
| # Check if server crashed | ||
| if server_crashed.is_set(): |
There was a problem hiding this comment.
if the server crashed, the testing framework should identify it automatically.
Signed-off-by: Volodymyr Yavdoshenko <[email protected]>
vyavdoshenko
changed the title
| std::string query_numeric = absl::StrCat("@numeric_field:[", random_val_numeric, " ", | ||
| (random_val_numeric + 100), "]"); | ||
| Run({"ft.search", kIndexName, query_content}); | ||
| Run({"ft.search", kIndexName, query_tags}); |
There was a problem hiding this comment.
I am pretty sure the bug is unrelated to query contents.
the goal is to provide a minimal reproducible example - everything else is just noise.
- please make the query constant
- there is no need for 3 calls. you can increase
kSearcherOpsif needed
There was a problem hiding this comment.
I made the test smaller
| Run({"ft.search", kIndexName, query_tags}); | ||
| Run({"ft.search", kIndexName, query_numeric}); | ||
| if (i % 50 == 0) | ||
| ThisFiber::SleepFor(std::chrono::microseconds(200 * (1 + i % 2))); |
There was a problem hiding this comment.
I suspect that a constant will suffice here
There was a problem hiding this comment.
I removed all sleeps. The bug is reproducible without them.
| "TEXT", "SORTABLE", "tags", "TAG", "SORTABLE", "numeric_field", "NUMERIC", "SORTABLE"}); | ||
| ThisFiber::SleepFor(std::chrono::milliseconds(10 + (i % 5 * 5))); | ||
| Run({"ft.dropindex", kIndexName}); | ||
| ThisFiber::SleepFor(std::chrono::microseconds(500 * (1 + i % 2))); |
There was a problem hiding this comment.
are you sure you need reindexer_fiber at all?
There was a problem hiding this comment.
I tried to remove every part. This test is as minimal as I could create to reproduce.
|
pls rebase as well |
Co-authored-by: Roman Gershman <[email protected]> Signed-off-by: Volodymyr Yavdoshenko <[email protected]>
| @@ -2768,6 +2768,47 @@ TEST_F(SearchFamilyTest, JsonSetIndexesBug) { | |||
| EXPECT_THAT(resp, IsUnordArrayWithSize(IsMap("text", "some text"))); | |||
| } | |||
|
|
|||
| TEST_F(SearchFamilyTest, SearchReindexWriteSearchRace) { | |||
| const std::string kIndexName = "myRaceIdx"; | |||
| const int kWriterOps = 1000; | |||
There was a problem hiding this comment.
it reproduces for me with all constants be 100
There was a problem hiding this comment.
please check if also for you. I prefer not having long running unit tests
There was a problem hiding this comment.
I updated to 200 because 100 sometimes passes
Fixes: #5173
Problem:
Server crashes with
Check failed: !lock_args.fps.empty()during concurrent search indexing operations (FT.CREATE, FT.DROPINDEX, FT.SEARCH, etc.).Root Cause:
Commands with
CO::NO_KEY_TRANSACTIONALflag don't have keys to lock, resulting in empty fingerprints (kv_fp_). However, the transaction system:KEYLOCK_ACQUIREDflag inScheduleInShard()CancelShardCb()Solution:
Add proper conditional checks in
CancelShardCb()