Repository navigation
Keep MultiDbClient health checks alive when failover has no candidate - #4756
Conversation
…lthy 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.
…vers 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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 934b285903
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| switchToHealthyDatabase(SwitchReason.HEALTH_CHECK, databaseWithHealthChange); | ||
| } catch (JedisFailoverException e) { |
There was a problem hiding this comment.
After this catch keeps health monitoring alive, a previously active endpoint can become healthy and serve commands again without passing through switchToHealthyDatabase, the only path that resets failoverAttemptCount. If the first outage already raised JedisPermanentlyNotAvailableException, a later outage with no immediately healthy candidate therefore starts with the counter above the configured maximum and becomes permanent on its first check instead of honoring a fresh retry window. Reset the failover state when the active database transitions back to healthy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping the original behavior on purpose — the event-based reset added in 484e601 was reverted in 406bb73: 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. The failover attempt budget resets on the first subsequent successful failover. Accepted consequence: a repeat total outage with no successful failover in between reports JedisPermanentlyNotAvailableException immediately — commands fail either way, and any successful failover or restart resets the counter.
utafrali
left a comment
utafrali
left a comment
There was a problem hiding this comment.
Correct, targeted fix for a real production bug. The dual-layer approach — catching JedisFailoverException in onHealthStatusChange for the expected no-candidate case, plus guardedHealthCheck as a general safety net — is sound and the new tests cover both the auto-recovery regression and the intentional no-auto-switch case for a lower-weight DB. Three minor items: a Javadoc gap between the two complementary guards, a pre-existing edge case where failoverAttemptCount is not reset on health-check-driven recovery (subsequent outages skip TEMPORARY escalation, not tested), and a fragile Thread.sleep in the negative-assertion test.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 484e601986
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 879459f05e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
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.
f93401f to
15fb75c
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 15fb75c. Configure here.
| // check, freezing this endpoint's status and preventing any future recovery. | ||
| log.warn("Active database {} became unhealthy, but no failover target is available", | ||
| endpoint, e); | ||
| } |
There was a problem hiding this comment.
Failover attempt count survives recovery
Medium Severity
Swallowing JedisFailoverException lets health checks continue, but failoverAttemptCount is still only cleared on a successful switch. When the active database later returns to HEALTHY without switching, a later outage makes handleNoHealthyDatabase throw JedisPermanentlyNotAvailableException immediately and skip the configured temporary retry window.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 15fb75c. Configure here.
…failover has no candidate (#4759) Keep MultiDbClient health checks alive when failover has no candidate (#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)
Fixes #4755
What
Keeps
MultiDbClienthealth checks alive when a failover attempt finds no healthy database, so the client recovers automatically after a total outage instead of staying permanently unavailable.Why
When the active database turned unhealthy while no other database was available,
onHealthStatusChangepropagatedJedisTemporarilyNotAvailableExceptionintoHealthCheckImpl'sscheduleAtFixedRatetask. The uncaught exception silently cancelled the periodic check, freezing the endpoint's status atUNHEALTHY. From then on every command failed withJedisPermanentlyNotAvailableException— even after the databases came back — until the application was restarted. Hit in production (support case).Changes
MultiDbConnectionProvider.onHealthStatusChange: catchJedisFailoverExceptionfromswitchToHealthyDatabase— a health-event listener has no caller that can act on it; log and return.HealthCheckImpl: run the scheduled task throughguardedHealthCheck(), which logs anyRuntimeExceptionat error level instead of letting it cancel periodic monitoring.MultiDbConnectionProviderTest:userCommand_afterPermanent_activeDatabaseHealthyAgain_autoRecovers_healthCheckDriven— fails without the fix (dead health check loop), passes with it;userCommand_connectionExceptions_thenMultipleTemporary_thenPermanent_inOrderasserting the permanent state is not sticky once the active database is re-enabled;userCommand_afterPermanent_onlySecondaryHealthyAgain_requiresManualSwitch_healthCheckDrivendocumenting the intended scope: if only a lower-weight database recovers, failover/failback do not switch to it and a manualsetActiveDatabase()is required.No release-notes entry: plain bug fix restoring expected behavior, no user action required.
Note
Medium Risk
Touches MultiDb failover and health-check scheduling—critical for availability—but changes are defensive exception handling with regression tests, restoring intended recovery behavior.
Overview
Fixes a production bug where MultiDbClient could stay permanently unavailable after a total outage even after Redis came back, because uncaught exceptions in the health-check scheduler stopped future checks and froze endpoint status at UNHEALTHY.
HealthCheckImpl now runs periodic checks through
guardedHealthCheck(), which logsRuntimeExceptioninstead of lettingscheduleAtFixedRatecancel the task when status-change listeners throw.MultiDbConnectionProvider.onHealthStatusChange catches
JedisFailoverExceptionwhen the active DB goes unhealthy butswitchToHealthyDatabasehas no healthy target—logs a warning instead of propagating into the scheduler thread.Tests add health-check-driven regression coverage: auto-recovery when the active DB becomes healthy again after a permanent-not-available phase, an assertion that re-enabling the active DB clears the “sticky” permanent state on the command path, and documentation that recovery when only a lower-weight secondary is healthy still requires manual
setActiveDatabase().Reviewed by Cursor Bugbot for commit 15fb75c. Bugbot is set up for automated code reviews on this repo. Configure here.