Repository navigation
Backport #4756 to 8.0.x: Keep MultiDbClient health checks alive when failover has no candidate - #4759
Merged
Merged
Conversation
…redis#4756) * Fix MultiDbClient not recovering when the active database becomes healthy again When the active database turned unhealthy while no failover candidate was available, onHealthStatusChange propagated JedisTemporarilyNotAvailableException into HealthCheckImpl's scheduleAtFixedRate task. The uncaught exception silently cancelled the periodic check, freezing the endpoint's status at UNHEALTHY, so the client stayed permanently unavailable even after the database came back. Catch the failover exception in onHealthStatusChange (a health-event listener has no caller that can act on it) and guard the scheduled task so no listener exception can stop monitoring. Covered by health-check-driven and setDisabled-based recovery tests: after JedisPermanentlyNotAvailableException, the client recovers on the next command once the active database is healthy again, with no manual interaction. * Show manual switch is required when only a lower-weight database recovers After JedisPermanentlyNotAvailableException, if only a lower-weight, non-active database becomes healthy again, nothing triggers a failover: the active database's circuit breaker never trips on the dead-endpoint errors and failback only promotes higher-weight databases. Commands surface the raw JedisConnectionException (past the grace period too) until setActiveDatabase() is called manually. Documented with a health-check-driven test. * Reset failover attempt budget when the client recovers In-place recovery of the active database (now possible since health checks survive a total outage) bypassed switchToHealthyDatabase, the only place that reset failoverAttemptCount. After a permanent episode the counter stayed above the maximum, so a later outage skipped the temporary phase and threw JedisPermanentlyNotAvailableException on its first failure. Reset failover state when the active database transitions back to healthy, and on any successful database switch - the latter also covers the pre-existing gap where periodic failback did not reset it. * Revert "Reset failover attempt budget when the client recovers" Keep the original behavior: the failover attempt budget is reset on the first subsequent successful failover, not on health status transitions. A HEALTHY status event does not by itself make the database operable (grace period and circuit breaker state still apply), so resetting there can refill the budget while commands are still failing. This reverts commit 484e601988fb37eb4e8b0b198056a950ec5a8dd7. * Wait on the grace-period deadline instead of a fixed sleep The no-recovery assertion claimed to hold past the active database's grace period but only slept a fixed 300ms; on a stalled runner the grace period could still be running at assertion time, silently weakening the claim. Await isInGracePeriod() expiring instead - tied to the real deadline, deterministic under CI load. (cherry picked from commit 6e5a393)
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
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.
Clean cherry-pick of
6e5a3934c(#4756) onto8.x.MultiDbClienthealth checks could stop for good after a total outage. When the active database went unhealthy with no failover candidate,onHealthStatusChangepropagatedJedisTemporarilyNotAvailableExceptionintoHealthCheckImpl'sscheduleAtFixedRatetask. The uncaught exception cancelled the periodic check and froze the endpoint atUNHEALTHY, so every command failed withJedisPermanentlyNotAvailableExceptionuntil the application restarted, even after the databases came back.Fixes #4755 for the 8.0.x line.
MultiDbConnectionProvider.onHealthStatusChange: catchesJedisFailoverExceptionfromswitchToHealthyDatabase, logs and returns.HealthCheckImpl: the scheduled task runs throughguardedHealthCheck(), which logs anyRuntimeExceptioninstead of cancelling monitoring.Backport notes
MultiDbConnectionProvider.javaauto-merges with a one-line offset:8.xlacks the master-only.maintenanceNotificationsConfig(...)line nearby. The main-source diff matches master line for line.8.x:HealthCheckImpl.start()schedulesthis::healthCheckunguarded.Testing
Run on JDK 8 against the local Docker Redis environment:
MultiDbConnectionProviderTest12/12 (repeated 4 times), plus the othermcfunit and integration tests: 275 tests, 0 failures.userCommand_afterPermanent_activeDatabaseHealthyAgain_autoRecovers_healthCheckDrivenfails on the unfixed8.xsources (times out waiting for recovery) and passes with the fix...._onlySecondaryHealthyAgain_requiresManualSwitch_healthCheckDrivenand thesetDisabled(false)extension of..._thenPermanent_inOrderalso pass without the fix; they document behavior rather than guard the regression.Note
Medium Risk
Changes failover and health-check error handling in MultiDbConnectionProvider; behavior on total outage improves but swallowed exceptions could hide operational issues unless logs are monitored.
Overview
Fixes a case where MultiDbClient could stay broken after a total outage even after Redis came back. When the active database turned unhealthy and there was no other healthy endpoint,
onHealthStatusChangecalledswitchToHealthyDatabase, which threwJedisFailoverException. That exception ran insideHealthCheckImpl’sscheduleAtFixedRatetask, which stops all future runs on an uncaught error—so health status froze at UNHEALTHY and the client never saw recovery.MultiDbConnectionProvider.onHealthStatusChangenow catchesJedisFailoverException, logs a warning, and does not rethrow.HealthCheckImplschedulesguardedHealthCheck, which wrapshealthCheck()in a try/catch so listener or check failures are logged but periodic monitoring continues.Tests extend the permanent-unavailability scenario and add health-check-driven integration coverage (auto-recovery when the active DB is healthy again; manual switch when only a lower-weight DB recovers).
Reviewed by Cursor Bugbot for commit c646281. Bugbot is set up for automated code reviews on this repo. Configure here.