Improve error handling on AWS DB failover - #39841
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughHealth-check and error handling for MySQL were extended to detect and react to writer demotion/read-only state. HealthCheck now queries Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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)
server/platform/mysql/retry.go (1)
42-53:⚠️ Potential issue | 🟡 MinorRead-only error check is skipped when rollback also fails.
If
fn(tx)returns a read-only error and the subsequenttx.Rollback()also fails (plausible during a failover — connections may be broken), the code at line 46 wraps the original error asbackoff.Permanentand returns without ever reaching theIsReadOnlyErrorcheck. This means the panic-to-restart path is bypassed.The health check (
SELECT @@read_only) provides a safety net, so this isn't critical, but you could move the read-only check before the rollback-error branch to ensure consistent behavior:Proposed fix
if err := fn(tx); err != nil { rbErr := tx.Rollback() + + // Read-only errors indicate a DB failover occurred (primary demoted to reader). + // Panic to force Fleet to restart and reconnect to the new primary. + if IsReadOnlyError(err) { + panic(fmt.Sprintf("database is read-only, possible failover detected: %v", err)) + } + if rbErr != nil && rbErr != sql.ErrTxDone { // Consider rollback errors to be non-retryable return backoff.Permanent(ctxerr.Wrapf(ctx, err, "got err '%s' rolling back after err", rbErr.Error())) } - // Read-only errors indicate a DB failover occurred (primary demoted to reader). - // Panic to force Fleet to restart and reconnect to the new primary. - if IsReadOnlyError(err) { - panic(fmt.Sprintf("database is read-only, possible failover detected: %v", err)) - } - if retryableError(err) {
🧹 Nitpick comments (1)
server/platform/mysql/errors_test.go (1)
14-44: Good test coverage for the core error codes.Consider adding a case for a non-MySQL error (e.g.,
errors.New("connection reset")) to explicitly verify that non-*mysql.MySQLErrortypes returnfalse. Currently only a*mysql.MySQLErrorwith a non-matching number is tested.Suggested additional test case
{ + name: "non-MySQL error", + err: errors.New("connection reset by peer"), + want: false, + }, + { name: "unrelated MySQL error",(Would require adding
"errors"to imports.)
There was a problem hiding this comment.
Pull request overview
This PR aims to improve Fleet’s behavior during AWS Aurora MySQL failovers by detecting when the “primary” connection is actually read-only (demoted writer) and surfacing that condition more aggressively so orchestration can restart Fleet and re-establish correct DB connections.
Changes:
- Add MySQL read-only error detection (
IsReadOnlyError) based on specific MySQL error codes. - Update transaction retry helper to panic on detected read-only DB errors.
- Update MySQL datastore health check to fail when
@@read_only=1on the primary connection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/platform/mysql/retry.go | Panics on read-only MySQL errors during transaction execution/commit. |
| server/platform/mysql/errors.go | Adds IsReadOnlyError helper and read-only-related MySQL error constants. |
| server/platform/mysql/errors_test.go | Unit tests for IsReadOnlyError. |
| server/datastore/mysql/mysql.go | Changes health check to query SELECT @@read_only and fail if primary is read-only. |
| changes/39228-db-failover | Adds release note entry describing the failover handling behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39841 +/- ##
==========================================
- Coverage 66.26% 64.60% -1.66%
==========================================
Files 2439 2438 -1
Lines 195405 195463 +58
Branches 8539 8539
==========================================
- Hits 129487 126282 -3205
- Misses 54189 57578 +3389
+ Partials 11729 11603 -126
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/platform/mysql/retry.go`:
- Around line 40-52: The race arises because triggerFatalError calls
fatalErrorOnce.Do(...) without holding fatalErrorMu, while SetFatalErrorHandler
may reset fatalErrorOnce under the write lock; fix by acquiring the read lock
(fatalErrorMu.RLock()) before reading fatalErrorHandler and keep that read lock
held across the fatalErrorOnce.Do(...) call so the Once state cannot be
concurrently reset; specifically, in triggerFatalError, RLock, read
fatalErrorHandler into a local, if handler==nil then RUnlock and panic as
before, otherwise call fatalErrorOnce.Do(func(){ handler(err) }) while still
holding the RLock, then RUnlock afterward; this uses the existing symbols
fatalErrorMu, fatalErrorHandler, fatalErrorOnce, triggerFatalError and avoids
deadlocks because the handler should not call SetFatalErrorHandler.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Related issue: Resolves #39228
Manually tested by triggering a failover on loadtest.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit