From 90a457644c0e38f9f7e98f190662d0e801085ce5 Mon Sep 17 00:00:00 2001 From: Alexei Pastuchov Date: Mon, 13 Jul 2026 21:52:42 +0200 Subject: [PATCH] Fix data race in round_robin --job-retries test _job_retry_TEST() started the worker on a background thread but read limit.count()/limit.expected() for its assertions right after gearman_client_do() returned, without joining that thread first. The worker thread can still be executing job_retry_WORKER() (writing limit->_count) at that point, racing the main thread's read with no mutex, atomic, or join between them. ThreadSanitizer confirmed this: 29/30 runs of the --job-retries=10 collection flagged a data race between Limit::count() (main thread, round_robin.cc:339) and Limit::increment() (worker thread, round_robin.cc:262), matching the intermittent "Assertion 'uint32_t(limit.expected())' != 'uint32_t(limit.count())'" failure reported in #469. Call handle->shutdown() (which signals and joins the worker thread) before checking the counters, mirroring the pattern already used in round_robin_epoch_does_not_block_regular_TEST() in the same file. Verified: 50/50 clean runs under ThreadSanitizer after the fix (was 29/30 failing before), and 10/10 clean runs in a normal build. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Alexei Pastuchov --- tests/round_robin.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/round_robin.cc b/tests/round_robin.cc index b50f15b81..8b8e463bf 100644 --- a/tests/round_robin.cc +++ b/tests/round_robin.cc @@ -336,6 +336,13 @@ static test_return_t _job_retry_TEST(Context *context, Limit& limit) NULL, 0, // workload NULL, // result size &rc)); + + // Join the background worker thread before inspecting limit's counters: + // job_retry_WORKER (running on that thread) writes limit->_count with no + // synchronization, so reading it here without a join first is a data race + // (and can observe a stale/torn value if the worker is still mid-retry). + handle->shutdown(); + ASSERT_EQ(uint32_t(limit.expected()), uint32_t(limit.count())); ASSERT_EQ(limit.response(), rc);