Skip to content

nexthop health status don't overflow fail count#13164

Merged
traeak merged 1 commit into
apache:masterfrom
traeak:nh_failcount
May 14, 2026
Merged

nexthop health status don't overflow fail count#13164
traeak merged 1 commit into
apache:masterfrom
traeak:nh_failcount

Conversation

@traeak
Copy link
Copy Markdown
Contributor

@traeak traeak commented May 14, 2026

This is a check to ensure that failcount doesn't overflow uint32.
default ATS configuration is from https://docs.trafficserver.apache.org/en/latest/admin-guide/files/records.yaml.en.html#proxy-config-http-parent-proxy-fail-threshold

proxy.config.http.parent_proxy.fail_threshold 10

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request prevents HostRecord::failCount (a std::atomic<uint32_t>) from overflowing/wrapping when repeatedly marking a next hop down, by saturating the counter at UINT32_MAX. This aligns with ATS’s parent fail threshold behavior (default 10) while making the failure accounting robust under extreme conditions.

Changes:

  • Saturate failCount increments in NextHopHealthStatus::markNextHop() to avoid uint32_t overflow.
  • Fix log formatting to correctly print the uint32_t fail count using PRIu32.
  • Add a unit test that forces failCount near UINT32_MAX and verifies it saturates (does not wrap to 0).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/proxy/http/remap/NextHopHealthStatus.cc Adds saturating increment logic for failCount and updates related log format specifiers.
src/proxy/http/remap/unit-tests/test_NextHopRoundRobin.cc Adds a regression test ensuring failCount saturates at UINT32_MAX during repeated markdowns.

@traeak traeak merged commit 0c052b1 into apache:master May 14, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants