Skip to content

Fix non-monotonic ProfileEvents increments#109180

Merged
azat merged 1 commit into
ClickHouse:masterfrom
azat:profile-events-inc-fix
Jul 3, 2026
Merged

Fix non-monotonic ProfileEvents increments#109180
azat merged 1 commit into
ClickHouse:masterfrom
azat:profile-events-inc-fix

Conversation

@azat

@azat azat commented Jul 2, 2026

Copy link
Copy Markdown
Member

NetworkReceiveBytes and AIOWriteBytes could be incremented with a negative ssize_t (-1 on socket timeout, -errno on AIO failure), wrapping the unsigned counter

MemoryOvercommitWaitTimeMicroseconds measured a duration with system_clock, which can step backwards. Guard the amounts and use steady_clock.

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Fix non-monotonic ProfileEvents increments

Version info

  • Merged into: 26.7.1.481 (included in 26.7 and later)

NetworkReceiveBytes and AIOWriteBytes could be incremented with a negative
ssize_t (-1 on socket timeout, -errno on AIO failure), wrapping the unsigned
counter

MemoryOvercommitWaitTimeMicroseconds measured a duration with
system_clock, which can step backwards. Guard the amounts and use
steady_clock.
@clickhouse-gh

clickhouse-gh Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [5e255c2]

Summary:

job_name test_name status info comment
AST fuzzer (arm_asan_ubsan) FAIL
Logical error: Unexpected return type from A. Expected B. Got C (STID: 3262-40aa) FAIL cidb
Finish Workflow FAIL
python3 ./ci/jobs/scripts/workflow_hooks/new_tests_check.py FAIL

AI Review

Summary

This PR fixes three sources of non-monotonic ProfileEvents by ignoring negative byte counts in the socket and AIO paths and by switching overcommit wait timing to steady_clock. The implementation looks correct, but the Bug Fix claim is still under-evidenced: there is no focused repro for the failure paths being fixed, so the wraparound could be reintroduced later without any test going red.

Tests
  • ⚠️ Add at least one targeted regression that drives a negative ssize_t or timeout path and asserts the affected ProfileEvents counter stays monotonic. The easiest candidate is the ReadBufferFromPocoSocket timeout path, which already has nearby integration coverage in tests/integration/test_secure_socket/test.py; a failing-before/passing-after check there would give this fix concrete evidence.
Final Verdict
  • Status: ⚠️ Request changes
  • Minimum required action: add a focused regression for one of the fixed failure paths, preferably the socket-timeout path, or explain why the bug cannot be exercised in a testable way.

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 2, 2026
@Diskein Diskein self-assigned this Jul 2, 2026
@azat
azat enabled auto-merge July 2, 2026 13:09
Comment thread src/IO/ReadBufferFromPocoSocket.cpp
@clickhouse-gh

clickhouse-gh Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.50% 85.50% +0.00%
Functions 92.70% 92.70% +0.00%
Branches 77.70% 77.70% +0.00%

Changed lines: Changed C/C++ lines covered: 20/21 (95.24%) · Uncovered code

Full report · Diff report

@azat

azat commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

@azat
azat added this pull request to the merge queue Jul 3, 2026
Merged via the queue into ClickHouse:master with commit d56646c Jul 3, 2026
171 of 174 checks passed
@azat
azat deleted the profile-events-inc-fix branch July 3, 2026 12:10
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants