{{ message }}
Fix race between async outdated parts loading and paranoid check on startup#96705
Merged
Conversation
…nStart` The assertion fires when a part exists in ZooKeeper and is covered by another part in ZooKeeper, but doesn't exist on any disk. This happens legitimately after merges or mutations: the old part's ZK entry can outlive its on-disk data if the server is stopped before the cleanup thread removes the stale ZK entry. In the stress test (MSan), this caused the server to abort repeatedly on restart, preventing the post-stress-test check phase from running. The warning log and `ReplicatedCoveredPartsInZooKeeperOnStart` profile event are preserved for monitoring. https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=7c0839ffc607a2585f6cf49eedd0e0a49c85c452&name_0=MasterCI&name_1=Stress%20test%20%28amd_msan%29 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Member
Author
When a part exists in ZooKeeper but not on any disk and is covered by another part in ZooKeeper, proactively remove the stale ZK entry during startup. This can happen when: - A ZK multi-op in `checkPartChecksumsAndCommit` registered the part in ZK, but the server crashed before the data was flushed to disk. - A fetch was optimized to download a covering part instead, leaving the original's ZK entry orphaned. Previously, this situation was detected by `paranoidCheckForCoveredPartsInZooKeeperOnStart` which only logged a warning and fired `chassert(false)`. The assertion caused the server to abort repeatedly in debug/sanitizer builds, preventing restart after stress tests. Now the function returns the list of stale parts, and `checkPartsImpl` removes them from ZooKeeper before proceeding. This prevents false-positive 'part is lost forever' messages and eliminates the crash. https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=7c0839ffc607a2585f6cf49eedd0e0a49c85c452&name_0=MasterCI&name_1=Stress%20test%20%28amd_msan%29 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tartup The `paranoidCheckForCoveredPartsInZooKeeperOnStart` checks for parts that exist in ZooKeeper and are covered by another ZK part but don't exist in the in-memory `data_parts` set. It uses `getPartIfExists` which checks the in-memory set, not the filesystem directly. During startup, outdated (covered) parts are loaded asynchronously by `loadOutdatedDataParts`. The paranoid check runs in the first (optimistic) call to `checkPartsImpl`, which happens concurrently with async loading. A covered part may exist on disk but not yet be in `data_parts`, causing `getPartIfExists` to return null and the `chassert(false)` to fire. The `checkParts` function already has a retry mechanism: if `checkPartsImpl` returns false, it calls `waitForOutdatedPartsToBeLoaded` and retries. But the `chassert` aborts the process before the retry can happen. The fix: skip the paranoid check when `incomplete_list_of_outdated_parts` is true. It will run on the second call after all outdated parts are loaded. This was observed in the Stress test (MSan) where slower execution widens the race window between async loading and the paranoid check. https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=7c0839ffc607a2585f6cf49eedd0e0a49c85c452&name_0=MasterCI&name_1=Stress%20test%20%28amd_msan%29 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alexey-milovidov
changed the title
…ranoid-check-assertion
robot-ch-test-poll2
added
the
pr-synced-to-cloud
1 task
groeneai
added a commit
to groeneai/ClickHouse
that referenced
this pull request
Apr 16, 2026
…rOnStart The paranoid check on startup verifies that all ZK parts covered by another ZK part either exist on disk or are in the parts_to_fetch list. However, it does not check whether a local active part already covers the ZK part. This legitimately happens after a merge or mutation: the covering part is active locally, the covered part was cleaned from disk by clearOldPartsAndRemoveFromZK, but the covered part's ZK entry has not been cleaned up yet (ZK cleanup can lag behind disk cleanup, or the server was restarted between the two). The data is preserved in the local covering part, and the stale ZK entry will be removed by the cleanup thread after startup. Without this fix, the chassert(false) fires as a false positive in debug/sanitizer builds during stress tests, particularly in the createLogEntriesToFetchBrokenParts() call path (STID: 2508-5dc3, 2508-6644). Prior fixes PR ClickHouse#96164, ClickHouse#96705, and ClickHouse#100652 addressed other false-positive scenarios in this function but missed the case where a local active part covers the ZK part. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
...
Description
Fix
chassert(false)inparanoidCheckForCoveredPartsInZooKeeperOnStartfiring during Stress test (MSan).Root cause:
paranoidCheckForCoveredPartsInZooKeeperOnStartchecks for parts that exist in ZooKeeper and are covered by another ZK part but don't exist in the in-memorydata_partsset (usinggetPartIfExists). During startup, outdated (covered) parts are loaded asynchronously byloadOutdatedDataParts. The paranoid check runs in the first (optimistic) call tocheckPartsImpl, concurrently with async loading. A covered part may exist on disk but not yet be loaded intodata_parts, causinggetPartIfExiststo return null and thechassert(false)to fire.The
checkPartsfunction already has a retry mechanism: ifcheckPartsImplreturns false, it callswaitForOutdatedPartsToBeLoadedand retries. But thechassertaborts the process before the retry can happen.Fix: Skip the paranoid check when
incomplete_list_of_outdated_partsis true. The check will run on the second call tocheckPartsImplafter all outdated parts have been loaded, which is consistent with how the rest of the function handles incomplete outdated parts (see the guard at line 1920).Under MSan, everything is slower, making the async loading take much longer and significantly widening this race window.
Stack trace from CI:
CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=7c0839ffc607a2585f6cf49eedd0e0a49c85c452&name_0=MasterCI&name_1=Stress%20test%20%28amd_msan%29
Version info
26.2.1.616