Workflow [PR], commit [06afc7d]

Summary: ✅

AI Review

Summary

This PR adds an AArch64/NEON vectorized path for find_symbols helpers, includes follow-up fixes for runtime-needle correctness/performance edge cases, and adds focused regression coverage for empty runtime needles, embedded \0, and long-haystack reverse runtime paths. After reviewing the current diff and all prior discussion threads/comments, I did not find unresolved correctness, safety, or evidence gaps in the latest PR head.

Final Verdict

• Status: ✅ Approve

@groeneai, fix Logical error: 'phi_squared <= max_phi_squared + 1e-6' (STID: 2508-2e40) and send a separate PR with the fix.

@alexey-milovidov — STID 2508-2e40 (phi_squared <= max_phi_squared + 1e-6) is the chronic CrossTab/CramersV state-corruption family already tracked under issue #104183.

We posted a comprehensive analysis there earlier today (2026-05-06): #104183 (comment)

TL;DR:

• 5 distinct stack signatures observed across 13 days, all dating from after PR Optimize cramersV, cramersVBiasCorrected, theilsU, and contingency when used with window functions #93384 (2026-04-23)
• The phi_squared assertion is one of those signatures; sibling signatures hit HashTable::write and HashTable::begin from CrossTabCountsState::serialize and CrossTabCountsState::merge
• Root cause hypothesis: a missing getStateVariant / canMergeStateFromDifferentVariant combinator override, or a sizeOfData mismatch between CrossTabAggregateData (GROUP BY state) and CrossTabPhiSquaredWindowData (window state) — leading to placement-new in a wrong-sized buffer and a torn HashMap that later yields a 0xa faulting address or an out-of-range phi_squared
• Cannot reproduce locally without the specific concurrent stress conditions; @nihalzp (PR Optimize cramersV, cramersVBiasCorrected, theilsU, and contingency when used with window functions #93384 author) was pinged for a combinator audit

Opening a separate PR purely for the phi_squared manifestation would splinter what is one underlying bug. Will file the fix under #104183 once the offending combinator is pinpointed. Happy to upstream debug-build invariants in CrossTabCountsState::serialize/merge as a stand-alone diagnostic PR if useful in the meantime.

— @groeneai (worker session cron:clickhouse-ci-task-worker:20260506-184500)

@groeneai, investigate the chronic Possible deadlock on shutdown (see gdb.log) failure on Stress test (arm_debug): https://s3.amazonaws.com/clickhouse-test-reports/PRs/104228/726ffb1642471e8ebfe17e1e802ddc1e09490cd2/stress_test_arm_debug/gdb.log and provide a fix in a separate PR. This fails 8-10 times per day on master (CIDB) and is unrelated to this PR (the stuck threads are in absl::Mutex shutdown, no find_symbols frames). If a fix is already in progress, link it here.

@alexey-milovidov investigation results — gdb.log analysis of PRs/104228/726ffb1.../stress_test_arm_debug/gdb.log.

Root cause: leaked local ThreadPool instances at server shutdown

Decoded the gdb.log (725 threads, 7.2MB). The deadlock is not in absl::Mutex shutdown itself — those frames are a side effect. The real blocker is in GlobalThreadPool::shutdown() which can't join its own std::threads because each is running a leaked local pool's worker() loop.

Main thread (LWP 499292)

#0  __futex_abstimed_wait_common64
#3  __pthread_clockjoin_ex                                              ← joining a worker thread
#5  std::thread::join (this=0xff337a7c8ae8)
#6  ThreadPoolImpl<std::thread>::ThreadFromThreadPool::join
#7  ThreadPoolImpl<std::thread>::finalize (this=0xff355d30bc00)        ← GlobalThreadPool::finalize
#8  Server::main::$_15::operator()                       Server.cpp:1482
#9  BasicScopeGuard::invoke
#11 Server::main                                          Server.cpp:3296

Server.cpp:1482 is the last scope guard:

SCOPE_EXIT_SAFE({
    LOG_INFO(log, "Waiting for background threads");
    GlobalThreadPool::instance().shutdown();      // <-- stuck here
    LOG_INFO(log, "Background threads finished in {} ms", watch.elapsedMilliseconds());
});

The thread it's waiting on (LWP 500233 "ThreadPool")

#0  __futex_abstimed_wait_common64
#5  std::condition_variable::wait
#7  std::condition_variable::wait<...lambda#2>(unique_lock, pred)
#8  ThreadPoolImpl<ThreadFromGlobalPoolImpl<false,true>>::ThreadFromThreadPool::worker
                                                          ThreadPool.cpp:736   ← inner pool's wait loop
#15 ThreadPoolImpl<std::thread>::ThreadFromThreadPool::worker
                                                          ThreadPool.cpp:809   ← outer std::thread running the job

The std::thread (frame #15, owned by GlobalThreadPool) is running a job which is "be the inner pool's worker forever" (frame #8 at line 736). The inner pool's parent_pool.new_job_or_shutdown.wait(...) only exits when parent_pool.shutdown is set. GlobalThreadPool::shutdown() sets its OWN shutdown, not the inner pool's — those are independent.

Scale of the leak (this run)

56 worker threads stuck at ThreadPool.cpp:736 across 7 distinct leaked pools:

All threads are named "ThreadPool" (the default — setThreadName runs inside the worker loop AFTER receiving a job, but these never received one). So the leaked pools can't be identified by thread name alone.

Why this matches the chronic 8-10/day rate

Local ThreadPool (= ThreadPoolImpl<ThreadFromGlobalPool>) instances must be destroyed (which calls finalize() → sets shutdown → notifies) before GlobalThreadPool::shutdown() runs. Some subsystem(s) hold a reference to a local ThreadPool past the scope guard at Server.cpp:1482. Likely candidates: long-lived singletons, registries, or thread-local resources that aren't drained in Server::main cleanup. Different stress runs hit different leakers, which is consistent with this being an umbrella affecting multiple subsystems (8-10/day spread across several specific patterns).

The absl::Mutex frames you saw

gRPC has its own WorkStealingThreadPool. 17 threads in this gdb.log are stuck in gRPC's WorkSignal::WaitWithTimeout (which uses absl::Mutex). They're not the blocker — they're just idle workers of gRPC's pool that haven't been told to exit because gRPC's EventEngine global is presumably one of the things that doesn't clean up before the Server::main scope guard. They're a symptom of the same broader pattern (subsystem lifetimes not properly ordered against GlobalThreadPool shutdown), not an independent deadlock.

Direction for fix

Two angles, complementary:

1. Targeted (per-subsystem): Identify the leaked local pools. The most practical way is to add a LOG_INFO listing name, num_threads, scheduled_jobs for every ThreadPoolImpl still alive immediately before the GlobalThreadPool::shutdown() call (or in GlobalThreadPool::finalize itself, before notify_all). When chronic hits land in CI, the log will name the leaker(s). Possibly enable behind a debug flag to avoid prod overhead. Then fix the lifetime in each named owner.

2. Structural: A global registry of live ThreadPoolImpl<ThreadFromGlobalPool> instances; GlobalThreadPool::shutdown() walks the registry, sets shutdown=true and notifies each registered pool's condvar before joining its own std::threads. This breaks the dependency on subsystem destruction order. Slight added cost per pool construction/destruction (register/unregister + lock); modest. Downside: papers over real lifetime bugs.

Recommendation: do (1) first to identify the offenders, then (2) as a backstop if the list of leakers is long-tailed.

Not pushing a fix yet

Per our policy, no fix PR without local reproduction first. Will:

• Build a debug binary with the diagnostic LOG_INFO patch from (1) and run stress locally to confirm at least one specific leaker
• Open a fix PR for whichever subsystem(s) show up
• If the list is long, also open (2) as a backstop

Tracking under task 2026-04-01-ci-hung-check-deadlock-chronic-trunk-is. CIDB rate (14d): 8-10/day on master with a 78-hit spike on May 11 (concentrated in 2 PRs, possibly a single bad run). Will keep this thread updated.

@groeneai, investigate the failure: 01410_nullable_key_and_index on Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) and provide a fix in a separate PR. This is unrelated to this PR (the diff is row ordering of a Nullable key under ParallelReplicas, no find_symbols frames). It is flaky on master (CIDB shows 4 hits in the last 30 days across 4 different PRs). If a fix is already in progress, link it here.

@groeneai, investigate the CH Inc sync failure: the sync PR ClickHouse/clickhouse-private#57269 fails on test_statistics_cache/test_smt.py::test_statistics_cache_smt (Integration tests (amd_asan_ubsan, db disk, old analyzer, 5/6)). This is unrelated to this PR — the diff is entirely under base/base/find_symbols.h (AArch64/NEON path + SSE2 zero-pad fix) and has no contact with the SharedMergeTree statistics cache. Please look into the chronic failure and provide a fix in a separate PR. If a fix is already in progress, link it here.

LLVM Coverage Report

Changed lines: 100.00% (109/109) · Uncovered code

Full report · Diff report

Improvements exactly where we expected:

@alexey-milovidov, investigation of the test_statistics_cache_smt failure on CH Inc sync PR clickhouse-private#57269.

What we can see on the public side

test_statistics_cache (the public MergeTree sibling — tests/integration/test_statistics_cache/test.py) has 0 master failures in the last 60 days. CIDB query:

SELECT countIf(pull_request_number=0) AS master, countIf(pull_request_number>0) AS prs
FROM default.checks
WHERE test_name LIKE '%test_statistics_cache%'
  AND test_status IN ('FAIL','ERROR')
  AND check_start_time > now() - INTERVAL 60 DAY
-- master=0, prs=1048, distinct_prs=15

All 1048 PR failures are concentrated on 2 PRs, both unmerged:

Notably your own PR #100185 makes every test_statistics_cache sub-test fail — the assertions you added fire deep inside the cache's statistics-loading path. PR #100185 touches ColumnReplicated.{cpp,h} among many other column types, which is the column used by SMT replication. Likely the same latent issue your assertions are designed to surface (column shared-ownership violation during IColumn::mutate) also exists in code reachable from the SMT statistics-load path.

What we can't see

The failing report URL is https://d1k2gkhrlfqv31.cloudfront.net/clickhouse-test-reports-private/... which returns HTTP 401 (Basic auth required) — we can't read the private CI artifact. The test file test_statistics_cache/test_smt.py is also in the private repo only (public has just test.py).

The shared code path

The LoadedStatisticsMicroseconds ProfileEvent that drives _assert_load lives in MergeTreeData::getConditionSelectivityEstimatorByPredicate (src/Storages/MergeTree/MergeTreeData.cpp:864). MergeTreeData is the public base for both StorageMergeTree and StorageSharedMergeTree, so the cache (cached_estimator, stats_mutex, refreshStatistics) is shared infrastructure. SMT-only divergence will be in:

• IMergeTreeDataPart::loadStatistics() overrides on the SMT-side DataPartShared/equivalent
• the part-discovery code that feeds getDataPartsVectorForInternalUsage under SMT

Recent public master commits touching this path that landed after the last green state and could be the sync regressor:

• 78d10eb43cd statistics retry after failure (@zoomxi, 2026-05-08) — restructures IMergeTreeDataPart::getEstimates to build a local new_estimates and only move on success; adds merge_tree_load_statistics_throw fail point. If the SMT override of loadStatistics throws once and the new retry contract isn't honored, the assertion _loaded_stats_us > 0 will silently fail.
• 801397d3c11 / 65ad40dfe08 / e9f27104b27 — null_count part-pruning + auto-statistics randomization (@zoomxi, PR Add NullCount statistics support for part pruning #104214). Changes how parts are pruned before statistics loading.

What we need from you

Without the private CI artifact we're guessing. To root-cause this we need any of:

1. The exact failing assertion line + stack trace from test_statistics_cache_smt (paste it in the comment).
2. The LoadedStatisticsMicroseconds value the failing query reports (0? or non-zero but cached unexpectedly?).
3. Confirmation of whether the sync PR includes commit 78d10eb43cd and/or 801397d3c11.
4. Read access to the private report bucket, or a re-publish of the failing test output as a public gist.

If (1) shows the failure is on the same _assert_load line and _loaded_stats_us == 0, the most likely public-side suspect is the new retry contract in 78d10eb43cd not propagating through the SMT-side part override. We'd then fix it on master and request a re-sync.