Repository navigation
Conversation
ThreadSanitizer reported a data race in `CASDecommission.FenceLossOnAListPageStillRunsTheConfirm`. The test installs clock callbacks after `Pool::openForDecommission` has started the lease renewer. The main thread assigns the clock's `std::function` while the renewer calls the same object from `CasOperation::replace`. Owning the captured clock keeps that clock alive, but does not make replacement of the enclosing `std::function` safe. Protect the clock and request-sleep callbacks in `CasRequests` with separate `SharedMutex` instances. Callback calls take `SharedLockGuard`, and test setters take `UniqueLock`. Hold shared ownership until the callback returns, including exception unwinding, so a setter cannot replace or destroy a callable that another thread is still using. Separate mutexes allow a sleep callback to issue a request that reads the clock without acquiring the same mutex again. Keep the existing `now_ms` and `sleep_ms` call expressions. These names now refer to private methods that acquire the guards, while the stored callbacks become `now_ms_fn` and `sleep_ms_fn`. Existing request loops, hot-key handling, and `pause` therefore use the protected accessors without changes to their call sites or retry logic. Apply the same protection to `CasRefLedger::recovery_retry_sleep_fn`. Both `setCasRetrySleepForTest` and `setRefRecoveryRetrySleepForTest` replace this callback under exclusive ownership. Recovery holds shared ownership during backoff, after releasing the table's `state_mutex`. The callback guard is released before the exception path reacquires `state_mutex`, preserving the existing recovery cleanup order. Use the project's TSA-aware lock wrappers and mark all three callback fields with `TSA_GUARDED_BY`. Document that the mutexes protect callback replacement by tests while background work is running. Shared ownership still allows concurrent callback calls; captured mutable state must remain thread-safe. Fix the shared callback APIs rather than only the failing decommission call site. This also covers other tests that replace callbacks on an already open pool, while preserving the existing decommission callback selection and clock precedence. CI: https://github.com/Altinity/ClickHouse/actions/runs/41911689821 Related: #2798 Validation: `git diff --check` passed. Source review covered callback accesses, recovery exception cleanup, and existing reentrant test callbacks. Builds, tests, and a TSan rerun were not run as requested. Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
k-morozov
marked this pull request as ready for review
November 5, 2026 18:15
Calling the stored `std::function` does not change the `CasRequests` object. The callback mutex is already `mutable`, so shared locking works in a const method while still protecting callback replacement. Keep the callback storage non-mutable and retain the existing lock scope. CI: https://github.com/Altinity/ClickHouse/actions/runs/41911689821 Validation: `git diff --check` passed. Builds and tests were not run as requested. Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
The original TSan race in `CASDecommission.FenceLossOnAListPageStillRunsTheConfirm` came from replacing an admin pool's clock callback after `Pool::openForDecommission` had already started its lease renewer. The previous repair protected callback replacement and invocation with shared mutexes, adding locking to ordinary production requests as well. Use initialization order to remove that overlap. All three request planes already receive `PoolConfig::boot_ms_fn` and `PoolConfig::retry_sleep_fn` in the `Pool` constructor. Initialize the ref recovery retry callback from the same configuration before `mountWritable` can start workers, and remove the post-open admin setters from `decommissionPoolMember`. Remove the callback mutexes and their guards from `CasRequests` and `CasRefLedger`. Restore direct `std::function` calls under the existing `now_ms` and `sleep_ms` names. Callback setters require setup without concurrent callback use; they no longer support replacement during active work. Captured mutable state still needs its own synchronization. Keep the local `control_requests` setup: its callbacks are installed before the first `admit`, and the admin workers use separate engines. The admin planes now use the configured callbacks. Explicit drain callbacks fill empty configuration fields and still directly configure the local control engine. No existing caller supplies different callbacks through both channels. Make the first async completion check fatal in `CASGCTeardownStop.OpenPlaneSleepReturnsAtOnceOnceTeardownBegan`. After a timeout, the test must not continue replacing a sleep callback while the earlier async `pause` may still be running. Validation: `git diff --check` passed. Independent source reviews covered setter callers, worker startup, detached snapshot and recovery tasks, callback ownership, and timeout paths. Builds, tests, and a TSan rerun were not run as requested. CI: https://github.com/Altinity/ClickHouse/actions/runs/41911689821 Related: #2798 Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
This branch has not been deployed
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
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.
ThreadSanitizer reported a data race in
CASDecommission.FenceLossOnAListPageStillRunsTheConfirm. The test installs clock callbacks afterPool::openForDecommissionhas started the lease renewer. The main thread assigns the clock'sstd::functionwhile the renewer calls the same object fromCasOperation::replace. Owning the captured clock keeps that clock alive, but does not make replacement of the enclosingstd::functionsafe.Protect the clock and request-sleep callbacks in
CasRequestswith separateSharedMutexinstances. Callback calls takeSharedLockGuard, and test setters takeUniqueLock. Hold shared ownership until the callback returns, including exception unwinding, so a setter cannot replace or destroy a callable that another thread is still using. Separate mutexes allow a sleep callback to issue a request that reads the clock without acquiring the same mutex again.Keep the existing
now_msandsleep_mscall expressions. These names now refer to private methods that acquire the guards, while the stored callbacks becomenow_ms_fnandsleep_ms_fn. Existing request loops, hot-key handling, andpausetherefore use the protected accessors without changes to their call sites or retry logic.Apply the same protection to
CasRefLedger::recovery_retry_sleep_fn. BothsetCasRetrySleepForTestandsetRefRecoveryRetrySleepForTestreplace this callback under exclusive ownership. Recovery holds shared ownership during backoff, after releasing the table'sstate_mutex. The callback guard is released before the exception path reacquiresstate_mutex, preserving the existing recovery cleanup order.Use the project's TSA-aware lock wrappers and mark all three callback fields with
TSA_GUARDED_BY. Document that the mutexes protect callback replacement by tests while background work is running. Shared ownership still allows concurrent callback calls; captured mutable state must remain thread-safe.Fix the shared callback APIs rather than only the failing decommission call site. This also covers other tests that replace callbacks on an already open pool, while preserving the existing decommission callback selection and clock precedence.
CI: https://github.com/Altinity/ClickHouse/actions/runs/41911689821
Related: #2798
Validation:
git diff --checkpassed. Source review covered callback accesses, recovery exception cleanup, and existing reentrant test callbacks. Builds, tests, and a TSan rerun were not run as requested.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed a data race when CAS tests replace clock and retry callbacks while background tasks are running.
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: