24.8.14 Backport of #75474 - Stricter checks when attaching/detaching threads to groups by ilejn · Pull Request #2016 · Altinity/ClickHouse

ilejn · 2026-07-07T11:00:11Z

Changelog category (leave one):

Not for changelog (changelog entry is not required)

In https://s3.amazonaws.com/clickhouse-test-reports/74014/dcdccd54f13e74c5018f6d40e5a4caa77858dc22/stress_test__debug_.html CurrentThread::getGroup() returned nullptr when called from TCPHandler::processOrdinaryQuery. This means that calls to CurrentThread::attachToGroup/detachFromGroupIfNotDetached are not properly nested: someone detached a thread that they didn't attach. (The thread is expected to be attached to a group because the caller of processOrdinaryQuery holds a QueryScope for the duration of the call.)

Almost all calls to CurrentThread::attachToGroupIfDetached/detachFromGroupIfNotDetached follow this pattern that looks incorrect:

    auto thread_func = [thread_group = CurrentThread::getGroup()]
    {
        SCOPE_EXIT_SAFE(
            if (thread_group)
                CurrentThread::detachFromGroupIfNotDetached();
        );

        if (thread_group)
            CurrentThread::attachToGroupIfDetached(thread_group);

        ...
    };

If the thread is already attached to a group, it'll end up detached after this function returns. Normally such thread_func is immediately scheduled on a thread pool; then the thread shouldn't be already attached to a group (... or so I'm assuming; there were no asserts about this, so CI on this PR may prove this analysis wrong). But some code sites sometimes call such function directly, e.g. Aggregator::prepareBlocksAndFillTwoLevelImpl. AFAICT, this would incorrectly detach the thread from the thread group. I'm guessing this is what caused these test failures (but I didn't try to reproduce this and don't have an explanation for why the crash happens so rarely.)

This PR replaces this pattern with a more correct one, with a bunch of asserts, wrapped in a RAII thing.

Most changes are mechanical. Most substantial ones are in ThreadStatus.h, ThreadStatusExt.cpp, and ProfileEventsExt.cpp.

(ClickHouse#75474 by @al13n321)

CI/CD Options

Exclude tests:

Regression jobs to run:

Stricter checks when attaching/detaching threads to groups Signed-off-by: Ilya Golshtein <igolshtein@altinity.com>

altinity-robot · 2026-07-07T11:45:44Z

This is an automated comment for commit f7498e9 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure

Successful checks

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Grype Scan altinityinfra/clickhouse-keeper:2016-24.8.14.10546.altinitytest	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Grype Scan altinityinfra/clickhouse-server:2016-24.8.14.10546.altinitytest-alpine	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Grype Scan altinityinfra/clickhouse-server:2016-24.8.14.10546.altinitytest	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Ready for release	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release aes_encryption	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release aggregate_functions_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release aggregate_functions_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release aggregate_functions_3	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release alter_attach_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release alter_attach_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release alter_move	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release alter_replace	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release atomic_insert	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release base_58	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release benchmark_aws_s3	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release benchmark_gcs	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release benchmark_minio	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release clickhouse_keeper_failover	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release clickhouse_keeper_no_ssl_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release clickhouse_keeper_no_ssl_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release clickhouse_keeper_ssl_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release clickhouse_keeper_ssl_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release data_types	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release datetime64_extended_range	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release disk_level_encryption	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release dns	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release engines	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release example	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release extended_precision_data_types	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release functions	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release iceberg_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release iceberg_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release jwt_authentication	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release kafka	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release kerberos	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release key_value	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release ldap_authentication	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release ldap_external_user_directory	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release ldap_role_mapping	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release lightweight_delete	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release memory	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release parquet	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release parquet_aws_s3	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release parquet_minio	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release part_moves_between_shards	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release rbac_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release rbac_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release rbac_3	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_aws_s3_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_aws_s3_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_azure_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_azure_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_export_part	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_export_partition	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_gcs_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_gcs_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_minio_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_minio_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release s3_minio_3	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release selects	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release session_timezone	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release settings	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release ssl_server_1	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release ssl_server_2	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release ssl_server_3	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release swarms	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release tiered_storage_local	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release tiered_storage_minio	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release tiered_storage_s3amazon	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release tiered_storage_s3gcs	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release version	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Regression release window_functions	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Sign release	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ success

zvonand

LGTM, almost identical to original one.

zvonand · 2026-07-15T19:40:50Z

@ilejn can we (you) verify it or we need attention from QA?

ilejn · 2026-07-16T08:25:10Z

@ilejn can we (you) verify it or we need attention from QA?

It is not trivial to reproduce. I think the choice is between 'me' and 'nobody', no reasons to involve QA.
Let's start from 'me'.

CarlosFelipeOR · 2026-07-20T18:39:06Z

AI audit note: This review comment was generated by AI (gpt-5.3-codex).

Audit update for PR #2016 (thread-group attach/detach refactor and callsite migration):

Confirmed defects:

Medium: ThreadGroupSwitcher suppresses attach-transition exceptions and can continue with inconsistent thread-group state
- Impact: work can proceed detached from the expected ThreadGroup, causing wrong memory/profile accounting and broken group restoration across thread-pool task boundaries.
- Anchor: src/Interpreters/ThreadStatusExt.cpp / ThreadGroupSwitcher; reachable from src/Storages/MergeTree/MergePlainMergeTreeTask.cpp, src/Storages/MergeTree/MutatePlainMergeTreeTask.cpp, src/Storages/MergeTree/ReplicatedMergeMutateTaskBase.cpp (allow_existing_group = true).
- Trigger: existing group is detached, then CurrentThread::attachToGroup throws during switch (for example, attach-path logical/OS exception).
- Why defect: constructor catches all exceptions, logs, nulls rollback fields, and returns; caller continues execution, while destructor becomes a no-op and cannot restore prior group.
- Fix direction (short): in ThreadGroupSwitcher, make ctor rollback-safe (restore previous group on failure) and avoid fail-open continuation after failed switch.
- Regression test direction (short): fault-inject exception in attach path and assert previous group is restored and task does not continue under wrong group state.

Coverage summary:

Scope reviewed: all 29 changed files, with full call-graph focus on thread-pool entrypoints -> ThreadGroupSwitcher transitions -> detach/restore, plus ThreadPool post-job invariants and ProfileEventsExt guards.
Categories failed: exception-safety/partial-rollback for group transition.
Categories passed: shared-state race/deadlock risk in changed paths, iterator/reference invalidation, integer/signedness issues, ownership/RAII leaks, fail-open/fail-closed checks in added guards.
Assumptions/limits: static audit only (diff + surrounding code context), no runtime fault-injection execution.

CarlosFelipeOR · 2026-07-20T18:40:14Z

@ilejn, I saw your comment. Since this PR was assigned to me, I went ahead and performed an AI-driven QA verification, focused mainly on the CI. Please find the QA Verification below — let me know if you'd like to run any further tests or validation before I add the verified label.

QA Verification — ✅ Approved

This PR is a faithful backport of upstream #75474 (already merged upstream) and does not introduce any regression. The most relevant CI configs for a threading fix — debug build + Stress test (debug), the exact config where the original crash surfaced — passed clean, and all sanitizer builds compiled successfully. The two remaining CI failures are environmental and unrelated to this change.

Summary

Item	Result
Diff vs upstream ClickHouse#75474	✅ Faithful — core RAII logic identical, divergences cosmetic/base-driven
File coverage	✅ All applicable sites converted; the 2 upstream files not touched don't exist / aren't applicable in 24.8.14
Stress test (debug)	✅ "No errors found" (asserts active, high-concurrency — the original crash config)
Stateless/Stateful/AST-fuzzer (debug)	✅ Pass (new `chassert` asserts active, no trips)
Sanitizer builds (ASAN/TSAN/MSAN/UBSAN/aarch64)	✅ All compile clean (test execution excluded per PR options)
Functional/Integration/Regression (release)	✅ Pass
`Integration tests (release) [2/4]`	⚠️ Non-blocking — job timeout (12000s), all groups `parallel0–6` FAILED=0/ERROR=0; batch-balancing, not a test failure
GrypeScan (keeper/server/-alpine)	⚠️ Non-blocking — Ubuntu base-image CVEs, fixed in #2062 (merged); unrelated to this PR

Evidence

1. Backport fidelity (vs upstream ClickHouse#75474)
The core ThreadGroupSwitcher RAII + asserts logic is byte-identical to upstream, all 29 sites are converted with the same transformation, and every divergence is cosmetic or driven by the older 24.8.14 base (the 2 upstream-only files aren't applicable in 24.8.14).

2. CI triage

Integration tests (release) [2/4]: Job timeout expired, fail: 0, passed: 712. Wrapper hit the 12000s limit and got SIGTERM (exit -15 → GH 241). A rerun should confirm timing variance; if consistent, the timeout should be raised or tests redistributed.
GrypeScan failures: base-image Ubuntu CVEs, addressed by Stable 24.8: Fix Grype scan and Ubuntu package CVEs #2062 (merged) — not caused by this PR's C++ changes.

3. On the AI audit note (fail-open in ThreadGroupSwitcher ctor)
This is byte-identical to upstream ClickHouse#75474 — a deliberate noexcept design choice (upstream comments it as "Unexpected. For caller's convenience avoid throwing exceptions"), not introduced by this backport. The trigger is also guarded: LockMemoryExceptionInThread suppresses memory-tracker exceptions right before attachToGroup, and the previous (pre-PR) pattern had no exception handling at all. Out of scope for this backport; if considered a real concern it should be raised upstream.

Note: ASAN/TSAN/MSAN/UBSAN test execution was excluded via PR options. Given the fix is upstream-validated, the backport is faithful, and debug+stress (asserts active) passed, residual risk is low. The full sanitizer matrix will run on merge.

ilejn · 2026-07-20T22:46:58Z

Hello @CarlosFelipeOR , thanks for the audit.
I agree that suppressing exceptions is not perfect.
In recent master this logic is ~~slightly~~ improved, although constructor is still noexcept, which seems a valid decision.

The improvements are in ClickHouse#105417 and ClickHouse#106462
I don't think that we have to backport these PRs to 24.8 unless it is requested specifically.

So I treat audit as positive.

Merge pull request ClickHouse#75474 from ClickHouse/tach

5cd9a99

Stricter checks when attaching/detaching threads to groups Signed-off-by: Ilya Golshtein <igolshtein@altinity.com>

ilejn added 24.8 24.8.14 labels Jul 13, 2026

zvonand approved these changes Jul 15, 2026

View reviewed changes

Merge branch 'customizations/24.8.14' into backports/24.8.14/75474

f7498e9

CarlosFelipeOR added stable verified Approved for release labels Jul 21, 2026

zvonand merged commit bfc3c2d into customizations/24.8.14 Jul 21, 2026
358 of 369 checks passed

svb-alt added 24.8.14.10547 and removed 24.8.14 labels Jul 23, 2026

Navigation Menu

Uh oh!

24.8.14 Backport of #75474 - Stricter checks when attaching/detaching threads to groups - #2016

24.8.14 Backport of #75474 - Stricter checks when attaching/detaching threads to groups#2016
zvonand merged 2 commits into
customizations/24.8.14from
backports/24.8.14/75474

ilejn commented Jul 7, 2026 •

edited

Loading

Uh oh!

altinity-robot commented Jul 7, 2026 •

edited

Loading

Uh oh!

zvonand left a comment

Uh oh!

zvonand commented Jul 15, 2026

Uh oh!

ilejn commented Jul 16, 2026

Uh oh!

CarlosFelipeOR commented Jul 20, 2026

Uh oh!

CarlosFelipeOR commented Jul 20, 2026

Uh oh!

ilejn commented Jul 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

ilejn commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

CI/CD Options

Exclude tests:

Regression jobs to run:

Uh oh!

altinity-robot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zvonand left a comment

Choose a reason for hiding this comment

Uh oh!

zvonand commented Jul 15, 2026

Uh oh!

ilejn commented Jul 16, 2026

Uh oh!

CarlosFelipeOR commented Jul 20, 2026

Uh oh!

CarlosFelipeOR commented Jul 20, 2026

QA Verification — ✅ Approved

Summary

Evidence

Uh oh!

ilejn commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ilejn commented Jul 7, 2026 •

edited

Loading

altinity-robot commented Jul 7, 2026 •

edited

Loading

ilejn commented Jul 20, 2026 •

edited

Loading