Skip `/proc/cpuinfo` lines that do not parse in `CPUFrequencyMHz` instead of throwing by groeneai · Pull Request #126540 · ClickHouse/ClickHouse

groeneai · 2026-10-17T22:49:53Z

Related: #39193

Changelog category (leave one):

Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Description

AsynchronousMetrics::update parses /proc/cpuinfo with std::stoi/std::stod and an unconditional ++position(). A line it does not expect throws std::invalid_argument or std::out_of_range, and tryLogCurrentException turns that std::logic_error into an abort on debug and sanitizer builds and with abort_on_logical_error. This was observed aborting the server mid-run in sanitizer CI jobs. A default release build instead logs a stack trace on every update and drops CPUFrequencyMHz for all cores.

There are three sites:

cpu MHz: if (auto colon = s.find_first_of(':')) tests truthiness instead of npos, and std::stod(s.substr(colon + 2)) throws even with a colon present: on an empty value after the colon or a colon at the end of the line.
processor: std::stoi throws when no digit follows or the number does not fit int.
A last line without \n: the step past EOF fails chassert(pos_addr <= end_addr) in the next readEscapedStringUntilEOL.

The loop moves to readCPUFrequenciesFromProcCPUInfo(ReadBuffer &). It uses non-throwing parses (std::from_chars, tryReadText) and checkChar('\n'), and it skips a line it cannot parse. A frequency after an unparseable processor line is no longer attributed to the previous core. Valid x86 and s390x (#39193) input gives the same values as before.

Validation: I bind-mounted a crafted file over /proc/cpuinfo in a private mount namespace (printf 'processor\t: 0\ncpu MHz\t\t: \n' is enough). A master debug server aborts during startup on each of six malformed shapes. With the fix, all of them stay up, and a file with one bad cpu MHz line still reports the other core. On a snapshot of a real 384-core /proc/cpuinfo, both binaries return identical CPUFrequencyMHz. A new gtest covers valid and malformed input, and reverting any one of the three changes makes it fail.

🤖 Generated with Claude Code

Workflow [PR]
Sync PR [sync-upstream/pr/126540]

… of throwing The /proc/cpuinfo scraper in AsynchronousMetrics::update used std::stod, std::stoi and substr on host text, and stepped the buffer past EOF when the last line had no EOL. An unexpected line threw std::invalid_argument or std::out_of_range (or failed the pos_addr <= end_addr chassert), and tryLogCurrentException aborts on a std::logic_error on debug and sanitizer builds and with abort_on_logical_error. On a default build it logged a stack trace on every update and dropped the metric for all cores. - cpu MHz: the colon was tested for truthiness, not npos, and stod(substr(colon + 2)) threw on an empty value or a colon at the end of the line, so an npos check alone is not enough. Now find(':') != npos, then tryReadText on the rest. - processor: std::from_chars instead of std::stoi. A line that does not parse unsets the core id, so the next cpu MHz is not attributed to the previous core. - EOL: checkChar('\n') instead of ++position(), a no-op at EOF. The loop moves to readCPUFrequenciesFromProcCPUInfo(ReadBuffer &) so it can be unit-tested. Valid x86 and s390x input gives the same values as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

groeneai · 2026-10-17T22:49:59Z

Internal second-model review: adjudication log (click to expand)

Pre-publication review by an independent model (engine: codex; 2 findings; 1 full pass).

#	Sev	Finding	Verdict	Evidence / action
1	⚠️	A last line ending in `\` at EOF makes `readEscapedStringUntilEOL` throw and drop all frequencies	DISAGREE	For a `String`, the call at `ReadHelpers.cpp:453` resolves to the non-template `bool parseComplexEscapeSequence(String &, ReadBuffer &)` (`ReadHelpers.h:2250`), which returns `false` at EOF instead of throwing; the loop then ends. Real `/proc/cpuinfo` has no backslashes.
2	⚠️	`cpu MHz : 1e9999` is published as `inf` instead of skipped	DISAGREE	`tryReadFloatTextPrecise` parses an overflow to `inf` by design (`readFloatText.cpp:592-604`, strtod semantics), so the line parses; `std::stod` already returned `inf`/`nan` for `inf`/`nan` before. The kernel prints `cpu MHz` as fixed-point (384/384 lines on a real host).

Severity: ❌ blocker / ⚠️ major / 💡 nit. DISAGREE verdicts carry recorded evidence and are
terminal per finding.

Session id: cron:clickhouse-review-slot-8:20261017-222100

groeneai · 2026-10-17T22:50:07Z

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. A crafted file bind-mounted over `/proc/cpuinfo` in a private mount namespace aborts a master debug server at startup on each of 6 malformed shapes (no colon, empty value, colon at end, `processor` without digit, `int` overflow, no final EOL).
b	Root cause explained?	`std::stod`/`std::stoi`/`substr` throw `std::logic_error` subclasses, and `++position()` steps past EOF; `tryLogCurrentException` aborts on a `std::logic_error` on debug/sanitizer builds and with `abort_on_logical_error`.
c	Fix matches root cause?	Yes. The parser no longer throws (`std::from_chars`, `tryReadText`, `checkChar('\n')`); the `catch` block and `tryLogCurrentException` are unchanged.
d	Test intent preserved / new tests added?	New gtest `AsynchronousMetricsCPUInfo.*` (valid x86/s390x/no-EOL/empty input, 6 malformed lines). No existing test changed.
e	Both directions demonstrated?	Yes. e2e: base aborts, fix stays up, on all 6 shapes. gtest: reverting each of the 3 changes alone turns it red (2 uncaught `std::invalid_argument`, 1 abort on `pos_addr <= end_addr`).
f	Fix is general across code paths?	Yes. All 3 throwing sites of the parser fixed; Keeper uses the same code; the other `/proc/cpuinfo` parser (`getNumberOfCPUCoresToUse`) already catches `std::exception`.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	Yes. Matrix of malformed and valid shapes in the gtest; a real 384-core `/proc/cpuinfo` gives byte-identical output on base and fix. No type wrappers apply.
h	Backward compatible? (maintainer-approved exception only)	Yes. No settings or formats; valid input gives the same values. An invalid line is now skipped instead of throwing.
i	Invariants and contracts preserved?	Yes. `readEscapedStringUntilEOL` stops only at `\n` or EOF, so the loop always progresses and never reads past the buffer end; a frequency is stored only under a parsed core id.

Session id: cron:clickhouse-impl-slot-4:20261017-215100

groeneai · 2026-10-17T22:50:39Z

cc @Algunenano @pamarcos, could you review this and add the can be tested label so CI runs? The /proc/cpuinfo parser behind CPUFrequencyMHz used std::stoi/std::stod and an unconditional step past the EOL, so a malformed line threw a std::logic_error that aborts debug and sanitizer builds; it now uses non-throwing parses and skips such lines.

clickhouse-gh · 2026-10-17T22:51:03Z

Workflow [PR], commit [a805db7]

AI Review

Summary

The PR makes CPUFrequencyMHz collection skip malformed /proc/cpuinfo fields and handle a final line without a newline, with focused regression tests. Review of the current diff, surrounding code, parsing helpers, and prior discussion found no actionable issues.

Missing context / blind spots

⚠️ Unit and sanitizer CI validation for a805db727a69e72244c83f033d5e254c4dbdb2b8 is still pending. No local build is available, so the added AsynchronousMetricsCPUInfo tests were reviewed but not run locally. A completed unit-test CI run would close this validation gap.

Final Verdict

Status: ✅ Approve, subject to the remaining CI checks.

clickhouse-gh Bot added pr-not-for-changelog This PR should not be mentioned in the changelog comp-logging Logging/monitoring/observability (logs, metrics, tracing, instrumentation). labels Oct 17, 2026

alexey-milovidov added the can be tested Allows running workflows for external contributors label Oct 18, 2026

Navigation Menu

Repository navigation

Skip `/proc/cpuinfo` lines that do not parse in `CPUFrequencyMHz` instead of throwing - #126540

Skip `/proc/cpuinfo` lines that do not parse in `CPUFrequencyMHz` instead of throwing#126540
groeneai wants to merge 1 commit into
ClickHouse:masterfrom
groeneai:groeneai/async-metrics-cpuinfo-no-throw

groeneai commented Oct 17, 2026 •

edited by clickhouse-gh Bot

Loading

Uh oh!

groeneai commented Oct 17, 2026

Uh oh!

groeneai commented Oct 17, 2026

Uh oh!

groeneai commented Oct 17, 2026

Uh oh!

clickhouse-gh Bot commented Oct 17, 2026 •

edited

Loading

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

groeneai commented Oct 17, 2026 • edited by clickhouse-gh Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Description

Uh oh!

groeneai commented Oct 17, 2026

Uh oh!

groeneai commented Oct 17, 2026

Uh oh!

groeneai commented Oct 17, 2026

Uh oh!

clickhouse-gh Bot commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Missing context / blind spots

Final Verdict

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

groeneai commented Oct 17, 2026 •

edited by clickhouse-gh Bot

Loading

clickhouse-gh Bot commented Oct 17, 2026 •

edited

Loading