Workflow [PR], commit [aa1f56a]

Summary: ✅

AI Review

Summary

This PR fixes incorrect OSIOWaitMicroseconds attribution when TasksStatsCounters::reset throws by catching reset failures at both reset call sites, disabling taskstats for the affected thread, and adding a focused regression test using a failpoint. I reviewed the current code and prior discussion threads, and the previously flagged issues are addressed in the latest head.

Final Verdict

• Status: ✅ Approve

LLVM Coverage Report

Changed lines: 73.08% (19/26) · Uncovered code

Full report · Diff report

I tried to pin it down, but query_thread_log is empty for affected pods.

Although excluding other possible reasons, leaving only unreset ::taskstats stats as the remaining suspect:

Kernel side: delayacct_blkio_ticks on long-idle workers. On the affected pod, cat /proc/<tid>/stat on long-lived workers (e.g. tid 1103 Fetch, tid 719 ThreadPool) shows delayacct_blkio_ticks ≈ 287,000,000 ticks ≈ 33.2 days. These threads do no actual block I/O for those 33 days - they sit in pthread_cond_*wait or similar futex sleeps. This is a well-known Linux behavior: tasks asleep with in_iowait set (or when the run-queue is empty and the scheduler tags the sleep as iowait) accumulate delayacct_blkio_ticks at HZ regardless of whether disk activity is happening. So curr.blkio_delay_total ≈ wall_time_since_thread_start for these workers - and they form a large population that rotates over weeks.

ClickHouse side: prev is zero when updateCounters runs. Given a HZ-growing curr, the only way safeDiff produces a value equal to pod uptime is prev == 0. Pre-fix, TasksStatsCounters::stats was declared ::taskstats stats; at src/Common/ThreadProfileEvents.h:210 - POD-typed, not value-initialized in the default constructor. After new TasksStatsCounters(...) the struct is indeterminate; reset() (src/Common/ThreadProfileEvents.cpp:143-147) is expected to overwrite it via stats = stats_getter();. If that assignment doesn't take effect (exception path before write, or a fresh TasksStatsCounters whose reset() hasn't run yet on the path that calls updateCounters), stats stays at whatever the constructor left there - and on platforms where uninitialized stack/heap happens to be zeroed, that's 0. Diff = curr − 0 = full kernel-side accumulation.

Hypotheses we explicitly ruled out:

• Parser misalignment in /proc/thread-self/stat. Manually walked the stat line of a thread on the affected pod and reproduced the parser's 41-token skip - it correctly lands on field 42 (delayacct_blkio_ticks). The big numbers in that line are arg_start/vsize/rsslim at much higher field positions; none has the right magnitude to be misread as the leak value.
• Procfs read failures silently zeroing prev. Queried system.text_log for CANNOT_READ_FROM_FILE_DESCRIPTOR, CANNOT_OPEN_FILE, FILE_DOESNT_EXIST, ProcfsMetricsProvider, ThreadGroupSwitcher log messages, and any message mentioning /proc/thread-self/* over the affected window on the affected pod - zero rows. The parser is not throwing.
• OSCPUWaitMicroseconds co-affected. It is not. Same struct, same safeDiff, same incrementProfileEvents, same reset() - and cpu_delay_total (from schedstat, not from stat) reads correctly. The asymmetry confirms the bug is at the field/init level, not at a higher layer.

What we couldn't fully pin down, and why it doesn't change the fix: which specific call-site reaches updateCounters with an uninitialized stats. system.query_thread_log was empty for the leaky queries - but that's because log_query_threads defaults to false and isn't enabled on these pods (src/Core/Settings.cpp:3537, src/Interpreters/ThreadStatusExt.cpp:578), not because the threads bypassed the counter. So we can't see per-thread attribution to localize the exact path. The value-init of ::taskstats stats{}; closes every path that could otherwise leak indeterminate-memory-as-prev, regardless of which one was firing in practice.

Why this PR is the right fix. It eliminates the only remaining route by which prev.blkio_delay_total can be effectively zero at the first updateCounters call: ensuring the struct is value-initialized at construction. Sibling fields (cpu_delay_total, cpu_run_virtual_total) inherit the same protection, hardening any future analogous condition. Validation should be a canary deploy + the existing times-enriched.log-style query - if no queries exceed the 3-day filter on the patched pod over the next few days, the fix is doing its job; if any do, we'll have a much narrower question to ask next.

Backport labels have been automatically added based on https://github.com/clickhouse/clickhouse-core-incidents/issues/1682

Labels added:

• v25.12-must-backport
• v26.4-must-backport

These labels ensure backport PRs are properly created for each affected release version. The release automation will create backport PRs as needed based on these labels.