Skip to content

Conversation

@masaori335
Copy link
Contributor

@masaori335 masaori335 commented Feb 2, 2026

The condition of HostDBInfo::is_down seems opposite.

This function is only used by HostDBRecord::select_best_srv() , so only SRV record case is affected.

// skip down targets.
if (rr[i].is_down(now, fail_window)) {
continue;

@masaori335 masaori335 added this to the 10.2.0 milestone Feb 2, 2026
@masaori335 masaori335 self-assigned this Feb 2, 2026
@bryancall bryancall requested a review from Copilot February 2, 2026 22:41
Copy link
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 PR fixes a logical error in the HostDBInfo::is_down method where the condition was inverted, causing it to incorrectly determine when a host should be considered down based on its failure window.

Changes:

  • Corrected the comparison operator in is_down() from < to >= to properly check if a host is still within its failure window

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

bneradt
bneradt previously approved these changes Feb 2, 2026
bryancall
bryancall previously approved these changes Feb 2, 2026
Copy link
Contributor

@bryancall bryancall left a comment

Choose a reason for hiding this comment

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

Please make comments, it is confusing

@bneradt
Copy link
Contributor

bneradt commented Feb 2, 2026

Approved. Maybe check with @serrislew and @cmcfarlen as well since it sounds like they looked into this previously.

@masaori335 masaori335 dismissed stale reviews from bryancall and bneradt via c0735a3 February 3, 2026 01:57
@masaori335
Copy link
Contributor Author

I wrote comment to clarify. Please take another look.

@masaori335
Copy link
Contributor Author

masaori335 commented Feb 3, 2026

I talked with @serrislew and @cmcfarlen, we found her patch has almost the same change in this function with other changes. And we agreed with shipping this PR first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants