[fix][client] Fix stale Healthy state in SameAuthParamsLookupAutoClusterFailover causing flaky test#25388
Merged
merlimat merged 1 commit intoapache:masterfrom Mar 23, 2026
Conversation
…terFailover causing spurious recovery bounce When findFailoverTo() skipped over an unavailable service, it left that service's state as stale Healthy. This caused a spurious recovery bounce (e.g. 0→2→1→2 instead of 0→2) on the next check cycle, as firstHealthyPulsarService() would immediately "recover" to the stale service. Combined with 3-second probe timeouts on dead services, this could push total failover time past the integration test's awaitility timeout, making SameAuthParamsLookupAutoClusterFailoverTest flaky. Fix: mark skipped services as Failed in findFailoverTo() when probing finds them unavailable. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
lhotari
force-pushed
the
lh-fix-SameAuthParamsLookupAutoClusterFailoverTest
branch
from
47fd724 to
189d32b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #25388 +/- ##
============================================
+ Coverage 72.13% 72.76% +0.63%
- Complexity 34023 34289 +266
============================================
Files 1954 1954
Lines 154857 154859 +2
Branches 17739 17740 +1
============================================
+ Hits 111699 112689 +990
+ Misses 34120 33110 -1010
- Partials 9038 9060 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
merlimat
approved these changes
Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The
SameAuthParamsLookupAutoClusterFailoverTest.testAutoClusterFailoverintegration test is flaky. The root cause is a bug inSameAuthParamsLookupAutoClusterFailover.findFailoverTo(): when it probes intermediate services and finds them unavailable, it does not update their state. They remain staleHealthy, which causes a spurious recovery bounce on the next check cycle.For example, when failing over from index 0 to index 2 (skipping index 1 which is unavailable), index 1's state remains
Healthy. On the next check,firstHealthyPulsarService()sees the staleHealthystate and immediately "recovers" to index 1 — which is actually a broken service. This causes unnecessary bouncing (0→2→1→2 instead of 0→2). Combined with 3-second probe timeouts on dead services, the extra bounce adds ~30 seconds, pushing the total failover time past the test's 60-second awaitility timeout.Modifications
findFailoverTo(), when a probe fails for a service, mark it asFailedimmediately. This prevents staleHealthystates from causing spurious recovery bounces after failover.pulsar-clientto verify the fix and prevent regression.Note on
failoverThreshold: The fix marks skipped services directly asFailed, bypassing the gradualHealthy → PreFail → Failedtransition thatcheckPulsarServicesuses withfailoverThreshold. This is intentional:findFailoverTo()is only called after the current service has already gone through the full threshold-based detection and reachedFailed. The intermediate services being skipped were never monitored bycheckPulsarServices(which only checksi <= currentPulsarServiceIndex), so there is no threshold to respect — we just probed them and they are down. TherecoverThresholdis still fully respected when these services come back up, via the normalFailed → PreRecover → Healthypath incheckPulsarServices.Verifying this change
This change added tests and can be verified as follows:
testFindFailoverToMarksSkippedServicesAsFailed— directly verifies thatfindFailoverTomarks skipped unavailable services asFailed(the core bug). Verified this test fails without the fix.testNoSpuriousRecoveryBounceAfterFailover— verifies no spurious recovery bounce occurs on the next check cycle after failover.testRecoveryAfterFindFailoverToMarksServiceFailed— verifies that recovery still works correctly after a service was markedFailedbyfindFailoverTo, once it becomes available again.Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc-not-neededMatching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE after creating forked PR -->
🤖 Generated with Claude Code