feat: optional fail recovery when post-failover hooks fail (#110) - #115
Conversation
Add FailRecoveryOnFailedPostFailoverProcesses (default false). When enabled, non-zero exits from PostFailoverProcesses, PostMasterFailoverProcesses, PostIntermediateMasterFailoverProcesses, and PostGracefulTakeoverProcesses mark the recovery unsuccessful, persist that state, return an error, and count metrics as failed. Topology changes are not rolled back. Async hooks (&) remain best-effort. Legacy behavior unchanged when the flag is false. Includes unit tests for executeProcesses failOnError and the policy wrapper, sample config, and docs.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a disabled-by-default configuration option that controls whether failed synchronous post-failover hooks mark recovery unsuccessful. Centralized handling persists the updated state, propagates errors, updates metrics, and is integrated across failover and graceful takeover paths. ChangesPost-failover hook failure handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go/logic/topology_recovery_postgresql.go (1)
96-113: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the duplicate
PostFailoverProcessescall fromcheckAndRecoverDeadPrimary.
PostgreSQL/DeadPrimaryrecovery is dispatched throughexecuteCheckAndRecoverFunction, which already runsconfig.Config.PostFailoverProcessesonce for any successful recovery withSuccessorKeyset.checkAndRecoverDeadPrimaryshould only run the type-specificPostMasterFailoverProcesseshere, matching the MySQL recovery paths. Otherwise enabled hooks run twice, including duplicatepersistResolvedRecoveryFn/state flips on hook failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go/logic/topology_recovery_postgresql.go` around lines 96 - 113, Remove the PostFailoverProcesses invocation from checkAndRecoverDeadPrimary’s successful promotedInstance branch. Keep assigning topologyRecovery.SuccessorKey and running the type-specific PostMasterFailoverProcesses hook there, while leaving the unsuccessful recovery handling unchanged so executeCheckAndRecoverFunction can run PostFailoverProcesses once.
🧹 Nitpick comments (1)
go/logic/topology_recovery.go (1)
2002-2036: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMetrics-after-hooks reordering is correct; combined-hook-failure error swallowing needs a fix.
The reordering so
topologyRecovery.IsSuccessfulreflects hook failures before metrics emission is correct. Two secondary notes:
- At line 2020-2024,
err = hookErronly firesif err == nil, so if the recovery-type-specific hook (e.g.PostMasterFailoverProcesses) already failed and seterr, a second, independentPostFailoverProcessesfailure is silently dropped from the returned error (see consolidated comment).durationnow includes the time spent running post-failover hooks (previously it likely only measured the topology recovery itself). If external hooks are slow,RecoveryDurationSecondswill no longer represent pure recovery time — worth a one-line doc/metric-description update if this is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go/logic/topology_recovery.go` around lines 2002 - 2036, Update the PostFailoverProcesses error handling in the topology recovery flow so a hookErr is not silently discarded when err already contains a recovery-specific hook failure; combine or otherwise preserve both errors in the returned result. Keep the existing assignment behavior when no earlier error exists, and update the RecoveryDurationSeconds metric description to state that post-failover hook time is included.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@go/logic/topology_recovery_postgresql.go`:
- Around line 96-113: Remove the PostFailoverProcesses invocation from
checkAndRecoverDeadPrimary’s successful promotedInstance branch. Keep assigning
topologyRecovery.SuccessorKey and running the type-specific
PostMasterFailoverProcesses hook there, while leaving the unsuccessful recovery
handling unchanged so executeCheckAndRecoverFunction can run
PostFailoverProcesses once.
---
Nitpick comments:
In `@go/logic/topology_recovery.go`:
- Around line 2002-2036: Update the PostFailoverProcesses error handling in the
topology recovery flow so a hookErr is not silently discarded when err already
contains a recovery-specific hook failure; combine or otherwise preserve both
errors in the returned result. Keep the existing assignment behavior when no
earlier error exists, and update the RecoveryDurationSeconds metric description
to state that post-failover hook time is included.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6dc0a1e9-cd1b-4563-a819-641209408521
📒 Files selected for processing (8)
conf/orchestrator-sample-sqlite.conf.jsonconf/orchestrator-sample.conf.jsondocs/configuration-recovery.mdgo/config/config.gogo/config/config_post_failover_test.gogo/logic/post_failover_hooks_test.gogo/logic/topology_recovery.gogo/logic/topology_recovery_postgresql.go
1 participant
Summary
Implements #110.
FailRecoveryOnFailedPostFailoverProcesses(defaultfalse)PostFailoverProcessesPostMasterFailoverProcessesPostIntermediateMasterFailoverProcessesPostGracefulTakeoverProcessesIsSuccessful=false, returns error to caller, emits recovery metrics after hookscommand &) still ignore exit codesTests
go test ./go/logic/— policy on/off, persist error, executeProcesses failOnErrorgo test ./go/config/— default falseDocs / samples
docs/configuration-recovery.mdTest plan
PostFailoverProcesses: ["false"], trigger recovery → UI shows failedSummary by CodeRabbit
FailRecoveryOnFailedPostFailoverProcessesoption (default false) to mark recovery as unsuccessful when synchronous post-failover/post-success hooks fail.... &) still run independently and their failures are ignored.