Fix lock inversion in ProcessList#96182
Merged
alexey-milovidov merged 2 commits intomasterfrom Feb 7, 2026
Merged
Conversation
Contributor
|
Workflow [PR], commit [048247a] Summary: ❌
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a potential deadlock in ProcessList teardown by preventing lock inversion between ProcessList::mutex, CancellationChecker, and the overcommit tracker.
Changes:
- Add
OvercommitTrackerinclude to enable thread-local blocking. - Block
OvercommitTrackerwhile appending done tasks toCancellationCheckerduringProcessListEntrydestruction.
Comment on lines
+402
to
+403
| /// We need to block the overcommit tracker here to avoid lock inversion because OvercommitTracker takes a lock on the ProcessList::mutex. | ||
| /// When task is added, we lock the ProcessList::mutex, and then the CancellatioCheker mutex. |
There was a problem hiding this comment.
The explanation is a bit confusing/mismatched with the destructor path (this code runs on destruction, not on task add). Consider rewording to describe the actual inversion: appendDoneTasks() locks CancellationChecker, and if overcommit triggers inside that path it may try to lock ProcessList::mutex, which can invert with the code path that holds ProcessList::mutex and then takes CancellationChecker.
Suggested change
| /// We need to block the overcommit tracker here to avoid lock inversion because OvercommitTracker takes a lock on the ProcessList::mutex. | |
| /// When task is added, we lock the ProcessList::mutex, and then the CancellatioCheker mutex. | |
| /// We need to block the overcommit tracker here to avoid lock inversion between CancellationChecker and ProcessList::mutex. | |
| /// appendDoneTasks() acquires the CancellationChecker mutex; if overcommit is triggered inside that path it may try to lock ProcessList::mutex, | |
| /// which would invert the order with other code paths that hold ProcessList::mutex first and then access CancellationChecker. |
Co-authored-by: Copilot <[email protected]>
yariks5s
approved these changes
Feb 6, 2026
This was referenced Feb 7, 2026
robot-clickhouse
added a commit
that referenced
this pull request
Feb 7, 2026
This was referenced Feb 7, 2026
robot-clickhouse
added a commit
that referenced
this pull request
Feb 7, 2026
This was referenced Feb 7, 2026
robot-clickhouse
added a commit
that referenced
this pull request
Feb 7, 2026
This was referenced Feb 7, 2026
robot-clickhouse
added a commit
that referenced
this pull request
Feb 7, 2026
clickhouse-gh bot
added a commit
that referenced
this pull request
Feb 7, 2026
Backport #96182 to 25.12: Fix lock inversion in ProcessList
antonio2368
added a commit
that referenced
this pull request
Feb 9, 2026
Backport #96182 to 25.8: Fix lock inversion in ProcessList
antonio2368
added a commit
that referenced
this pull request
Feb 9, 2026
Backport #96182 to 25.11: Fix lock inversion in ProcessList
antonio2368
added a commit
that referenced
this pull request
Feb 9, 2026
Backport #96182 to 26.1: Fix lock inversion in ProcessList
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.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix possible deadlock in ProcessList. It can happen because of possible lock inversion if memory overcommit tracker triggers when we are adding task to cancellation checker.
The bug was introduced here #95535
We need to backport the fix also.
Another option is to avoid double locking in other place also, but it requires more thoughtful approach as we can easily introduce nasty bugs if we are not careful.
Documentation entry for user-facing changes