Fix race condition in sharded hashed dictionary parallel loading

alexey-milovidov · claude · alexey-milovidov · commit 552bbb77b6ee · 2026-02-15T04:32:40.000+01:00
There was a TOCTOU race in the worker thread loop: after `tryPop` timed out (confirming the queue was empty), a context switch could allow the main thread to push the final block and call `finish()` before the worker checked `isFinished()`. The worker would then see the queue as finished and exit, leaving the last block unprocessed. This caused occasional loss of ~2000-3000 rows (one shard's portion of the last source block) in sharded dictionaries loaded in parallel. The fix replaces `isFinished()` with `isFinishedAndEmpty()`, which atomically verifies both that the queue is marked finished AND that no items remain. If items were pushed between the `tryPop` timeout and this check, the worker correctly retries and processes them. Closes #84468 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
diff --git a/src/Dictionaries/HashedDictionaryParallelLoader.h b/src/Dictionaries/HashedDictionaryParallelLoader.h
@@ -186,8 +186,16 @@ class HashedDictionaryParallelLoader : public boost::noncopyable
             if (!shard_queue.tryPop(block, /* milliseconds= */ 100))
             {
                 /// Check if we need to stop
-                if (stop_all_workers || shard_queue.isFinished())
+                if (stop_all_workers)
                     break;
+
+                /// Note: we use isFinishedAndEmpty() instead of isFinished() to avoid
+                /// a race condition where items could be pushed to the queue between
+                /// tryPop timing out and this check. With isFinished(), the worker
+                /// could exit while the queue still had unprocessed blocks.
+                if (shard_queue.isFinishedAndEmpty())
+                    break;
+
                 /// Timeout expired, but the queue is not finished yet, try again
                 continue;
             }
