aurora: Implement autopurge & fix monitor race condition - #5660
aurora: Implement autopurge & fix monitor race condition#5660wazir-ahmed wants to merge 17 commits into
Conversation
- Move cluster_simulator from the Jenkins scripts repo into test/deps/cluster_simulator/ (sources, configs, docs, payloads). - Rework the simulator Makefile to use PROXYSQL_PATH + makefiles_*.mk and the repo's vendored deps/. - Switch vendored json.hpp includes to $(JSON_IDIR). - Read TEST_<FAMILY> SQLite endpoint from env vars with legacy fallbacks. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
- Add build_cluster_simulator{,_debug} that recurse into
test/deps/cluster_simulator.
- Wire it as a prerequisite of testaurora/testgalera/testgrouprep/
testreadonly/testreplicationlag.
- .gitignore the cluster_simulator binary.
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
- Add 5 simulator-backed TAP groups (aurora, galera, group_repl, read_only, repl_lag). - Add a TAP test per group — thin wrappers around cluster_simulator sharing cluster_sim_runner.h. - start-proxysql-isolated.bash: inject CLUSTER_SIM_HOST_FILE entries into the proxysql container via docker --add-host. - ensure-infras.bash / destroy-infras.bash: tolerate groups that omit infras.lst; ensure-infras also dispatches both pre-proxysql.bash and pre-proxysql.sql. - test/infra/README.md documents the per-family build requirement. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new cluster_simulator test framework (binary, build rules, docs, jq/node helpers, and many JSON fixtures) and implements Aurora autopurge support: schema/runtime column, admin persistence and disk-upgrade, monitor-side autopurge logic, and integration into monitor/hostgroup workflows. ChangesCluster Simulator (tests/tooling/runtime simulation DAG)
Aurora autopurge & schema integration (runtime + admin DAG)
Sequence Diagram(s)sequenceDiagram
participant Runner as "cluster_simulator\n(Runner)"
participant Admin as "ProxySQL Admin\n(runtime/sqlite)"
participant Monitor as "MySQL_Monitor\n(thread)"
participant MySQL as "MySQL Instances"
Runner->>Admin: prepare/INSERT mysql_servers & hostgroups (init)
Runner->>Monitor: trigger monitor cycle / set monitor vars
Monitor->>MySQL: query cluster-specific status (wsrep/replication/aurora)
MySQL-->>Monitor: return host status payloads
Monitor->>Admin: update runtime tables (REPLICA_HOST_STATUS / READONLY_STATUS / etc.)
Monitor->>Monitor: if aurora.autopurge_missing_checks>0 → aws_aurora_autopurge_servers()
Monitor->>Admin: remove missing replica rows (after threshold) and update runtime
Admin-->>Runner: SELECT runtime/host_status for verification
Runner->>Runner: compare actual vs expected, emit JSON result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
wazir-ahmed
changed the title
![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 introduces the ProxySQL Cluster Simulator, a tool for simulating and verifying cluster behaviors across various modes (Galera, ReadOnly, Group Replication, Replication Lag, and Aurora) using an internal SQLite database. The implementation includes the core simulator logic, utility libraries, and an extensive collection of test payloads. The review feedback highlights several critical safety and robustness concerns, including missing null checks for mysql_init() return values, potential resource leaks from unclosed connections in cleanup paths, and a Makefile path detection logic that could trigger an infinite loop. Suggestions were also provided to improve memory safety by nullifying pointers after closing connections and using sizeof for buffer limits in strftime.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (18)
test/deps/cluster_simulator/cluster_simulator.cpp-2218-2415 (1)
2218-2415:⚠️ Potential issue | 🟠 Major
proxysql_sqlitehandle leaked on connection error paths.In every cluster-type branch the code does:
proxysql_sqlite = mysql_init(NULL); if (!mysql_real_connect(proxysql_sqlite, ...)) { result = invalid_input_error(...); goto cleanup; // <-- leaks proxysql_sqlite }
cleanup:only callsmysql_close(proxysql_admin)— the initializedproxysql_sqliteMYSQL* is never freed. The per-iterationmysql_close(proxysql_sqlite)at line 2414 is also skipped by thegoto. Additionally, even on the success path, if the previous iteration already closedproxysql_sqliteand a later iteration hits a connection error before the nextmysql_init, the same leak occurs.Suggest closing the handle before jumping to cleanup (or in
cleanup:after ensuring it's safe):🔧 Proposed fix
cleanup: std::string t_str_res { serialize_result(result.second) }; std::cout << t_str_res << std::endl; mysql_close(proxysql_admin); + if (proxysql_sqlite) { + mysql_close(proxysql_sqlite); + } return result.first;And reset
proxysql_sqlite = nullptr;after eachmysql_close(proxysql_sqlite)at line 2414 so the cleanup guard remains accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/cluster_simulator.cpp` around lines 2218 - 2415, The proxysql_sqlite MYSQL* is leaked on error paths because mysql_init(...) is called then mysql_real_connect(...) can fail and code jumps to the cleanup label without closing proxysql_sqlite; fix by closing and nulling proxysql_sqlite before any goto cleanup on connection failure (referencing proxysql_sqlite, mysql_init, mysql_real_connect, result and the cleanup label), and also make cleanup robust by checking if proxysql_sqlite != nullptr and calling mysql_close(proxysql_sqlite) then setting proxysql_sqlite = nullptr; finally ensure after the normal per-iteration mysql_close(proxysql_sqlite) you reset proxysql_sqlite = nullptr to keep the guard accurate.test/deps/cluster_simulator/cluster_simulator.cpp-254-258 (1)
254-258:⚠️ Potential issue | 🟠 MajorUnused computed state suggests incomplete or incorrect logic across all simulators.
galera_update_cluster_stateis designed to return a merged state by comparingservers_state_p(initial) againstservers_state_n(new) and updating only fields that differ (viagalera_update_state). However, the result is assigned togalera_new_state_to_setand never used; instead, the rawgalera_new_servers_stateis passed toprepare_galera_cluster_state.The same pattern occurs in all other simulators:
readonly_update_cluster_state(lines 624–627):replication_new_state_to_setunused.grouprep_update_cluster_state(lines 1033–1036):group_replication_new_state_to_setunused.aurora_update_cluster_state(lines 1797–1801):aurora_new_state_to_setunused.
prepare_galera_cluster_stateiterates through the entire input vector and inserts each server into the database (lines 301+). This implies it expects the complete, merged state, not just the raw new servers. If the test uses partial state patches, passing only rawgalera_new_servers_stateinstead of the merged result would silently discard fields from the initial state that should be preserved.Either
prepare_*_cluster_stateshould receive the merged*_new_state_to_set, or the*_update_cluster_statecalls and unused locals should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/cluster_simulator.cpp` around lines 254 - 258, The code computes a merged state via galera_update_cluster_state but then passes the raw galera_new_servers_state into prepare_galera_cluster_state (discarding preserved fields); change the call to pass galera_new_state_to_set into prepare_galera_cluster_state (and do the analogous fixes for readonly_update_cluster_state -> use replication_new_state_to_set, grouprep_update_cluster_state -> use group_replication_new_state_to_set, and aurora_update_cluster_state -> use aurora_new_state_to_set), or if you prefer the alternate approach remove the unused *_update_cluster_state calls and locals; update each simulator so prepare_*_cluster_state receives the merged state variable (or delete the dead merge logic) to ensure initial fields are preserved.test/deps/cluster_simulator/docs/payloads_processing/node/payload.js-3-16 (1)
3-16:⚠️ Potential issue | 🟠 MajorMake the helper parse-clean and avoid implicit globals.
Line 7 fails Biome parsing with "Illegal return statement outside of a function," and lines 4 and 16 assign undeclared variables (
fileandpayloads), creating implicit globals. Replace the top-levelreturnwithprocess.exit(1), declare all variable bindings, and set a non-zero exit code in the error handler for proper failure signaling.Proposed fix
-if (process.argv[2]) { - file = process.argv[2]; -} else { - console.log('No file for processing supplied.'); - return -1; -} +const file = process.argv[2]; + +if (!file) { + console.error('No file for processing supplied.'); + process.exit(1); +} fs.readFile(file, 'utf8', (err, data) => { if (err) { console.error(err); + process.exitCode = 1; return; } - payloads = JSON.parse(data); + const payloads = JSON.parse(data);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/docs/payloads_processing/node/payload.js` around lines 3 - 16, The helper uses implicit globals and a top-level return causing parse errors: declare variables instead of assigning to undeclared identifiers (e.g., create a const/let for file and let payloads), replace the top-level "return -1" with process.exit(1) after logging "No file for processing supplied.", and in the fs.readFile callback set a non-zero exit code on error (e.g., call process.exit(1) after console.error(err)) before returning; keep JSON.parse(data) to assign to the declared payloads variable so there are no implicit globals and the script exits with failure codes on error.test/deps/cluster_simulator/docs/payloads_processing/jq_filters/servers_in_reader_hostgroups.jq-4-4 (1)
4-4:⚠️ Potential issue | 🟠 MajorInclude
portin the dedup key to avoid dropping valid servers.On Line 4, deduplicating by only
hostgroup_id+hostnamecan collapse distinct entries that differ by port.Proposed fix
-| unique_by([.hostgroup_id, .hostname]) +| unique_by([.hostgroup_id, .hostname, .port])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/docs/payloads_processing/jq_filters/servers_in_reader_hostgroups.jq` at line 4, The dedup key in the jq filter uses unique_by([.hostgroup_id, .hostname]) which drops distinct server entries that only differ by port; update the deduplication key to include port by changing the expression to unique_by([.hostgroup_id, .hostname, .port]) so servers with the same hostgroup and hostname but different ports are preserved.test/deps/cluster_simulator/docs/payloads_processing/jq_filters/all_init_state_writers.jq-2-3 (1)
2-3:⚠️ Potential issue | 🟠 MajorFix field name in filter comparison.
Line 3 references
.writers, butmysql_replication_hostgroupspayloads usewriter_hostgroup. The filter currently produces empty output.Fix
.[0] | [.proxysql_init_state[] as $server - | (.mysql_replication_hostgroups[] | if $server.hostgroup_id == .writers then $server else empty end)] + | (.mysql_replication_hostgroups[] | if $server.hostgroup_id == .writer_hostgroup then $server else empty end)] | unique_by([.hostgroup_id, .hostname]) | .[]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/docs/payloads_processing/jq_filters/all_init_state_writers.jq` around lines 2 - 3, The jq filter is comparing $server.hostgroup_id to a non-existent field `.writers`, so it yields empty results; update the comparison inside the pipeline that iterates `.mysql_replication_hostgroups[]` to use `.writer_hostgroup` instead of `.writers` (i.e., change the conditional `if $server.hostgroup_id == .writers` to `if $server.hostgroup_id == .writer_hostgroup`) so the `$server` selection works when using `.proxysql_init_state[]` and `.mysql_replication_hostgroups[]`.test/deps/cluster_simulator/docs/payloads_processing/jq_filters/init_state_writers_only.jq-7-20 (1)
7-20:⚠️ Potential issue | 🟠 MajorFix server identity correlation in writer selection logic.
The filter selects a writer merely because some init-state server exists in a writer hostgroup, without correlating by hostname and port. Additionally, the reader exclusion loop emits
$writerfor each non-matching server/hostgroup row rather than excluding servers that appear in reader hostgroups.The reproduction confirms the issue:
writer.exampleis incorrectly emitted even though it appears in both writer and reader hostgroups with no matching init-state entry.Proposed rewrite
.[0] - | { - init_state: .proxysql_init_state, - replication_hostgroups: .mysql_replication_hostgroups, - writers_only: - [ - [.mysql_servers[] as $server - | .proxysql_init_state[] as $init_server - | .mysql_replication_hostgroups[] - | if $server.hostgroup_id == .writer_hostgroup and - $init_server.hostgroup_id == .writer_hostgroup - then $server - else empty end - ][] as $writer - | .mysql_servers[] as $server - | .mysql_replication_hostgroups[] - | if $server.hostname == $writer.hostname and - $server.hostgroup_id == .reader_hostgroup - then empty - else $writer end - ] - } - | .writers_only - | unique_by([.hostname, .hostgroup_id]) | .[] + as $root + | ($root.mysql_replication_hostgroups | map(.writer_hostgroup)) as $writer_hostgroups + | ($root.mysql_replication_hostgroups | map(.reader_hostgroup)) as $reader_hostgroups + | [ + $root.mysql_servers[] + | select(.hostgroup_id as $hostgroup_id | any($writer_hostgroups[]; . == $hostgroup_id)) + | select(. as $server + | any($root.proxysql_init_state[]; + .hostname == $server.hostname + and .port == $server.port + and (.hostgroup_id as $hostgroup_id | any($writer_hostgroups[]; . == $hostgroup_id)))) + | select(. as $writer + | all($root.mysql_servers[]; + ((.hostname == $writer.hostname + and .port == $writer.port + and (.hostgroup_id as $hostgroup_id | any($reader_hostgroups[]; . == $hostgroup_id))) + | not))) + ] + | unique_by([.hostname, .hostgroup_id]) + | .[]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/docs/payloads_processing/jq_filters/init_state_writers_only.jq` around lines 7 - 20, The writer selection currently matches any server in the writer hostgroup if any proxysql_init_state row exists in that hostgroup and later the reader exclusion emits $writer repeatedly; update the logic in the jq filter so (1) when iterating .mysql_servers as $server and .proxysql_init_state[] as $init_server you require both hostgroup_id AND identity (hostname and port) to match (use $server.hostname == $init_server.hostname and $server.port == $init_server.port) before emitting $server as $writer, and (2) change the reader exclusion loop over .mysql_servers and .mysql_replication_hostgroups so you filter out writers by checking identity (hostname and port) against reader hostgroup entries instead of emitting $writer for every non-matching row (i.e., only emit $writer when no reader hostgroup entry matches the writer's hostname+port).test/deps/cluster_simulator/lib/aurora_utils.cpp-543-569 (1)
543-569:⚠️ Potential issue | 🟠 Major
aurora_update_statereturns identity-less tuples that cannot be correlated back to servers, and the result ofaurora_update_cluster_stateis currently unused.
resultis default-constructed, sostd::get<0>(SERVER_ID) andstd::get<1>(DOMAIN_NAME) are empty strings. Only indices2and3are conditionally populated. Inaurora_update_cluster_state(lines 571–602), these identity-less tuples are pushed into the returned vector, but the caller's result (aurora_new_state_to_setat line 1798 incluster_simulator.cpp) is never used—it only appears at assignment. If this code is intended for future use (e.g., logging state changes, auditing), the identity fields must be present; as-is, the function design will fail. Additionally, localsst1_read_only/st2_read_only(lines 556–557) are named forread_onlybut actually holdREPLICA_LAG_IN_MILLISECONDS— rename for clarity.🛠️ Suggested fix
aurora_server_state_t aurora_update_state( const aurora_server_state_t& st1, const aurora_server_state_t& st2 ) { - aurora_server_state_t result {}; + // Preserve the server identity so callers can correlate the update back to a server. + aurora_server_state_t result { + std::get<AURORA_SERVER_STATE::SERVER_ID>(st2), + std::get<AURORA_SERVER_STATE::DOMAIN_NAME>(st2), + "", + -1 + }; // TODO: Make this proper doc. // SERVER_ID and DOMAIN_NAME **can't** be changed, because the are part of the server 'id'. Only the other // fields are allowed to change, otherwise, the verification step should have failed. const string st1_session_id { std::get<AURORA_SERVER_STATE::SESSION_ID>(st1) }; const string st2_session_id { std::get<AURORA_SERVER_STATE::SESSION_ID>(st2) }; - int32_t st1_read_only { std::get<AURORA_SERVER_STATE::REPLICA_LAG_IN_MILLISECONDS>(st1) }; - int32_t st2_read_only { std::get<AURORA_SERVER_STATE::REPLICA_LAG_IN_MILLISECONDS>(st2) }; + int32_t st1_replica_lag { std::get<AURORA_SERVER_STATE::REPLICA_LAG_IN_MILLISECONDS>(st1) }; + int32_t st2_replica_lag { std::get<AURORA_SERVER_STATE::REPLICA_LAG_IN_MILLISECONDS>(st2) }; // Since empty 'SESSION_IDs' have no meaning, we ignore them for updated states if (st2_session_id != "" && st1_session_id != st2_session_id) { - std::get<2>(result) = st2_session_id; + std::get<AURORA_SERVER_STATE::SESSION_ID>(result) = st2_session_id; } - if (st2_read_only != -1 && st1_read_only != st2_read_only) { - std::get<3>(result) = st2_read_only; + if (st2_replica_lag != -1 && st1_replica_lag != st2_replica_lag) { + std::get<AURORA_SERVER_STATE::REPLICA_LAG_IN_MILLISECONDS>(result) = st2_replica_lag; } return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/aurora_utils.cpp` around lines 543 - 569, aurora_update_state currently returns tuples with empty identity fields (SERVER_ID, DOMAIN_NAME) and misnamed variables for replica lag; update aurora_update_state to copy the immutable identity fields from st1 (or st2 if preferred but keep consistent) into result[SERVER_ID] and result[DOMAIN_NAME], populate indices 2/3 only as before, and rename st1_read_only/st2_read_only to st1_replica_lag/st2_replica_lag to reflect REPLICA_LAG_IN_MILLISECONDS; also ensure callers (aurora_update_cluster_state) push the fully-populated tuples into the return vector (or otherwise consume/return aurora_update_state results) so the produced state changes can be correlated back to servers.test/deps/cluster_simulator/tests/grouprep_test_payloads_shunned_preservation/five_node_multimaster-offline_soft.json-56-57 (1)
56-57:⚠️ Potential issue | 🟠 MajorFill in the expected final ProxySQL state for the SHUNNED/OFFLINE_SOFT transition.
This fixture configures five servers but leaves
proxysql_final_stateempty. The cluster_simulator framework validates fixtures against actual computed state during verification (check_cluster_status()in cluster_simulator.cpp), so an empty array will fail verification. Similar fixtures in the same directory—such asfive_node_multimaster-unshunning.jsonandthree_node_grouprep-offline_soft.json—all have populated final states reflecting expected OFFLINE_SOFT or status transitions. This fixture needs a corresponding final state matching the SHUNNED/OFFLINE_SOFT behavior it describes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/tests/grouprep_test_payloads_shunned_preservation/five_node_multimaster-offline_soft.json` around lines 56 - 57, Add the expected ProxySQL final state entries to the proxysql_final_state array in the five_node_multimaster-offline_soft.json fixture: determine per-server final statuses and hostgroup/weight entries that represent the SHUNNED -> OFFLINE_SOFT transition (matching how proxysql_final_state is structured in five_node_multimaster-unshunning.json and three_node_grouprep-offline_soft.json), then populate proxysql_final_state with one object per server reflecting those OFFLINE_SOFT/shunned states so check_cluster_status() can validate the fixture.test/deps/cluster_simulator/tests/grouprep_test_payloads_shunned_preservation/five_node_multimaster-unshunning.json-3-13 (1)
3-13:⚠️ Potential issue | 🟠 MajorClarify which server is undergoing the SHUNNED→unshunning transition.
The initial
mysql_serversconfig seeds127.2.1.1asSHUNNED(line 13), but the comment on line 8 says "Server '127.2.1.3' should be moved as 'SHUNNED'...". The final state shows127.2.1.1moved to backup_writer (unshunned) and127.2.1.3promoted to writer, indicating two concurrent transitions are being tested. Rewrite the comment to clearly describe which server exhibits the SHUNNED→unshunning transition versus which is being promoted to writer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/tests/grouprep_test_payloads_shunned_preservation/five_node_multimaster-unshunning.json` around lines 3 - 13, The test comment is ambiguous about which host is unshunned vs promoted; update the "__comment__" block so it explicitly states that mysql_servers entry "127.2.1.1" is the server that starts as SHUNNED and will be unshunned/moved to BACKUP WRITER, while "127.2.1.3" is the server that will be promoted to WRITER (and ensure the comment mentions the expected log entry showing "UNSHUNNED" for 127.2.1.1 and promotion for 127.2.1.3); keep the rest of the scenario details (monitor_groupreplication_max_transaction_behind_for_read_only = 0, max_writers = 2) intact.test/deps/cluster_simulator/Makefile-10-11 (1)
10-11:⚠️ Potential issue | 🟠 MajorAvoid reusing release-compiled objects when running
make debug.The
debugtarget only changesOPTvia target-specific variable assignment, but both release and debug modes write object files to./obj. After a release build, object files are timestamped and up-to-date; GNU Make does not track variable changes as dependencies. Runningmake debugnext will find the.oand.afiles already present and skip recompilation, resulting in a "debug" executable containing release-optimized objects (-O2) instead of debug flags (-O0 -DDEBUG).Separate object directories by build mode (or add another rebuild trigger) so that flag changes force recompilation.
🔧 Proposed fix: separate object/archive paths by build mode
+BUILD_MODE ?= release UTILSIDIR := ./lib -UTILSLDIR := ./obj +UTILSLDIR = ./obj/$(BUILD_MODE) @@ .PHONY: debug +debug: BUILD_MODE := debug debug: OPT := $(STDCPP) -O0 -DDEBUG -ggdb -DDISABLE_WARNING_COUNT_LOGGING -Wl,--no-as-needed -Wl,-rpath,$(TAP_LDIR) -Wl,-rpath,$(POSTGRESQL_PATH)/interfaces/libpq -Wl,-rpath,$(RE2_LDIR) $(WGCOV) $(WASAN) -DGITVERSION=\"$(GIT_VERSION)\" debug: cluster_simulator🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/Makefile` around lines 10 - 11, The Makefile currently reuses the same object/archive directories (UTILSIDIR and UTILSLDIR) for both release and debug builds, causing make to skip recompilation when only OPT changes; update the Makefile so debug and release builds use distinct directories (e.g., UTILSIDIR_DEBUG / UTILSLDIR_DEBUG or append the build mode to UTILSIDIR/UTILSLDIR) or otherwise make the debug target depend on a rebuild trigger; modify references to UTILSIDIR and UTILSLDIR in the debug-specific rules and in targets that create .o/.a (and any rules that use OPT or the debug target-specific variable assignment) so debug objects are written to and read from the separate debug directories, ensuring debug builds actually recompile with OPT=-O0 -DDEBUG.Makefile-202-219 (1)
202-219:⚠️ Potential issue | 🟠 MajorSequence the simulator build after the TEST-flavored lib build to avoid race conditions under parallel make.
The test targets list
build_cluster_simulatoras a sibling prerequisite alongsidebuild_src_test<family>, allowing GNU Make to run both in parallel undermake -j. Sincebuild_cluster_simulatorhas no prerequisites, it can start and complete beforebuild_lib_test<family>finishes, causing the simulator to link against the previouslibproxysql.ainstead of the newly built TEST-flavored library—contradicting the design intent stated at lines 354-358.Move the simulator build into the recipe to enforce sequential execution:
Proposed fix
.PHONY: testaurora -testaurora: build_src_testaurora build_cluster_simulator +testaurora: build_src_testaurora + ${MAKE} build_cluster_simulator # cd test/tap && OPTZ="${O0} -ggdb -DDEBUG -DTEST_AURORA" CC=${CC} CXX=${CXX} ${MAKE} # cd test/tap/tests && OPTZ="${O0} -ggdb -DDEBUG -DTEST_AURORA" CC=${CC} CXX=${CXX} ${MAKE} $(MAKECMDGOALS) .PHONY: testgalera -testgalera: build_src_testgalera build_cluster_simulator +testgalera: build_src_testgalera + ${MAKE} build_cluster_simulator cd test/tap && OPTZ="${O0} -ggdb -DDEBUG -DTEST_GALERA" CC=${CC} CXX=${CXX} ${MAKE} cd test/tap/tests && OPTZ="${O0} -ggdb -DDEBUG -DTEST_GALERA" CC=${CC} CXX=${CXX} ${MAKE} $(MAKECMDGOALS) .PHONY: testgrouprep -testgrouprep: build_src_testgrouprep build_cluster_simulator +testgrouprep: build_src_testgrouprep + ${MAKE} build_cluster_simulator .PHONY: testreadonly -testreadonly: build_src_testreadonly build_cluster_simulator +testreadonly: build_src_testreadonly + ${MAKE} build_cluster_simulator .PHONY: testreplicationlag -testreplicationlag: build_src_testreplicationlag build_cluster_simulator +testreplicationlag: build_src_testreplicationlag + ${MAKE} build_cluster_simulator🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 202 - 219, The test targets (testaurora, testgalera, testgrouprep, testreadonly, testreplicationlag) currently list build_cluster_simulator as a prerequisite, allowing parallel make to run it before the TEST-flavored lib (e.g., build_src_testaurora) finishes; remove build_cluster_simulator from the prerequisite lists and instead invoke the simulator build as a command inside each target's recipe so it runs sequentially after the corresponding build_src_test<family> step (e.g., after build_src_testaurora complete run build_cluster_simulator via ${MAKE} or equivalent); update testgalera, testaurora, testgrouprep, testreadonly, and testreplicationlag targets accordingly.test/deps/cluster_simulator/lib/readonly_utils.cpp-260-312 (1)
260-312:⚠️ Potential issue | 🟠 MajorUninitialized
int port/int read_onlyrisk on exception path.Lines 261–262 declare
int port;andint read_only;uninitialized. In the init-state branch they are always set before use insidetry, but a caller-visiblereturnpath through the catch block leaves them unused which is fine — the actual risk is in theelsebranch: if all threem_*values are non-null and readable, they are set; otherwise the sentinel values (-1) are assigned. That's OK, butread_onlybeinginthere is inconsistent with the header'sstd::tuple<hostname, port, bool>— assigning-1to aboolviastd::make_tuplewill narrow totrue, silently losing the "unset" sentinel. Either widen the tuple's third field toint(to match this code's-1sentinel semantics) or stop emitting the-1sentinel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/readonly_utils.cpp` around lines 260 - 312, The code uses an int sentinel (-1) for read_only but the header's return type is std::tuple<..., bool>, causing silent narrowing; to fix, change the header's third tuple element from bool to int (preserving the -1 sentinel) and update all call sites and any std::make_tuple usages to expect an int for the third element, converting to bool explicitly where the boolean semantics are needed; this keeps readonly_utils.cpp (variables port and read_only) consistent with the function's declared return type.test/deps/cluster_simulator/lib/common_utils.cpp-1042-1080 (1)
1042-1080:⚠️ Potential issue | 🟠 Major
std::stoican throw outside the existing try/catch — uncaught exception on bad numeric data.Lines 1056–1058 call
std::stoionweight/max_connections/use_sslfetched fromruntime_mysql_servers. These aren't wrapped in atry/catch(theextract_cluster_statuscall below is, but these conversions are not). A non-numeric or empty value would terminate the simulator via an uncaughtstd::invalid_argument/std::out_of_range.atoiis used for the same columns elsewhere — consider usingatoihere too, or wrap in try/catch and return an error pair.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 1042 - 1080, The loop converting proxysql_init_state fields can throw from std::stoi (calls in the loop that set "weight", "max_connections", "use_ssl"); replace those std::stoi calls with safe atoi-style conversions or wrap them in try/catch and on error populate err_res and return (same failure pattern used when extract_cluster_status fails) so an invalid/non-numeric value doesn't raise an uncaught exception; update the conversion block that iterates j_servers["proxysql_init_state"] and ensure you propagate errors into err_res before returning out_cluster_status or exiting, referencing the same error formatting pattern used with string_format and extract_cluster_status.test/deps/cluster_simulator/lib/common_utils.h-235-272 (1)
235-272: 🛠️ Refactor suggestion | 🟠 MajorDuplicate declarations — remove one of each pair.
check_present_and_typeis declared at Line 235 and again at Line 263;matching_server_statusis declared at Line 244 and again at Line 272. Identical declarations are legal but obscure which doc block applies and create maintenance drag (any signature change must be done in two places).-/** - * `@brief` TODO - * - * `@param` j - * `@param` path - * `@param` type - * - * `@return` - */ -bool check_present_and_type(const json& j, const std::vector<std::string>& path, const json::value_t& type); -/** - * `@brief` TODO - * - * `@param` srv_st1 - * `@param` srv_st2 - * - * `@return` - */ -bool matching_server_status(const server_status& srv_st1, const server_status& srv_st2); /** * `@brief`(Apply to Lines 254–272 — the second duplicated block.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/common_utils.h` around lines 235 - 272, Remove the duplicated declarations and doc blocks for check_present_and_type and matching_server_status: keep the original declarations (the first occurrences) and delete the second identical declarations/doc-comments that repeat the same signatures; ensure only one declaration exists for each symbol (check_present_and_type and matching_server_status) so future signature/doc updates are made in a single place.test/deps/cluster_simulator/lib/common_utils.cpp-863-904 (1)
863-904:⚠️ Potential issue | 🟠 MajorResource leak:
mysql_store_resultresult is never freed inget_current_mysql_servers.
my_servers_resis allocated on Line 878 and passed toparse_result_to_jsonbutmysql_free_resultis never called (contrast withget_current_cluster_statuson Line 1048 which does). Every call leaks a result set.🛡️ Proposed fix
MYSQL_RES* my_servers_res = mysql_store_result(proxysql_admin); parse_result_to_json(my_servers_res, j_servers["mysql_servers"]); + mysql_free_result(my_servers_res);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 863 - 904, get_current_mysql_servers currently calls mysql_store_result and assigns it to my_servers_res but never calls mysql_free_result, leaking the MYSQL_RES; after calling parse_result_to_json(my_servers_res, ...) and after extract_mysql_servers(...) returns (both success and failure paths), ensure mysql_free_result(my_servers_res) is always called (e.g., free it before setting err_res in the extract failure branch and in the else/mysql_error branch, and after successful assignment to out_cur_mysql_servers) so the result set is released regardless of outcome.test/deps/cluster_simulator/lib/grouprep_utils.h-21-29 (1)
21-29:⚠️ Potential issue | 🟠 Major
grouprep_server_statetuple usesboolslots that the implementation treats asintwith-1sentinel.
viable_candidateandread_onlyare declaredboolhere, butgrouprep_utils.cpp::grouprep_state_members_diff(L659-678) andgrouprep_update_state(L733-755) extract them asintand gate updates on!= -1. Because bool→int is always 0/1, the sentinel never matches and the “unspecified” semantics silently don’t work. Either change those two slots toint(matching the Galera side) or drop the sentinel checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/grouprep_utils.h` around lines 21 - 29, The tuple type grouprep_server_state currently declares the VIABLE_CANDIDATE and READ_ONLY slots as bool but the implementation (grouprep_state_members_diff and grouprep_update_state) extracts them as int and checks for -1; change those two slots in grouprep_server_state from bool to int so the sentinel -1 semantics work (i.e., update the typedef/using declaration to use int for the third and fourth elements corresponding to VIABLE_CANDIDATE and READ_ONLY) and verify callers referencing those tuple positions (and the GROUPREP_SERVER_STATE enum constants) continue to treat them as int.test/deps/cluster_simulator/lib/grouprep_utils.cpp-659-681 (1)
659-681:⚠️ Potential issue | 🟠 Major
-1sentinel is dead code forviable_candidate/read_only(stored asbool).Per the header (L21), those slots are
bool, sostd::get<2>(st)/std::get<3>(st)yield0or1; the!= -1guard is always true and the intended "field unspecified" semantics do not work here. Same pattern is duplicated ingrouprep_update_stateat L733-755. Consider either widening those tuple elements toint(matching how Galera does it with-1sentinels) or dropping the sentinel check for boolean fields and using a dedicated "present" mask.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/grouprep_utils.cpp` around lines 659 - 681, The boolean tuple fields st1_viable_candidate/std::get<2>(st) and st1_read_only/std::get<3>(st) currently use a -1 sentinel check that never triggers; fix by either (A) changing those tuple element types from bool to int everywhere they are defined/constructed so the -1 sentinel semantics work (update the tuple typedef/constructor sites and all uses including grouprep_update_state), or (B) remove the redundant "!= -1" guards and implement a proper presence indicator (e.g., add a dedicated bitmask or optional flags field to the tuple/struct and check that instead before pushing "read_only" / "viable_candidate"); apply the same fix in the duplicate logic at grouprep_update_state so the presence semantics are correct and consistent.test/deps/cluster_simulator/lib/grouprep_utils.cpp-721-755 (1)
721-755:⚠️ Potential issue | 🟠 Major
grouprep_update_statereturns a tuple with identity fields left default.
resultis default-constructed and only changed fields are written, so the returned tuple loseshostname,port, andmembers— every updated server ingrouprep_update_cluster_stateends up with empty hostname and port = 0. Callers relying on this vector to identify which server changed will see all entries collapsed onto the same “empty” identity. Start fromst2(orst1) and then overwrite only the diffed fields.♻️ Suggested shape
- grouprep_server_state result {}; + grouprep_server_state result { st2 }; // carry identity fields (hostname/port/members)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/grouprep_utils.cpp` around lines 721 - 755, grouprep_update_state currently default-constructs result so hostname/port/members are lost; change it to initialize result from st2 (or st1) and then apply the conditional overwrites for viable_candidate, read_only, and transactions_behind so identity fields are preserved; update the function grouprep_update_state to return a copy of st2 with only the differing fields replaced (this will also fix grouprep_update_cluster_state consumers that expect host/port/members to remain intact).
🧹 Nitpick comments (11)
test/deps/cluster_simulator/cluster_simulator.cpp (2)
35-43: Empty@brief TODOdoc comments.The Doxygen blocks for
operation_mode(line 35) andsimulator_options(line 42) are placeholders (@brief TODO). Please fill them in or drop the comment — they currently add noise without information.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/cluster_simulator.cpp` around lines 35 - 43, Replace the placeholder Doxygen brief comments for the enum operation_mode and the struct/class simulator_options by either removing the empty `@brief` blocks or, preferably, replacing them with concise descriptions: for operation_mode document the two modes (simulate — run simulation; verify — run verification checks) and for simulator_options summarize what configuration it holds (e.g., simulation parameters, flags, timeouts). Update the Doxygen comments directly above the operation_mode declaration and the simulator_options definition to reflect these meaningful summaries.
589-592: Replaceusleepwith portable sleep alternatives.
sleep_delayis computed as..._interval_s + size*timeout_s*0.1 + 1, so it's always ≥ 1 second, meaning theusleepargument is always ≥ 10⁶ microseconds. While POSIX specifies EINVAL as a valid return value for useconds ≥ 1,000,000, glibc and musl handle it by internally converting tonanosleep, making this pattern work on Linux but non-portable. Usestd::this_thread::sleep_for(std::chrono::duration<double>(sleep_delay))ornanosleepinstead. Same pattern at lines 635, 997, 1044, 1373, 1416, 1760, 1809.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/cluster_simulator.cpp` around lines 589 - 592, The code uses usleep(sleep_delay * pow(10, 6)) where sleep_delay (double) is computed (e.g., readonly_interval_s + mysql_servers.size()*readonly_timeout_s*0.1 + 1), which is non-portable; replace each such usleep call (the one shown and the similar occurrences at the other listed sites) with std::this_thread::sleep_for(std::chrono::duration<double>(sleep_delay)) and add the required headers (<thread> and <chrono>) to the file; search for the usleep usages in cluster_simulator.cpp (the sleep_delay variable and the usleep(...) call) and change them all to the std::this_thread::sleep_for variant for portability.test/deps/cluster_simulator/lib/aurora_utils.cpp (1)
395-405: Unconditional 1-secondusleepinside a helper called on every state transition.
usleep(1000 * 1000)between the cleanup and the transactional insert is a hard-coded sleep that runs unconditionally on every call toprepare_aurora_cluster_state. Across 10 aurora scenarios this adds ~10s of wall time per run, and the rationale isn't commented. If this is waiting for ProxySQL's monitor loop to observe the cleanup before the new inserts, it should be a named constant tied to the monitor interval (or, better, poll until the state is observable) rather than a magic sleep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/aurora_utils.cpp` around lines 395 - 405, The unconditional usleep(1000 * 1000) in prepare_aurora_cluster_state causes a fixed 1s delay on every state transition; replace it with a bounded wait that either polls until ProxySQL observes the cleanup or uses a named constant tied to the monitor interval. Specifically, remove the hard sleep and implement a short loop (with timeout) that checks the observable condition (or a helper that queries proxysql_sqlite / monitor state) before proceeding to the transactional mysql_query(proxysql_sqlite, "BEGIN IMMEDIATE"); introduce a descriptive constant like PROXYSQL_MONITOR_POLL_MS or MAX_WAIT_FOR_MONITOR_MS and use it for polling/backoff and timeout to avoid indefinite hangs.test/deps/cluster_simulator/tests/aurora_tests_payloads/multiple_clusters-failover_replica_lag.json (1)
92-100: Known-issue note is helpful; consider linking an issue tracker.The
__proxysql_final_state_details__documents an accepted race betweenaws_aurora_replication_lag_actionandupdate_aws_aurora_set_writeracross clusters. If a GitHub issue or ticket tracks the potential fix ("regeneratemyhgm.mysql_serversafter a replication_lag action"), consider referencing its ID here so future readers can follow up without grepping the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/tests/aurora_tests_payloads/multiple_clusters-failover_replica_lag.json` around lines 92 - 100, Update the documented known-issue entry under "__proxysql_final_state_details__" to include a direct reference to the tracking ticket/issue so readers can follow progress; specifically append one more array element with the issue ID or URL (for example "Tracked in GH issue `#1234`: <url>" or a JIRA ticket) and mention the related symbols "aws_aurora_replication_lag_action", "update_aws_aurora_set_writer", and the proposed fix "regenerate myhgm.mysql_servers after a replication_lag action" so the reference is discoverable.test/deps/cluster_simulator/docs/payloads_creation/README.md (1)
34-35: Preposition nit."Moved in
reader_hostgroup" reads awkwardly; consider "Moved toreader_hostgroup" (applies to lines 34-35 and the mirrored text at line 122).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/docs/payloads_creation/README.md` around lines 34 - 35, Replace the awkward phrase "Moved in `reader_hostgroup`" with "Moved to `reader_hostgroup`" wherever it appears in the README payload examples (specifically the lines describing `writer_hostgroup` detections showing `RO=1` with `writer_is_also_reader=0` and `writer_is_also_reader=1` and the mirrored occurrence later in the file); update the two instances of the exact string "Moved in `reader_hostgroup`" to "Moved to `reader_hostgroup`" to fix the preposition and keep wording consistent.test/deps/cluster_simulator/lib/common_utils.cpp (1)
56-88: Redundantmysql_fetch_fieldscall.Line 62 calls
mysql_fetch_fields(result)and discards the return value; Line 64 calls it again and keeps it. Drop the first call.- mysql_fetch_fields(result); int num_fields = mysql_num_fields(result); MYSQL_FIELD* fields = mysql_fetch_fields(result);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 56 - 88, In parse_result_to_json(MYSQL_RES *result, ordered_json& j) remove the redundant call to mysql_fetch_fields(result) at the earlier location so only the single call that assigns MYSQL_FIELD* fields = mysql_fetch_fields(result); remains; ensure num_fields = mysql_num_fields(result) is still computed before iterating and that field_names is populated from fields as currently done.test/deps/cluster_simulator/lib/replicationlag_utils.cpp (1)
392-405:replicationlag_state_members_diffsignature mismatch vs..cppstorage type.The header declares
replicationlag_server_stateasstd::tuple<hostname, port, std::shared_ptr<int>>, andstd::get<2>here is accessed asstd::shared_ptr<int>. However,extract_replicationlag_servers_state(Lines 105, 117, 151) stores values into astd::unique_ptr<int>local then moves it intostd::make_tuple, relying onunique_ptr→shared_ptrimplicit conversion at tuple construction — this works but is confusing and will silently stop working if someone changes the header to useunique_ptr(or vice versa). Consider usingstd::make_shared<int>(...)directly to match the stored type exactly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/replicationlag_utils.cpp` around lines 392 - 405, The comparison function accesses std::get<2> as std::shared_ptr<int> but elsewhere (extract_replicationlag_servers_state) you create a std::unique_ptr<int> and rely on implicit conversion when building the tuple; change those factories to create std::shared_ptr<int> directly (use std::make_shared<int>(value)) when constructing the replicationlag_server_state tuple so the stored type exactly matches the header and replicationlag_state_members_diff's use of std::shared_ptr<int>, avoiding the fragile unique_ptr→shared_ptr conversion.test/deps/cluster_simulator/lib/galera_utils.h (1)
40-53: Consider a named struct or enum for the 11-fieldgalera_server_statetuple.An 11-element heterogeneous tuple with repeated
intfields (positions 0, 2, 3, 4, 5, 6, 8) is easy to index wrong — e.g.,std::get<4>could mean any of several similarly typed members. Either use anenum-based accessor likeMYSQL_SERVER_STATUS_Tincommon_utils.h, or switch to a named struct as done foraurora_hostgroup_config_t. This prevents silent field-position bugs in the simulator code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/galera_utils.h` around lines 40 - 53, The 11-element heterogeneous tuple alias galera_server_state is error-prone due to repeated int positions; replace it with a named struct (e.g., struct galera_server_state_t) containing clearly named fields for each tuple element (or define an enum accessor similar to MYSQL_SERVER_STATUS_T) and update uses of galera_server_state to access members by name (or via enum indices) instead of std::get<N>; reference the existing galera_server_state alias, MYSQL_SERVER_STATUS_T in common_utils.h, and aurora_hostgroup_config_t as examples for naming and usage.test/deps/cluster_simulator/lib/aurora_utils.h (1)
39-44: Prefer scoped enum or struct-wrapped enum to avoid name collisions.
AURORA_SERVER_STATEis an unscoped enum with very generic identifiers (SERVER_ID,DOMAIN_NAME,SESSION_ID,REPLICA_LAG_IN_MILLISECONDS) that leak into the enclosing namespace.SERVER_IDin particular is a common name and is likely to collide with existing macros/identifiers elsewhere in the ProxySQL tree as more code is linked. Use the struct-wrapped pattern already adopted incommon_utils.h(MYSQL_SERVER_STATUS_T,HOSTGROUP_ATTRIBUTES_T) or anenum classfor consistency.-enum AURORA_SERVER_STATE { - SERVER_ID, - DOMAIN_NAME, - SESSION_ID, - REPLICA_LAG_IN_MILLISECONDS -}; +struct AURORA_SERVER_STATE_T { + enum { + SERVER_ID, + DOMAIN_NAME, + SESSION_ID, + REPLICA_LAG_IN_MILLISECONDS + }; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/aurora_utils.h` around lines 39 - 44, AURORA_SERVER_STATE is an unscoped enum exposing generic names (SERVER_ID, DOMAIN_NAME, SESSION_ID, REPLICA_LAG_IN_MILLISECONDS) into the global namespace; change it to a scoped enum or the project’s struct-wrapped pattern (e.g., wrap in a struct like AURORA_SERVER_STATE_T or use enum class AURORA_SERVER_STATE) and update all uses to qualify the enumerators (e.g., AURORA_SERVER_STATE_T::SERVER_ID or AURORA_SERVER_STATE::SERVER_ID) to avoid symbol collisions with existing identifiers/macros.test/deps/cluster_simulator/lib/readonly_utils.cpp (1)
420-457:prepare_readonly_cluster_state: unconditional 1-second sleep on every call.
usleep(1000 * 1000);(Line 437) runs for every invocation even whencleanup == falseand there were no prior DDL/runtime loads to settle. Across the ~9 readonly test payloads this adds serial wall-clock time with no corresponding barrier. Consider gating it behindif (cleanup)or removing it if the caller already synchronizes via ProxySQL runtime reloads. Same observation applies toprepare_replicationlag_cluster_state(Line 261).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/readonly_utils.cpp` around lines 420 - 457, The unconditional usleep call inside prepare_readonly_cluster_state causes an unnecessary 1s delay on every invocation; change the behavior so the usleep(1000 * 1000) is only executed when cleanup is true (i.e. gate the sleep behind if (cleanup)) or remove it entirely if the caller already ensures synchronization, and apply the same fix to prepare_replicationlag_cluster_state to avoid the serial delay on every call; locate the usleep invocation and the cleanup boolean in those functions to make the modification.test/deps/cluster_simulator/lib/grouprep_utils.h (1)
42-51: UsesMYSQL*without explicitly including<mysql.h>.The declarations at L42, L47, L63, L65 take
MYSQL*but the header only includes<string>,<vector>,<tuple>,<utility>,json.hpp, andcommon_utils.h. This compiles today only becausecommon_utils.hhappens to pull<mysql.h>in transitively — any reorg there would break this header. Add a direct include for robustness.♻️ Suggested include
`#include` <utility> +#include <mysql.h> + `#include` "json.hpp" `#include` "common_utils.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/grouprep_utils.h` around lines 42 - 51, The header uses the MYSQL type in declarations (e.g., prepare_mysql_group_replication_hostgroups and prepare_grouprep_cluster_state) but does not directly include <mysql.h>, relying on a transitive include from common_utils.h; add a direct `#include` <mysql.h> at the top of test/deps/cluster_simulator/lib/grouprep_utils.h alongside the existing includes so MYSQL is defined regardless of include reorganization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: acd37cb6-e8e4-4302-b9f9-d86e3fceb558
📒 Files selected for processing (115)
.gitignoreMakefiletest/deps/cluster_simulator/Makefiletest/deps/cluster_simulator/README.mdtest/deps/cluster_simulator/cluster_simulator.cpptest/deps/cluster_simulator/configs/aurora_sim_conf.cnftest/deps/cluster_simulator/configs/galera_sim_conf.cnftest/deps/cluster_simulator/configs/grouprep_sim_conf.cnftest/deps/cluster_simulator/configs/readonly_sim_conf.cnftest/deps/cluster_simulator/configs/repl_sim_conf.cnftest/deps/cluster_simulator/configs/repl_sim_conf.sqltest/deps/cluster_simulator/constantstest/deps/cluster_simulator/docs/example_payloads/simple_test_err_payload.jsontest/deps/cluster_simulator/docs/example_payloads/simple_test_payload.jsontest/deps/cluster_simulator/docs/payloads_creation/README.mdtest/deps/cluster_simulator/docs/payloads_processing/README.mdtest/deps/cluster_simulator/docs/payloads_processing/examples/read_only_test_multiple_servers_multiple_hostgroups_incomplete.jsontest/deps/cluster_simulator/docs/payloads_processing/examples/read_only_test_multiple_servers_multiple_hostgroups_invalid.jsontest/deps/cluster_simulator/docs/payloads_processing/jq_filters/add_servers_comment.jqtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/add_servers_comment.mdtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/all_init_state_readers.jqtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/all_init_state_readers.mdtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/all_init_state_writers.jqtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/hostgroups_by_type_by_hostname.jqtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/hostgroups_by_type_by_hostname.mdtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/init_state_rm_invalid_readers.jqtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/init_state_rm_invalid_readers.mdtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/init_state_writers_only.jqtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/init_state_writers_only.mdtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/mv_readers_to_writers.jqtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/mv_readers_to_writers.mdtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/servers_in_reader_hostgroups.jqtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/servers_in_reader_hostgroups.mdtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/servers_in_writer_hostgroups.jqtest/deps/cluster_simulator/docs/payloads_processing/jq_filters/servers_in_writer_hostgroups.mdtest/deps/cluster_simulator/docs/payloads_processing/node/payload.jstest/deps/cluster_simulator/lib/aurora_utils.cpptest/deps/cluster_simulator/lib/aurora_utils.htest/deps/cluster_simulator/lib/common_utils.cpptest/deps/cluster_simulator/lib/common_utils.htest/deps/cluster_simulator/lib/galera_utils.cpptest/deps/cluster_simulator/lib/galera_utils.htest/deps/cluster_simulator/lib/grouprep_utils.cpptest/deps/cluster_simulator/lib/grouprep_utils.htest/deps/cluster_simulator/lib/readonly_utils.cpptest/deps/cluster_simulator/lib/readonly_utils.htest/deps/cluster_simulator/lib/replicationlag_utils.cpptest/deps/cluster_simulator/lib/replicationlag_utils.htest/deps/cluster_simulator/tests/aurora_tests_payloads/multiple_clusters-autodiscovery.jsontest/deps/cluster_simulator/tests/aurora_tests_payloads/multiple_clusters-duplicated_servers.jsontest/deps/cluster_simulator/tests/aurora_tests_payloads/multiple_clusters-failover.jsontest/deps/cluster_simulator/tests/aurora_tests_payloads/multiple_clusters-failover_replica_lag.jsontest/deps/cluster_simulator/tests/aurora_tests_payloads/multiple_clusters-replica_lag.jsontest/deps/cluster_simulator/tests/aurora_tests_payloads/three_nodes_cluster-autodiscovery.jsontest/deps/cluster_simulator/tests/aurora_tests_payloads/three_nodes_cluster-failover_only.jsontest/deps/cluster_simulator/tests/aurora_tests_payloads/three_nodes_cluster-failover_replica_lag.jsontest/deps/cluster_simulator/tests/aurora_tests_payloads/three_nodes_cluster-new_reader_weight.jsontest/deps/cluster_simulator/tests/aurora_tests_payloads/three_nodes_cluster-replica_lag.jsontest/deps/cluster_simulator/tests/galera_tests_payloads/test_template.jsontest/deps/cluster_simulator/tests/galera_tests_payloads/three_node_galera-SST.jsontest/deps/cluster_simulator/tests/galera_tests_payloads/three_node_galera-no_valid_nodes.jsontest/deps/cluster_simulator/tests/galera_tests_payloads/three_node_galera-wsrep_desync.jsontest/deps/cluster_simulator/tests/galera_tests_payloads/three_node_galera-wsrep_reject_queries.jsontest/deps/cluster_simulator/tests/grouprep_test_payloads_shunned_preservation/README.mdtest/deps/cluster_simulator/tests/grouprep_test_payloads_shunned_preservation/five_node_multimaster-offline_soft.jsontest/deps/cluster_simulator/tests/grouprep_test_payloads_shunned_preservation/five_node_multimaster-unshunning.jsontest/deps/cluster_simulator/tests/grouprep_test_payloads_shunned_preservation/five_node_multimaster.jsontest/deps/cluster_simulator/tests/grouprep_tests_payloads/five_node_multimaster-extra_hostgroup.jsontest/deps/cluster_simulator/tests/grouprep_tests_payloads/five_node_multimaster-one_primary_shunning.jsontest/deps/cluster_simulator/tests/grouprep_tests_payloads/five_node_multimaster-shunned_promotion.jsontest/deps/cluster_simulator/tests/grouprep_tests_payloads/five_node_multimaster-two_primary_shunning.jsontest/deps/cluster_simulator/tests/grouprep_tests_payloads/grouprep_test_template.jsontest/deps/cluster_simulator/tests/grouprep_tests_payloads/single_primary_autodiscovery.jsontest/deps/cluster_simulator/tests/grouprep_tests_payloads/three_node_grouprep-extra_hostgroup.jsontest/deps/cluster_simulator/tests/grouprep_tests_payloads/three_node_grouprep-offline_soft.jsontest/deps/cluster_simulator/tests/grouprep_tests_payloads/three_node_grouprep-replication_lag.jsontest/deps/cluster_simulator/tests/readonly_tests_payloads/read_only_test_multiple_hostgroups.jsontest/deps/cluster_simulator/tests/readonly_tests_payloads/read_only_test_multiple_hostgroups_writer_only.jsontest/deps/cluster_simulator/tests/readonly_tests_payloads/read_only_test_multiple_servers_multiple_hostgroups.jsontest/deps/cluster_simulator/tests/readonly_tests_payloads/read_only_test_multiple_servers_single_hostgroup.jsontest/deps/cluster_simulator/tests/readonly_tests_payloads/read_only_test_single_server_multiple_hostgroups.jsontest/deps/cluster_simulator/tests/readonly_tests_payloads/read_only_test_single_server_single_hostgroups.jsontest/deps/cluster_simulator/tests/readonly_tests_payloads/read_only_test_single_server_two_hostgroups.jsontest/deps/cluster_simulator/tests/readonly_tests_payloads/read_only_test_two_server_single_hostgroup.jsontest/deps/cluster_simulator/tests/readonly_tests_payloads/read_only_test_two_server_two_hostgroups.jsontest/deps/cluster_simulator/tests/repl_tests_payloads/replication_lag_test.jsontest/deps/cluster_simulator/tests/repl_tests_payloads/replication_lag_test_by_host.jsontest/deps/cluster_simulator/tests/repl_tests_payloads/replication_lag_test_template.jsontest/infra/README.mdtest/infra/control/destroy-infras.bashtest/infra/control/ensure-infras.bashtest/infra/control/start-proxysql-isolated.bashtest/tap/groups/cluster_sim_aurora/add-hoststest/tap/groups/cluster_sim_aurora/env.shtest/tap/groups/cluster_sim_aurora/pre-proxysql.bashtest/tap/groups/cluster_sim_aurora/pre-proxysql.sqltest/tap/groups/cluster_sim_galera/env.shtest/tap/groups/cluster_sim_galera/pre-proxysql.bashtest/tap/groups/cluster_sim_galera/pre-proxysql.sqltest/tap/groups/cluster_sim_group_repl/env.shtest/tap/groups/cluster_sim_group_repl/pre-proxysql.bashtest/tap/groups/cluster_sim_group_repl/pre-proxysql.sqltest/tap/groups/cluster_sim_read_only/env.shtest/tap/groups/cluster_sim_read_only/pre-proxysql.bashtest/tap/groups/cluster_sim_read_only/pre-proxysql.sqltest/tap/groups/cluster_sim_repl_lag/env.shtest/tap/groups/cluster_sim_repl_lag/pre-proxysql.bashtest/tap/groups/cluster_sim_repl_lag/pre-proxysql.sqltest/tap/groups/groups.jsontest/tap/tests/cluster_sim_runner.htest/tap/tests/test_cluster_sim_aurora-t.cpptest/tap/tests/test_cluster_sim_galera-t.cpptest/tap/tests/test_cluster_sim_group_repl-t.cpptest/tap/tests/test_cluster_sim_read_only-t.cpptest/tap/tests/test_cluster_sim_repl_lag-t.cpp
| string comment {}; | ||
| json j_comment {}; | ||
|
|
||
| // Default values for optional fields | ||
| int32_t min_lag_ms = 30; | ||
| int32_t add_lag_ms = 30; | ||
|
|
||
| try { | ||
| writer_hostgroup = j_aurora_hg.at("writer_hostgroup"); | ||
| reader_hostgroup = j_aurora_hg.at("reader_hostgroup"); | ||
| active = j_aurora_hg.at("active"); | ||
| domain_name = j_aurora_hg.at("domain_name"); | ||
| max_lag_ms = j_aurora_hg.at("max_lag_ms"); | ||
| writer_is_also_reader = j_aurora_hg.at("writer_is_also_reader"); | ||
| new_reader_weight = j_aurora_hg.at("new_reader_weight"); | ||
|
|
||
| // Optional fields | ||
| if (j_aurora_hg.find("add_lag_ms") != j_aurora_hg.end()) { | ||
| add_lag_ms = j_aurora_hg.at("add_lag_ms"); | ||
| } | ||
| if (j_aurora_hg.find("min_lag_ms") != j_aurora_hg.end()) { | ||
| min_lag_ms = j_aurora_hg.at("min_lag_ms"); | ||
| } | ||
| } catch (const std::exception& e) { | ||
| // TODO: Improve error message here | ||
| return { EXIT_FAILURE, e.what() }; | ||
| } | ||
|
|
||
| if (j_comment == nullptr) { | ||
| comment = "NULL"; | ||
| } else { | ||
| try { | ||
| comment = j_aurora_hg.at("comment"); | ||
| } catch (const std::exception& e) { | ||
| // TODO: Improve error message here | ||
| return { EXIT_FAILURE, e.what() }; | ||
| } | ||
| } |
There was a problem hiding this comment.
j_comment is never assigned — the comment field is always set to the literal string "NULL".
j_comment is default-constructed to a null JSON value on line 105 and nothing ever assigns to it before the check on line 132, so j_comment == nullptr is unconditionally true and the else branch (which would actually read j_aurora_hg.at("comment")) is dead code. As a result every inserted row gets comment = 'NULL' (a string literal), regardless of what the payload provides.
Note this is further compounded by t_aurora_hostgroup_insert formatting comment with single quotes (line 53: '%s'), so even an intentional SQL NULL cannot be produced via this path.
🛠️ Suggested fix
int new_reader_weight = 0;
string comment {};
- json j_comment {};
+ json j_comment {};
// Default values for optional fields
int32_t min_lag_ms = 30;
int32_t add_lag_ms = 30;
try {
writer_hostgroup = j_aurora_hg.at("writer_hostgroup");
reader_hostgroup = j_aurora_hg.at("reader_hostgroup");
active = j_aurora_hg.at("active");
domain_name = j_aurora_hg.at("domain_name");
max_lag_ms = j_aurora_hg.at("max_lag_ms");
writer_is_also_reader = j_aurora_hg.at("writer_is_also_reader");
new_reader_weight = j_aurora_hg.at("new_reader_weight");
+
+ if (j_aurora_hg.find("comment") != j_aurora_hg.end()) {
+ j_comment = j_aurora_hg.at("comment");
+ }
// Optional fields
if (j_aurora_hg.find("add_lag_ms") != j_aurora_hg.end()) {
add_lag_ms = j_aurora_hg.at("add_lag_ms");
}
if (j_aurora_hg.find("min_lag_ms") != j_aurora_hg.end()) {
min_lag_ms = j_aurora_hg.at("min_lag_ms");
}
} catch (const std::exception& e) {
// TODO: Improve error message here
return { EXIT_FAILURE, e.what() };
}
- if (j_comment == nullptr) {
+ if (j_comment.is_null()) {
comment = "NULL";
} else {
- try {
- comment = j_aurora_hg.at("comment");
- } catch (const std::exception& e) {
- // TODO: Improve error message here
- return { EXIT_FAILURE, e.what() };
- }
+ comment = j_comment.get<std::string>();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/deps/cluster_simulator/lib/aurora_utils.cpp` around lines 104 - 141, The
code never assigns j_comment from j_aurora_hg so the check "if (j_comment ==
nullptr)" is always true and comment is always the literal "NULL"; fix by
reading the "comment" field from j_aurora_hg (use j_aurora_hg.find("comment") or
j_aurora_hg.contains("comment") to avoid exceptions) into j_comment before the
null check, then set comment to "NULL" if j_comment.is_null() or set comment =
j_comment.get<string>() otherwise; also be aware t_aurora_hostgroup_insert
formats comment with '%s' (single quotes) so if you want an actual SQL NULL
handle that separately (e.g., leave an empty indicator or adjust the insert
formatting) to avoid inserting the string 'NULL'.
| } else { | ||
| try { | ||
| const auto& m_server_id = server_state.at("SERVER_ID"); | ||
| const auto& m_session_id = server_state.at("SESSION_ID"); | ||
| const auto& m_replica_lag_in_ms = server_state.at("REPLICA_LAG_IN_MILLISECONDS"); | ||
|
|
||
| if (m_server_id == nullptr) { | ||
| server_id = "nullptr"; | ||
| } else { | ||
| try { | ||
| server_id = server_state["SERVER_ID"]; | ||
| } catch (const std::exception& e) { | ||
| return { EXIT_FAILURE, e.what() }; | ||
| } | ||
| } | ||
| if (m_session_id == nullptr) { | ||
| session_id = true; | ||
| } else { | ||
| try { | ||
| session_id = server_state["SESSION_ID"]; | ||
| } catch (const std::exception& e) { | ||
| return { EXIT_FAILURE, e.what() }; | ||
| } | ||
| } | ||
| if (m_replica_lag_in_ms == nullptr) { | ||
| replica_lag_in_ms = -1; | ||
| } else { | ||
| try { | ||
| replica_lag_in_ms = server_state["REPLICA_LAG_IN_MILLISECONDS"]; | ||
| } catch (const std::exception& e) { | ||
| return { EXIT_FAILURE, e.what() }; | ||
| } | ||
| } | ||
| } catch (const std::exception& e) { | ||
| // TODO: Improve | ||
| return { EXIT_FAILURE, e.what() }; | ||
| } | ||
| } |
There was a problem hiding this comment.
session_id = true; assigns a bool to a std::string — almost certainly a typo.
On line 266, when SESSION_ID is JSON null in the new_state branch, the code assigns the bool literal true to string session_id. Through bool → int → char this compiles silently and sets session_id to a one-byte string containing \x01, which is then used as the session identity in diffs/updates and the downstream REPLICA_HOST_STATUS INSERT. This is inconsistent with the adjacent handling of m_server_id == nullptr (which sets "nullptr") and of m_replica_lag_in_ms == nullptr (which sets -1 as sentinel).
Given the downstream aurora_update_state/aurora_state_members_diff treat "" as the "ignore" sentinel for SESSION_ID (line 496, 560), the intended value here is very likely "" (or a named sentinel).
🛠️ Suggested fix
if (m_session_id == nullptr) {
- session_id = true;
+ session_id = "";
} else {📝 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.
| } else { | |
| try { | |
| const auto& m_server_id = server_state.at("SERVER_ID"); | |
| const auto& m_session_id = server_state.at("SESSION_ID"); | |
| const auto& m_replica_lag_in_ms = server_state.at("REPLICA_LAG_IN_MILLISECONDS"); | |
| if (m_server_id == nullptr) { | |
| server_id = "nullptr"; | |
| } else { | |
| try { | |
| server_id = server_state["SERVER_ID"]; | |
| } catch (const std::exception& e) { | |
| return { EXIT_FAILURE, e.what() }; | |
| } | |
| } | |
| if (m_session_id == nullptr) { | |
| session_id = true; | |
| } else { | |
| try { | |
| session_id = server_state["SESSION_ID"]; | |
| } catch (const std::exception& e) { | |
| return { EXIT_FAILURE, e.what() }; | |
| } | |
| } | |
| if (m_replica_lag_in_ms == nullptr) { | |
| replica_lag_in_ms = -1; | |
| } else { | |
| try { | |
| replica_lag_in_ms = server_state["REPLICA_LAG_IN_MILLISECONDS"]; | |
| } catch (const std::exception& e) { | |
| return { EXIT_FAILURE, e.what() }; | |
| } | |
| } | |
| } catch (const std::exception& e) { | |
| // TODO: Improve | |
| return { EXIT_FAILURE, e.what() }; | |
| } | |
| } | |
| } else { | |
| try { | |
| const auto& m_server_id = server_state.at("SERVER_ID"); | |
| const auto& m_session_id = server_state.at("SESSION_ID"); | |
| const auto& m_replica_lag_in_ms = server_state.at("REPLICA_LAG_IN_MILLISECONDS"); | |
| if (m_server_id == nullptr) { | |
| server_id = "nullptr"; | |
| } else { | |
| try { | |
| server_id = server_state["SERVER_ID"]; | |
| } catch (const std::exception& e) { | |
| return { EXIT_FAILURE, e.what() }; | |
| } | |
| } | |
| if (m_session_id == nullptr) { | |
| session_id = ""; | |
| } else { | |
| try { | |
| session_id = server_state["SESSION_ID"]; | |
| } catch (const std::exception& e) { | |
| return { EXIT_FAILURE, e.what() }; | |
| } | |
| } | |
| if (m_replica_lag_in_ms == nullptr) { | |
| replica_lag_in_ms = -1; | |
| } else { | |
| try { | |
| replica_lag_in_ms = server_state["REPLICA_LAG_IN_MILLISECONDS"]; | |
| } catch (const std::exception& e) { | |
| return { EXIT_FAILURE, e.what() }; | |
| } | |
| } | |
| } catch (const std::exception& e) { | |
| // TODO: Improve | |
| return { EXIT_FAILURE, e.what() }; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/deps/cluster_simulator/lib/aurora_utils.cpp` around lines 250 - 287, The
code incorrectly assigns the boolean literal true to the std::string variable
session_id when m_session_id == nullptr; change that sentinel assignment to an
empty string (""), matching the intended "ignore" sentinel used by
aurora_update_state / aurora_state_members_diff; update the branch in the try
block that checks m_session_id to set session_id = "" (and ensure the
surrounding logic that reads server_state["SESSION_ID"] remains unchanged) so
SESSION_ID nulls are handled consistently with m_server_id and
m_replica_lag_in_ms handling.
| if (state == cluster_state::init_state) { | ||
| json_key = "proxysql_init_state"; | ||
| } else { | ||
| json_key = "proxysql_final_state"; | ||
| } | ||
|
|
||
| bool has_proxysql_init_state = | ||
| check_present_and_type( | ||
| galera_test_def, { json_key }, json::value_t::object | ||
| ); | ||
|
|
||
| if (has_proxysql_init_state) { | ||
| std::string t_err_msg { | ||
| "Invalid input. Unable to find required field '%s'" | ||
| }; | ||
| std::string err_msg {}; | ||
| string_format(t_err_msg, err_msg, json_key.c_str()); | ||
|
|
||
| return { | ||
| EXIT_FAILURE, | ||
| err_msg | ||
| }; | ||
| } | ||
|
|
||
| ordered_json m_proxysql_init_state = galera_test_def[json_key]; | ||
| if (!m_proxysql_init_state.is_array()) { | ||
| std::string t_err_msg { | ||
| "Invalid input. '%s' isn't of expected type 'array'" | ||
| }; | ||
| std::string err_msg {}; | ||
| string_format(t_err_msg, err_msg, json_key.c_str()); | ||
|
|
||
| return { | ||
| EXIT_FAILURE, | ||
| err_msg | ||
| }; | ||
| } |
There was a problem hiding this comment.
Critical: extract_cluster_status has inverted presence check and wrong expected type.
Two bugs in the payload validation of proxysql_init_state / proxysql_final_state:
- Line 943 asks
check_present_and_type(..., json::value_t::object), but the payload is an array (confirmed byis_array()on Line 960 and the downstream for-loop treating entries as objects). - Line 946 branches on
if (has_proxysql_init_state)and returns "Unable to find required field" — i.e., the function errors out precisely when the field IS present and of the checked type, and falls through when it is missing.
Net effect: when a valid array is provided, the first check is false (array ≠ object) so execution continues "by luck"; if the field is missing, execution also continues and then hits galera_test_def[json_key] returning null, producing a misleading is_array failure. A test payload with an object there would be wrongly rejected with a "field not found" message.
🐛 Proposed fix
- bool has_proxysql_init_state =
- check_present_and_type(
- galera_test_def, { json_key }, json::value_t::object
- );
-
- if (has_proxysql_init_state) {
+ bool has_proxysql_init_state =
+ check_present_and_type(
+ galera_test_def, { json_key }, json::value_t::array
+ );
+
+ if (!has_proxysql_init_state) {
std::string t_err_msg {
"Invalid input. Unable to find required field '%s'"
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 935 - 971, The
presence/type check for proxysql_init_state/proxysql_final_state is inverted and
uses the wrong expected type: change the call to
check_present_and_type(galera_test_def, { json_key }, json::value_t::array) and
invert the conditional to if (!has_proxysql_init_state) to return the "Unable to
find required field" error; keep the subsequent retrieval into ordered_json
m_proxysql_init_state and the is_array() guard (or remove redundant is_array()
after the corrected check) so valid array payloads pass and missing/non-array
payloads produce the correct error messages; references: json_key,
check_present_and_type, has_proxysql_init_state, m_proxysql_init_state,
extract_cluster_status.
| const std::string t_galera_hosgroup_insert { | ||
| "INSERT INTO mysql_galera_hostgroups (" | ||
| " writer_hostgroup," | ||
| " backup_writer_hostgroup," | ||
| " reader_hostgroup," | ||
| " offline_hostgroup," | ||
| " active," | ||
| " max_writers," | ||
| " writer_is_also_reader," | ||
| " max_transactions_behind," | ||
| " comment" | ||
| ") VALUES (" | ||
| " %d, %d, %d, %d, %d, %d, %d, %d, %s" | ||
| ")" | ||
| }; | ||
|
|
||
| std::pair<int, std::string> prepare_mysql_galera_hostgroups( | ||
| MYSQL* proxysql_admin, | ||
| const std::vector<galera_hostgroup_config>& hostgroups_configs | ||
| ) { | ||
| int query_error = 0; | ||
|
|
||
| const std::string hostgroups_cleanup { "DELETE FROM mysql_galera_hostgroups" }; | ||
| query_error = mysql_query(proxysql_admin, hostgroups_cleanup.c_str()); | ||
| if (query_error) { return create_query_error(proxysql_admin, hostgroups_cleanup, __FILE__, __LINE__); } | ||
|
|
||
| for (const auto& hostgroup_config : hostgroups_configs) { | ||
| std::string galera_hostgroup_insert {}; | ||
|
|
||
| // NOTE: Comment can't be null, no need of special handling | ||
| string_format( | ||
| t_galera_hosgroup_insert, | ||
| galera_hostgroup_insert, | ||
| std::get<0>(hostgroup_config), | ||
| std::get<1>(hostgroup_config), | ||
| std::get<2>(hostgroup_config), | ||
| std::get<3>(hostgroup_config), | ||
| std::get<4>(hostgroup_config), | ||
| std::get<5>(hostgroup_config), | ||
| std::get<6>(hostgroup_config), | ||
| std::get<7>(hostgroup_config), | ||
| std::get<8>(hostgroup_config).c_str() | ||
| ); | ||
|
|
||
| query_error = mysql_query(proxysql_admin, galera_hostgroup_insert.c_str()); | ||
| if (query_error) { return create_query_error(proxysql_admin, galera_hostgroup_insert, __FILE__, __LINE__); } |
There was a problem hiding this comment.
Comment placeholder %s is unquoted — INSERT breaks for any non-NULL comment.
t_galera_hosgroup_insert ends with ..., %d, %d, %d, %s) while extractor sets comment = "NULL" when the JSON value is null but otherwise preserves the raw string ("my comment"). Since the template does not wrap %s in single quotes, substituting a real comment produces ..., 0, 0, my comment) which is a syntax error. The grouprep analogue at L48 uses '%s' for the same reason.
Either quote the placeholder and keep storing literal "NULL" (loses the real NULL semantics), or branch on the null case and emit bare NULL vs '<comment>'.
🐛 Minimal fix that matches grouprep behavior
- " %d, %d, %d, %d, %d, %d, %d, %d, %s"
+ " %d, %d, %d, %d, %d, %d, %d, %d, '%s'"| if (m_read_only == nullptr) { | ||
| read_only = "nullptr"; | ||
| } else { | ||
| try { | ||
| read_only = | ||
| server_state["read_only"] == "YES" ? true : false; | ||
| } catch (const std::exception& e) { | ||
| return { EXIT_FAILURE, e.what() }; | ||
| } | ||
| } |
There was a problem hiding this comment.
read_only = "nullptr" assigns a string literal to a bool (always true).
read_only is declared bool at L312. The string literal decays to a non-null const char*, which converts to true. The null-branch effectively forces read_only=true regardless of intent. Either drop the null-handling (the tuple can’t represent a tri-state) or use a dedicated sentinel (e.g., change the tuple type).
🐛 Minimal fix
- if (m_read_only == nullptr) {
- read_only = "nullptr";
- } else {
+ if (m_read_only == nullptr) {
+ // TODO: tuple is bool; cannot represent NULL. Default to true (read_only) for now.
+ read_only = true;
+ } else {
try {
read_only =
server_state["read_only"] == "YES" ? true : false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/deps/cluster_simulator/lib/grouprep_utils.cpp` around lines 366 - 375,
The code assigns the string "nullptr" to the bool variable read_only (declared
at L312), which always converts to true; fix by removing the invalid string
assignment and handle the m_read_only == nullptr case consistently: either set
read_only to a sensible default (e.g., false) or bail out/return an error.
Update the block around m_read_only, replacing the branch that does read_only =
"nullptr" with a deterministic boolean assignment or an early return; ensure you
reference m_read_only, read_only and server_state in the change and remember the
surrounding tuple cannot represent a tri-state so do not attempt to assign a
non-bool sentinel.
| int q_res = mysql_query( | ||
| proxysql_admin, | ||
| "SELECT hostname, port, read_only FROM READONLY_STATUS" | ||
| ); | ||
|
|
||
| if (q_res == 0) { | ||
| ordered_json j_servers {}; | ||
| j_servers["cluster_type"] = "readonly"; | ||
| j_servers["readonly_servers_init_state"] = {}; | ||
|
|
||
| MYSQL_RES* my_servers_res = mysql_store_result(proxysql_admin); | ||
| parse_result_to_json(my_servers_res, j_servers["readonly_servers_init_state"]); | ||
|
|
||
| // convert the fields into the proper types | ||
| for (ordered_json& j_server : j_servers["readonly_servers_init_state"]) { | ||
| j_server["hostname"] = atoi(std::string {j_server["hostname"]}.c_str()); | ||
| j_server["port"] = atoi(std::string {j_server["port"]}.c_str()); | ||
| j_server["read_only"] = atoi(std::string {j_server["read_only"]}.c_str()); | ||
| j_server["comment"] = atoi(std::string {j_server["commetn"]}.c_str()); | ||
| } | ||
|
|
||
| std::vector<readonly_server_state> cur_readonly_servers_state {}; | ||
| std::pair<int, std::string> ext_res = | ||
| extract_readonly_servers_state(readonly_state_id::init_state, j_servers, cur_readonly_servers_state); | ||
|
|
||
| if (ext_res.first == EXIT_SUCCESS) { | ||
| out_cur_readonly_servers_state = cur_readonly_servers_state; | ||
| } else { | ||
| string_format(t_err_msg, err_msg, ext_res.second.c_str()); | ||
| err_res = { EXIT_FAILURE, err_msg }; | ||
| } | ||
| } else { | ||
| string_format(t_err_msg, err_msg, mysql_error(proxysql_admin)); | ||
| err_res = { EXIT_FAILURE, err_msg }; | ||
| } | ||
|
|
||
| return err_res; | ||
| } |
There was a problem hiding this comment.
Critical: get_current_readonly_servers_state will always throw.
Same class of bug as in replicationlag_utils.cpp:
- Line 352 applies
atoitohostname(overwrites the string with an int). - Line 355:
j_server["commetn"]— typo forcomment, and the SELECT at Line 339 only returnshostname, port, read_only, so this field doesn't exist. Accessing it withoperator[]insertsnulland thenstd::string{null}throws. Even if the typo were fixed, the column isn't in the query. extract_readonly_servers_statewould also reject acommentkey sincevalid_readonly_entriesdoesn't include it.
🐛 Proposed fix
for (ordered_json& j_server : j_servers["readonly_servers_init_state"]) {
- j_server["hostname"] = atoi(std::string {j_server["hostname"]}.c_str());
+ j_server["hostname"] = std::string { j_server["hostname"] };
j_server["port"] = atoi(std::string {j_server["port"]}.c_str());
j_server["read_only"] = atoi(std::string {j_server["read_only"]}.c_str());
- j_server["comment"] = atoi(std::string {j_server["commetn"]}.c_str());
}Also consider adding mysql_free_result(my_servers_res); for symmetry with other query helpers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/deps/cluster_simulator/lib/readonly_utils.cpp` around lines 337 - 374,
In get_current_readonly_servers_state: stop overwriting hostname with atoi —
leave j_server["hostname"] as a string (do not call atoi on it); convert only
numeric fields (j_server["port"] and j_server["read_only"]) to ints using a safe
string->int conversion after ensuring the keys exist; remove the attempted
access to j_server["commetn"] (typo) since the SELECT only returns hostname,
port, read_only — either add comment to the SELECT if you need it and to
valid_readonly_entries, or drop any comment handling; call
mysql_free_result(my_servers_res) after parse_result_to_json to free resources;
ensure extract_readonly_servers_state is called with the expected keys from
j_servers.
| int q_res = mysql_query( | ||
| proxysql_admin, | ||
| "SELECT hostname, port, seconds_behind_master FROM REPLICATIONLAG_HOST_STATUS" | ||
| ); | ||
|
|
||
| if (q_res == 0) { | ||
| ordered_json j_servers {}; | ||
| j_servers["cluster_type"] = "replicationlag"; | ||
| j_servers["replicationlag_servers_init_state"] = {}; | ||
|
|
||
| MYSQL_RES* my_servers_res = mysql_store_result(proxysql_admin); | ||
| parse_result_to_json(my_servers_res, j_servers["replicationlag_servers_init_state"]); | ||
|
|
||
| // convert the fields into the proper types | ||
| for (ordered_json& j_server : j_servers["replicationlag_servers_init_state"]) { | ||
| j_server["hostname"] = atoi(std::string {j_server["hostname"]}.c_str()); | ||
| j_server["port"] = atoi(std::string {j_server["port"]}.c_str()); | ||
|
|
||
| const auto& seconds_behind_master = j_server.at("seconds_behind_master"); | ||
| if (seconds_behind_master == nullptr || seconds_behind_master.is_null()) { | ||
| j_server["seconds_behind_master"] = nullptr; | ||
| } else { | ||
| j_server["seconds_behind_master"] = atoi(std::string {j_server["seconds_behind_master"]}.c_str()); | ||
| } | ||
| j_server["comment"] = atoi(std::string {j_server["comment"]}.c_str()); | ||
| } | ||
|
|
||
| std::vector<replicationlag_server_state> cur_replicationlag_servers_state {}; | ||
| std::pair<int, std::string> ext_res = | ||
| extract_replicationlag_servers_state( | ||
| replicationlag_state_id::init_state, j_servers, cur_replicationlag_servers_state | ||
| ); | ||
|
|
||
| if (ext_res.first == EXIT_SUCCESS) { | ||
| out_cur_replicationlag_servers_state = std::move(cur_replicationlag_servers_state); | ||
| } else { | ||
| string_format(t_err_msg, err_msg, ext_res.second.c_str()); | ||
| err_res = { EXIT_FAILURE, err_msg }; | ||
| } | ||
| } else { | ||
| string_format(t_err_msg, err_msg, mysql_error(proxysql_admin)); | ||
| err_res = { EXIT_FAILURE, err_msg }; | ||
| } | ||
|
|
||
| return err_res; | ||
| } |
There was a problem hiding this comment.
Critical: get_current_replicationlag_servers_state will always fail at runtime.
Multiple problems in the post-query conversion block:
- Line 202 applies
atoitohostnameand overwrites the JSON field with an integer — this clobbers a string value and thenextract_replicationlag_servers_statelater expects a string. - Line 211 references
j_server["comment"], but the SELECT at Line 189 only returnshostname, port, seconds_behind_master. Access-by-bracket will insert anullentry, thenstd::string{ null }throws — the whole path dies before reachingextract_replicationlag_servers_state. - The downstream
extract_replicationlag_servers_statewill then also reject the payload due toinvalid_keysbecausecommentisn't invalid_replicationlag_entries.
🐛 Proposed fix
for (ordered_json& j_server : j_servers["replicationlag_servers_init_state"]) {
- j_server["hostname"] = atoi(std::string {j_server["hostname"]}.c_str());
+ j_server["hostname"] = std::string { j_server["hostname"] };
j_server["port"] = atoi(std::string {j_server["port"]}.c_str());
const auto& seconds_behind_master = j_server.at("seconds_behind_master");
if (seconds_behind_master == nullptr || seconds_behind_master.is_null()) {
j_server["seconds_behind_master"] = nullptr;
} else {
j_server["seconds_behind_master"] = atoi(std::string {j_server["seconds_behind_master"]}.c_str());
}
- j_server["comment"] = atoi(std::string {j_server["comment"]}.c_str());
}Also add mysql_free_result(my_servers_res); after parse_result_to_json for consistency with other modules.
📝 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.
| int q_res = mysql_query( | |
| proxysql_admin, | |
| "SELECT hostname, port, seconds_behind_master FROM REPLICATIONLAG_HOST_STATUS" | |
| ); | |
| if (q_res == 0) { | |
| ordered_json j_servers {}; | |
| j_servers["cluster_type"] = "replicationlag"; | |
| j_servers["replicationlag_servers_init_state"] = {}; | |
| MYSQL_RES* my_servers_res = mysql_store_result(proxysql_admin); | |
| parse_result_to_json(my_servers_res, j_servers["replicationlag_servers_init_state"]); | |
| // convert the fields into the proper types | |
| for (ordered_json& j_server : j_servers["replicationlag_servers_init_state"]) { | |
| j_server["hostname"] = atoi(std::string {j_server["hostname"]}.c_str()); | |
| j_server["port"] = atoi(std::string {j_server["port"]}.c_str()); | |
| const auto& seconds_behind_master = j_server.at("seconds_behind_master"); | |
| if (seconds_behind_master == nullptr || seconds_behind_master.is_null()) { | |
| j_server["seconds_behind_master"] = nullptr; | |
| } else { | |
| j_server["seconds_behind_master"] = atoi(std::string {j_server["seconds_behind_master"]}.c_str()); | |
| } | |
| j_server["comment"] = atoi(std::string {j_server["comment"]}.c_str()); | |
| } | |
| std::vector<replicationlag_server_state> cur_replicationlag_servers_state {}; | |
| std::pair<int, std::string> ext_res = | |
| extract_replicationlag_servers_state( | |
| replicationlag_state_id::init_state, j_servers, cur_replicationlag_servers_state | |
| ); | |
| if (ext_res.first == EXIT_SUCCESS) { | |
| out_cur_replicationlag_servers_state = std::move(cur_replicationlag_servers_state); | |
| } else { | |
| string_format(t_err_msg, err_msg, ext_res.second.c_str()); | |
| err_res = { EXIT_FAILURE, err_msg }; | |
| } | |
| } else { | |
| string_format(t_err_msg, err_msg, mysql_error(proxysql_admin)); | |
| err_res = { EXIT_FAILURE, err_msg }; | |
| } | |
| return err_res; | |
| } | |
| MYSQL_RES* my_servers_res = mysql_store_result(proxysql_admin); | |
| parse_result_to_json(my_servers_res, j_servers["replicationlag_servers_init_state"]); | |
| // convert the fields into the proper types | |
| for (ordered_json& j_server : j_servers["replicationlag_servers_init_state"]) { | |
| j_server["hostname"] = std::string { j_server["hostname"] }; | |
| j_server["port"] = atoi(std::string {j_server["port"]}.c_str()); | |
| const auto& seconds_behind_master = j_server.at("seconds_behind_master"); | |
| if (seconds_behind_master == nullptr || seconds_behind_master.is_null()) { | |
| j_server["seconds_behind_master"] = nullptr; | |
| } else { | |
| j_server["seconds_behind_master"] = atoi(std::string {j_server["seconds_behind_master"]}.c_str()); | |
| } | |
| } | |
| std::vector<replicationlag_server_state> cur_replicationlag_servers_state {}; | |
| std::pair<int, std::string> ext_res = | |
| extract_replicationlag_servers_state( | |
| replicationlag_state_id::init_state, j_servers, cur_replicationlag_servers_state | |
| ); | |
| if (ext_res.first == EXIT_SUCCESS) { | |
| out_cur_replicationlag_servers_state = std::move(cur_replicationlag_servers_state); | |
| } else { | |
| string_format(t_err_msg, err_msg, ext_res.second.c_str()); | |
| err_res = { EXIT_FAILURE, err_msg }; | |
| } | |
| } else { | |
| string_format(t_err_msg, err_msg, mysql_error(proxysql_admin)); | |
| err_res = { EXIT_FAILURE, err_msg }; | |
| } | |
| return err_res; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/deps/cluster_simulator/lib/replicationlag_utils.cpp` around lines 187 -
232, The post-query conversion in get_current_replicationlag_servers_state is
corrupting and accessing fields that aren't present: do not atoi the hostname
(leave j_server["hostname"] as string), only convert j_server["port"] and
j_server["seconds_behind_master"] to integers when non-null, and remove the
erroneous conversion/access of j_server["comment"] (since SELECT returns only
hostname, port, seconds_behind_master) so extract_replicationlag_servers_state
receives the expected keys; also call mysql_free_result(my_servers_res) after
parse_result_to_json to free the result.
| replicationlag_server_state replicationlag_update_state( | ||
| const replicationlag_server_state& st1, | ||
| const replicationlag_server_state& st2 | ||
| ) { | ||
| replicationlag_server_state result {}; | ||
|
|
||
| // TODO: Make this proper doc. | ||
| // hostname and port **can't** be changed, | ||
| // because the are part of the server 'id'. Only the | ||
| // other fields are allowed to change, otherwise, the | ||
| // verification step should have failed. | ||
|
|
||
| const std::shared_ptr<int>& st1_replicationlag = std::get<2>(st1); | ||
| const std::shared_ptr<int>& st2_replicationlag = std::get<2>(st2); | ||
|
|
||
| if (st1_replicationlag != nullptr && st2_replicationlag != nullptr) { | ||
| if (st1_replicationlag != st2_replicationlag) { | ||
| std::get<2>(result) = st2_replicationlag; | ||
| } | ||
| } else { | ||
| std::get<2>(result) = st2_replicationlag; | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Critical: replicationlag_update_state drops hostname/port, producing broken next-state tuples.
result is default-constructed and only std::get<2>(result) is ever assigned. The returned tuple therefore has an empty hostname, port = 0, and the lag maybe set. replicationlag_update_cluster_state then push_backs this into result, meaning the "updated cluster state" loses every server's identity.
Additionally, Line 461 compares the raw shared_ptr pointers rather than their pointees:
if (st1_replicationlag != st2_replicationlag) // different pointers, not valuesThis should be *st1_replicationlag != *st2_replicationlag.
🐛 Proposed fix
replicationlag_server_state result {};
- // TODO: Make this proper doc.
- // hostname and port **can't** be changed,
- // because the are part of the server 'id'. Only the
- // other fields are allowed to change, otherwise, the
- // verification step should have failed.
+ // hostname and port are part of the identity — preserve them from st1.
+ std::get<0>(result) = std::get<0>(st1);
+ std::get<1>(result) = std::get<1>(st1);
+ std::get<2>(result) = std::get<2>(st1);
const std::shared_ptr<int>& st1_replicationlag = std::get<2>(st1);
const std::shared_ptr<int>& st2_replicationlag = std::get<2>(st2);
if (st1_replicationlag != nullptr && st2_replicationlag != nullptr) {
- if (st1_replicationlag != st2_replicationlag) {
+ if (*st1_replicationlag != *st2_replicationlag) {
std::get<2>(result) = st2_replicationlag;
}
} else {
std::get<2>(result) = st2_replicationlag;
}The analogous readonly_update_state in readonly_utils.cpp has the same identity-loss bug — see the separate comment on that file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/deps/cluster_simulator/lib/replicationlag_utils.cpp` around lines 445 -
469, replicationlag_update_state currently returns a default-constructed
replicationlag_server_state so hostname and port are lost; change the function
to initialize result from the incoming tuple (e.g., copy st1 or st2 since
identity must be identical) and only update the replication-lag field
(std::get<2>(result)); also fix the pointer comparison: when both
std::shared_ptr<int> st1_replicationlag and st2_replicationlag are non-null
compare their pointees (*st1_replicationlag != *st2_replicationlag) rather than
the shared_ptr addresses; apply the same identity-preserving pattern to the
analogous readonly_update_state if present.
- Closes sysown#5546 and sysown#5547 Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
…hecks` - Extend the simulator's Aurora hostgroup payload schema with two optional fields, `check_interval_ms` and `autopurge_missing_checks`, plumb them through `aurora_hostgroup_config_t`, the `valid_aurora_hostgroup_entries` allowlist, the INSERT template and the parser/binder so payload-driven tests can exercise the autopurge path added for issue sysown#5547. - Add `three_nodes_cluster-autopurge_missing_replica.json` with two scenarios: - autopurge_missing_checks=2: a reader vanishes from REPLICA_HOST_STATUS and ProxySQL must drop it from mysql_servers within the threshold. - autopurge_missing_checks=0: the same vanishing reader stays in mysql_servers, asserting the default backwards-compatible behavior. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
…er the purge - Pass cleanup=1 to `prepare_aurora_cluster_state` for the new_state transition. Without it, INSERT-OR-REPLACE only adds/updates rows; a reader removed from `aurora_servers_new_state` was silently kept in `REPLICA_HOST_STATUS`, so the monitor never saw the absence and autopurge never fired. - With cleanup=1 the simulator now treats `aurora_servers_new_state` as the full desired state and deletes anything missing. This is backwards compatible since every existing aurora payload already lists the complete server set. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ProxySQL_Config.cpp (1)
1400-1443:⚠️ Potential issue | 🟠 MajorKeep Aurora hostgroup config round-tripping.
autopurge_missing_checksis now loaded intomysql_aws_aurora_hostgroups, but the serializer still uses the older fixed Aurora column order, so saving config will drop this value. Also validate the 0–100 range before insert; otherwise bad configs will trip the table CHECK and this loader won't surface the failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ProxySQL_Config.cpp` around lines 1400 - 1443, The INSERT format string q and the corresponding sprintf call in the mysql_aws_aurora_hostgroups serialization must be updated to include autopurge_missing_checks in the correct column order so that saved configs round-trip (adjust q to list autopurge_missing_checks in VALUES and include autopurge_missing_checks in the sprintf argument list in the same position), and validate autopurge_missing_checks before building the query (in the loader code that sets the autopurge_missing_checks variable) to ensure it is within 0–100—if out of range, log a proxy_error and clamp it to the nearest valid value (0 or 100) prior to formatting the query so the DB CHECK cannot fail silently.
🧹 Nitpick comments (1)
test/deps/cluster_simulator/lib/aurora_utils.h (1)
20-51: New Aurora types don't follow the repository naming convention.If these types are going to stay public in the simulator API, I'd align them now instead of spreading
aurora_hostgroup_config_t/aurora_server_state_t/aurora_state_idto more call sites.As per coding guidelines,
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/aurora_utils.h` around lines 20 - 51, Rename the public Aurora types to follow PascalCase + prefix convention and update all references: change struct aurora_hostgroup_config_t to AuroraHostgroupConfig and update the vector parameter type in extract_aurora_hostgroup_config, change using aurora_server_state_t to using AuroraServerState, rename enum AURORA_SERVER_STATE to a PascalCase identifier (e.g., AuroraServerStateIndex or keep namespaced members and adjust usages), and rename enum class aurora_state_id to enum class AuroraStateId; update the function signature for extract_aurora_hostgroup_config and any call sites to use the new type names (refer to symbols aurora_hostgroup_config_t, aurora_server_state_t, AURORA_SERVER_STATE, aurora_state_id, and extract_aurora_hostgroup_config to locate and rename).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/MySQL_HostGroups_Manager.cpp`:
- Around line 3743-3750: The calculation for the shun grace period currently
truncates sub-second values by doing integer division on b (where b is 2 *
mysql_thread___monitor_ping_interval), so change the division to round up
instead of floor: compute the seconds by dividing the millisecond value with
rounding up (e.g., use std::ceil on b/1000.0 or add 999 before integer divide)
so mysql_thread___shun_recovery_time_sec gets the intended safety margin and
avoids losing sub-second intervals; update the code around variables b,
mysql_thread___monitor_ping_interval and mysql_thread___shun_recovery_time_sec
accordingly.
In `@lib/MySQL_Monitor.cpp`:
- Around line 6597-6600: The current use of server_id.rfind(domain_name) can
match domain_name anywhere in the hostname; change the logic to only strip
domain_name when it is a suffix: after finding pos, verify pos != npos AND pos +
domain_name.size() == server_id.size() (or use an ends-with check) before
calling server_id.erase(pos); apply the same suffix-only check for the other
occurrence handling the same variables at the later block (the lines that
currently use rfind(domain_name) around the second occurrence).
In `@test/deps/cluster_simulator/cluster_simulator.cpp`:
- Around line 936-950: The call to set_monitor_variables (its return stored in
set_monitor_variables_err) is currently ignored—add an error check after each
set_monitor_variables invocation (including the REPLICATION_LAG and AURORA code
paths and the other occurrences around lines referenced) that mirrors the
existing mysql_query error handling: on non-zero set_monitor_variables_err
create a query_error (or appropriate error using
create_query_error/create_monitor_error pattern used elsewhere), convert to
internal_error/result, and goto exit to stop the simulator; update the branches
that call set_monitor_variables so they abort on failure rather than continuing
with stale runtime variables.
- Around line 531-564: The current flow pushes monitor_variables into runtime
before applying defaults, causing READ_ONLY to inherit prior state; move the
defaulting step so monitor_variables =
set_monitor_variables_defaults(monitor_variables, monitor_defaults_variables) is
called before invoking set_monitor_variables(proxysql_admin,
rephostgrp_monitor_variables, monitor_variables) and before the subsequent
mysql_query(proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME") so the runtime
load receives the defaulted vector; adjust the code around
set_monitor_variables, set_monitor_variables_defaults, and the "LOAD MYSQL
VARIABLES TO RUNTIME" call accordingly.
In
`@test/deps/cluster_simulator/tests/aurora_tests_payloads/three_nodes_cluster-autopurge_missing_replica.json`:
- Around line 52-53: Update the scenario prose to match the fixture's expected
final state: change the "__details__" (or "__comment__") text that currently
says the missing reader stays "just OFFLINE" to state that the missing reader
remains "ONLINE" (or otherwise indicate it remains present but marked ONLINE) so
the description aligns with the assertions around the expected entry at lines
95-98; edit the "__details__" string in the JSON fixture accordingly.
---
Outside diff comments:
In `@lib/ProxySQL_Config.cpp`:
- Around line 1400-1443: The INSERT format string q and the corresponding
sprintf call in the mysql_aws_aurora_hostgroups serialization must be updated to
include autopurge_missing_checks in the correct column order so that saved
configs round-trip (adjust q to list autopurge_missing_checks in VALUES and
include autopurge_missing_checks in the sprintf argument list in the same
position), and validate autopurge_missing_checks before building the query (in
the loader code that sets the autopurge_missing_checks variable) to ensure it is
within 0–100—if out of range, log a proxy_error and clamp it to the nearest
valid value (0 or 100) prior to formatting the query so the DB CHECK cannot fail
silently.
---
Nitpick comments:
In `@test/deps/cluster_simulator/lib/aurora_utils.h`:
- Around line 20-51: Rename the public Aurora types to follow PascalCase +
prefix convention and update all references: change struct
aurora_hostgroup_config_t to AuroraHostgroupConfig and update the vector
parameter type in extract_aurora_hostgroup_config, change using
aurora_server_state_t to using AuroraServerState, rename enum
AURORA_SERVER_STATE to a PascalCase identifier (e.g., AuroraServerStateIndex or
keep namespaced members and adjust usages), and rename enum class
aurora_state_id to enum class AuroraStateId; update the function signature for
extract_aurora_hostgroup_config and any call sites to use the new type names
(refer to symbols aurora_hostgroup_config_t, aurora_server_state_t,
AURORA_SERVER_STATE, aurora_state_id, and extract_aurora_hostgroup_config to
locate and rename).
🪄 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
Run ID: c1954577-9b3e-4860-89fa-fcb2c409abd9
📒 Files selected for processing (21)
include/Base_HostGroups_Manager.hinclude/MySQL_HostGroups_Manager.hinclude/MySQL_Monitor.hppinclude/ProxySQL_Admin_Tables_Definitions.hinclude/ProxySQL_Cluster.hpplib/Base_HostGroups_Manager.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Monitor.cpplib/ProxySQL_Admin.cpplib/ProxySQL_Admin_Disk_Upgrade.cpplib/ProxySQL_Cluster.cpplib/ProxySQL_Config.cpptest/deps/cluster_simulator/cluster_simulator.cpptest/deps/cluster_simulator/lib/aurora_utils.cpptest/deps/cluster_simulator/lib/aurora_utils.htest/deps/cluster_simulator/tests/aurora_tests_payloads/three_nodes_cluster-autopurge_missing_replica.jsontest/infra/control/docker-fs-helper.bashtest/infra/control/start-proxysql-isolated.bashtest/infra/control/stop-proxysql-isolated.bashtest/infra/infra-mysql57-binlog/docker-compose-init.bashtest/tap/groups/groups.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test/deps/cluster_simulator/lib/aurora_utils.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: run / trigger
🧰 Additional context used
📓 Path-based instructions (3)
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
#ifndef __CLASS_*_Hinclude guards in header files
Files:
include/MySQL_Monitor.hppinclude/ProxySQL_Cluster.hppinclude/MySQL_HostGroups_Manager.hinclude/ProxySQL_Admin_Tables_Definitions.hinclude/Base_HostGroups_Manager.h
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
include/MySQL_Monitor.hpplib/Base_HostGroups_Manager.cppinclude/ProxySQL_Cluster.hpplib/ProxySQL_Admin_Disk_Upgrade.cpplib/ProxySQL_Config.cppinclude/MySQL_HostGroups_Manager.hlib/ProxySQL_Cluster.cppinclude/ProxySQL_Admin_Tables_Definitions.hinclude/Base_HostGroups_Manager.hlib/ProxySQL_Admin.cpplib/MySQL_Monitor.cpptest/deps/cluster_simulator/lib/aurora_utils.htest/deps/cluster_simulator/cluster_simulator.cpplib/MySQL_HostGroups_Manager.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file typically in
lib/directory
Files:
lib/Base_HostGroups_Manager.cpplib/ProxySQL_Admin_Disk_Upgrade.cpplib/ProxySQL_Config.cpplib/ProxySQL_Cluster.cpplib/ProxySQL_Admin.cpplib/MySQL_Monitor.cpplib/MySQL_HostGroups_Manager.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` and link against `libproxysql.a` via the custom test harness
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, etc. for feature-gated code
Applied to files:
include/ProxySQL_Cluster.hpplib/ProxySQL_Admin_Disk_Upgrade.cpplib/ProxySQL_Config.cpplib/ProxySQL_Cluster.cpplib/ProxySQL_Admin.cpptest/deps/cluster_simulator/cluster_simulator.cpp
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Use feature flags `PROXYSQL31=1` for v3.1.x Innovative tier and `PROXYSQLGENAI=1` for v4.0.x AI/MCP tier; PROXYSQLGENAI implies both PROXYSQL31 and PROXYSQLFFTO/PROXYSQLTSDB
Applied to files:
include/ProxySQL_Cluster.hpplib/ProxySQL_Admin_Disk_Upgrade.cpplib/ProxySQL_Config.cpplib/ProxySQL_Cluster.cpplib/ProxySQL_Admin.cpp
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
lib/ProxySQL_Admin_Disk_Upgrade.cpplib/ProxySQL_Config.cpp
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
test/deps/cluster_simulator/lib/aurora_utils.htest/deps/cluster_simulator/cluster_simulator.cpp
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` and link against `libproxysql.a` via the custom test harness
Applied to files:
test/deps/cluster_simulator/cluster_simulator.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/deps/cluster_simulator/cluster_simulator.cpp
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/deps/cluster_simulator/cluster_simulator.cpp
📚 Learning: 2026-02-13T05:55:42.693Z
Learnt from: mevishalr
Repo: sysown/proxysql PR: 5364
File: lib/MySQL_Logger.cpp:1211-1232
Timestamp: 2026-02-13T05:55:42.693Z
Learning: In ProxySQL, the MySQL_Logger and PgSQL_Logger destructors run after all worker threads have been joined during shutdown. The sequence in src/main.cpp is: (1) join all worker threads, (2) call ProxySQL_Main_shutdown_all_modules() which deletes the loggers. Therefore, there is no concurrent thread access during logger destruction, and lock ordering in the destructors cannot cause deadlocks.
Applied to files:
test/deps/cluster_simulator/cluster_simulator.cpp
🪛 Cppcheck (2.20.0)
test/deps/cluster_simulator/cluster_simulator.cpp
[warning] 329-329: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 384-384: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[error] 326-326: If memory allocation fails
(nullPointerArithmeticOutOfMemory)
🔇 Additional comments (16)
lib/Base_HostGroups_Manager.cpp (1)
1955-1957: Looks correct.Adding
autopurge_missing_checkskeepsdump_table_mysql()aligned with the 15-column INSERT/bind sequence used downstream, so the Aurora hostgroup projection stays consistent. As shown inlib/ProxySQL_Admin.cpp:7676-7702, this column order matters.lib/ProxySQL_Cluster.cpp (1)
2373-2397: Autopurge column integration is consistent and correct.
autopurge_missing_checksis wired in at the correct Aurora column position, and the row indexing/format arguments were updated coherently (row[13]for autopurge,row[14]for comment).lib/ProxySQL_Admin.cpp (1)
7676-7678: Column/bind alignment for Aurora hostgroups looks correct.The new
autopurge_missing_checkscolumn is inserted and bound in the correct position, andcommentis correctly shifted to?15. This matches the expected 15-column layout.Also applies to: 7701-7702
include/ProxySQL_Cluster.hpp (1)
73-73: Looks aligned with the schema update.Including
autopurge_missing_checksin the Aurora projection keeps the cluster checksum/query shape in sync with the new table definition.lib/ProxySQL_Admin_Disk_Upgrade.cpp (1)
486-505: Upgrade path looks correct.The new V2_0_9 → V2_0_10 migration preserves the existing Aurora fields and lets
autopurge_missing_checkstake its default for legacy rows.include/MySQL_Monitor.hpp (1)
553-553: Good public hook for the new monitor action.The declaration is consistent with the Aurora autopurge flow added elsewhere in this PR.
include/Base_HostGroups_Manager.h (2)
85-87: Schema extension looks good.
autopurge_missing_checksis added with the expected constraint and default.
413-413: Ctor/update wiring looks consistent.Propagating the new field through
AWS_Aurora_Infokeeps the in-memory model aligned with the persisted schema.Also applies to: 420-421
include/ProxySQL_Admin_Tables_Definitions.h (1)
231-237: Schema bump looks consistent.Adding
autopurge_missing_checksto both the admin and runtime Aurora table definitions, then remapping the default macro toV2_0_10, is internally consistent with the versioned schema pattern here.include/MySQL_HostGroups_Manager.h (1)
348-356: AllAWS_Aurora_Infocall sites have been properly updated.The new
autopurge_missing_checksparameter in the constructor andupdate()method has been wired through all invocation points in lib/MySQL_HostGroups_Manager.cpp (lines 6234, 6239), database queries, schema definitions, and Monitor code. No stale call sites remain.lib/MySQL_HostGroups_Manager.cpp (4)
2247-2249: Aurora dump stays in sync with the schema.
dump_table_mysql()now exposesautopurge_missing_checksalongside the other Aurora columns, so the exported shape matches the updated table layout.
5795-5915: AuroraInfo now carries the new purge threshold end-to-end.The constructor/update pair persists
autopurge_missing_checksand marks the object dirty when it changes, which keeps the in-memory Aurora state aligned with the stored row.
6179-6241: The Aurora table write path matches the new column order.The prepared INSERT, row parsing, and
AWS_Aurora_Infoconstruction all use the sameautopurge_missing_checksposition, so I don’t see a column-shift bug here.
6809-6810: Good: the monitor checksum now includes the purge threshold.Adding
autopurge_missing_checksto the monitor-facing Aurora query ensures that threshold changes participate in the resultset checksum and refresh flow.lib/MySQL_Monitor.cpp (2)
6329-6331: Good safeguard on autopurge trigger conditions.Line 6329 correctly gates autopurge on enabled threshold + successful query + non-empty host status, which helps avoid purging on transient fetch failures.
6587-6608: Lock/reentrancy analysis is not required here—this code is safe as written.
remove_server_in_hg()does not re-acquire the write lock; it only marks the serverOFFLINE_HARD, drops free connections, and deletes the sqlite record. The in-memorymysrvscontainer is not modified. Index-based iteration (whgc->mysrvs->idx(j)) is inherently safe during element status changes, and thecontinuecheck forMYSQL_SERVER_STATUS_OFFLINE_HARDensures proper handling. No deadlock or element-skipping risk exists.
| int a = mysql_thread___shun_recovery_time_sec; | ||
| int b = mysql_thread___monitor_ping_interval; | ||
|
|
||
| // Issue #5546: Add safety margin to prevent race condition | ||
| // where server can be unshunned before next ping arrives. | ||
| // Using 2 * ping_interval provides buffer for scheduling delays. | ||
| int b = 2 * mysql_thread___monitor_ping_interval; | ||
|
|
||
| b = b/1000; |
There was a problem hiding this comment.
Round the shun grace period instead of truncating it.
b = b / 1000 floors the doubled ping interval to whole seconds, so sub-second or near-second ping intervals can lose the intended safety margin and reintroduce the race you’re trying to avoid.
🔧 Suggested fix
- int a = mysql_thread___shun_recovery_time_sec;
+ int a = mysql_thread___shun_recovery_time_sec;
// Issue `#5546`: Add safety margin to prevent race condition
// where server can be unshunned before next ping arrives.
- // Using 2 * ping_interval provides buffer for scheduling delays.
- int b = 2 * mysql_thread___monitor_ping_interval;
-
- b = b/1000;
- if (b > a) {
- t = t + (b - a);
+ // Using 2 * ping_interval provides buffer for scheduling delays.
+ const auto recovery_margin_sec =
+ (2LL * mysql_thread___monitor_ping_interval + 999) / 1000;
+ if (recovery_margin_sec > a) {
+ t = t + (recovery_margin_sec - a);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/MySQL_HostGroups_Manager.cpp` around lines 3743 - 3750, The calculation
for the shun grace period currently truncates sub-second values by doing integer
division on b (where b is 2 * mysql_thread___monitor_ping_interval), so change
the division to round up instead of floor: compute the seconds by dividing the
millisecond value with rounding up (e.g., use std::ceil on b/1000.0 or add 999
before integer divide) so mysql_thread___shun_recovery_time_sec gets the
intended safety margin and avoids losing sub-second intervals; update the code
around variables b, mysql_thread___monitor_ping_interval and
mysql_thread___shun_recovery_time_sec accordingly.
| size_t pos = server_id.rfind(domain_name); | ||
| if (pos != std::string::npos) { | ||
| server_id.erase(pos); | ||
| } |
There was a problem hiding this comment.
Use suffix-only domain stripping before server-id comparison.
Line 6597 and Line 6625 use rfind(domain_name) without enforcing suffix match. If domain_name appears earlier in the hostname, it can truncate the wrong portion and falsely purge healthy servers.
Suggested fix
- size_t pos = server_id.rfind(domain_name);
- if (pos != std::string::npos) {
- server_id.erase(pos);
- }
+ if (!domain_name.empty() &&
+ server_id.size() > domain_name.size() &&
+ server_id.compare(server_id.size() - domain_name.size(), domain_name.size(), domain_name) == 0) {
+ server_id.erase(server_id.size() - domain_name.size());
+ }
...
- size_t pos = server_id.rfind(domain_name);
- if (pos != std::string::npos) {
- server_id.erase(pos);
- }
+ if (!domain_name.empty() &&
+ server_id.size() > domain_name.size() &&
+ server_id.compare(server_id.size() - domain_name.size(), domain_name.size(), domain_name) == 0) {
+ server_id.erase(server_id.size() - domain_name.size());
+ }Also applies to: 6625-6628
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/MySQL_Monitor.cpp` around lines 6597 - 6600, The current use of
server_id.rfind(domain_name) can match domain_name anywhere in the hostname;
change the logic to only strip domain_name when it is a suffix: after finding
pos, verify pos != npos AND pos + domain_name.size() == server_id.size() (or use
an ends-with check) before calling server_id.erase(pos); apply the same
suffix-only check for the other occurrence handling the same variables at the
later block (the lines that currently use rfind(domain_name) around the second
occurrence).
| /** | ||
| * @brief Change the monitor variable values before server loading for READ_ONLY. | ||
| * @details Due to the non-convergent nature of READ_ONLY actions due to 'writer_is_also_reader', servers could | ||
| * not match the right 'exp_init_cluster_status' if the configuration value for 'writer_is_also_reader' changes | ||
| * after previous monitoring actions has been already taken. | ||
| */ | ||
| { | ||
| // prepare monitor variables | ||
| set_monitor_variables_err = | ||
| set_monitor_variables(proxysql_admin, rephostgrp_monitor_variables, monitor_variables); | ||
|
|
||
| // final LOAD for 'mysql' variables after configuration has been setup | ||
| load_to_run_err = mysql_query(proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME"); | ||
| if (load_to_run_err) { | ||
| const auto& query_err = | ||
| create_query_error(proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME", __FILE__, __LINE__); | ||
| result = internal_error(query_err.second, __FILE__, __LINE__); | ||
| goto exit; | ||
| } | ||
| } | ||
|
|
||
| // final LOAD after all the configuration has been setup | ||
| load_to_run_err = mysql_query(proxysql_admin, "LOAD MYSQL SERVERS TO RUNTIME"); | ||
| if (load_to_run_err) { | ||
| const auto& query_err = | ||
| create_query_error(proxysql_admin, "LOAD MYSQL SERVERS TO RUNTIME", __FILE__, __LINE__); | ||
| result = internal_error(query_err.second, __FILE__, __LINE__); | ||
| goto exit; | ||
| } | ||
|
|
||
| // give the variables some defaults if they are not present | ||
| monitor_variables = set_monitor_variables_defaults( | ||
| monitor_variables, monitor_defaults_variables | ||
| ); |
There was a problem hiding this comment.
Set READ_ONLY defaults before loading monitor variables.
When mysql_monitor_config omits monitor_writer_is_also_reader, this path pushes the pre-default vector into runtime and only applies the default locally afterward. That makes READ_ONLY simulations depend on whatever value a previous scenario left behind.
Suggested fix
{
+ // give the variables some defaults if they are not present
+ monitor_variables = set_monitor_variables_defaults(
+ monitor_variables, monitor_defaults_variables
+ );
+
// prepare monitor variables
set_monitor_variables_err =
set_monitor_variables(proxysql_admin, rephostgrp_monitor_variables, monitor_variables);
+ if (set_monitor_variables_err.first) {
+ result = internal_error(set_monitor_variables_err.second, __FILE__, __LINE__);
+ goto exit;
+ }
// final LOAD for 'mysql' variables after configuration has been setup
load_to_run_err = mysql_query(proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME");
if (load_to_run_err) {
const auto& query_err =
@@
- // give the variables some defaults if they are not present
- monitor_variables = set_monitor_variables_defaults(
- monitor_variables, monitor_defaults_variables
- );
-
set_monitor_tos_err = set_readonly_monitor_check_times(proxysql_admin);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/deps/cluster_simulator/cluster_simulator.cpp` around lines 531 - 564,
The current flow pushes monitor_variables into runtime before applying defaults,
causing READ_ONLY to inherit prior state; move the defaulting step so
monitor_variables = set_monitor_variables_defaults(monitor_variables,
monitor_defaults_variables) is called before invoking
set_monitor_variables(proxysql_admin, rephostgrp_monitor_variables,
monitor_variables) and before the subsequent mysql_query(proxysql_admin, "LOAD
MYSQL VARIABLES TO RUNTIME") so the runtime load receives the defaulted vector;
adjust the code around set_monitor_variables, set_monitor_variables_defaults,
and the "LOAD MYSQL VARIABLES TO RUNTIME" call accordingly.
| result = { | ||
| EXIT_SUCCESS, | ||
| nlohmann::ordered_json { | ||
| { "err_type", "none" }, | ||
| { "result", | ||
| nlohmann::ordered_json { | ||
| { "cluster_type", "replication" }, | ||
| { "mysql_servers", j_test_definition.at("mysql_servers") }, | ||
| { "mysql_replication_hostgroups", j_test_definition.at("mysql_replication_hostgroups") }, | ||
| { "readonly_servers_init_state", j_test_definition.at("readonly_servers_init_state") }, | ||
| { "readonly_servers_new_state", j_test_definition.at("readonly_servers_new_state") }, | ||
| { "proxysql_init_state_checksum", init_state_checksum }, | ||
| { "proxysql_init_state", j_test_definition.at("proxysql_init_state") }, | ||
| { "proxysql_final_state_checksum", final_state_checksum }, | ||
| { "proxysql_final_state", j_final_cluster_status }, | ||
| } | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Emit canonical cluster_type values in simulate mode.
get_test_type() only accepts READ_ONLY and REPLICATION_LAG, but these simulate-mode payloads currently emit replication and replicationlag. The generated payloads for those families cannot be fed back into this same simulator.
Suggested fix
- { "cluster_type", "replication" },
+ { "cluster_type", "READ_ONLY" },- { "cluster_type", "replicationlag" },
+ { "cluster_type", "REPLICATION_LAG" },Also applies to: 1438-1455
| // prepare monitor variables | ||
| set_monitor_variables_err = | ||
| set_monitor_variables( | ||
| proxysql_admin, grouprep_monitor_variables, monitor_variables | ||
| ); | ||
|
|
||
| // final LOAD for 'mysql' variables after configuration has been setup | ||
| load_to_run_err = mysql_query(proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME"); | ||
| if (load_to_run_err) { | ||
| const auto& query_err = | ||
| create_query_error( | ||
| proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME", __FILE__, __LINE__ | ||
| ); | ||
| result = internal_error(query_err.second, __FILE__, __LINE__); | ||
| goto exit; |
There was a problem hiding this comment.
Stop if setting monitor variables fails.
These branches store set_monitor_variables_err and then ignore it. If the update fails, the simulator keeps running with stale runtime variables, which can turn an invalid setup into a false-positive test result.
Suggested fix
set_monitor_variables_err =
set_monitor_variables(
proxysql_admin, grouprep_monitor_variables, monitor_variables
);
+ if (set_monitor_variables_err.first) {
+ result = internal_error(set_monitor_variables_err.second, __FILE__, __LINE__);
+ goto exit;
+ }
// final LOAD for 'mysql' variables after configuration has been setup
load_to_run_err = mysql_query(proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME");Apply the same check in the REPLICATION_LAG and AURORA paths.
Also applies to: 1321-1335, 1700-1714
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/deps/cluster_simulator/cluster_simulator.cpp` around lines 936 - 950,
The call to set_monitor_variables (its return stored in
set_monitor_variables_err) is currently ignored—add an error check after each
set_monitor_variables invocation (including the REPLICATION_LAG and AURORA code
paths and the other occurrences around lines referenced) that mirrors the
existing mysql_query error handling: on non-zero set_monitor_variables_err
create a query_error (or appropriate error using
create_query_error/create_monitor_error pattern used elsewhere), convert to
internal_error/result, and goto exit to stop the simulator; update the branches
that call set_monitor_variables so they abort on failure rather than continuing
with stale runtime variables.
| "__comment__": "Three Nodes Aurora Cluster - Autopurge disabled (default)", | ||
| "__details__": "Without autopurge_missing_checks, missing reader stays in mysql_servers (just OFFLINE). Verifies autopurge=0 keeps backwards-compatible behavior.", |
There was a problem hiding this comment.
Align the scenario description with the asserted final state.
Line 53 says the missing reader stays "just OFFLINE", but Lines 95-98 expect that same entry to remain ONLINE. Please make the prose match the fixture, otherwise this case is hard to trust when debugging autopurge_missing_checks=0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@test/deps/cluster_simulator/tests/aurora_tests_payloads/three_nodes_cluster-autopurge_missing_replica.json`
around lines 52 - 53, Update the scenario prose to match the fixture's expected
final state: change the "__details__" (or "__comment__") text that currently
says the missing reader stays "just OFFLINE" to state that the missing reader
remains "ONLINE" (or otherwise indicate it remains present but marked ONLINE) so
the description aligns with the assertions around the expected entry at lines
95-98; edit the "__details__" string in the JSON fixture accordingly.
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
|
I reviewed the PR locally on 1. Aurora autopurge updates
|
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/deps/cluster_simulator/lib/common_utils.cpp (3)
62-76: 💤 Low valueRemove dead code: duplicate
mysql_fetch_fieldscall and unusedlengthsvariable.Line 62 calls
mysql_fetch_fields()but discards the result (called again on line 64). Thelengthsvariable (lines 75-76) is fetched but never used.♻️ Proposed cleanup
void parse_result_to_json(MYSQL_RES *result, ordered_json& j) { if(!result) { return; } // Get the fields std::vector<std::string> field_names {}; - mysql_fetch_fields(result); int num_fields = mysql_num_fields(result); MYSQL_FIELD* fields = mysql_fetch_fields(result); for(int i = 0; i < num_fields; i++) { field_names.push_back(fields[i].name); } // Get each of the row values MYSQL_ROW row = nullptr; while ((row = mysql_fetch_row(result))) { ordered_json j_row {}; - unsigned long *lengths; - lengths = mysql_fetch_lengths(result); for(int i = 0; i < num_fields; i++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 62 - 76, Remove the dead code by deleting the redundant mysql_fetch_fields() call (the first call that discards its return) and stop fetching the unused lengths variable: remove the declaration "unsigned long *lengths;" and the call to mysql_fetch_lengths(result). Keep the single useful call to MYSQL_FIELD* fields = mysql_fetch_fields(result) and ensure the loop that populates field_names and the row-processing while loop (which uses MYSQL_ROW row and mysql_fetch_row(result)) remain unchanged.
412-421: 💤 Low valueSorting key omits port, causing undefined order for multi-port setups.
The comparator builds
srv_st_idfromhostgroup_id + hostnamebut excludesport. If multiple servers share the same hostgroup and hostname with different ports, sort order is undefined, potentially causingstd::equalto compare mismatched pairs.♻️ Proposed fix (also apply to other comparators at lines 423-432, 451-460, 462-471)
std::sort( c_exp_status.begin(), c_exp_status.end(), [] (const server_status& srv_st1, const server_status& srv_st2) -> bool { - const std::string srv_st1_id { std::to_string(std::get<0>(srv_st1)) + std::get<1>(srv_st1) }; - const std::string srv_st2_id { std::to_string(std::get<0>(srv_st2)) + std::get<1>(srv_st2) }; + const std::string srv_st1_id { std::to_string(std::get<0>(srv_st1)) + std::get<1>(srv_st1) + std::to_string(std::get<2>(srv_st1)) }; + const std::string srv_st2_id { std::to_string(std::get<0>(srv_st2)) + std::get<1>(srv_st2) + std::to_string(std::get<2>(srv_st2)) }; return srv_st1_id > srv_st2_id; } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 412 - 421, The comparator used in the std::sort of c_exp_status builds srv_st_id from std::to_string(std::get<0>(srv_st)) + std::get<1>(srv_st) and omits the port, which makes ordering undefined for servers that share hostgroup_id and hostname but differ by port; update the comparator lambda (and the other similar comparator lambdas in this file that build srv_st_id) to include the port (e.g., append ":" + std::to_string(std::get<2>(srv_st)) or otherwise incorporate std::get<2>()) into the key before comparing so sorting is deterministic and std::equal compares matching server tuples.
1365-1375: 💤 Low valueAvoid reserved identifier prefix
__in variable names.Identifiers starting with double underscore are reserved by the C++ standard for any use. While unlikely to cause issues in practice, this is technically undefined behavior.
♻️ Proposed fix
std::string get_fmt_time() { - time_t __timer; - char __buffer[30]; + time_t timer; + char buffer[30]; - struct tm __tm_info {}; - time(&__timer); - localtime_r(&__timer, &__tm_info); - strftime(__buffer, 25, "%Y-%m-%d %H:%M:%S", &__tm_info); + struct tm tm_info {}; + time(&timer); + localtime_r(&timer, &tm_info); + strftime(buffer, 25, "%Y-%m-%d %H:%M:%S", &tm_info); - return std::string(__buffer); + return std::string(buffer); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 1365 - 1375, The function get_fmt_time uses reserved identifiers (__timer, __buffer, __tm_info); rename these to non-reserved names (e.g., timer, buffer, tm_info) throughout the function and keep existing logic (time(), localtime_r, strftime) intact; update usages in get_fmt_time to reference the new names and ensure buffer size/strftime call remains consistent with the char array length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/deps/cluster_simulator/lib/common_utils.cpp`:
- Around line 874-879: The code calls mysql_store_result(proxysql_admin) to
populate MYSQL_RES* my_servers_res and passes it to parse_result_to_json but
never frees it; add a null-check for my_servers_res and call
mysql_free_result(my_servers_res) after parse_result_to_json to avoid the memory
leak (similar to the pattern used in get_current_cluster_status); reference the
symbols mysql_store_result, my_servers_res, parse_result_to_json and
mysql_free_result when making the change.
---
Nitpick comments:
In `@test/deps/cluster_simulator/lib/common_utils.cpp`:
- Around line 62-76: Remove the dead code by deleting the redundant
mysql_fetch_fields() call (the first call that discards its return) and stop
fetching the unused lengths variable: remove the declaration "unsigned long
*lengths;" and the call to mysql_fetch_lengths(result). Keep the single useful
call to MYSQL_FIELD* fields = mysql_fetch_fields(result) and ensure the loop
that populates field_names and the row-processing while loop (which uses
MYSQL_ROW row and mysql_fetch_row(result)) remain unchanged.
- Around line 412-421: The comparator used in the std::sort of c_exp_status
builds srv_st_id from std::to_string(std::get<0>(srv_st)) + std::get<1>(srv_st)
and omits the port, which makes ordering undefined for servers that share
hostgroup_id and hostname but differ by port; update the comparator lambda (and
the other similar comparator lambdas in this file that build srv_st_id) to
include the port (e.g., append ":" + std::to_string(std::get<2>(srv_st)) or
otherwise incorporate std::get<2>()) into the key before comparing so sorting is
deterministic and std::equal compares matching server tuples.
- Around line 1365-1375: The function get_fmt_time uses reserved identifiers
(__timer, __buffer, __tm_info); rename these to non-reserved names (e.g., timer,
buffer, tm_info) throughout the function and keep existing logic (time(),
localtime_r, strftime) intact; update usages in get_fmt_time to reference the
new names and ensure buffer size/strftime call remains consistent with the char
array length.
🪄 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
Run ID: 78b12988-6431-415a-adaf-ae65f8742981
📒 Files selected for processing (4)
lib/MySQL_Monitor.cpptest/deps/cluster_simulator/lib/common_utils.cpptest/tap/groups/groups.jsontest/tap/tests/reg_test_5546-shun_recovery-t.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/MySQL_Monitor.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_) for class names
Use snake_case for member variables
Use UPPER_SNAKE_CASE for constants and macros
Require C++17 standard for all code
Use conditional compilation via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSEfor feature-gated code
Use RAII for resource management throughout the codebase
Use jemalloc for memory allocation
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/deps/cluster_simulator/lib/common_utils.cpp
🔇 Additional comments (2)
test/deps/cluster_simulator/lib/common_utils.cpp (2)
941-957: Critical:extract_cluster_statusstill has inverted presence check and wrong expected type.The bugs flagged in the previous review are still present:
- Line 943 checks for
json::value_t::object, but the payload is an array (confirmed byis_array()check on line 960).- Line 946 returns an error when the field IS present (
if (has_proxysql_init_state)), which is inverted logic.
355-391: LGTM! Port comparison bug has been fixed.The comparison at line 360 now correctly compares
std::get<2>(srv_st1) == std::get<2>(srv_st2), ensuring the port field is properly validated between the two server statuses.
| if (q_res == 0) { | ||
| ordered_json j_servers {}; | ||
| j_servers["mysql_servers"] = {}; | ||
|
|
||
| MYSQL_RES* my_servers_res = mysql_store_result(proxysql_admin); | ||
| parse_result_to_json(my_servers_res, j_servers["mysql_servers"]); |
There was a problem hiding this comment.
Memory leak: my_servers_res is never freed.
mysql_store_result() allocates a result set that must be freed with mysql_free_result(). Compare with get_current_cluster_status (line 1048) which correctly frees its result.
🐛 Proposed fix
if (q_res == 0) {
ordered_json j_servers {};
j_servers["mysql_servers"] = {};
MYSQL_RES* my_servers_res = mysql_store_result(proxysql_admin);
parse_result_to_json(my_servers_res, j_servers["mysql_servers"]);
+ mysql_free_result(my_servers_res);
// convert the fields into the proper types🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 874 - 879, The
code calls mysql_store_result(proxysql_admin) to populate MYSQL_RES*
my_servers_res and passes it to parse_result_to_json but never frees it; add a
null-check for my_servers_res and call mysql_free_result(my_servers_res) after
parse_result_to_json to avoid the memory leak (similar to the pattern used in
get_current_cluster_status); reference the symbols mysql_store_result,
my_servers_res, parse_result_to_json and mysql_free_result when making the
change.
- `check_table_structure`, `build_table` and `check_and_build_table` never mutate their string arguments, so widen the parameter types from `char *` to `const char *`. - Drop the matching C-style casts at every call site that stripped const from string literals. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
- Drop placeholder `// TODO: Improve` comments in catch blocks that carry no actionable information. - Change some of the `TODO` markers to `Note`. - Replace empty `@brief TODO` Doxygen stubs with proper doxygen comments. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ProxySQL_Statistics.cpp (1)
1919-1939:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDNS resolution blocks the probe timeout.
getaddrinfo()is a blocking call that runs before the non-blocking connect/poll path. The 1s poll timeout only covers the TCP handshake attempt, not DNS resolution time. A slow resolver will stall the entiretsdb_monitor_loopbatch beyond the configured monitor interval.🤖 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 `@lib/ProxySQL_Statistics.cpp` around lines 1919 - 1939, The blocking getaddrinfo call before the non-blocking connect can make the whole probe exceed the monitor interval; wrap the blocking DNS resolution in an asynchronous call with a bounded timeout (e.g., use std::async or a separate thread/future and wait_for) instead of calling getaddrinfo directly in the probe path, so if the resolution does not complete within the configured probe timeout you skip/fail the probe early; locate the getaddrinfo(...) invocation in ProxySQL_Statistics.cpp (the res variable and subsequent loop over ai) and replace it with an async-resolve that returns success/failure within the timeout, ensure you still call freeaddrinfo(res) on success and handle the case where resolution timed out by not attempting connect and recording a timed-out probe for tsdb_monitor_loop.lib/sqlite3db.cpp (1)
549-555:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInitialize
statementtonullptrinSQLite3DB::check_table_structure.At lines 549–555,
statementis declared without initialization, thensqlite3_finalize()is called on it ifsqlite3_prepare_v2()fails. Sincesqlite3_prepare_v2()does not write to the output parameter on failure, the pointer remains uninitialized. Callingsqlite3_finalize()with an uninitialized pointer causes undefined behavior (segfault or heap corruption). SQLite only guarantees safe behavior whenfinalize()receives eitherNULL(harmless no-op) or a valid prepared statement handle.Initialize
statementtonullptrat declaration so the finalize call is safe on the error path.Suggested fix
- sqlite3_stmt *statement; + sqlite3_stmt *statement = nullptr; char *buff=(char *)calloc(1,l); sprintf(buff, q1, table_name , table_def); if((*proxy_sqlite3_prepare_v2)(db, buff, -1, &statement, 0) != SQLITE_OK) { proxy_debug(PROXY_DEBUG_SQLITE, 1, "SQLITE: Error on (*proxy_sqlite3_prepare_v2)() running query \"%s\" : %s\n", buff, (*proxy_sqlite3_errmsg)(db)); (*proxy_sqlite3_finalize)(statement); free(buff); assert(0); }🤖 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 `@lib/sqlite3db.cpp` around lines 549 - 555, The local variable `statement` in SQLite3DB::check_table_structure is used in the error path without being initialized; set `sqlite3_stmt *statement = nullptr;` at declaration so that when (*proxy_sqlite3_prepare_v2)(...) fails the subsequent call to (*proxy_sqlite3_finalize)(statement) is safe (finalize accepts NULL) and avoids undefined behavior; update the declaration near the prepare call that uses proxy_sqlite3_prepare_v2 and proxy_sqlite3_finalize accordingly.
♻️ Duplicate comments (9)
test/deps/cluster_simulator/lib/common_utils.cpp (2)
873-879:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMemory leak:
my_servers_resis still not freed inget_current_mysql_servers.
mysql_store_result()allocates a result that must be released viamysql_free_result(). The analogousget_current_cluster_status(Line 1046) already does this; this path does not.🐛 Proposed fix
MYSQL_RES* my_servers_res = mysql_store_result(proxysql_admin); parse_result_to_json(my_servers_res, j_servers["mysql_servers"]); + mysql_free_result(my_servers_res);🤖 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 `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 873 - 879, In get_current_mysql_servers, after calling MYSQL_RES* my_servers_res = mysql_store_result(proxysql_admin) and passing it to parse_result_to_json(j_servers["mysql_servers"]), ensure you call mysql_free_result(my_servers_res) to release the allocated result (and guard against nullptr before freeing if needed); this mirrors the cleanup done in get_current_cluster_status and prevents the memory leak.
940-970:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
extract_cluster_statusstill has an inverted presence check and wrong expected type.The previously flagged bugs remain in the current code:
- Line 942 asks
check_present_and_type(..., json::value_t::object), but the payload is an array (confirmed byis_array()at Line 959 and the for-loop treating entries as objects).- Line 945 branches on
if (has_proxysql_init_state)and returns "Unable to find required field" — the function errors out exactly when the field IS present and falls through when it is missing.🐛 Proposed fix
bool has_proxysql_init_state = check_present_and_type( - galera_test_def, { json_key }, json::value_t::object + galera_test_def, { json_key }, json::value_t::array ); - if (has_proxysql_init_state) { + if (!has_proxysql_init_state) { std::string t_err_msg { "Invalid input. Unable to find required field '%s'" };🤖 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 `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 940 - 970, In extract_cluster_status change the presence/type check to expect an array and invert the branch: call check_present_and_type(galera_test_def, { json_key }, json::value_t::array) to set has_proxysql_init_state, then if (!has_proxysql_init_state) return the "Unable to find required field '%s'" error; keep the subsequent ordered_json m_proxysql_init_state = galera_test_def[json_key] and the is_array() guard (or remove redundant is_array() after the corrected check) so the code treats m_proxysql_init_state as an array as intended.test/deps/cluster_simulator/lib/galera_utils.cpp (3)
140-140:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: comment
%splaceholder is still unquoted in the INSERT template.
t_galera_hosgroup_insertends with..., %d, %d, %d, %s)whileextract_galera_hostgroup_configonly forcescomment = "NULL"for null payload values and otherwise preserves the raw string. Any non-null comment substitutes as bare text, producing SQL like..., 0, 0, my comment)— a syntax error. The grouprep template uses'%s'; this path should match.🐛 Proposed fix
- " %d, %d, %d, %d, %d, %d, %d, %d, %s" + " %d, %d, %d, %d, %d, %d, %d, %d, '%s'"🤖 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 `@test/deps/cluster_simulator/lib/galera_utils.cpp` at line 140, The INSERT printf format in t_galera_hosgroup_insert ends with an unquoted %s which causes raw comments to produce invalid SQL; update the format string to quote the comment placeholder (use '%s' instead of %s) so it matches the grouprep path, and ensure extract_galera_hostgroup_config still returns the literal "NULL" (without quotes) for absent payloads while non-null comments are safely passed into the quoted '%s' slot in t_galera_hosgroup_insert to avoid syntax errors; adjust any surrounding formatting logic in t_galera_hosgroup_insert and keep extract_galera_hostgroup_config behavior unchanged.
808-885:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
galera_update_statecopy-paste bugs + identity loss persist.Four slots are still being written from the wrong source variable, and
resultis still default-constructed (sohostgroup_id,hostname,portare returned as zeros/empty whenever this function is consulted):
- Line 830:
read_onlyslot ←st2_wsrep_local_state(should best2_read_only).- Line 847:
wsrep_desyncslot ←st2_wsrep_local_recv_queue(should best2_wsrep_desync).- Line 874:
wsrep_cluster_statusslot ←st2_wsrep_sst_donor_rejects_queries(should best2_cluster_status).- Line 881:
pxc_maint_modeslot ←st2_wsrep_sst_donor_rejects_queries(should best2_pxc_maint_mode).🐛 Proposed fix
- galera_server_state result {}; + galera_server_state result { st1 }; ... if (st2_read_only != -1 && st1_read_only != st2_read_only) { - std::get<4>(result) = st2_wsrep_local_state; + std::get<4>(result) = st2_read_only; } ... if (st2_wsrep_desync != -1 && st1_wsrep_desync != st2_wsrep_desync) { - std::get<6>(result) = st2_wsrep_local_recv_queue; + std::get<6>(result) = st2_wsrep_desync; } ... if (st2_cluster_status != "NULL" && st1_cluster_status != st2_cluster_status) { - std::get<9>(result) = st2_wsrep_sst_donor_rejects_queries; + std::get<9>(result) = st2_cluster_status; } ... if (st2_pxc_maint_mode != "NULL" && st1_pxc_maint_mode != st2_pxc_maint_mode) { - std::get<10>(result) = st2_wsrep_sst_donor_rejects_queries; + std::get<10>(result) = st2_pxc_maint_mode; }🤖 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 `@test/deps/cluster_simulator/lib/galera_utils.cpp` around lines 808 - 885, galera_update_state currently returns a default-constructed result (losing identity fields) and has multiple copy-paste errors writing the wrong source variables into tuple slots; fix by initializing result from st1 for the immutable identity fields (copy std::get<0/1/2>(st1) into std::get<0/1/2>(result)), and correct the four wrong assignments inside galera_update_state so that std::get<4>(result) is set from st2_read_only, std::get<6>(result) is set from st2_wsrep_desync, std::get<9>(result) is set from st2_cluster_status, and std::get<10>(result) is set from st2_pxc_maint_mode (leave the existing conditional checks intact).
778-789:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
galera_state_members_diffstill pushesst1_wsrep_sst_donor_rejects_queries(the old value).Every other field in this function reports
st2_...; onlywsrep_sst_donor_rejects_queriesreports the prior value, so its diff entry describes the wrong side of the change.🐛 Proposed fix
result.push_back({ "wsrep_sst_donor_rejects_queries", - std::to_string(st1_wsrep_sst_donor_rejects_queries) + std::to_string(st2_wsrep_sst_donor_rejects_queries) });🤖 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 `@test/deps/cluster_simulator/lib/galera_utils.cpp` around lines 778 - 789, In galera_state_members_diff, the diff entry for wsrep_sst_donor_rejects_queries incorrectly pushes st1_wsrep_sst_donor_rejects_queries (the old value) instead of the new value; update the result.push_back call to use st2_wsrep_sst_donor_rejects_queries so the diff reflects the new state. Locate the block comparing st2_wsrep_sst_donor_rejects_queries and st1_wsrep_sst_donor_rejects_queries and replace the value passed to std::to_string in the result.push_back for "wsrep_sst_donor_rejects_queries" from st1_wsrep_sst_donor_rejects_queries to st2_wsrep_sst_donor_rejects_queries.test/deps/cluster_simulator/lib/replicationlag_utils.cpp (2)
441-464:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
replicationlag_update_statestill loses identity and comparesshared_ptraddresses.Both previously flagged issues remain unresolved:
resultis default-constructed, so the returned tuple has emptyhostnameandport = 0.replicationlag_update_cluster_statethen pushes these identity-less tuples into its result.- Line 456 compares
st1_replicationlag != st2_replicationlag— i.e., the rawshared_ptraddresses, not their pointees. Same defect in pointer-vs-value comparison.🐛 Proposed fix
- replicationlag_server_state result {}; + replicationlag_server_state result { st1 }; const std::shared_ptr<int>& st1_replicationlag = std::get<2>(st1); const std::shared_ptr<int>& st2_replicationlag = std::get<2>(st2); if (st1_replicationlag != nullptr && st2_replicationlag != nullptr) { - if (st1_replicationlag != st2_replicationlag) { + if (*st1_replicationlag != *st2_replicationlag) { std::get<2>(result) = st2_replicationlag; } } else { std::get<2>(result) = st2_replicationlag; }🤖 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 `@test/deps/cluster_simulator/lib/replicationlag_utils.cpp` around lines 441 - 464, The function replicationlag_update_state currently returns a default-constructed replicationlag_server_state (losing hostname/port identity) and compares shared_ptr addresses instead of their pointees; fix it by initializing result from st1 (copy hostname/port and existing fields) then update only the replicationlag field: obtain st1_replicationlag and st2_replicationlag (std::get<2> on st1/st2), and if both are non-null compare *st1_replicationlag and *st2_replicationlag (value comparison) — if values differ assign st2_replicationlag into std::get<2>(result); if one is null assign st2_replicationlag into std::get<2>(result) as well; leave hostname/port from st1 untouched (they are part of the identity).
197-208:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: post-query type conversion in
get_current_replicationlag_servers_stateis still broken.The previously flagged issues are still present in the current code:
- Line 198 calls
atoionhostname, replacing the string with an integer — downstreamextract_replicationlag_servers_stateexpects a string.- Line 207 reads
j_server["comment"], but the SELECT at Line 185 returns onlyhostname, port, seconds_behind_master. Bracket-access on a missing key inserts JSONnull, thenstd::string{ null }throws.my_servers_resfrommysql_store_resultis also never freed (compare withget_current_cluster_statusincommon_utils.cppLine 1046).🐛 Proposed fix
MYSQL_RES* my_servers_res = mysql_store_result(proxysql_admin); parse_result_to_json(my_servers_res, j_servers["replicationlag_servers_init_state"]); + mysql_free_result(my_servers_res); // convert the fields into the proper types for (ordered_json& j_server : j_servers["replicationlag_servers_init_state"]) { - j_server["hostname"] = atoi(std::string {j_server["hostname"]}.c_str()); + j_server["hostname"] = std::string { j_server["hostname"] }; j_server["port"] = atoi(std::string {j_server["port"]}.c_str()); const auto& seconds_behind_master = j_server.at("seconds_behind_master"); if (seconds_behind_master == nullptr || seconds_behind_master.is_null()) { j_server["seconds_behind_master"] = nullptr; } else { j_server["seconds_behind_master"] = atoi(std::string {j_server["seconds_behind_master"]}.c_str()); } - j_server["comment"] = atoi(std::string {j_server["comment"]}.c_str()); }🤖 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 `@test/deps/cluster_simulator/lib/replicationlag_utils.cpp` around lines 197 - 208, In get_current_replicationlag_servers_state: stop converting hostname to an integer (remove the atoi on j_server["hostname"]) so extract_replicationlag_servers_state continues to receive a string; guard access to "comment" (do not blindly read j_server["comment"] with std::string if the key is missing—only convert if key exists and is not null, otherwise leave absent/null); keep the existing handling for seconds_behind_master but avoid constructing std::string from a null JSON value (check is_null() before converting); and ensure the MYSQL_RES* my_servers_res returned by mysql_store_result is freed with mysql_free_result(my_servers_res) before returning—these changes touch get_current_replicationlag_servers_state, the local j_server handling loop, and the my_servers_res cleanup to prevent crashes and leaks.test/deps/cluster_simulator/lib/aurora_utils.cpp (2)
273-281:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
session_id = true;still assigns a bool tostd::string.Line 274 still assigns the bool literal
truetostring session_id. Throughbool → int → charthis compiles silently and stores the one-byte string"\x01", which then participates in identity diffs and theREPLICA_HOST_STATUSINSERT. The downstreamaurora_update_state/aurora_state_members_diffuse""as the "ignore" sentinel forSESSION_ID(Lines 505, 568), so the intent here is almost certainly"".🐛 Proposed fix
if (m_session_id == nullptr) { - session_id = true; + session_id = ""; } else {🤖 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 `@test/deps/cluster_simulator/lib/aurora_utils.cpp` around lines 273 - 281, The code incorrectly assigns the bool literal true to the std::string variable session_id when m_session_id == nullptr, producing a one-byte "\x01" instead of the intended empty-string sentinel; change the assignment in that branch to session_id = "" (empty string) so session_id is a proper std::string sentinel consistent with how SESSION_ID is checked in aurora_update_state and aurora_state_members_diff, and ensure no other branches assign non-string types to session_id (e.g., the try branch that reads server_state["SESSION_ID"] remains unchanged).
107-150:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
j_commentis still never assigned — every row getscomment = 'NULL'.
j_commentis default-constructed as a JSON null on Line 108 and nothing ever writes to it before thej_comment == nullptrtest at Line 142 — so theelsebranch (which would actually readj_aurora_hg.at("comment")) is dead. Every inserted row gets the string literal"NULL". Combined with the single-quoted'%s'formatting int_aurora_hostgroup_insert(Line 57), an actual SQLNULLcannot be produced through this path.🐛 Proposed fix
try { ... new_reader_weight = j_aurora_hg.at("new_reader_weight"); + + if (j_aurora_hg.find("comment") != j_aurora_hg.end()) { + j_comment = j_aurora_hg.at("comment"); + } ... } catch (const std::exception& e) { return { EXIT_FAILURE, e.what() }; } - if (j_comment == nullptr) { + if (j_comment.is_null()) { comment = "NULL"; } else { - try { - comment = j_aurora_hg.at("comment"); - } catch (const std::exception& e) { - return { EXIT_FAILURE, e.what() }; - } + comment = j_comment.get<std::string>(); }🤖 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 `@test/deps/cluster_simulator/lib/aurora_utils.cpp` around lines 107 - 150, j_comment is never initialized from j_aurora_hg so the NULL-check always takes the "NULL" branch; fix by reading the "comment" field from j_aurora_hg into j_comment before the j_comment == nullptr test (use j_aurora_hg.find("comment") to detect presence and assign either the value or json(nullptr)), and then set the string variable comment only after that check; additionally ensure t_aurora_hostgroup_insert (the SQL formatting that currently uses a single-quoted '%s') can emit a true SQL NULL (remove the surrounding quotes or switch to an explicit NULL token when comment is null) so real NULL values are inserted.
🤖 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 `@test/deps/cluster_simulator/lib/common_utils.cpp`:
- Around line 336-354: check_present_and_type currently always queries the root
json `j` for each `step` and never descends; change the traversal to start with
`json cur_j = j;` and for each `step` check `cur_j.contains(step)` then set
`cur_j = cur_j.at(step)` (not `j.at(step)`), and after consuming the final
element compare `type == cur_j.type()` and return that result; remove the bogus
address comparison `&step == &path.back()` and instead detect the final step by
index or by checking after advancing whether you've reached the last element;
return false on any missing step.
In `@test/deps/cluster_simulator/lib/galera_utils.cpp`:
- Around line 319-326: The cleanup DELETE uses "WHERE hostname NOT IN (...) AND
port NOT IN (...)" which only removes rows missing both values; change it so
rows whose (hostname,port) pair is not in the new set are removed. Update the
cleanup_query construction in galera_utils.cpp (the cleanup_query variable used
with mysql_query) to either: 1) build a tuple-based predicate "DELETE FROM
HOST_STATUS_GALERA WHERE (hostname,port) NOT IN ((h1,p1),(h2,p2),...)" matching
the srv_hostnames/srv_ports pairs, or 2) simplify to "DELETE FROM
HOST_STATUS_GALERA" and let the subsequent INSERTs repopulate; ensure the
replacement string correctly formats quotes/commas and is passed to mysql_query
exactly where query_error is checked.
In `@test/deps/cluster_simulator/lib/grouprep_utils.cpp`:
- Around line 478-484: The DELETE uses "hostname NOT IN (...) AND port NOT IN
(...)" which keeps rows that match any new port but incorrect hostname; instead
delete rows whose (hostname,port) pair is not among the supplied servers. In the
cleanup==1 branch (the cleanup_query construction in grouprep_utils.cpp), build
a composite-condition or tuple IN list and use "WHERE (hostname, port) NOT IN
((h1,p1),(h2,p2),...)" (or, if your SQL dialect doesn't support tuple IN,
generate explicit ORed mismatch checks per pair) so rows are removed unless
their exact (hostname,port) matches one of the provided server pairs.
- Around line 714-747: The function grouprep_update_state constructs result with
empty hostname/port so callers get "ghost" servers; fix by initializing result
from st1 (e.g., copy the full tuple from st1) then apply the conditional updates
for viable_candidate/read_only/transactions_behind from st2 so identity
(hostname/port) is preserved when grouprep_update_cluster_state pushes it. Also
fix the sentinel checks: viable_candidate and read_only are bools so tests
against -1 are invalid—either make those fields tri-state (std::optional or
enum) or change the update logic to compare st2 values against st1 (or use an
explicit "no-change" marker) so updates only occur when st2 intends a change.
---
Outside diff comments:
In `@lib/ProxySQL_Statistics.cpp`:
- Around line 1919-1939: The blocking getaddrinfo call before the non-blocking
connect can make the whole probe exceed the monitor interval; wrap the blocking
DNS resolution in an asynchronous call with a bounded timeout (e.g., use
std::async or a separate thread/future and wait_for) instead of calling
getaddrinfo directly in the probe path, so if the resolution does not complete
within the configured probe timeout you skip/fail the probe early; locate the
getaddrinfo(...) invocation in ProxySQL_Statistics.cpp (the res variable and
subsequent loop over ai) and replace it with an async-resolve that returns
success/failure within the timeout, ensure you still call freeaddrinfo(res) on
success and handle the case where resolution timed out by not attempting connect
and recording a timed-out probe for tsdb_monitor_loop.
In `@lib/sqlite3db.cpp`:
- Around line 549-555: The local variable `statement` in
SQLite3DB::check_table_structure is used in the error path without being
initialized; set `sqlite3_stmt *statement = nullptr;` at declaration so that
when (*proxy_sqlite3_prepare_v2)(...) fails the subsequent call to
(*proxy_sqlite3_finalize)(statement) is safe (finalize accepts NULL) and avoids
undefined behavior; update the declaration near the prepare call that uses
proxy_sqlite3_prepare_v2 and proxy_sqlite3_finalize accordingly.
---
Duplicate comments:
In `@test/deps/cluster_simulator/lib/aurora_utils.cpp`:
- Around line 273-281: The code incorrectly assigns the bool literal true to the
std::string variable session_id when m_session_id == nullptr, producing a
one-byte "\x01" instead of the intended empty-string sentinel; change the
assignment in that branch to session_id = "" (empty string) so session_id is a
proper std::string sentinel consistent with how SESSION_ID is checked in
aurora_update_state and aurora_state_members_diff, and ensure no other branches
assign non-string types to session_id (e.g., the try branch that reads
server_state["SESSION_ID"] remains unchanged).
- Around line 107-150: j_comment is never initialized from j_aurora_hg so the
NULL-check always takes the "NULL" branch; fix by reading the "comment" field
from j_aurora_hg into j_comment before the j_comment == nullptr test (use
j_aurora_hg.find("comment") to detect presence and assign either the value or
json(nullptr)), and then set the string variable comment only after that check;
additionally ensure t_aurora_hostgroup_insert (the SQL formatting that currently
uses a single-quoted '%s') can emit a true SQL NULL (remove the surrounding
quotes or switch to an explicit NULL token when comment is null) so real NULL
values are inserted.
In `@test/deps/cluster_simulator/lib/common_utils.cpp`:
- Around line 873-879: In get_current_mysql_servers, after calling MYSQL_RES*
my_servers_res = mysql_store_result(proxysql_admin) and passing it to
parse_result_to_json(j_servers["mysql_servers"]), ensure you call
mysql_free_result(my_servers_res) to release the allocated result (and guard
against nullptr before freeing if needed); this mirrors the cleanup done in
get_current_cluster_status and prevents the memory leak.
- Around line 940-970: In extract_cluster_status change the presence/type check
to expect an array and invert the branch: call
check_present_and_type(galera_test_def, { json_key }, json::value_t::array) to
set has_proxysql_init_state, then if (!has_proxysql_init_state) return the
"Unable to find required field '%s'" error; keep the subsequent ordered_json
m_proxysql_init_state = galera_test_def[json_key] and the is_array() guard (or
remove redundant is_array() after the corrected check) so the code treats
m_proxysql_init_state as an array as intended.
In `@test/deps/cluster_simulator/lib/galera_utils.cpp`:
- Line 140: The INSERT printf format in t_galera_hosgroup_insert ends with an
unquoted %s which causes raw comments to produce invalid SQL; update the format
string to quote the comment placeholder (use '%s' instead of %s) so it matches
the grouprep path, and ensure extract_galera_hostgroup_config still returns the
literal "NULL" (without quotes) for absent payloads while non-null comments are
safely passed into the quoted '%s' slot in t_galera_hosgroup_insert to avoid
syntax errors; adjust any surrounding formatting logic in
t_galera_hosgroup_insert and keep extract_galera_hostgroup_config behavior
unchanged.
- Around line 808-885: galera_update_state currently returns a
default-constructed result (losing identity fields) and has multiple copy-paste
errors writing the wrong source variables into tuple slots; fix by initializing
result from st1 for the immutable identity fields (copy std::get<0/1/2>(st1)
into std::get<0/1/2>(result)), and correct the four wrong assignments inside
galera_update_state so that std::get<4>(result) is set from st2_read_only,
std::get<6>(result) is set from st2_wsrep_desync, std::get<9>(result) is set
from st2_cluster_status, and std::get<10>(result) is set from st2_pxc_maint_mode
(leave the existing conditional checks intact).
- Around line 778-789: In galera_state_members_diff, the diff entry for
wsrep_sst_donor_rejects_queries incorrectly pushes
st1_wsrep_sst_donor_rejects_queries (the old value) instead of the new value;
update the result.push_back call to use st2_wsrep_sst_donor_rejects_queries so
the diff reflects the new state. Locate the block comparing
st2_wsrep_sst_donor_rejects_queries and st1_wsrep_sst_donor_rejects_queries and
replace the value passed to std::to_string in the result.push_back for
"wsrep_sst_donor_rejects_queries" from st1_wsrep_sst_donor_rejects_queries to
st2_wsrep_sst_donor_rejects_queries.
In `@test/deps/cluster_simulator/lib/replicationlag_utils.cpp`:
- Around line 441-464: The function replicationlag_update_state currently
returns a default-constructed replicationlag_server_state (losing hostname/port
identity) and compares shared_ptr addresses instead of their pointees; fix it by
initializing result from st1 (copy hostname/port and existing fields) then
update only the replicationlag field: obtain st1_replicationlag and
st2_replicationlag (std::get<2> on st1/st2), and if both are non-null compare
*st1_replicationlag and *st2_replicationlag (value comparison) — if values
differ assign st2_replicationlag into std::get<2>(result); if one is null assign
st2_replicationlag into std::get<2>(result) as well; leave hostname/port from
st1 untouched (they are part of the identity).
- Around line 197-208: In get_current_replicationlag_servers_state: stop
converting hostname to an integer (remove the atoi on j_server["hostname"]) so
extract_replicationlag_servers_state continues to receive a string; guard access
to "comment" (do not blindly read j_server["comment"] with std::string if the
key is missing—only convert if key exists and is not null, otherwise leave
absent/null); keep the existing handling for seconds_behind_master but avoid
constructing std::string from a null JSON value (check is_null() before
converting); and ensure the MYSQL_RES* my_servers_res returned by
mysql_store_result is freed with mysql_free_result(my_servers_res) before
returning—these changes touch get_current_replicationlag_servers_state, the
local j_server handling loop, and the my_servers_res cleanup to prevent crashes
and leaks.
🪄 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
Run ID: 710ab782-2fb0-4c4b-b124-591fefc3bb7f
📒 Files selected for processing (21)
.gitignoreinclude/sqlite3db.hlib/ProxySQL_Admin.cpplib/ProxySQL_Admin_Disk_Upgrade.cpplib/ProxySQL_Statistics.cpplib/sqlite3db.cpptest/deps/cluster_simulator/cluster_simulator.cpptest/deps/cluster_simulator/lib/aurora_utils.cpptest/deps/cluster_simulator/lib/aurora_utils.htest/deps/cluster_simulator/lib/common_utils.cpptest/deps/cluster_simulator/lib/common_utils.htest/deps/cluster_simulator/lib/galera_utils.cpptest/deps/cluster_simulator/lib/galera_utils.htest/deps/cluster_simulator/lib/grouprep_utils.cpptest/deps/cluster_simulator/lib/grouprep_utils.htest/deps/cluster_simulator/lib/readonly_utils.cpptest/deps/cluster_simulator/lib/readonly_utils.htest/deps/cluster_simulator/lib/replicationlag_utils.cpptest/tap/groups/groups.jsontest/tap/tests/sqlite3-t.cpptest/tap/tests/unit/admin_disk_upgrade_unit-t.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- test/deps/cluster_simulator/lib/grouprep_utils.h
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Member variables must use snake_case naming convention
Constants and macros must use UPPER_SNAKE_CASE naming convention
Use conditional compilation via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSEfor feature tier gating in core code
Files:
test/deps/cluster_simulator/lib/replicationlag_utils.cpplib/sqlite3db.cppinclude/sqlite3db.htest/deps/cluster_simulator/lib/aurora_utils.cpptest/deps/cluster_simulator/lib/common_utils.cpptest/deps/cluster_simulator/cluster_simulator.cpptest/deps/cluster_simulator/lib/galera_utils.cpptest/deps/cluster_simulator/lib/common_utils.htest/deps/cluster_simulator/lib/readonly_utils.htest/deps/cluster_simulator/lib/readonly_utils.cpptest/deps/cluster_simulator/lib/grouprep_utils.cpptest/deps/cluster_simulator/lib/galera_utils.hlib/ProxySQL_Admin.cpplib/ProxySQL_Admin_Disk_Upgrade.cpplib/ProxySQL_Statistics.cpptest/deps/cluster_simulator/lib/aurora_utils.h
**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards in header files must use
#ifndef __CLASS_*_Hpattern
Files:
include/sqlite3db.htest/deps/cluster_simulator/lib/common_utils.htest/deps/cluster_simulator/lib/readonly_utils.htest/deps/cluster_simulator/lib/galera_utils.htest/deps/cluster_simulator/lib/aurora_utils.h
🧠 Learnings (1)
📚 Learning: 2026-03-26T16:38:58.553Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:38:58.553Z
Learning: In `lib/mysql_connection.cpp`, when reviewing `SHOW WARNINGS` handling, treat the digest source as an intentional design choice: both `update_warning_count_from_connection()` and the `add_eof()` call under `ASYNC_USE_RESULT_CONT` detect warnings using `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This is expected to fail/behave differently when `mysql-query_digests_keep_comment=1` (digest_text includes comments) or when `mysql-query_digests=0` (digest_text unavailable). Do not require a change unless the regression test coverage is expanded (noting `reg_test_5306-show_warnings_with_comment-t` explicitly excludes these configurations as an accepted limitation).
Applied to files:
.gitignore
🪛 Cppcheck (2.20.0)
test/deps/cluster_simulator/cluster_simulator.cpp
[warning] 329-329: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 384-384: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[error] 326-326: If memory allocation fails
(nullPointerArithmeticOutOfMemory)
lib/ProxySQL_Admin_Disk_Upgrade.cpp
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (9)
.gitignore (1)
85-85: LGTM! Proper ignore rule for the cluster simulator binary.The addition correctly ignores the cluster_simulator executable built under test/deps/cluster_simulator/, preventing the test binary artifact from being committed to version control. The placement in the "#binary" section alongside other executable ignore rules is appropriate.
test/deps/cluster_simulator/cluster_simulator.cpp (3)
538-565: ⚡ Quick winREAD_ONLY: defaults still applied after
LOAD MYSQL VARIABLES TO RUNTIME.
set_monitor_variables_defaults(...)at lines 562-565 still runs after the runtime load on lines 540-550, so missing keys (e.g.monitor_writer_is_also_reader) keep whatever the previous scenario left in runtime. Same fix as previously suggested: default first, thenset_monitor_variables+LOAD MYSQL VARIABLES TO RUNTIME.
538-552: ⚡ Quick win
set_monitor_variables_errstill silently dropped on all four paths.In READ_ONLY (540-541), GROUP_REPLICATION (938-941), REPLICATION_LAG (1323-1326) and AWS_AURORA (1702-1705), the result of
set_monitor_variables(...)is stored inset_monitor_variables_errand then the very next thing the code does is runLOAD MYSQL VARIABLES TO RUNTIME. If the SET commands themselves failed, the simulator proceeds with stale runtime values and can report a false-positive verdict.Suggested fix (apply at each of the four sites)
set_monitor_variables_err = set_monitor_variables(proxysql_admin, ..., monitor_variables); + if (set_monitor_variables_err.first) { + result = internal_error(set_monitor_variables_err.second, __FILE__, __LINE__); + goto exit; + } load_to_run_err = mysql_query(proxysql_admin, "LOAD MYSQL VARIABLES TO RUNTIME");Also applies to: 937-952, 1321-1337, 1700-1715
658-676: ⚡ Quick win
simulatemode still emits non-canonicalcluster_typestrings.Line 664 emits
"replication"and line 1445 emits"replicationlag", butget_test_type()(lines 1952-1968) only accepts"READ_ONLY"and"REPLICATION_LAG". Payloads regenerated bysimulatemode for these two families cannot be replayed by the same simulator. The verbose branches (712, 1499) already emit the canonical values; thesimulatebranches should match.Also applies to: 1438-1456
test/deps/cluster_simulator/lib/grouprep_utils.cpp (2)
169-173: ⚡ Quick winSELECT still fetches 4 columns and references non-existent
check_type.
mysql_group_replication_hostgroupsdoesn't have acheck_typecolumn (so the query errors against a real ProxySQL admin), and the loop at lines 184-200 reads 9 columns from each parsed row. This function is unusable as-is.Fix
- "SELECT writer_hostgroup, reader_hostgroup, check_type, comment" - " FROM mysql_group_replication_hostgroups" + "SELECT writer_hostgroup, backup_writer_hostgroup, reader_hostgroup," + " offline_hostgroup, active, max_writers, writer_is_also_reader," + " max_transactions_behind, comment FROM mysql_group_replication_hostgroups"
360-369: ⚡ Quick win
read_only = "nullptr"still assigns a string literal to abool.
read_onlyis declaredboolat line 307; the literal"nullptr"decays to a non-nullconst char*and converts totrue, so the null branch silently forcesread_only=trueregardless of intent. Either drop the null-handling or change the tuple field to a tri-state. Same issue applies symmetrically toviable_candidate(alsobool); it’s harmless today only because the null branch defaults totrueanyway.test/deps/cluster_simulator/lib/readonly_utils.cpp (2)
330-367: ⚡ Quick win
get_current_readonly_servers_stateis still guaranteed to fail.The three issues from the prior review are all still present:
- Line 345 calls
atoi(...)on thehostnamefield, replacing the string with an int.- Line 348 reads
j_server["commetn"](typo) and converts to int viaatoi; even after fixing the typo,commentis not selected and is not part ofvalid_readonly_entries(lines 179-183), soextract_readonly_servers_statewould reject it.- The SELECT at line 332 only fetches
hostname, port, read_only— nocommentcolumn.Suggested fix
- for (ordered_json& j_server : j_servers["readonly_servers_init_state"]) { - j_server["hostname"] = atoi(std::string {j_server["hostname"]}.c_str()); - j_server["port"] = atoi(std::string {j_server["port"]}.c_str()); - j_server["read_only"] = atoi(std::string {j_server["read_only"]}.c_str()); - j_server["comment"] = atoi(std::string {j_server["commetn"]}.c_str()); - } + for (ordered_json& j_server : j_servers["readonly_servers_init_state"]) { + j_server["hostname"] = std::string { j_server["hostname"] }; + j_server["port"] = atoi(std::string { j_server["port"] }.c_str()); + j_server["read_only"] = atoi(std::string { j_server["read_only"] }.c_str()); + } + mysql_free_result(my_servers_res);
609-628: ⚡ Quick win
readonly_update_statestill loses hostname/port.
resultis default-constructed at line 613 (empty hostname, port = 0, read_only = 0); onlystd::get<2>(result)is written conditionally.readonly_update_cluster_state(lines 657-660) then pushes this into the returned vector, so any caller that uses identity from the result will see ghost servers.Suggested fix
readonly_server_state result {}; + // hostname and port are part of the server identity — preserve them. + std::get<0>(result) = std::get<0>(st1); + std::get<1>(result) = std::get<1>(st1); + std::get<2>(result) = std::get<2>(st1); int st1_read_only = std::get<2>(st1); int st2_read_only = std::get<2>(st2);lib/ProxySQL_Admin.cpp (1)
7514-7540: Aurora hostgroup INSERT/bind alignment looks correct.The 15-column statement and bind mapping (
autopurge_missing_checksat position 14,commentat 15) are consistent with the updated schema and sibling paths.
| bool check_present_and_type(const json& j, const std::vector<std::string>& path, const json::value_t& type) { | ||
| bool res = false; | ||
|
|
||
| json cur_j {}; | ||
|
|
||
| for (const auto& step : path) { | ||
| if (j.contains(step)) { | ||
| cur_j = j.at(step); | ||
|
|
||
| if (&step == &path.back()) { | ||
| return type == cur_j.type(); | ||
| } | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return res; | ||
| } |
There was a problem hiding this comment.
check_present_and_type doesn't actually traverse nested paths.
Despite the contract documented in the header (and the multi-step path parameter), the loop checks j.contains(step) and reads j.at(step) for every step, never recursing into cur_j. For a path like {"a", "b"} it asks whether the root contains "b", not whether j["a"] does.
All current callers happen to use single-element paths, so the bug is latent — but anyone relying on the documented behavior in the future will silently get wrong results.
🐛 Proposed fix
bool check_present_and_type(const json& j, const std::vector<std::string>& path, const json::value_t& type) {
- bool res = false;
-
- json cur_j {};
-
- for (const auto& step : path) {
- if (j.contains(step)) {
- cur_j = j.at(step);
-
- if (&step == &path.back()) {
- return type == cur_j.type();
- }
- } else {
- break;
- }
- }
-
- return res;
+ const json* cur = &j;
+ for (const auto& step : path) {
+ if (!cur->is_object() || !cur->contains(step)) {
+ return false;
+ }
+ cur = &cur->at(step);
+ }
+ return cur->type() == type;
}🤖 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 `@test/deps/cluster_simulator/lib/common_utils.cpp` around lines 336 - 354,
check_present_and_type currently always queries the root json `j` for each
`step` and never descends; change the traversal to start with `json cur_j = j;`
and for each `step` check `cur_j.contains(step)` then set `cur_j =
cur_j.at(step)` (not `j.at(step)`), and after consuming the final element
compare `type == cur_j.type()` and return that result; remove the bogus address
comparison `&step == &path.back()` and instead detect the final step by index or
by checking after advancing whether you've reached the last element; return
false on any missing step.
| std::string cleanup_query { | ||
| "DELETE FROM HOST_STATUS_GALERA WHERE hostname NOT IN (" + | ||
| srv_hostnames + ") AND port NOT IN (" + srv_ports + ")" | ||
| }; | ||
|
|
||
| // Cleanup the current servers from 'HOST_STATUS_GALERA' and insert the ones for testing the cluster | ||
| query_error = mysql_query(proxysql_sqlite, cleanup_query.c_str()); | ||
| if (query_error) { return create_query_error(proxysql_sqlite, cleanup_query, __FILE__, __LINE__); } |
There was a problem hiding this comment.
Cleanup query: AND should not span hostname/port — stale rows survive when only one component matches.
DELETE FROM HOST_STATUS_GALERA WHERE hostname NOT IN (...) AND port NOT IN (...) only deletes rows where both the hostname and port are absent from the new set. Any row sharing a hostname with one new server and a port with a different new server is silently retained, even though its (hostname, port) pair is not in the new list.
Example: new = [(host_a, 100)], existing extra row (host_a, 200) → host_a NOT IN ('host_a') is false, so the WHERE clause is false and the stale row remains.
The intent is "delete rows whose (hostname, port) is not in the new set". Either branch on the tuple, or just unconditionally DELETE FROM HOST_STATUS_GALERA and rely on the subsequent INSERTs.
🐛 Suggested fix
- std::string cleanup_query {
- "DELETE FROM HOST_STATUS_GALERA WHERE hostname NOT IN (" +
- srv_hostnames + ") AND port NOT IN (" + srv_ports + ")"
- };
+ // Delete every row whose (hostname, port) tuple is not part of the new set.
+ std::string cleanup_query { "DELETE FROM HOST_STATUS_GALERA" };
+ for (size_t i = 0; i < servers.size(); ++i) {
+ cleanup_query += i == 0 ? " WHERE NOT (" : " OR NOT (";
+ cleanup_query +=
+ "hostname = '" + std::get<1>(servers[i]) + "'"
+ " AND port = " + std::to_string(std::get<2>(servers[i])) + ")";
+ }🤖 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 `@test/deps/cluster_simulator/lib/galera_utils.cpp` around lines 319 - 326, The
cleanup DELETE uses "WHERE hostname NOT IN (...) AND port NOT IN (...)" which
only removes rows missing both values; change it so rows whose (hostname,port)
pair is not in the new set are removed. Update the cleanup_query construction in
galera_utils.cpp (the cleanup_query variable used with mysql_query) to either:
1) build a tuple-based predicate "DELETE FROM HOST_STATUS_GALERA WHERE
(hostname,port) NOT IN ((h1,p1),(h2,p2),...)" matching the
srv_hostnames/srv_ports pairs, or 2) simplify to "DELETE FROM
HOST_STATUS_GALERA" and let the subsequent INSERTs repopulate; ensure the
replacement string correctly formats quotes/commas and is passed to mysql_query
exactly where query_error is checked.
|
|
||
| if (cleanup == 1) { | ||
| cleanup_query = "DELETE FROM GR_MEMBER_ROUTING_CANDIDATE_STATUS WHERE hostname NOT IN (" + | ||
| srv_hostnames + ") AND port NOT IN (" + srv_ports + ")"; | ||
| } else { | ||
| cleanup_query = "DELETE FROM GR_MEMBER_ROUTING_CANDIDATE_STATUS"; | ||
| } |
There was a problem hiding this comment.
cleanup == 1 DELETE keeps unrelated rows that share a port.
DELETE FROM GR_MEMBER_ROUTING_CANDIDATE_STATUS
WHERE hostname NOT IN (...) AND port NOT IN (...)The AND is too narrow: a stale row with a hostname that isn't in the new list will still be retained as long as its port matches any of the new ports (3306 in practice for every entry). The intent is "delete rows whose (hostname, port) pair isn't among the supplied servers".
Suggested fix
- cleanup_query = "DELETE FROM GR_MEMBER_ROUTING_CANDIDATE_STATUS WHERE hostname NOT IN (" +
- srv_hostnames + ") AND port NOT IN (" + srv_ports + ")";
+ // Build "(hostname,port) NOT IN (('h1',p1),('h2',p2), ...)"
+ std::string pairs;
+ for (const auto& server : servers) {
+ if (!pairs.empty()) pairs += ",";
+ pairs += "('" + std::get<0>(server) + "'," +
+ std::to_string(std::get<1>(server)) + ")";
+ }
+ cleanup_query = "DELETE FROM GR_MEMBER_ROUTING_CANDIDATE_STATUS"
+ " WHERE (hostname, port) NOT IN (" + pairs + ")";🤖 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 `@test/deps/cluster_simulator/lib/grouprep_utils.cpp` around lines 478 - 484,
The DELETE uses "hostname NOT IN (...) AND port NOT IN (...)" which keeps rows
that match any new port but incorrect hostname; instead delete rows whose
(hostname,port) pair is not among the supplied servers. In the cleanup==1 branch
(the cleanup_query construction in grouprep_utils.cpp), build a
composite-condition or tuple IN list and use "WHERE (hostname, port) NOT IN
((h1,p1),(h2,p2),...)" (or, if your SQL dialect doesn't support tuple IN,
generate explicit ORed mismatch checks per pair) so rows are removed unless
their exact (hostname,port) matches one of the provided server pairs.
| grouprep_server_state grouprep_update_state( | ||
| const grouprep_server_state& st1, | ||
| const grouprep_server_state& st2 | ||
| ) { | ||
| grouprep_server_state result {}; | ||
|
|
||
| // hostname and port **can't** be changed, | ||
| // because the are part of the server 'id'. Only the | ||
| // other fields are allowed to change, otherwise, the | ||
| // verification step should have failed. | ||
|
|
||
| int st1_viable_candidate = std::get<2>(st1); | ||
| int st2_viable_candidate = std::get<2>(st2); | ||
|
|
||
| int st1_read_only = std::get<3>(st1); | ||
| int st2_read_only = std::get<3>(st2); | ||
|
|
||
| int st1_transactions_behind = std::get<4>(st1); | ||
| int st2_transactions_behind = std::get<4>(st2); | ||
|
|
||
| if (st2_viable_candidate != -1 && st1_viable_candidate != st2_viable_candidate) { | ||
| std::get<2>(result) = st2_viable_candidate; | ||
| } | ||
|
|
||
| if (st2_read_only != -1 && st1_read_only != st2_read_only) { | ||
| std::get<3>(result) = st2_read_only; | ||
| } | ||
|
|
||
| if (st2_transactions_behind != -1 && st1_transactions_behind != st2_transactions_behind) { | ||
| std::get<4>(result) = st2_transactions_behind; | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
grouprep_update_state returns ghost servers (hostname/port discarded).
result is default-constructed at line 718, so hostname (idx 0) and port (idx 1) stay empty/0. Only conditional fields are written, and grouprep_update_cluster_state then pushes this back at line 779. The returned vector is unusable for any caller that relies on identity. Same shape of bug already flagged for readonly_update_state, but the grouprep variant is independent and equally broken.
Suggested fix
grouprep_server_state result {};
+ // hostname and port are part of the server identity — preserve them.
+ std::get<0>(result) = std::get<0>(st1);
+ std::get<1>(result) = std::get<1>(st1);
+ // carry forward existing values; only override fields that legitimately changed.
+ std::get<2>(result) = std::get<2>(st1);
+ std::get<3>(result) = std::get<3>(st1);
+ std::get<4>(result) = std::get<4>(st1);
+ std::get<5>(result) = std::get<5>(st1);Note: viable_candidate/read_only are bool, so the != -1 sentinel guards (lines 734, 738) can never short-circuit; you may want to make those fields tri-state if you actually need a "no change" signal.
📝 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.
| grouprep_server_state grouprep_update_state( | |
| const grouprep_server_state& st1, | |
| const grouprep_server_state& st2 | |
| ) { | |
| grouprep_server_state result {}; | |
| // hostname and port **can't** be changed, | |
| // because the are part of the server 'id'. Only the | |
| // other fields are allowed to change, otherwise, the | |
| // verification step should have failed. | |
| int st1_viable_candidate = std::get<2>(st1); | |
| int st2_viable_candidate = std::get<2>(st2); | |
| int st1_read_only = std::get<3>(st1); | |
| int st2_read_only = std::get<3>(st2); | |
| int st1_transactions_behind = std::get<4>(st1); | |
| int st2_transactions_behind = std::get<4>(st2); | |
| if (st2_viable_candidate != -1 && st1_viable_candidate != st2_viable_candidate) { | |
| std::get<2>(result) = st2_viable_candidate; | |
| } | |
| if (st2_read_only != -1 && st1_read_only != st2_read_only) { | |
| std::get<3>(result) = st2_read_only; | |
| } | |
| if (st2_transactions_behind != -1 && st1_transactions_behind != st2_transactions_behind) { | |
| std::get<4>(result) = st2_transactions_behind; | |
| } | |
| return result; | |
| } | |
| grouprep_server_state grouprep_update_state( | |
| const grouprep_server_state& st1, | |
| const grouprep_server_state& st2 | |
| ) { | |
| grouprep_server_state result {}; | |
| // hostname and port are part of the server identity — preserve them. | |
| std::get<0>(result) = std::get<0>(st1); | |
| std::get<1>(result) = std::get<1>(st1); | |
| // carry forward existing values; only override fields that legitimately changed. | |
| std::get<2>(result) = std::get<2>(st1); | |
| std::get<3>(result) = std::get<3>(st1); | |
| std::get<4>(result) = std::get<4>(st1); | |
| std::get<5>(result) = std::get<5>(st1); | |
| // hostname and port **can't** be changed, | |
| // because the are part of the server 'id'. Only the | |
| // other fields are allowed to change, otherwise, the | |
| // verification step should have failed. | |
| int st1_viable_candidate = std::get<2>(st1); | |
| int st2_viable_candidate = std::get<2>(st2); | |
| int st1_read_only = std::get<3>(st1); | |
| int st2_read_only = std::get<3>(st2); | |
| int st1_transactions_behind = std::get<4>(st1); | |
| int st2_transactions_behind = std::get<4>(st2); | |
| if (st2_viable_candidate != -1 && st1_viable_candidate != st2_viable_candidate) { | |
| std::get<2>(result) = st2_viable_candidate; | |
| } | |
| if (st2_read_only != -1 && st1_read_only != st2_read_only) { | |
| std::get<3>(result) = st2_read_only; | |
| } | |
| if (st2_transactions_behind != -1 && st1_transactions_behind != st2_transactions_behind) { | |
| std::get<4>(result) = st2_transactions_behind; | |
| } | |
| return result; | |
| } |
🤖 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 `@test/deps/cluster_simulator/lib/grouprep_utils.cpp` around lines 714 - 747,
The function grouprep_update_state constructs result with empty hostname/port so
callers get "ghost" servers; fix by initializing result from st1 (e.g., copy the
full tuple from st1) then apply the conditional updates for
viable_candidate/read_only/transactions_behind from st2 so identity
(hostname/port) is preserved when grouprep_update_cluster_state pushes it. Also
fix the sentinel checks: viable_candidate and read_only are bools so tests
against -1 are invalid—either make those fields tri-state (std::optional or
enum) or change the update logic to compare st2 values against st1 (or use an
explicit "no-change" marker) so updates only occur when st2 intends a change.
|
|
Closing this in favor of #5760 |
2 participants
Summary
REPLICA_HOST_STATUSare removed frommysql_serversafter the configuredautopurge_missing_checksthreshold.cluster_simulatorfrom the Jenkins scripts repo intotest/deps/cluster_simulator/and wire it into the top-level Makefile somake test<cluster_type>builds bothsrc/proxysql(with-DTEST_<TYPE>)and the simulator binary.
cluster_sim_{aurora,galera,group_repl,read_only,repl_lag}-g1— each driving the simulator via TAP teststest_cluster_sim_<type>-t--add-hostsupport instart-proxysql-isolated.bash;.sqlpre-proxysql hook dispatcher and missing-infras.lsttolerance inensure-infras/destroy-infras.How to run
Validation
cluster_sim_aurora-g1cluster_sim_galera-g1cluster_sim_group_repl-g1cluster_sim_read_only-g1cluster_sim_repl_lag-g1Summary by CodeRabbit
New Features
Build / Test
New Configuration / Monitoring