fix: prevent false DeadMaster and drain MariaDB relay logs (#106) - #108
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds IO-aware master failure classification, MariaDB relay-log draining before promotion, candidate validation before sibling repointing, and MariaDB functional coverage across MySQL-compatible test infrastructure and CI. ChangesMariaDB recovery behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MariaDBReplica
participant Orchestrator
participant CandidateReplica
participant RecoveryTopology
MariaDBReplica->>Orchestrator: report IO running and SQL stopped
Orchestrator->>MariaDBReplica: classify master reachability
Orchestrator->>CandidateReplica: drain relay logs
CandidateReplica-->>Orchestrator: return SQL-aligned candidate
Orchestrator->>RecoveryTopology: validate candidate
RecoveryTopology-->>Orchestrator: repoint siblings and promote
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses false DeadMaster detections by treating replicas with running IO threads as UnreachableMaster and introduces a safe relay log draining mechanism (DrainRelayLogs) for MariaDB to prevent data loss during failover. The review feedback highlights several critical issues, including potential nil pointer dereferences when reading topology instances, inconsistent error return values in DrainRelayLogs, a logic bypass in candidate selection when no candidate is chosen, and unused helper code in analysis_classification.go.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| instance, err = ReadTopologyInstance(instanceKey) | ||
| log.Infof("Drained relay logs on %+v, Self:%+v, Exec:%+v", *instanceKey, instance.SelfBinlogCoordinates, instance.ExecBinlogCoordinates) | ||
| return instance, err |
There was a problem hiding this comment.
If ReadTopologyInstance fails and returns a nil instance along with an error, accessing instance.SelfBinlogCoordinates and instance.ExecBinlogCoordinates in the log statement will cause a panic.
The error should be handled or checked before accessing the fields of instance.
instance, err = ReadTopologyInstance(instanceKey)
if err != nil {
return instance, log.Errore(err)
}
log.Infof("Drained relay logs on %+v, Self:%+v, Exec:%+v", *instanceKey, instance.SelfBinlogCoordinates, instance.ExecBinlogCoordinates)
return instance, nil| if _, err := ExecInstance(instanceKey, instance.QSP.start_slave_sql_thread()); err != nil { | ||
| return nil, log.Errorf("%+v: DrainRelayLogs: start sql thread failed: %+v", *instanceKey, err) | ||
| } |
There was a problem hiding this comment.
Returning nil on error is inconsistent with other error paths in this function (which return instance, err). Returning instance is safer and maintains consistency.
| if _, err := ExecInstance(instanceKey, instance.QSP.start_slave_sql_thread()); err != nil { | |
| return nil, log.Errorf("%+v: DrainRelayLogs: start sql thread failed: %+v", *instanceKey, err) | |
| } | |
| if _, err := ExecInstance(instanceKey, instance.QSP.start_slave_sql_thread()); err != nil { | |
| return instance, log.Errorf("%+v: DrainRelayLogs: start sql thread failed: %+v", *instanceKey, err) | |
| } |
| // classifyUnreachableMasterWhenIORunning returns UnreachableMaster when the | ||
| // master check failed but at least one replica still has IO running. | ||
| // Returns NoProblem when this rule does not apply (caller continues other branches). | ||
| func classifyUnreachableMasterWhenIORunning(isMaster bool, lastCheckValid bool, countValidIORunningReplicas uint) AnalysisCode { |
There was a problem hiding this comment.
The helper function classifyUnreachableMasterWhenIORunning is defined and tested but is never used in the actual classification logic in analysis_dao.go (which implements the same logic inline).
To improve maintainability and avoid dead code, either use this helper function in analysis_dao.go or remove it.
| if err != nil || candidateReplica == nil { | ||
| return candidateReplica, aheadReplicas, equalReplicas, laterReplicas, cannotReplicateReplicas, err | ||
| } |
There was a problem hiding this comment.
If chooseCandidateReplica returns candidateReplica == nil (with err == nil) on the first iteration, the function returns immediately. This bypasses the descriptive error at the end of the function for forRematchPurposes.
The check should handle candidateReplica == nil when forRematchPurposes is true by returning the appropriate error.
if err != nil {
return nil, aheadReplicas, equalReplicas, laterReplicas, cannotReplicateReplicas, err
}
if candidateReplica == nil {
if forRematchPurposes {
return nil, aheadReplicas, equalReplicas, laterReplicas, cannotReplicateReplicas, fmt.Errorf("GetCandidateReplica: no drainable candidate replica found for %+v", *masterKey)
}
return nil, aheadReplicas, equalReplicas, laterReplicas, cannotReplicateReplicas, nil
}| refreshed, readErr := inst.ReadTopologyInstance(&candidate.Key) | ||
| if readErr != nil { | ||
| return readErr | ||
| } | ||
| *candidate = *refreshed |
There was a problem hiding this comment.
If ReadTopologyInstance succeeds but returns a nil pointer for refreshed (along with a nil error), dereferencing it (*refreshed) will cause a panic.
A nil check on refreshed should be performed before dereferencing.
| refreshed, readErr := inst.ReadTopologyInstance(&candidate.Key) | |
| if readErr != nil { | |
| return readErr | |
| } | |
| *candidate = *refreshed | |
| refreshed, readErr := inst.ReadTopologyInstance(&candidate.Key) | |
| if readErr != nil { | |
| return readErr | |
| } | |
| if refreshed == nil { | |
| return fmt.Errorf("DelayMasterPromotionIfSQLThreadNotUpToDate: refreshed instance is nil for %+v", candidate.Key) | |
| } | |
| *candidate = *refreshed |
CI failures on #108 were test harness issues, not the product fix: - Restore flat topology (mysql1 master) before the suite; earlier failover tests leave mysql2 promoted and break SHOW REPLICA STATUS assumptions - Parse replica status with robust field splitters (MySQL 8/9 + MariaDB) - Restart SQL_THREAD (not full START SLAVE) after the false-DeadMaster check - Avoid deprecated VALUES() in ON DUPLICATE KEY UPDATE - Honor COMPOSE env in lib.sh version detection for MariaDB overlays
b86665f to
a4f7386
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/functional/lib.sh (2)
208-216: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCompose-command plumbing for the MariaDB override is inconsistent. Some helpers/steps honor
${COMPOSE}(with both compose files) while others hardcode or omit the base file; the MariaDB run currently only works because the derived compose project name happens to match.
tests/functional/lib.sh#L208-L216: use the${COMPOSE:-docker compose -f tests/functional/docker-compose.yml}fallback inmysql_source_hostinstead of the hardcoded base-file command..github/workflows/functional.yml#L268-L272: addCOMPOSE: docker compose -f tests/functional/docker-compose.yml -f tests/functional/docker-compose.mariadb.ymlto the smoke-test and relay-drain steps, matching the "Setup replication" step.🤖 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 `@tests/functional/lib.sh` around lines 208 - 216, Update mysql_source_host in tests/functional/lib.sh:208-216 to use the ${COMPOSE:-docker compose -f tests/functional/docker-compose.yml} fallback for both compose commands. In .github/workflows/functional.yml:268-272, add COMPOSE with both the base and MariaDB compose files to the smoke-test and relay-drain steps, matching Setup replication.
159-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
mysql_change_source_channel_sqlwas not given a MariaDB branch.Sibling helpers all gained
mysql_is_mariadbhandling; this one still emitsCHANGE REPLICATION SOURCE ... FOR CHANNELfor MariaDB. MariaDB uses named connections (CHANGE MASTER 'name' TO ...), so any MariaDB run of the named-channels suite would fail. Not currently exercised by the MariaDB CI job, so this can be deferred — but worth a comment noting MySQL-only support.🤖 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 `@tests/functional/lib.sh` around lines 159 - 166, Update mysql_change_source_channel_sql to explicitly handle MariaDB, using MariaDB named-connection syntax with the channel name instead of MySQL’s FOR CHANNEL form; alternatively, if support is intentionally deferred, add a comment clearly documenting that this helper is MySQL-only.
🧹 Nitpick comments (5)
go/inst/analysis_dao.go (1)
565-570: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the tested classification helper here.
The DAO duplicates
classifyUnreachableMasterWhenIORunning, so its new unit tests do not protect the production branch. Route this condition through the helper to prevent drift.Proposed fix
-} else if a.IsMaster && !a.LastCheckValid && a.CountValidIORunningReplicas > 0 { +} else if analysis := classifyUnreachableMasterWhenIORunning( + a.IsMaster, a.LastCheckValid, a.CountValidIORunningReplicas, +); analysis != NoProblem { // IO thread running on a valid replica proves the master is accepting replica connections. // Do not treat SQL-thread-stopped replicas as evidence of a dead master (issue `#106`). - a.Analysis = UnreachableMaster + a.Analysis = analysis🤖 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/inst/analysis_dao.go` around lines 565 - 570, Replace the duplicated classification logic in the Analysis branch for a.IsMaster, !a.LastCheckValid, and a.CountValidIORunningReplicas > 0 with the tested classifyUnreachableMasterWhenIORunning helper. Preserve the existing UnreachableMaster result and description behavior while ensuring production uses the helper covered by the unit tests..github/workflows/functional.yml (1)
183-194: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider a least-privilege
permissions:block andpersist-credentials: false.zizmor flags the new job for default (broad) token permissions and credential persistence via checkout.
permissions: contents: readpluspersist-credentials: falseonactions/checkoutwould close both. The rest of the file has the same gap, so this can be a follow-up sweep.🤖 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 @.github/workflows/functional.yml around lines 183 - 194, Update the functional-mariadb job with a least-privilege permissions block granting only contents: read, and set persist-credentials: false on its actions/checkout@v4 step; leave the remaining workflow jobs unchanged.Source: Linters/SAST tools
tests/functional/lib.sh (1)
208-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mysql_source_hoststill hardcodes the base compose file.The version helpers now honor
${COMPOSE}, but this function does not. Use the sameCOMPOSE_CMDfallback so overrides apply uniformly.♻️ Suggested change
mysql_source_host() { local CONTAINER="$1" + local COMPOSE_CMD="${COMPOSE:-docker compose -f tests/functional/docker-compose.yml}" if mysql_is_57 || mysql_is_mariadb; then - docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ + $COMPOSE_CMD exec -T "$CONTAINER" \ mysql -uroot -ptestpass -Nse "SHOW SLAVE STATUS" 2>/dev/null | awk -F'\t' '{print $2; exit}' else - docker compose -f tests/functional/docker-compose.yml exec -T "$CONTAINER" \ + $COMPOSE_CMD exec -T "$CONTAINER" \ mysql -uroot -ptestpass -Nse "SHOW REPLICA STATUS" 2>/dev/null | awk -F'\t' '{print $2; exit}' fi }🤖 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 `@tests/functional/lib.sh` around lines 208 - 216, Update mysql_source_host to invoke Docker Compose through the shared COMPOSE_CMD fallback, matching the version helper behavior, in both the MySQL 5.7/MariaDB and newer replica-status branches. Preserve the existing compose file, container, credentials, and query logic while allowing ${COMPOSE} overrides to apply.go/inst/instance_topology_dao.go (1)
286-324: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the MaxScale-tolerant
STOP SLAVEinto a helper.The same stop-and-swallow-1199 block now appears twice here and once in
StopReplicationNicely(Lines 364-370).🤖 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/inst/instance_topology_dao.go` around lines 286 - 324, Extract the duplicated MaxScale-tolerant STOP SLAVE logic from the current flow and StopReplicationNicely into a shared helper. The helper should execute the instance’s stop_slave command, clear the error only for MaxScale error 1199 (“Slave connection is not running”), and return all other errors unchanged; replace each existing inline block with calls to this helper.tests/functional/test-mariadb-relay-drain.sh (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
STOP_REPLICAis unused.Drop it, or use it in place of the raw
STOP REPLICA; STOP SLAVE;at Line 73.🤖 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 `@tests/functional/test-mariadb-relay-drain.sh` at line 21, Remove the unused STOP_REPLICA assignment in the MariaDB relay drain test, or reuse STOP_REPLICA at the existing raw stop-command location so the helper’s SQL is actually executed.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@docs/superpowers/plans/2026-07-16-mariadb-deadmaster-relay-drain.md`:
- Line 15: Restore the heading hierarchy in the document around the “File map”
section by adding the required intermediate heading level between the document’s
H1 and the existing “### File map” heading, preserving the section’s content and
wording.
In `@go/inst/instance_topology_dao.go`:
- Around line 304-332: Update the DrainRelayLogs flow around the SQLDelay check
and final SQLThreadUpToDate assertion to handle delayed replicas explicitly.
When instance.SQLDelay is nonzero, return a distinct result that clearly
indicates the replica is delayed and should not be treated as a generic
undrainable or promotion failure; preserve the existing wait, stop, and
post-wait validation behavior for non-delayed replicas.
In `@go/inst/instance_topology.go`:
- Around line 2309-2330: The retry loop in GetCandidateReplica can spend
drainTimeout serially on every candidate, causing excessive failover latency.
Add an overall drain-time budget or bounded attempt count for the
forRematchPurposes path, and stop trying further candidates once that limit is
reached while preserving the existing candidate classification and error-return
behavior.
- Around line 2314-2333: Copy the replicas slice when initializing remaining so
candidate removal cannot mutate replicas through shared backing storage. Update
the remaining initialization in the chooseCandidateReplica loop, while
preserving the existing RemoveInstance behavior and use of replicas for the
warning context.
In `@tests/functional/setup-replication.sh`:
- Around line 42-55: Update both STATUS assignments in the MariaDB and
non-MariaDB branches of the replication retry loop to tolerate a non-zero
command-substitution pipeline by appending an unconditional success fallback,
allowing the loop to continue retrying while the replica starts.
In `@tests/functional/test-mariadb-relay-drain.sh`:
- Line 5: Guard the initial directory change in the test script by exiting
immediately if `cd "$(dirname "$0")/../.."` fails, before sourcing
`tests/functional/lib.sh`.
- Around line 73-79: Update the replica shutdown cleanup around RESET_REPLICA so
STOP REPLICA and STOP SLAVE execute as separate best-effort mysql statements,
allowing the supported command to run even when the other is unavailable; keep
the existing error suppression and subsequent reset/start sequence unchanged.
---
Outside diff comments:
In `@tests/functional/lib.sh`:
- Around line 208-216: Update mysql_source_host in
tests/functional/lib.sh:208-216 to use the ${COMPOSE:-docker compose -f
tests/functional/docker-compose.yml} fallback for both compose commands. In
.github/workflows/functional.yml:268-272, add COMPOSE with both the base and
MariaDB compose files to the smoke-test and relay-drain steps, matching Setup
replication.
- Around line 159-166: Update mysql_change_source_channel_sql to explicitly
handle MariaDB, using MariaDB named-connection syntax with the channel name
instead of MySQL’s FOR CHANNEL form; alternatively, if support is intentionally
deferred, add a comment clearly documenting that this helper is MySQL-only.
---
Nitpick comments:
In @.github/workflows/functional.yml:
- Around line 183-194: Update the functional-mariadb job with a least-privilege
permissions block granting only contents: read, and set persist-credentials:
false on its actions/checkout@v4 step; leave the remaining workflow jobs
unchanged.
In `@go/inst/analysis_dao.go`:
- Around line 565-570: Replace the duplicated classification logic in the
Analysis branch for a.IsMaster, !a.LastCheckValid, and
a.CountValidIORunningReplicas > 0 with the tested
classifyUnreachableMasterWhenIORunning helper. Preserve the existing
UnreachableMaster result and description behavior while ensuring production uses
the helper covered by the unit tests.
In `@go/inst/instance_topology_dao.go`:
- Around line 286-324: Extract the duplicated MaxScale-tolerant STOP SLAVE logic
from the current flow and StopReplicationNicely into a shared helper. The helper
should execute the instance’s stop_slave command, clear the error only for
MaxScale error 1199 (“Slave connection is not running”), and return all other
errors unchanged; replace each existing inline block with calls to this helper.
In `@tests/functional/lib.sh`:
- Around line 208-216: Update mysql_source_host to invoke Docker Compose through
the shared COMPOSE_CMD fallback, matching the version helper behavior, in both
the MySQL 5.7/MariaDB and newer replica-status branches. Preserve the existing
compose file, container, credentials, and query logic while allowing ${COMPOSE}
overrides to apply.
In `@tests/functional/test-mariadb-relay-drain.sh`:
- Line 21: Remove the unused STOP_REPLICA assignment in the MariaDB relay drain
test, or reuse STOP_REPLICA at the existing raw stop-command location so the
helper’s SQL is actually executed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b1c0877e-fbd4-4fa1-81cb-41dde95e7319
📒 Files selected for processing (19)
.github/workflows/functional.ymldocs/failure-detection.mddocs/superpowers/plans/2026-07-16-mariadb-deadmaster-relay-drain.mddocs/superpowers/specs/2026-07-16-mariadb-deadmaster-relay-drain-design.mddocs/user-manual.mdgo/inst/analysis.gogo/inst/analysis_classification.gogo/inst/analysis_classification_test.gogo/inst/analysis_dao.gogo/inst/instance_topology.gogo/inst/instance_topology_dao.gogo/logic/topology_recovery.gotests/functional/docker-compose.mariadb.ymltests/functional/lib.shtests/functional/mysql/mariadb-master.cnftests/functional/mysql/mariadb-replica.cnftests/functional/mysql/mariadb-replica2.cnftests/functional/setup-replication.shtests/functional/test-mariadb-relay-drain.sh
|
|
||
| --- | ||
|
|
||
| ### File map |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore heading hierarchy.
### File map follows the H1 directly, triggering markdownlint MD001.
Proposed fix
-### File map
+## File map📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### File map | |
| ## File map |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 15-15: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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 `@docs/superpowers/plans/2026-07-16-mariadb-deadmaster-relay-drain.md` at line
15, Restore the heading hierarchy in the document around the “File map” section
by adding the required intermediate heading level between the document’s H1 and
the existing “### File map” heading, preserving the section’s content and
wording.
Source: Linters/SAST tools
| remaining := replicas | ||
| for len(remaining) > 0 { | ||
| candidateReplica, aheadReplicas, equalReplicas, laterReplicas, cannotReplicateReplicas, err = chooseCandidateReplica(remaining) | ||
| if err != nil || candidateReplica == nil { | ||
| return candidateReplica, aheadReplicas, equalReplicas, laterReplicas, cannotReplicateReplicas, err | ||
| } | ||
| if !forRematchPurposes { | ||
| break | ||
| } | ||
| // Skip candidates that cannot apply remaining relay logs (e.g. SQL error 1062). | ||
| drained, drainErr := ensureReplicaRelayDrained(candidateReplica, drainTimeout) | ||
| if drainErr != nil { | ||
| _ = log.Errorf("GetCandidateReplica: skipping %+v; cannot drain relay logs: %+v", candidateReplica.Key, drainErr) | ||
| remaining = RemoveInstance(remaining, &candidateReplica.Key) | ||
| candidateReplica = nil | ||
| continue | ||
| } | ||
| candidateReplica = drained | ||
| // Recompute sibling buckets against the drained candidate coordinates. | ||
| remaining = RemoveInstance(remaining, &candidateReplica.Key) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern $'func RemoveInstance($$$) $$$ {
$$$
}' --lang go go/instRepository: ProxySQL/orchestrator
Length of output: 795
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- candidate replication snippet ---\n'
sed -n '2308,2366p' go/inst/instance_topology.go | nl -ba -v2308
printf '\n--- RemoveInstance implementation ---\n'
sed -n '168,184p' go/inst/instance_utils.go | nl -ba -v168
printf '\n--- relevant warnings/logs using replicas[0] ---\n'
sed -n '2350,2362p' go/inst/instance_topology.go | nl -ba -v2350
printf '\n--- Go slice mutation probe ---\n'
cat >/tmp/slice_alias_probe.go <<'EOF'
package main
import "fmt"
type Element struct{ ID int }
func RemoveInstance(in []*Element, id int) []*Element {
for i := len(in) - 1; i >= 0; i-- {
if in[i].ID == id {
in = append(in[:i], in[i+1:]...)
}
}
return in
}
func main() {
replicas := []*Element{{1}, {2}, {3}}
remaining := replicas
remaining = RemoveInstance(remaining, 2)
fmt.Printf("replicas len=%d, first=%d, last=%d\n", len(replicas), replicas[0].ID, replicas[len(replicas)-1].ID)
fmt.Printf("remaining references same backing array? %t\n", &replicas[0] == &remaining[0])
}
EOF
go run /tmp/slice_alias_probe.go
rm /tmp/slice_alias_probe.goRepository: ProxySQL/orchestrator
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate replication snippet ---'
sed -n '2308,2366p' go/inst/instance_topology.go | nl -ba -v2308
printf '%s\n' '--- RemoveInstance implementation ---'
sed -n '168,184p' go/inst/instance_utils.go | nl -ba -v168
printf '%s\n' '--- relevant warnings/logs using replicas[0] ---'
sed -n '2350,2362p' go/inst/instance_topology.go | nl -ba -v2350
printf '%s\n' '--- Go slice mutation probe ---'
cat >/tmp/slice_alias_probe.go <<'EOF'
package main
import "fmt"
type Element struct{ ID int }
func RemoveInstance(in []*Element, id int) []*Element {
for i := len(in) - 1; i >= 0; i-- {
if in[i].ID == id {
in = append(in[:i], in[i+1:]...)
}
}
return in
}
func main() {
replicas := []*Element{{1}, {2}, {3}}
remaining := replicas
remaining = RemoveInstance(remaining, 2)
fmt.Printf("replicas len=%d, first=%d, last=%d\n", len(replicas), replicas[0].ID, replicas[len(replicas)-1].ID)
fmt.Printf("remaining references same backing array? %t\n", &replicas[0] == &remaining[0])
}
EOF
go run /tmp/slice_alias_probe.go
rm /tmp/slice_alias_probe.goRepository: ProxySQL/orchestrator
Length of output: 237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate replication snippet ---'
sed -n '2308,2366p' go/inst/instance_topology.go
echo '--- RemoveInstance implementation ---'
sed -n '168,184p' go/inst/instance_utils.go
echo '--- relevant warnings/logs using replicas[0] ---'
sed -n '2350,2362p' go/inst/instance_topology.go
echo '--- Go slice mutation probe ---'
cat >/tmp/slice_alias_probe.go <<'EOF'
package main
import "fmt"
type Element struct{ ID int }
func RemoveInstance(in []*Element, id int) []*Element {
for i := len(in) - 1; i >= 0; i-- {
if in[i].ID == id {
in = append(in[:i], in[i+1:]...)
}
}
return in
}
func main() {
replicas := []*Element{{1}, {2}, {3}}
remaining := replicas
remaining = RemoveInstance(remaining, 2)
fmt.Printf("replicas len=%d, first=%d, last=%d\n", len(replicas), replicas[0].ID, replicas[len(replicas)-1].ID)
fmt.Printf("remaining references same backing array? %t\n", &replicas[0] == &remaining[0])
}
EOF
go run /tmp/slice_alias_probe.go
rm /tmp/slice_alias_probe.goRepository: ProxySQL/orchestrator
Length of output: 4456
Don’t alias replicas through remaining.
remaining := replicas shares the backing array, and RemoveInstance compacts its slice in-place. If any candidate is skipped before success, replicas[0] used for the warning can point at the wrong/corrupted tail entry. Copy remaining into a separate slice before removing candidates.
🤖 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/inst/instance_topology.go` around lines 2314 - 2333, Copy the replicas
slice when initializing remaining so candidate removal cannot mutate replicas
through shared backing storage. Update the remaining initialization in the
chooseCandidateReplica loop, while preserving the existing RemoveInstance
behavior and use of replicas for the warning context.
| if echo "$MYSQL_FULL_VERSION" | grep -qi mariadb; then | ||
| STATUS=$($COMPOSE exec -T "$REPLICA" mysql -uroot -ptestpass -e \ | ||
| "SHOW SLAVE STATUS\G" 2>/dev/null | awk '/Slave_IO_Running:/{print $2; exit}' | tr -d '[:space:]') | ||
| if [ "$STATUS" = "Yes" ]; then | ||
| echo "$REPLICA: replication OK (IO thread Yes)" | ||
| break | ||
| fi | ||
| else | ||
| STATUS=$($COMPOSE exec -T "$REPLICA" mysql -uroot -ptestpass -Nse \ | ||
| "SELECT SERVICE_STATE FROM performance_schema.replication_connection_status" 2>/dev/null | tr -d '[:space:]') | ||
| if [ "$STATUS" = "ON" ]; then | ||
| echo "$REPLICA: replication OK (IO thread ON)" | ||
| break | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Retry loop can abort under set -e when the replica isn't ready yet.
STATUS=$(...) is a simple assignment, so a non-zero exit from the mysql pipeline (replica still starting, pipefail in effect) terminates the script instead of retrying. Append || true to both substitutions.
🛡️ Suggested fix
- STATUS=$($COMPOSE exec -T "$REPLICA" mysql -uroot -ptestpass -e \
- "SHOW SLAVE STATUS\G" 2>/dev/null | awk '/Slave_IO_Running:/{print $2; exit}' | tr -d '[:space:]')
+ STATUS=$($COMPOSE exec -T "$REPLICA" mysql -uroot -ptestpass -e \
+ "SHOW SLAVE STATUS\G" 2>/dev/null | awk '/Slave_IO_Running:/{print $2; exit}' | tr -d '[:space:]' || true)
@@
- STATUS=$($COMPOSE exec -T "$REPLICA" mysql -uroot -ptestpass -Nse \
- "SELECT SERVICE_STATE FROM performance_schema.replication_connection_status" 2>/dev/null | tr -d '[:space:]')
+ STATUS=$($COMPOSE exec -T "$REPLICA" mysql -uroot -ptestpass -Nse \
+ "SELECT SERVICE_STATE FROM performance_schema.replication_connection_status" 2>/dev/null | tr -d '[:space:]' || true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if echo "$MYSQL_FULL_VERSION" | grep -qi mariadb; then | |
| STATUS=$($COMPOSE exec -T "$REPLICA" mysql -uroot -ptestpass -e \ | |
| "SHOW SLAVE STATUS\G" 2>/dev/null | awk '/Slave_IO_Running:/{print $2; exit}' | tr -d '[:space:]') | |
| if [ "$STATUS" = "Yes" ]; then | |
| echo "$REPLICA: replication OK (IO thread Yes)" | |
| break | |
| fi | |
| else | |
| STATUS=$($COMPOSE exec -T "$REPLICA" mysql -uroot -ptestpass -Nse \ | |
| "SELECT SERVICE_STATE FROM performance_schema.replication_connection_status" 2>/dev/null | tr -d '[:space:]') | |
| if [ "$STATUS" = "ON" ]; then | |
| echo "$REPLICA: replication OK (IO thread ON)" | |
| break | |
| fi | |
| if echo "$MYSQL_FULL_VERSION" | grep -qi mariadb; then | |
| STATUS=$($COMPOSE exec -T "$REPLICA" mysql -uroot -ptestpass -e \ | |
| "SHOW SLAVE STATUS\G" 2>/dev/null | awk '/Slave_IO_Running:/{print $2; exit}' | tr -d '[:space:]' || true) | |
| if [ "$STATUS" = "Yes" ]; then | |
| echo "$REPLICA: replication OK (IO thread Yes)" | |
| break | |
| fi | |
| else | |
| STATUS=$($COMPOSE exec -T "$REPLICA" mysql -uroot -ptestpass -Nse \ | |
| "SELECT SERVICE_STATE FROM performance_schema.replication_connection_status" 2>/dev/null | tr -d '[:space:]' || true) | |
| if [ "$STATUS" = "ON" ]; then | |
| echo "$REPLICA: replication OK (IO thread ON)" | |
| break | |
| fi |
🤖 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 `@tests/functional/setup-replication.sh` around lines 42 - 55, Update both
STATUS assignments in the MariaDB and non-MariaDB branches of the replication
retry loop to tolerate a non-zero command-substitution pipeline by appending an
unconditional success fallback, allowing the loop to continue retrying while the
replica starts.
| # Issue #106: MariaDB / SQL-stopped relay drain and false DeadMaster avoidance. | ||
| # Works on MariaDB and MySQL topologies (SQL_THREAD stop + unapplied relay + failover). | ||
| set -uo pipefail | ||
| cd "$(dirname "$0")/../.." |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the cd.
Without || exit, a failed cd leaves the script running from an arbitrary directory and the subsequent source tests/functional/lib.sh will pick up nothing (or the wrong file).
🛡️ Suggested fix
-cd "$(dirname "$0")/../.."
+cd "$(dirname "$0")/../.." || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cd "$(dirname "$0")/../.." | |
| cd "$(dirname "$0")/../.." || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 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 `@tests/functional/test-mariadb-relay-drain.sh` at line 5, Guard the initial
directory change in the test script by exiting immediately if `cd "$(dirname
"$0")/../.."` fails, before sourcing `tests/functional/lib.sh`.
Source: Linters/SAST tools
Spec for issue #106: IO-running-aware analysis, MariaDB-safe relay drain, skip undrainable candidates, validate before re-point, and functional CI. Signed-off-by: Rene Cannao <rene@proxysql.com>
Treat valid replicas with a running IO thread as UnreachableMaster rather than DeadMaster when the master check fails. Add MariaDB-safe DrainRelayLogs (SQL thread only), skip undrainable promotion candidates, and validate the candidate before re-pointing siblings. Include unit tests, MariaDB CI job, and docs updates. Signed-off-by: Rene Cannao <rene@proxysql.com>
Signed-off-by: Rene Cannao <rene@proxysql.com>
Always re-read/drain candidates instead of trusting stale SQLThreadUpToDate. Lengthen drain wait, require up-to-date after drain, and fix functional test recovery API parsing plus wait-for-relay readiness before failover. Signed-off-by: Rene Cannao <rene@proxysql.com>
CI failures on #108 were test harness issues, not the product fix: - Restore flat topology (mysql1 master) before the suite; earlier failover tests leave mysql2 promoted and break SHOW REPLICA STATUS assumptions - Parse replica status with robust field splitters (MySQL 8/9 + MariaDB) - Restart SQL_THREAD (not full START SLAVE) after the false-DeadMaster check - Avoid deprecated VALUES() in ON DUPLICATE KEY UPDATE - Honor COMPOSE env in lib.sh version detection for MariaDB overlays
- Soft-path: skip rebuild when replicas already IO/SQL=Yes (MariaDB job) - Prefer CHANGE+START over RESET (RESET left SQL=No on MariaDB) - Explicitly START SQL_THREAD when IO=Yes and SQL=No - Run relay-drain before failover suites in MySQL CI matrix so topology is still the clean setup-replication layout
a4f7386 to
ab1e44e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In @.github/workflows/functional.yml:
- Around line 184-195: Add a job-level permissions block to functional-mariadb
granting only contents: read and actions: read, supporting its checkout and
artifact download steps while preventing inherited broader token permissions.
In `@tests/functional/test-mariadb-relay-drain.sh`:
- Around line 172-195: The Test 1 scenario must exercise an unreachable master
instead of leaving mysql1 reachable. After stopping the SQL thread on mysql2,
verify the replica remains IO=Yes/SQL=No, isolate the orchestrator from mysql1
while preserving that replica state, then query replication analysis and assert
mysql1’s entry is UnreachableMaster; restore connectivity afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0695bff-945d-4cd7-ad6e-983f4eb9a20a
📒 Files selected for processing (19)
.github/workflows/functional.ymldocs/failure-detection.mddocs/superpowers/plans/2026-07-16-mariadb-deadmaster-relay-drain.mddocs/superpowers/specs/2026-07-16-mariadb-deadmaster-relay-drain-design.mddocs/user-manual.mdgo/inst/analysis.gogo/inst/analysis_classification.gogo/inst/analysis_classification_test.gogo/inst/analysis_dao.gogo/inst/instance_topology.gogo/inst/instance_topology_dao.gogo/logic/topology_recovery.gotests/functional/docker-compose.mariadb.ymltests/functional/lib.shtests/functional/mysql/mariadb-master.cnftests/functional/mysql/mariadb-replica.cnftests/functional/mysql/mariadb-replica2.cnftests/functional/setup-replication.shtests/functional/test-mariadb-relay-drain.sh
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/functional/mysql/mariadb-replica.cnf
- go/inst/analysis_classification_test.go
- tests/functional/mysql/mariadb-master.cnf
- tests/functional/docker-compose.mariadb.yml
- tests/functional/mysql/mariadb-replica2.cnf
- tests/functional/setup-replication.sh
- go/inst/analysis_classification.go
- go/inst/analysis.go
- docs/user-manual.md
- docs/failure-detection.md
- go/inst/analysis_dao.go
- go/logic/topology_recovery.go
- go/inst/instance_topology_dao.go
- tests/functional/lib.sh
- go/inst/instance_topology.go
| functional-mariadb: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 30 | ||
| needs: build | ||
|
|
||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| mariadb_version: ['10.6', '10.11'] | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files =="
git ls-files '.github/workflows/*' | sed -n '1,120p'
echo
echo "== functional.yml relevant context =="
if [ -f .github/workflows/functional.yml ]; then
wc -l .github/workflows/functional.yml
sed -n '150,240p' .github/workflows/functional.yml | cat -n
else
echo "missing .github/workflows/functional.yml"
fi
echo
echo "== GITHUB_TOKEN permissions in functional.yml =="
python3 - <<'PY'
from pathlib import Path
p=Path('.github/workflows/functional.yml')
if not p.exists():
print('missing')
raise SystemExit
for i,line in enumerate(p.read_text().splitlines(),1):
if 'functional-mariadb:' in line or 'permissions:' in line or 'GITHUB_TOKEN' in line:
print(f"{i}:{line}")
PY
echo
echo "== job steps around functional-mariadb =="
python3 - <<'PY'
from pathlib import Path
lines = Path('.github/workflows/functional.yml').read_text().splitlines()
for start,name in enumerate(lines):
if name.rstrip().startswith('functional-mariadb:'):
for j in range(start, min(len(lines), start+50)):
print(f"{j+1}: {lines[j]}")
break
PYRepository: ProxySQL/orchestrator
Length of output: 4148
🌐 Web query:
GitHub Actions default GITHUB_TOKEN permissions workflow job permissions contents read artifacts
💡 Result:
The GITHUB_TOKEN is an automatically generated, short-lived secret used to authenticate API requests within GitHub Actions workflows [1][2]. Its default permissions are determined by enterprise, organization, or repository-level settings [3][4]. Currently, many new repositories default to a restricted, read-only configuration, often providing read access only to contents and packages [3][1][4]. To configure permissions for specific workflows or jobs, you use the permissions key in your YAML file [1][5]. Key points regarding GITHUB_TOKEN permissions: 1. Granular Control: You can specify permissions at the top-level (workflow-wide) or within individual jobs [5][6]. When defined at the job level, the job-level configuration entirely replaces any workflow-level permissions; they do not merge [5][6]. 2. Default Behavior: If you specify any permission within the permissions block, all other permissions not explicitly listed are automatically set to none [1][5]. 3. Least Privilege: It is a security best practice to explicitly define only the minimum permissions required for each job [7][1][6]. 4. Pull Requests from Forks: For security reasons, workflows triggered by pull requests from public forks are always granted read-only access to the GITHUB_TOKEN, regardless of your repository's configured default settings [1]. Example of setting read-only contents and no other access: permissions: contents: read If you need specific artifacts-related permissions, ensure you include them explicitly in your job definition, such as: jobs: job_name: runs-on: ubuntu-latest permissions: contents: read actions: read # If needed for artifact access steps: -... Always verify your required scopes, as write access includes read access [1][5].
Citations:
- 1: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/
- 2: https://docs.github.com/en/actions/concepts/security/github_token
- 3: https://github.blog/changelog/2023-02-02-github-actions-updating-the-default-github_token-permissions-to-read-only/
- 4: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository
- 5: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
- 6: https://adaptive-enforcement-lab.com/secure/github-actions-security/token-permissions/job-scoping/
- 7: https://docs.github.com/en/actions/tutorials/authenticate-with-github_token
Restrict the MariaDB job token permissions.
functional-mariadb does not set a job-level permissions block, so it can inherit repository-default token permissions instead of least privilege. Scope the job to the read-only permissions it needs for checkout and artifacts; include both contents: read and actions: read because the job uses actions/checkout and actions/download-artifact.
Proposed fix
functional-mariadb:
+ permissions:
+ contents: read
+ actions: read
runs-on: ubuntu-latest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| functional-mariadb: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 30 | |
| needs: build | |
| strategy: | |
| fail-fast: false | |
| matrix: | |
| mariadb_version: ['10.6', '10.11'] | |
| steps: | |
| - uses: actions/checkout@v4 | |
| functional-mariadb: | |
| permissions: | |
| contents: read | |
| actions: read | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 30 | |
| needs: build | |
| strategy: | |
| fail-fast: false | |
| matrix: | |
| mariadb_version: ['10.6', '10.11'] | |
| steps: | |
| - uses: actions/checkout@v4 |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 195-195: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 184-295: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/functional.yml around lines 184 - 195, Add a job-level
permissions block to functional-mariadb granting only contents: read and
actions: read, supporting its checkout and artifact download steps while
preventing inherited broader token permissions.
Source: Linters/SAST tools
| echo "--- Test 1: SQL stopped + IO running is not DeadMaster while master is up ---" | ||
|
|
||
| $COMPOSE exec -T mysql2 mysql -uroot -ptestpass -e "$STOP_SQL_THREAD" 2>/dev/null | ||
| curl -s --max-time 10 "$ORC_URL/api/discover/mysql2/3306" >/dev/null 2>&1 | ||
| sleep 3 | ||
|
|
||
| ANALYSIS=$(curl -s --max-time 10 "$ORC_URL/api/replication-analysis" 2>/dev/null || echo "[]") | ||
| if echo "$ANALYSIS" | python3 -c " | ||
| import json, sys | ||
| data = json.load(sys.stdin) | ||
| if isinstance(data, dict): | ||
| data = data.get('Details') or data.get('data') or [] | ||
| if not isinstance(data, list): | ||
| sys.exit(0) | ||
| for a in data: | ||
| code = a.get('AnalysisCode') or a.get('Analysis') or '' | ||
| if code == 'DeadMaster': | ||
| sys.exit(1) | ||
| sys.exit(0) | ||
| " 2>/dev/null; then | ||
| pass "No DeadMaster while master is reachable (SQL stopped on replica)" | ||
| else | ||
| fail "Unexpected DeadMaster while master is still up" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Exercise an unreachable-master condition here.
Keeping mysql1 reachable means this never reaches the old false-DeadMaster branch, so it passes on the pre-fix implementation too. Isolate orchestrator from mysql1 while preserving replica IO=Yes/SQL=No, then assert the analysis entry for mysql1 is UnreachableMaster (and verify the stopped-thread precondition first).
🤖 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 `@tests/functional/test-mariadb-relay-drain.sh` around lines 172 - 195, The
Test 1 scenario must exercise an unreachable master instead of leaving mysql1
reachable. After stopping the SQL thread on mysql2, verify the replica remains
IO=Yes/SQL=No, isolate the orchestrator from mysql1 while preserving that
replica state, then query replication analysis and assert mysql1’s entry is
UnreachableMaster; restore connectivity afterward.
Root causes of remaining CI red: - MariaDB job did not export MYSQL_IMAGE/COMPOSE into the relay-drain step, so the suite mis-detected MySQL and issued START REPLICA / CHANGE REPLICATION SOURCE (leaving SQL=No). - Running relay-drain early left mysql1 stopped and broke later topology tests even when the drain assertions themselves passed. Export MYSQL_IMAGE+COMPOSE for MariaDB steps, detect mariadb via image name, and keep the suite last in the MySQL matrix.
MariaDB relay-drain ensure_flat failed with errno 1950 (gtid strict out-of-order) after prior local GTIDs on replicas. Disable strict mode in test cnf, RESET + clear gtid_slave_pos on repoint, and require both IO and SQL running in setup-replication.
…rain MySQL 9.5/9.6 left SHOW REPLICA STATUS empty after ad-hoc RESET+CHANGE following failover suites. Reuse setup-replication.sh (with super_read_only cleared and STOP before CHANGE) when ensure_flat needs a rebuild.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@tests/functional/test-mariadb-relay-drain.sh`:
- Around line 348-350: Replace the mysql1-only cleanup in the test’s
post-failover cleanup flow with a complete topology reset: either restore
verified flat replication and ProxySQL discovery/routing for every node, or tear
down and recreate the Compose project before summary generation. Ensure this
cleanup runs on mid-suite and CI failure paths so no promoted replica or stale
routing state remains for later tests.
- Line 83: Update the read-only reset command in the MariaDB relay-drain test to
avoid setting the unsupported super_read_only variable. Execute the
MariaDB-compatible read_only=0 reset before repointing, ensuring the reset
succeeds instead of being masked by || true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e02d0ef-986d-414b-94dc-5ccaacf96245
📒 Files selected for processing (6)
.github/workflows/functional.ymltests/functional/mysql/mariadb-master.cnftests/functional/mysql/mariadb-replica.cnftests/functional/mysql/mariadb-replica2.cnftests/functional/setup-replication.shtests/functional/test-mariadb-relay-drain.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/functional/mysql/mariadb-replica2.cnf
- tests/functional/mysql/mariadb-replica.cnf
- tests/functional/mysql/mariadb-master.cnf
| local r="$1" | ||
| local out | ||
| # Clear read-only so reconfiguration is allowed after a prior promotion. | ||
| $COMPOSE exec -T "$r" mysql -uroot -ptestpass -e "SET GLOBAL super_read_only=0; SET GLOBAL read_only=0;" 2>/dev/null || true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate file and relevant section =="
if [ -f tests/functional/test-mariadb-relay-drain.sh ]; then
wc -l tests/functional/test-mariadb-relay-relay-drain.sh 2>/dev/null || true
sed -n '1,140p' tests/functional/test-mariadb-relay-drain.sh
else
echo "file not found"
fd -a 'test-mariadb-relay-drain.sh' .
fi
echo
echo "== Search super_read_only/read_only usages in tests/context =="
rg -n --hidden --glob '!*.lock' 'super_read_only|read_only|repoint_replica_to_mysql1' . || true
echo
echo "== Docker/compose hints around maria image =="
fd -a 'compose|docker-compose|Dockerfile|mariadb' . | head -100 | sed -n '1,160p'
rg -n --hidden --glob '!*.lock' 'mariadb|server_version|read_only|super_read_only' docker-compose* compose* . -g '!vendor' -g '!node_modules' | head -200 || trueRepository: ProxySQL/orchestrator
Length of output: 50378
🌐 Web query:
MariaDB super_read_only system variable MySQL MariaDB compatibility differences
💡 Result:
The super_read_only system variable is a feature specific to MySQL and Percona Server, and it is not available in MariaDB [1][2][3]. In MySQL, super_read_only extends the functionality of the standard read_only variable by prohibiting updates even from users who possess the SUPER privilege [2][4]. Because this variable does not exist in MariaDB, it is frequently listed in MariaDB documentation as a system variable difference between MariaDB and MySQL [1][3][5][6]. While feature requests to implement super_read_only in MariaDB have been submitted, it remains unimplemented as a system variable [2][7]. MariaDB users seeking similar functionality typically rely on the standard read_only variable or external access control and security configurations to restrict user write permissions [4].
Citations:
- 1: https://mariadb.com/docs/release-notes/community-server/about/compatibility-and-differences/system-variable-differences-between-mariadb-and-mysql/system-variable-differences-between-mariadb-and-mysql-unmaintained-series/system-variable-differences-between-mariadb-10-3-and-mysql-8-0
- 2: https://jira.mariadb.org/browse/MDEV-9458
- 3: https://mariadb.com/docs/release-notes/community-server/about/compatibility-and-differences/system-variable-differences-between-mariadb-and-mysql/system-variable-differences-between-mariadb-and-mysql-unmaintained-series/system-variable-differences-between-mariadb-11-2-and-mysql-8-0
- 4: https://www.percona.com/blog/mysql-101-using-super_read_only/
- 5: https://mariadb.com/docs/release-notes/community-server/about/compatibility-and-differences/system-variable-differences-between-mariadb-and-mysql/system-variable-differences-between-mariadb-and-mysql-unmaintained-series/system-variable-differences-between-mariadb-10-7-and-mysql-8-0
- 6: https://mariadb.com/docs/release-notes/community-server/about/compatibility-and-differences/system-variable-differences-between-mariadb-and-mysql/system-variable-differences-between-mariadb-and-mysql-unmaintained-series/system-variable-differences-between-mariadb-11-0-and-mysql-8-0
- 7: https://archive.lists.launchpad.net/maria-developers/msg08801.html
🌐 Web query:
MariaDB mysql client execute multiple statements -e error second statement executed after first error
💡 Result:
In the MariaDB and MySQL command-line clients, the behavior regarding errors when executing multiple statements depends on the execution mode and the options provided [1]. By default, if a statement fails during a batch execution, the client stops processing subsequent statements [2][3]. To control this behavior, consider the following points: 1. Use the --force option: The most direct way to ensure the client continues execution despite SQL errors is to use the --force command-line option [3][4][5]. This instructs the client to proceed with subsequent statements even if a previous one encounters an error. 2. C API and Programmatic Execution: If you are executing multiple statements programmatically (e.g., using the C API with CLIENT_MULTI_STATEMENTS), the application is responsible for iterating through the result sets [6][7][8]. The functions mysql_query or mysql_real_query only return the status of the first statement [2]. To detect errors in subsequent statements, you must loop through all results using mysql_next_result and check for errors after each call [2][8]. Simply checking the return value of the initial execution function is insufficient to guarantee that every statement in a multi-statement string succeeded [2]. 3. SQL-Level Handling: To prevent errors from occurring in the first place, you can use defensive SQL patterns such as DROP TABLE IF EXISTS or DROP USER IF EXISTS [3]. These statements will trigger a warning instead of a hard error if the object does not exist, allowing the script to continue naturally without requiring the --force flag [3]. In summary, for command-line scripts, --force is the standard solution to avoid stopping on errors [3]. For developers writing custom applications, you must implement a result-processing loop that inspects the return code of mysql_next_result for every statement in the batch [6][7][8].
Citations:
- 1: https://dev.mysql.com/doc/refman/9.1/en/mysql.html
- 2: https://bugs.mysql.com/bug.php?id=6383
- 3: https://unix.stackexchange.com/questions/408431/mysql-cli-continue-execution-even-if-the-operation-failed
- 4: https://dev.mysql.com/doc/refman/9.7/en/mysql-command-options.html
- 5: https://dev.mysql.com/doc/refman/9.1/en/mysql-command-options.html
- 6: https://dev.mysql.com/doc/c-api/9.7/en/c-api-multiple-queries.html
- 7: https://dev.mysql.com/doc/c-api/8.0/en/c-api-multiple-queries.html
- 8: https://dev.mysql.com/doc/c-api/8.4/en/c-api-multiple-queries.html
Use a MariaDB-compatible read-only reset.
super_read_only is MariaDB-unsupported, and the multi-statement -e execution stops on that error before read_only=0 runs; || true hides the failure, so the replica can remain read-only and repointing can fail. Split the reset or use only read_only=0 for MariaDB before re-pointing.
🤖 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 `@tests/functional/test-mariadb-relay-drain.sh` at line 83, Update the
read-only reset command in the MariaDB relay-drain test to avoid setting the
unsupported super_read_only variable. Execute the MariaDB-compatible read_only=0
reset before repointing, ensuring the reset succeeds instead of being masked by
|| true.
Source: MCP tools
| # Best-effort restore so a mid-suite run does not poison later tests. | ||
| echo "--- Cleanup: restart mysql1 (best-effort) ---" | ||
| $COMPOSE start mysql1 >/dev/null 2>&1 || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file size =="
wc -l tests/functional/test-mariadb-relay-drain.sh
echo "== relevant sections =="
sed -n '1,120p' tests/functional/test-mariadb-relay-drain.sh | cat -n
echo "---"
sed -n '300,410p' tests/functional/test-mariadb-relay-drain.sh | cat -n
echo "== compose teardown / restart / summary usages =="
rg -n "\\b(start|stop|down|rm|restart|summary|COMPOSE_PROJECT_NAME|mysql)[A-Za-z0-9_\\-]*" tests/functional/test-mariadb-relay-drain.shRepository: ProxySQL/orchestrator
Length of output: 15235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== functional lib summary and teardown helpers =="
sed -n '1,260p' tests/functional/lib.sh | cat -n
echo "== compose files =="
for f in tests/functional/docker-compose.yml tests/functional/docker-compose.mariadb.yml; do
[ -f "$f" ] || continue
echo "--- $f ---"
sed -n '1,220p' "$f" | cat -n
done
echo "== summary usages in functional tests =="
rg -n '^[[:space:]]*(summary|ensure_flat_topology|teardown|tearDown|clean|down|rm)' tests/functional/lib.sh tests/functional -S || trueRepository: ProxySQL/orchestrator
Length of output: 19635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== run-all functional execution =="
sed -n '1,130p' tests/functional/run-all.sh | cat -n
echo "== test names in functional subdir =="
git ls-files tests/functional | sed -n '1,160p'
echo "== orchestrator failover config snippets =="
for f in tests/functional/orchestrator-test.conf.json tests/functional/orchestrator-pg-test.conf.json; do
[ -f "$f" ] || continue
echo "--- $f ---"
rg -n 'FailoverCommand|DeadMaster|mysql1|mysql2|mysql3|Command|Replace' "$f" || true
doneRepository: ProxySQL/orchestrator
Length of output: 5303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test-mariadb-relay-drain failover cleanup with context =="
sed -n '186,200p' tests/functional/test-mariadb-relay-drain.sh | cat -n
sed -n '280,352p' tests/functional/...-relay-drain.sh | cat -n 2>/dev/null || sed -n '280,352p' tests/functional/test-mariadb-relay-drain.sh | cat -n
echo "== other failover test teardown/cleanup patterns =="
for f in tests/functional/test-failover.sh tests/functional/test-failover-advanced.sh; do
[ -f "$f" ] || continue
echo "--- $f last 80 lines ---"
sed -n '$(( $(wc -l < "$f") - 79 )),$ p' "$f" | cat -n
done
echo "== inspect failed if path to ensure_flat_topology call before failover =="
sed -n '170,190p' tests/functional/test-mariadb-relay-drain.sh | cat -nRepository: ProxySQL/orchestrator
Length of output: 4232
Restore the full topology or tear it down after failover.
$COMPOSE start mysql1 only restarts the stopped mysql1 container; it leaves the promoted replica, sibling replication, and ProxySQL discovery/routing in the post-recovery state. Restore all paths to a verified flat topology, or tear down/recreate the Compose project before summary, so mid-suite or CI failure paths cannot pollute later runs.
🤖 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 `@tests/functional/test-mariadb-relay-drain.sh` around lines 348 - 350, Replace
the mysql1-only cleanup in the test’s post-failover cleanup flow with a complete
topology reset: either restore verified flat replication and ProxySQL
discovery/routing for every node, or tear down and recreate the Compose project
before summary generation. Ensure this cleanup runs on mid-suite and CI failure
paths so no promoted replica or stale routing state remains for later tests.
On MySQL 9.5/9.6, setup-replication reported IO ON via performance_schema while replica_status_fields used SHOW REPLICA STATUS and got empty output (status=|||), failing ensure_flat after a successful rebuild. Read IO/SQL from performance_schema for MySQL 8+, keep SHOW SLAVE for MariaDB/5.7, and relax the relay pre-check when positions are unavailable.
1 participant
Summary
Fixes #106
UnreachableMasterinstead ofDeadMasterwhen the master check fails (SQL stopped alone is not proof the master is dead).DrainRelayLogsstarts the SQL thread only (no IO stop/start), waits for catch-up, then stops replication. MariaDB nice-stop now uses this path so relay events are applied before promotion.GetCandidateReplicatries the next replica if drain fails (e.g. Error 1062); fails recovery if none can drain.RegroupReplicasGTIDWithValidation).test-mariadb-relay-drain.sh, MariaDB 10.6/10.11 CI job, docs updates.Design / plan
docs/superpowers/specs/2026-07-16-mariadb-deadmaster-relay-drain-design.mddocs/superpowers/plans/2026-07-16-mariadb-deadmaster-relay-drain.mdTest plan
go test ./go/inst/ -count=1go build ./go/cmd/orchestratorfunctional-mariadb(10.6, 10.11)Summary by CodeRabbit