aurora: Detect blue/green deployment switchovers via DNS mismatch#5802
aurora: Detect blue/green deployment switchovers via DNS mismatch#5802tabacco wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds DNS-cache IP-membership checks, targeted connection-pool purging by hostname, Aurora blue/green switchover detection that refreshes DNS and records failovers, a small decision helper, and TAP unit tests plus Makefile registration. ChangesAWS Aurora Blue/Green Switchover Detection
sequenceDiagram
participant AuroraMonitor
participant DNS_Cache
participant getaddrinfo
participant MySQL_Monitor_Connection_Pool
participant mysql_server_aws_aurora_failovers
AuroraMonitor->>DNS_Cache: contains_ip(hostname, peer_ip)
DNS_Cache-->>AuroraMonitor: true|false
AuroraMonitor->>getaddrinfo: resolve(hostname) [blocking]
getaddrinfo-->>AuroraMonitor: live_ips
AuroraMonitor->>MySQL_Monitor_Connection_Pool: purge_connections_for_hostnames(cluster_hostnames)
AuroraMonitor->>mysql_server_aws_aurora_failovers: insert(failover row)
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements AWS Aurora Blue/Green deployment switchover detection by verifying connection peer IPs against DNS records, adding support in the DNS_Cache and connection pool to purge stale connections, and including new unit tests. Feedback includes a critical security concern regarding unescaped hostnames in SQL queries, a performance optimization to avoid blocking DNS lookups when the cache is disabled, and a recommendation to reserve vector capacity to improve efficiency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@lib/MySQL_Monitor.cpp`:
- Around line 7049-7069: contains_ip currently returns false for both
cache-disabled/hostname-missing and for a cached hostname that lacks the given
IP; change it to return a tri-state so callers can distinguish a cache miss from
an explicit negative match. Update DNS_Cache::contains_ip to return either an
enum (e.g., CacheResult {CACHE_MISS, IP_PRESENT, IP_ABSENT}) or
std::optional<bool> (std::nullopt = cache miss, true = present, false = absent),
preserve the read-locking using rwlock_ and checks of enabled and
records/hostname lookup, and ensure all call sites that use contains_ip are
updated to handle the new tri-state result instead of assuming false means
absent.
- Around line 6368-6442: The DNS/stale-peer-IP detection currently runs after
evaluate_aws_aurora_results(), which allows stale REPLICA_HOST_STATUS to affect
writer/reader decisions for a whole iteration; move the entire peer-IP check
block (the code that calls get_connected_peer_ip_from_socket, checks
GloMyMon->dns_cache->contains_ip, performs getaddrinfo, compares IPs, logs
proxy_warning, calls GloMyMon->My_Conn_Pool->destroy_mysql_connection, purges
hg_hostnames, calls MySQL_Monitor::trigger_dns_cache_update, and
MyHGM->shun_and_killall) to run before evaluate_aws_aurora_results() and, when a
mismatch is detected, perform the purge/disconnect actions and immediately skip
the rest of the monitoring cycle (return or continue the loop) so
evaluate_aws_aurora_results() and subsequent decision logic do not run on stale
information.
- Around line 508-542: purge_connections_for_hostnames() only removes idle
connections from servers->conns but does not handle checked-out MYSQL* that will
later be returned by put_connection(), so implement a hostname
quarantine/generation scheme: add a per-hostname generation counter (e.g.,
extend MonMySrvC with an unsigned generation or maintain a quarantine map keyed
by srv->address), increment that generation when
purge_connections_for_hostnames() is called for a hostname, and on
put_connection(MYSQL *), check the returning connection's stored generation
against the current generation for its MonMySrvC (or check if its hostname is
currently quarantined); if generations mismatch or hostname is quarantined,
close the MYSQL* instead of re-adding it to conns, otherwise repool as normal.
Update the checkout path to tag/record the generation on the MYSQL* or in the
checkout metadata so the put_connection can compare, and ensure
purge_connections_for_hostnames() also sets the quarantine flag/advances
generation and closes existing idle conns in srv->conns as it already does.
- Line 6418: The write to force_dns_cache_update in
MySQL_Monitor::trigger_dns_cache_update() races with the unsynchronized
read/reset in monitor_dns_cache(); make the flag safe by changing
force_dns_cache_update to a std::atomic<bool> (or protect it with a
pthread_mutex_t) and update both MySQL_Monitor::trigger_dns_cache_update() and
monitor_dns_cache() to use atomic store/load (e.g., store(true) in trigger and
load+store(false) in monitor with appropriate memory_order acquire/release) or
wrap both accesses with the same pthread mutex to serialize the write and
read/reset per the project guideline.
🪄 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: 6574ce66-ed6e-4984-845a-32cf2c369ace
📒 Files selected for processing (4)
include/MySQL_Monitor.hpplib/MySQL_Monitor.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/dns_cache_unit-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
include/MySQL_Monitor.hpptest/tap/tests/unit/dns_cache_unit-t.cpplib/MySQL_Monitor.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards should use
#ifndef_CLASS*_H format
Files:
include/MySQL_Monitor.hpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should follow naming pattern test_*.cpp or *-t.cpp in test/tap/tests/
Files:
test/tap/tests/unit/dns_cache_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/dns_cache_unit-t.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-20T09:34:19.124Z
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:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/dns_cache_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/dns_cache_unit-t.cpp
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/dns_cache_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (4)
include/MySQL_Monitor.hpp (1)
432-438: LGTM!lib/MySQL_Monitor.cpp (1)
248-252: LGTM!test/tap/tests/unit/Makefile (1)
375-376: LGTM!test/tap/tests/unit/dns_cache_unit-t.cpp (1)
1-116: LGTM!Comprehensive test coverage for
DNS_Cache::contains_ip()across all relevant scenarios: basic multi-IP matching, unknown hostnames, cache updates during switchover, disabled cache, empty cache, IPv6, and post-removal. The TAP plan correctly accounts for all 16 assertions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/MySQL_Monitor.cpp`:
- Line 243: quarantined_hostnames currently never expires so once a host is
added pooling remains disabled; change its type to store expiry metadata (e.g.
std::unordered_map<std::string, std::chrono::steady_clock::time_point> or a
generation counter) and update the places that add/check it (the
put_connection() call path and the blocks around the symbols handling quarantine
at the later regions) so that when inserting you record an expiry = now +
QUARANTINE_DURATION and when checking you remove entries whose expiry <= now
(treat missing/expired entries as not quarantined). Ensure the same logic is
applied to the other quarantine usages referenced around lines 436-449 and
530-532 so pre-switchover connections are rejected but post-switchover
connections can be pooled again.
🪄 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: 8280459f-fa41-46e0-ace6-9fdcc0b5d176
📒 Files selected for processing (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}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/MySQL_Monitor.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/MySQL_Monitor.cpp`:
- Line 243: The quarantined_hostnames approach using a fixed 30s expiry is
unsafe; change it to track a per-host switchover generation (or switchover
timestamp) and record a matching generation/timestamp on each connection when
checked-out; in put_connection() and purge_connections_for_hostnames()
reject/evict any returned connection whose checkout generation/timestamp is
older than the host's current generation. Replace the simple unsigned long long
quarantined_hostnames expiry map with a map of host -> generation/timestamp and
add per-connection metadata (e.g., checkout_generation in the connection
struct), update quarantined_hostnames on switchover, and ensure all code paths
that accept returned sockets (put_connection, purge_connections_for_hostnames,
and any repool logic) compare connection.checkout_generation against
quarantined_hostnames[hostname] and refuse/close connections with an older
generation.
🪄 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: 07c4e4d7-9988-47e5-9c78-23a866b78fa0
📒 Files selected for processing (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}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/MySQL_Monitor.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/MySQL_Monitor.cpp (3)
529-535:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPurge only the hostnames passed into this call.
Lines 531-535 use
switchover_timestampsas the purge filter, so any hostname that has ever switched over gets drained again on later, unrelated switchovers. That broadenspurge_connections_for_hostnames()beyond its contract and can churn monitor sockets for other Aurora clusters. Use the currenthostnamesargument to decide which idle pools to drain, and keepswitchover_timestampsonly forput_connection()rejection.Suggested fix
void MySQL_Monitor_Connection_Pool::purge_connections_for_hostnames(const std::vector<std::string>& hostnames) { std::lock_guard<std::mutex> lock(mutex); `#ifdef` DEBUG pthread_mutex_lock(&m2); `#endif` + std::unordered_set<std::string> target_hostnames(hostnames.begin(), hostnames.end()); unsigned long long now = monotonic_time(); for (const auto& hostname : hostnames) { switchover_timestamps[hostname] = now; } for (unsigned int i = 0; i < servers->len; i++) { MonMySrvC *srv = (MonMySrvC *)servers->index(i); - if (switchover_timestamps.find(srv->address) == switchover_timestamps.end()) continue; + if (target_hostnames.find(srv->address) == target_hostnames.end()) continue; while (srv->conns->len) { MYSQL *my = (MYSQL *)srv->conns->remove_index_fast(0); if (!my) continue; close_mysql(my); }🤖 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/MySQL_Monitor.cpp` around lines 529 - 535, The purge loop is using switchover_timestamps as the filter which causes purge_connections_for_hostnames() to act on all previously-switched hostnames; change the purge to use only the current hostnames argument: iterate over the hostnames vector passed into the function and call purge_connections_for_hostnames() (or drain idle pools) for each of those, leaving switchover_timestamps unchanged except for recording timestamps used by put_connection() rejection logic (i.e., keep the assignment to switchover_timestamps[hostname] = now but do not use switchover_timestamps as the list to purge).
436-438:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep switchover metadata out of
MYSQL::net.buff.Line 437 is reading the pre-checkout timestamp from
my->net.buff, but by the time a connection is returned that buffer has already been reused by ping/query traffic, so it no longer reliably contains the value written at Line 454. That makes the stale-socket check nondeterministic: pre-switchover sockets can slip back into the pool, and fresh sockets can be discarded. Track the last-pooled/check-out time in pool-owned metadata keyed byMYSQL*instead of piggybacking on the client library buffer.🤖 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/MySQL_Monitor.cpp` around lines 436 - 438, The code reads last_pooled from my->net.buff (unsigned long long) which is unsafe because that buffer is reused; replace this by storing per-connection pool metadata keyed by MYSQL* (e.g. an unordered_map<MYSQL*, unsigned long long> lastPooledMap). When a connection is returned to the pool (where the code previously wrote the timestamp into my->net.buff at the checkout/return site), write the timestamp into lastPooledMap[conn]; then in the stale-socket check use lastPooledMap.find(my) and compare sit->second against lastPooledMap[my] instead of dereferencing my->net.buff. Also ensure to erase the map entry when a MYSQL* is closed/removed to avoid leaks.
6067-6067:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSynchronize the forced DNS refresh flag.
Line 6067 writes a plain
boolthatmonitor_dns_cache()reads and clears from another thread at Lines 4903-4906. That is a data race, so the switchover-triggered refresh can be missed under load. Guard the flag with apthread_mutex_tor make the access atomic end-to-end. As per coding guidelines,**/*.{cpp,h,hpp}: Use pthread mutexes for synchronization and std::atomic<> for counters.🤖 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/MySQL_Monitor.cpp` at line 6067, The forced DNS refresh boolean accessed by MySQL_Monitor::trigger_dns_cache_update() and monitor_dns_cache() is a data race; protect it by adding a pthread_mutex_t member to MySQL_Monitor (e.g., dns_refresh_mutex) and wrap all reads/clears/writes of the "forced DNS refresh" flag in pthread_mutex_lock/unlock in both trigger_dns_cache_update() and monitor_dns_cache(), or alternatively change the flag to std::atomic_bool and use atomic load/store/exchange so the clear is atomic; follow the project guideline preference for pthread mutexes for synchronization and ensure the same protection is applied wherever that flag is touched.
🤖 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 `@lib/MySQL_Monitor.cpp`:
- Around line 6019-6049: The helper currently returns false when a live
getaddrinfo() resolves peer_ip but doesn't update the local DNS cache; before
returning when ip_found is true (after the loop that sets ip_found), call the
DNS cache refresh method on GloMyMon->dns_cache to add/overwrite the hostname
entry for mmsd->hostname with peer_ip (use the cache API provided in your
codebase — e.g., add/insert/update/refresh method on dns_cache) so future checks
(contains_ip) will hit the cache instead of redoing getaddrinfo.
---
Duplicate comments:
In `@lib/MySQL_Monitor.cpp`:
- Around line 529-535: The purge loop is using switchover_timestamps as the
filter which causes purge_connections_for_hostnames() to act on all
previously-switched hostnames; change the purge to use only the current
hostnames argument: iterate over the hostnames vector passed into the function
and call purge_connections_for_hostnames() (or drain idle pools) for each of
those, leaving switchover_timestamps unchanged except for recording timestamps
used by put_connection() rejection logic (i.e., keep the assignment to
switchover_timestamps[hostname] = now but do not use switchover_timestamps as
the list to purge).
- Around line 436-438: The code reads last_pooled from my->net.buff (unsigned
long long) which is unsafe because that buffer is reused; replace this by
storing per-connection pool metadata keyed by MYSQL* (e.g. an
unordered_map<MYSQL*, unsigned long long> lastPooledMap). When a connection is
returned to the pool (where the code previously wrote the timestamp into
my->net.buff at the checkout/return site), write the timestamp into
lastPooledMap[conn]; then in the stale-socket check use lastPooledMap.find(my)
and compare sit->second against lastPooledMap[my] instead of dereferencing
my->net.buff. Also ensure to erase the map entry when a MYSQL* is closed/removed
to avoid leaks.
- Line 6067: The forced DNS refresh boolean accessed by
MySQL_Monitor::trigger_dns_cache_update() and monitor_dns_cache() is a data
race; protect it by adding a pthread_mutex_t member to MySQL_Monitor (e.g.,
dns_refresh_mutex) and wrap all reads/clears/writes of the "forced DNS refresh"
flag in pthread_mutex_lock/unlock in both trigger_dns_cache_update() and
monitor_dns_cache(), or alternatively change the flag to std::atomic_bool and
use atomic load/store/exchange so the clear is atomic; follow the project
guideline preference for pthread mutexes for synchronization and ensure the same
protection is applied wherever that flag is touched.
🪄 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: d86ac32e-6a82-402b-99c3-d6b6f4afb4ff
📒 Files selected for processing (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}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/MySQL_Monitor.cpp
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/MySQL_Monitor.cpp`:
- Around line 437-440: The code reads last_pooled from my->net.buff and later
overwrites that same buffer in MySQL_Monitor_Connection_Pool::put_connection(),
which makes pooled timestamp metadata unstable; change the pool to store
checkout/return timestamps in a dedicated field (e.g. add a timestamp member on
the pool entry or use MySQL_Monitor_State_Data) and update the logic in the
place checking should_reject_pooled_connection(hostname, ...) to read that
dedicated timestamp instead of *(unsigned long long*)my->net.buff, ensuring
checkout writes the initial timestamp to the pool state and put_connection
writes the updated "now" into that same pool field (leave switchover_timestamps
and should_reject_pooled_connection usage intact but compare against the new
pool/state timestamp rather than net.buff).
🪄 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: 0db92993-73d5-4d1b-836f-8963227f23b5
📒 Files selected for processing (4)
include/AuroraMonitorDecision.hlib/MySQL_Monitor.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
✅ Files skipped from review due to trivial changes (1)
- include/AuroraMonitorDecision.h
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpplib/MySQL_Monitor.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should follow naming pattern test_*.cpp or *-t.cpp in test/tap/tests/
Files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-20T09:34:19.124Z
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:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
🔇 Additional comments (3)
test/tap/tests/unit/Makefile (1)
376-377: LGTM!test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp (1)
1-58: LGTM!lib/MySQL_Monitor.cpp (1)
26-26: LGTM!Also applies to: 532-535, 539-539, 6040-6055
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/MySQL_Monitor.cpp (1)
6025-6055:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
mysql_thread___resolution_familywhen populating DNS cache in Aurora blue/green switchover detection
aws_aurora_check_blue_green_switchover()hardcodeshints.ai_family = AF_UNSPEC, resolves both A and AAAA, and then overwritesGloMyMon->dns_cacheviaDNS_Cache::add()(it replacesrecords[hostname].ips). The normal DNS resolver path useshints.ai_family = mysql_resolution_family_to_ai_family(mysql_thread___resolution_family), so the configured family restriction is ignored only in this switchover confirmation path. SinceMySQL_Monitor::dns_lookup()consumes the cache, this can change later connection targets after switchover to include the undesired IP family.🤖 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/MySQL_Monitor.cpp` around lines 6025 - 6055, The DNS lookup in aws_aurora_check_blue_green_switchover() currently sets hints.ai_family = AF_UNSPEC and populates GloMyMon->dns_cache with both A/AAAA results, which ignores the configured mysql_thread___resolution_family; change the code to set hints.ai_family = mysql_resolution_family_to_ai_family(mysql_thread___resolution_family) (or otherwise respect mysql_thread___resolution_family) before calling getaddrinfo(), and/or filter resolved_ips to only include addresses matching mysql_thread___resolution_family, then call GloMyMon->dns_cache->add(std::string(mmsd->hostname), std::move(resolved_ips)) so the cached records match the configured resolution family used by MySQL_Monitor::dns_lookup(). Ensure references are to aws_aurora_check_blue_green_switchover(), mysql_thread___resolution_family, mysql_resolution_family_to_ai_family(), and GloMyMon->dns_cache->add().
♻️ Duplicate comments (1)
lib/MySQL_Monitor.cpp (1)
6074-6074:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSynchronize the forced DNS refresh flag.
Line 6074 now relies on
trigger_dns_cache_update(), butforce_dns_cache_updateis still a plain cross-thread flag. The write at Line 6991 races the read/reset at Lines 4907-4910, so the refresh can be dropped exactly when a blue/green switchover is detected.#!/bin/bash set -euo pipefail FILE="lib/MySQL_Monitor.cpp" echo "== Write site ==" awk 'NR>=6989 && NR<=6993 {printf "%d\t%s\n", NR, $0}' "$FILE" echo echo "== Read/reset site ==" awk 'NR>=4907 && NR<=4911 {printf "%d\t%s\n", NR, $0}' "$FILE" echo echo "== Switchover trigger call site ==" awk 'NR>=6072 && NR<=6075 {printf "%d\t%s\n", NR, $0}' "$FILE"As per coding guidelines
**/*.{cpp,h,hpp}: Use pthread mutexes for synchronization and std::atomic<> for counters.🤖 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/MySQL_Monitor.cpp` at line 6074, The cross-thread flag force_dns_cache_update races between the writer (site that sets it around the code near Line ~6991) and the reader/reset (code around Lines ~4907-4910) when trigger_dns_cache_update() is invoked; change the flag from a plain bool to an atomic<bool> (std::atomic<bool> force_dns_cache_update) and update all writers to use force_dns_cache_update.store(true, std::memory_order_relaxed) (or stronger ordering if needed) and the reader/reset to use force_dns_cache_update.exchange(false, std::memory_order_acq_rel) so the read-and-reset is atomic and cannot be dropped; keep trigger_dns_cache_update() unchanged but ensure any internal checks reference the atomic flag through the new API.
🤖 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 `@lib/MySQL_Monitor.cpp`:
- Around line 438-440: The code uses mutable mmsd->t1 as the checkout boundary
for should_reject_pooled_connection, but mmsd->t1 is modified later in
set_wait_timeout(), allowing pre-switchover sockets to be treated as new; fix by
capturing an immutable checkout timestamp at the moment the MYSQL* is obtained
(e.g., grab a local unsigned long long checkout_ts when acquiring the
connection) and use that local checkout_ts instead of mmsd->t1 in the call to
should_reject_pooled_connection, ensuring put_connection() and
set_wait_timeout() cannot mutate the value used for stale-socket rejection.
---
Outside diff comments:
In `@lib/MySQL_Monitor.cpp`:
- Around line 6025-6055: The DNS lookup in
aws_aurora_check_blue_green_switchover() currently sets hints.ai_family =
AF_UNSPEC and populates GloMyMon->dns_cache with both A/AAAA results, which
ignores the configured mysql_thread___resolution_family; change the code to set
hints.ai_family =
mysql_resolution_family_to_ai_family(mysql_thread___resolution_family) (or
otherwise respect mysql_thread___resolution_family) before calling
getaddrinfo(), and/or filter resolved_ips to only include addresses matching
mysql_thread___resolution_family, then call
GloMyMon->dns_cache->add(std::string(mmsd->hostname), std::move(resolved_ips))
so the cached records match the configured resolution family used by
MySQL_Monitor::dns_lookup(). Ensure references are to
aws_aurora_check_blue_green_switchover(), mysql_thread___resolution_family,
mysql_resolution_family_to_ai_family(), and GloMyMon->dns_cache->add().
---
Duplicate comments:
In `@lib/MySQL_Monitor.cpp`:
- Line 6074: The cross-thread flag force_dns_cache_update races between the
writer (site that sets it around the code near Line ~6991) and the reader/reset
(code around Lines ~4907-4910) when trigger_dns_cache_update() is invoked;
change the flag from a plain bool to an atomic<bool> (std::atomic<bool>
force_dns_cache_update) and update all writers to use
force_dns_cache_update.store(true, std::memory_order_relaxed) (or stronger
ordering if needed) and the reader/reset to use
force_dns_cache_update.exchange(false, std::memory_order_acq_rel) so the
read-and-reset is atomic and cannot be dropped; keep trigger_dns_cache_update()
unchanged but ensure any internal checks reference the atomic flag through the
new API.
🪄 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: 857c877c-b6a9-44cc-a4c1-0d9447b766ff
📒 Files selected for processing (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}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/MySQL_Monitor.cpp
🔇 Additional comments (1)
lib/MySQL_Monitor.cpp (1)
526-545: LGTM!Also applies to: 6456-6475, 7078-7099
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/MySQL_Monitor.cpp`:
- Around line 452-454: The code currently erases the switchover marker
(switchover_timestamps via sit) as soon as one post-switchover connection is
accepted, which allows later-returning pre-switchover sockets to be re-pooled;
instead, keep the marker until there are no in-flight pre-switchover checkouts
for that hostname. Modify the logic around switchover_timestamps (and the sit
erase) to only remove the entry when a per-host in-flight pre-switchover counter
is zero (e.g., maintain/increment a pre_switchover_inflight_count[hostname] on
checkout of pre-switchover sockets and decrement on return), or equivalently
check an existing structure that tracks the oldest active checkout timestamp for
the host and only erase when that oldest timestamp is >= switchover time; change
the code that currently erases switchover_timestamps (using sit) to perform this
conditional check and only erase when safe.
🪄 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: 779f84e7-6cf7-47d1-8ce4-f4f9a4037bf0
📒 Files selected for processing (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}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/MySQL_Monitor.cpp
🔇 Additional comments (1)
lib/MySQL_Monitor.cpp (1)
6033-6050: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/AuroraMonitorDecision.h (1)
9-10: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the repository-standard include guard format.
Please rename this guard to the required
__CLASS_*_Hpattern (for example,__CLASS_AURORA_MONITOR_DECISION_H) to match project conventions.As per coding guidelines,
include/**/*.{h,hpp}: Include guards should use#ifndef __CLASS_*_Hformat.Also applies to: 33-33
🤖 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 `@include/AuroraMonitorDecision.h` around lines 9 - 10, The include guard macro AURORA_MONITOR_DECISION_H does not follow the repository standard; rename it to the required pattern __CLASS_AURORA_MONITOR_DECISION_H by replacing both occurrences of AURORA_MONITOR_DECISION_H in the `#ifndef` and `#define` lines and update the corresponding `#endif` (if it has a trailing comment or reference) to match the new __CLASS_AURORA_MONITOR_DECISION_H macro so the guard is consistent across the file.
🤖 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 `@lib/MySQL_Monitor.cpp`:
- Around line 399-401: Freshly created MYSQL* connections from
create_new_connection() are not stamped with pool_checkout_time, so
put_connection() later sees a zero timestamp and incorrectly treats them as
pre-switchover-safe; set mmsd->pool_checkout_time to now immediately after
creating a new connection (the same way it's set when checking out from
srv->conns). Update the code paths in create_new_connection() (and any place
that constructs/returns a new MYSQL* leading into put_connection()) to assign
mmsd->pool_checkout_time = now (using the same now variable or timestamp helper)
for the new connection object (mmsd / my) so that subsequent put_connection()
calls correctly reflect the checkout time; apply the same change to the other
analogous spot mentioned around the second block (lines 441-443) to ensure
consistency.
---
Outside diff comments:
In `@include/AuroraMonitorDecision.h`:
- Around line 9-10: The include guard macro AURORA_MONITOR_DECISION_H does not
follow the repository standard; rename it to the required pattern
__CLASS_AURORA_MONITOR_DECISION_H by replacing both occurrences of
AURORA_MONITOR_DECISION_H in the `#ifndef` and `#define` lines and update the
corresponding `#endif` (if it has a trailing comment or reference) to match the
new __CLASS_AURORA_MONITOR_DECISION_H macro so the guard is consistent across
the file.
🪄 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: 92294c9f-d001-41b1-956f-6723dc4e944f
📒 Files selected for processing (4)
include/AuroraMonitorDecision.hinclude/MySQL_Monitor.hpplib/MySQL_Monitor.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
include/AuroraMonitorDecision.htest/tap/tests/unit/aurora_monitor_decision_unit-t.cppinclude/MySQL_Monitor.hpplib/MySQL_Monitor.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards should use
#ifndef_CLASS*_H format
Files:
include/AuroraMonitorDecision.hinclude/MySQL_Monitor.hpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should follow naming pattern test_*.cpp or *-t.cpp in test/tap/tests/
Files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-20T09:34:19.124Z
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:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
🔇 Additional comments (4)
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp (2)
19-24: LGTM!
52-58: LGTM!include/MySQL_Monitor.hpp (1)
266-271: LGTM!Also applies to: 438-447
include/AuroraMonitorDecision.h (1)
24-31: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/AuroraMonitorDecision.h (1)
19-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix parameter name mismatch in documentation.
The docstring at line 19 refers to
last_pooled_time, but the actual parameter name at line 25 ischeckout_time. Update the documentation to match the implementation.📝 Proposed fix
- * `@param` last_pooled_time Timestamp when the connection was last returned to pool. + * `@param` checkout_time Timestamp when the connection was last returned to pool.Also applies to: 24-26
🤖 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 `@include/AuroraMonitorDecision.h` at line 19, The docstring parameter name is out of sync: update the documentation comment that currently mentions last_pooled_time to use the actual parameter name checkout_time (or rename the function parameter to last_pooled_time if you prefer API change); locate the comment in AuroraMonitorDecision.h that documents the parameter (the block referencing last_pooled_time) and change it to checkout_time so the `@param` name matches the implementation (ensure any other occurrences in the same comment block for that parameter are updated as well).
♻️ Duplicate comments (1)
lib/MySQL_Monitor.cpp (1)
6317-6317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStamping only the Aurora worker leaves other fresh monitor sockets able to bypass switchover rejection.
put_connection()usespool_checkout_timefor all returned monitor connections, but this timestamp is only set on the Aurora thread here. The othercreate_new_connection()paths in this file still return fresh sockets withpool_checkout_time == 0, so a pre-switchover ping/read_only/lag connection for the same hostname can still be re-pooled afterswitchover_timestamps[hostname]is set. Please move the stamp intoMySQL_Monitor_State_Data::create_new_connection()(or a shared post-connect helper) so every fresh monitor connection gets immutable checkout metadata.Suggested centralization
--- a/lib/MySQL_Monitor.cpp +++ b/lib/MySQL_Monitor.cpp @@ - if (mmsd->mysql) { - GloMyMon->My_Conn_Pool->conn_register(mmsd); - mmsd->pool_checkout_time = monotonic_time(); - } + if (mmsd->mysql) { + GloMyMon->My_Conn_Pool->conn_register(mmsd); + } @@ bool MySQL_Monitor_State_Data::create_new_connection() { mysql=mysql_init(NULL); assert(mysql); @@ } else { + pool_checkout_time = monotonic_time(); // mariadb client library disables NONBLOCK for SSL connections ... re-enable it! mysql_options(mysql, MYSQL_OPT_NONBLOCK, 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/MySQL_Monitor.cpp` at line 6317, The issue is that pool_checkout_time is only set in the Aurora worker path, allowing other freshly-created monitor sockets to bypass switchover checks; to fix it, set mmsd->pool_checkout_time at the point every new monitor connection is created by updating MySQL_Monitor_State_Data::create_new_connection() (or a shared post-connect helper called by it) so each fresh connection returned from create_new_connection() has pool_checkout_time = monotonic_time() before being returned; ensure put_connection() no longer needs to set it and that any other create_new_connection() call sites rely on the centralized timestamp assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@include/AuroraMonitorDecision.h`:
- Line 19: The docstring parameter name is out of sync: update the documentation
comment that currently mentions last_pooled_time to use the actual parameter
name checkout_time (or rename the function parameter to last_pooled_time if you
prefer API change); locate the comment in AuroraMonitorDecision.h that documents
the parameter (the block referencing last_pooled_time) and change it to
checkout_time so the `@param` name matches the implementation (ensure any other
occurrences in the same comment block for that parameter are updated as well).
---
Duplicate comments:
In `@lib/MySQL_Monitor.cpp`:
- Line 6317: The issue is that pool_checkout_time is only set in the Aurora
worker path, allowing other freshly-created monitor sockets to bypass switchover
checks; to fix it, set mmsd->pool_checkout_time at the point every new monitor
connection is created by updating
MySQL_Monitor_State_Data::create_new_connection() (or a shared post-connect
helper called by it) so each fresh connection returned from
create_new_connection() has pool_checkout_time = monotonic_time() before being
returned; ensure put_connection() no longer needs to set it and that any other
create_new_connection() call sites rely on the centralized timestamp assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a49a573-7a98-4f43-8004-16d824e4db09
📒 Files selected for processing (2)
include/AuroraMonitorDecision.hlib/MySQL_Monitor.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
include/AuroraMonitorDecision.hlib/MySQL_Monitor.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards should use
#ifndef_CLASS*_H format
Files:
include/AuroraMonitorDecision.h
🔇 Additional comments (3)
include/AuroraMonitorDecision.h (3)
9-10: LGTM!Also applies to: 33-33
28-30: LGTM!
1-8: LGTM!Also applies to: 12-18, 24-24, 32-32
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
I think this is ready for review. Obviously AI was quite involved in the authoring, so please be brutal in your review and I'll do my best to address it. The remaining SonarQube issues are hardcoded RFC1918 IPs in tests, which I think should be okay? |
|
Hi @tabacco . Thank you for the PR. Ping @wazir-ahmed |
tabacco
force-pushed
the
add-aurora-bg-support
branch
2 times, most recently
from
e136065 to
2b8a27f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/MySQL_Monitor.cpp (1)
6342-6369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip rotating
lasts_aseon a detected switchover.
blue_green_detectedcorrectly skipsevaluate_aws_aurora_results(), but this block still writesase_lintolasts_ase[ase_idx]and advances the ring. On the next healthy cycle,prev_aasecan still point at old-cluster data, so readers whose lag state did not change may look like a no-op and stay SHUNNED aftershun_and_killall(). Only append/advance Aurora history for non-switchover iterations, or clear that slot instead.🤖 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/MySQL_Monitor.cpp` around lines 6342 - 6369, When a blue_green_detected switchover occurs we must not rotate/append the new ase_l into the circular history or advance ase_idx; instead clear that slot so prev history remains consistent. Concretely: only perform the assignment lasts_ase[ase_idx] = ase_l and increment ase_idx when !blue_green_detected (the same condition used for evaluate_aws_aurora_results); if blue_green_detected then free/delete ase_l (or set lasts_ase[ase_idx] to nullptr) and do not change ase_idx so readers and subsequent evaluate_aws_aurora_results calls keep the previous cluster data (affecting lasts_ase, ase_l, ase_idx, blue_green_detected and downstream logic like shun_and_killall).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/MySQL_Monitor.cpp`:
- Around line 6342-6369: When a blue_green_detected switchover occurs we must
not rotate/append the new ase_l into the circular history or advance ase_idx;
instead clear that slot so prev history remains consistent. Concretely: only
perform the assignment lasts_ase[ase_idx] = ase_l and increment ase_idx when
!blue_green_detected (the same condition used for evaluate_aws_aurora_results);
if blue_green_detected then free/delete ase_l (or set lasts_ase[ase_idx] to
nullptr) and do not change ase_idx so readers and subsequent
evaluate_aws_aurora_results calls keep the previous cluster data (affecting
lasts_ase, ase_l, ase_idx, blue_green_detected and downstream logic like
shun_and_killall).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da448a89-bc26-4320-bb07-c795701cf344
📒 Files selected for processing (8)
include/AuroraMonitorDecision.hinclude/DNS_Cache.hppinclude/MySQL_Monitor.hpplib/DNS_Cache.cpplib/MySQL_Monitor.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/aurora_monitor_decision_unit-t.cpptest/tap/tests/unit/dns_cache_unit-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
{test/infra/**/*.bash,test/tap/**/Makefile,.github/workflows/**/*.yml}
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS use
run-tests-isolated.bashfor running TAP tests. It handles infrastructure setup, ProxySQL start, test execution, and cleanup. Never manually create Docker networks, start containers, or run init scripts.
Files:
test/tap/tests/unit/Makefile
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers are controlled via flags:PROXYSQL31=1for v3.1.x features (FFTO, TSDB),PROXYSQL40=1for v4.0.x features (plugin loader).PROXYSQL40=1implies bothPROXYSQL31=1andPROXYSQLFFTO=1andPROXYSQLTSDB=1. Use conditional compilation with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Class names usePascalCasewith protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session).
Member variables usesnake_case.
Constants and macros useUPPER_SNAKE_CASE.
Use C++17; conditional compilation for feature tiers via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Use RAII for resource management; use jemalloc for memory allocation.
Use pthread mutexes for synchronization; usestd::atomic<>for counters.
Files:
lib/DNS_Cache.cppinclude/DNS_Cache.hppinclude/MySQL_Monitor.hpptest/tap/tests/unit/dns_cache_unit-t.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpplib/MySQL_Monitor.cppinclude/AuroraMonitorDecision.h
{lib,src}/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
GenAI/MCP/RAG/LLM features live entirely in
plugins/genai/and load as a.soat runtime viadlopen. Do not guard withPROXYSQLGENAIin core code — that flag no longer guards any core code as of Step 7 of the GenAI plugin carve-out.
Files:
lib/DNS_Cache.cpplib/MySQL_Monitor.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards in headers use
#ifndef __CLASS_*_Hformat (e.g.,#ifndef __MYSQL_PROTOCOL_H).
Files:
include/DNS_Cache.hppinclude/MySQL_Monitor.hppinclude/AuroraMonitorDecision.h
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppintest/tap/tests/. Test binaries are built via pattern rulemake <testname>-twhich compiles<testname>-t.cppinto<testname>-t. Register new tests ingroups.json.
Files:
test/tap/tests/unit/dns_cache_unit-t.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must use
test_globals.handtest_init.hand link againstlibproxysql.avia the custom test harness defined indoc/agents/project-conventions.md.
Files:
test/tap/tests/unit/dns_cache_unit-t.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-20T09:34:19.124Z
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:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/dns_cache_unit-t.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/dns_cache_unit-t.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
🪛 Cppcheck (2.20.0)
lib/DNS_Cache.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
test/tap/tests/unit/dns_cache_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (9)
include/DNS_Cache.hpp (1)
1-177: LGTM!lib/DNS_Cache.cpp (1)
20-33: LGTM!Also applies to: 35-62, 64-177, 180-208, 358-379
test/tap/tests/unit/dns_cache_unit-t.cpp (1)
1-128: LGTM!test/tap/tests/unit/Makefile (1)
146-147: LGTM!Also applies to: 375-377, 401-403, 746-754
include/AuroraMonitorDecision.h (1)
1-34: LGTM!test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp (1)
1-71: LGTM!lib/MySQL_Monitor.cpp (2)
399-401: LGTM!Also applies to: 535-558, 1701-1701
806-837: LGTM!Also applies to: 2373-2394, 6860-6865
include/MySQL_Monitor.hpp (1)
7-7: LGTM!Also applies to: 267-272, 395-397, 443-447
tabacco
force-pushed
the
add-aurora-bg-support
branch
2 times, most recently
from
265c30d to
00e4fe0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/MySQL_Monitor.cpp (2)
5957-5957:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSynchronize the forced DNS refresh flag.
This path still flips
force_dns_cache_updateviatrigger_dns_cache_update(), whilemonitor_dns_cache()reads and clears the same flag without synchronization. That refresh request can race or be missed under load. As per coding guidelines,**/*.{cpp,h,hpp}: Use pthread mutexes for synchronization; usestd::atomic<>for counters.🤖 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/MySQL_Monitor.cpp` at line 5957, The code flips force_dns_cache_update in MySQL_Monitor::trigger_dns_cache_update() without synchronization while monitor_dns_cache() reads/clears it, causing races; protect that flag with a pthread mutex: add a pthread_mutex_t (e.g., dns_cache_mutex) as a member, initialize it in the MySQL_Monitor constructor and destroy it in the destructor, and wrap all accesses/assignments to force_dns_cache_update inside pthread_mutex_lock(&dns_cache_mutex)/pthread_mutex_unlock(&dns_cache_mutex) in both MySQL_Monitor::trigger_dns_cache_update() and MySQL_Monitor::monitor_dns_cache(); alternatively, if you prefer an atomic type, convert force_dns_cache_update to std::atomic<bool> and use atomic load/store/compare_exchange semantics when reading and clearing the flag.
5904-5905:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t treat cache miss as a confirmed DNS mismatch.
contains_ip()is still a boolean here, so this branch cannot distinguish “hostname not cached / cache disabled” from “cached and peer IP absent”. On a cold or disabled cache, every successful Aurora check falls into the blockinggetaddrinfo()path.🤖 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/MySQL_Monitor.cpp` around lines 5904 - 5905, The current check uses GloMyMon->dns_cache->contains_ip(mmsd->hostname, peer_ip) as a boolean and treats a cache miss the same as a definitive DNS mismatch; change the logic so you can distinguish "hostname not cached / cache disabled" from "hostname cached but peer_ip not present." Add or use a tri-state or two-step API on dns_cache (e.g., has_entry/lookup or contains_hostname + contains_ip) and update the call site in MySQL_Monitor.cpp: first check whether the cache contains an entry for mmsd->hostname (e.g., dns_cache->has(mmsd->hostname) or dns_cache->lookup), and only if the hostname is cached assert the absence of peer_ip and return false; otherwise, allow the code to continue to getaddrinfo() when the cache is missing or disabled. Ensure references to GloMyMon, dns_cache, contains_ip, mmsd->hostname, and peer_ip are updated consistently.
🤖 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.
Duplicate comments:
In `@lib/MySQL_Monitor.cpp`:
- Line 5957: The code flips force_dns_cache_update in
MySQL_Monitor::trigger_dns_cache_update() without synchronization while
monitor_dns_cache() reads/clears it, causing races; protect that flag with a
pthread mutex: add a pthread_mutex_t (e.g., dns_cache_mutex) as a member,
initialize it in the MySQL_Monitor constructor and destroy it in the destructor,
and wrap all accesses/assignments to force_dns_cache_update inside
pthread_mutex_lock(&dns_cache_mutex)/pthread_mutex_unlock(&dns_cache_mutex) in
both MySQL_Monitor::trigger_dns_cache_update() and
MySQL_Monitor::monitor_dns_cache(); alternatively, if you prefer an atomic type,
convert force_dns_cache_update to std::atomic<bool> and use atomic
load/store/compare_exchange semantics when reading and clearing the flag.
- Around line 5904-5905: The current check uses
GloMyMon->dns_cache->contains_ip(mmsd->hostname, peer_ip) as a boolean and
treats a cache miss the same as a definitive DNS mismatch; change the logic so
you can distinguish "hostname not cached / cache disabled" from "hostname cached
but peer_ip not present." Add or use a tri-state or two-step API on dns_cache
(e.g., has_entry/lookup or contains_hostname + contains_ip) and update the call
site in MySQL_Monitor.cpp: first check whether the cache contains an entry for
mmsd->hostname (e.g., dns_cache->has(mmsd->hostname) or dns_cache->lookup), and
only if the hostname is cached assert the absence of peer_ip and return false;
otherwise, allow the code to continue to getaddrinfo() when the cache is missing
or disabled. Ensure references to GloMyMon, dns_cache, contains_ip,
mmsd->hostname, and peer_ip are updated consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 992ae2bb-1688-4f8a-82ae-bcdbb10149fe
📒 Files selected for processing (8)
include/AuroraMonitorDecision.hinclude/DNS_Cache.hppinclude/MySQL_Monitor.hpplib/DNS_Cache.cpplib/MySQL_Monitor.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/aurora_monitor_decision_unit-t.cpptest/tap/tests/unit/dns_cache_unit-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers are controlled via flags:PROXYSQL31=1for v3.1.x features (FFTO, TSDB),PROXYSQL40=1for v4.0.x features (plugin loader).PROXYSQL40=1implies bothPROXYSQL31=1andPROXYSQLFFTO=1andPROXYSQLTSDB=1. Use conditional compilation with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Class names usePascalCasewith protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session).
Member variables usesnake_case.
Constants and macros useUPPER_SNAKE_CASE.
Use C++17; conditional compilation for feature tiers via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Use RAII for resource management; use jemalloc for memory allocation.
Use pthread mutexes for synchronization; usestd::atomic<>for counters.
Files:
include/AuroraMonitorDecision.hinclude/DNS_Cache.hppinclude/MySQL_Monitor.hpplib/DNS_Cache.cpptest/tap/tests/unit/dns_cache_unit-t.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpplib/MySQL_Monitor.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards in headers use
#ifndef __CLASS_*_Hformat (e.g.,#ifndef __MYSQL_PROTOCOL_H).
Files:
include/AuroraMonitorDecision.hinclude/DNS_Cache.hppinclude/MySQL_Monitor.hpp
{test/infra/**/*.bash,test/tap/**/Makefile,.github/workflows/**/*.yml}
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS use
run-tests-isolated.bashfor running TAP tests. It handles infrastructure setup, ProxySQL start, test execution, and cleanup. Never manually create Docker networks, start containers, or run init scripts.
Files:
test/tap/tests/unit/Makefile
{lib,src}/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
GenAI/MCP/RAG/LLM features live entirely in
plugins/genai/and load as a.soat runtime viadlopen. Do not guard withPROXYSQLGENAIin core code — that flag no longer guards any core code as of Step 7 of the GenAI plugin carve-out.
Files:
lib/DNS_Cache.cpplib/MySQL_Monitor.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppintest/tap/tests/. Test binaries are built via pattern rulemake <testname>-twhich compiles<testname>-t.cppinto<testname>-t. Register new tests ingroups.json.
Files:
test/tap/tests/unit/dns_cache_unit-t.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must use
test_globals.handtest_init.hand link againstlibproxysql.avia the custom test harness defined indoc/agents/project-conventions.md.
Files:
test/tap/tests/unit/dns_cache_unit-t.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-20T09:34:19.124Z
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:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/dns_cache_unit-t.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/dns_cache_unit-t.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/dns_cache_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (8)
include/DNS_Cache.hpp (1)
86-95: LGTM!lib/DNS_Cache.cpp (1)
351-379: LGTM!test/tap/tests/unit/dns_cache_unit-t.cpp (1)
1-128: LGTM!test/tap/tests/unit/Makefile (1)
375-377: LGTM!include/AuroraMonitorDecision.h (1)
1-35: LGTM!test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp (1)
1-71: LGTM!include/MySQL_Monitor.hpp (1)
267-272: LGTM!lib/MySQL_Monitor.cpp (1)
26-26: LGTM!Also applies to: 244-255, 400-402, 442-456, 537-560, 1704-1704, 6346-6372
tabacco
force-pushed
the
add-aurora-bg-support
branch
from
00e4fe0 to
dcf2649
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/MySQL_Monitor.cpp`:
- Around line 5903-5907: The DNS-gating helper runs even when the Aurora/mysql
check failed; update the initial guard in the helper (the block using mmsd,
mmsd->mysql and GloMyMon->dns_cache) to also return false when the check
recorded an error — i.e., add a condition that skips DNS-based pool purge if
mmsd->mysql_error_msg is non-empty (or otherwise indicates a failed mysql
check), so the DNS mismatch logic only runs after a successful Aurora/mysql
check.
🪄 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: f3e491bd-e540-4a82-8be0-2f855ab43368
📒 Files selected for processing (8)
include/AuroraMonitorDecision.hinclude/DNS_Cache.hppinclude/MySQL_Monitor.hpplib/DNS_Cache.cpplib/MySQL_Monitor.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/aurora_monitor_decision_unit-t.cpptest/tap/tests/unit/dns_cache_unit-t.cpp
✅ Files skipped from review due to trivial changes (1)
- test/tap/tests/unit/Makefile
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers are controlled via flags:PROXYSQL31=1for v3.1.x features (FFTO, TSDB),PROXYSQL40=1for v4.0.x features (plugin loader).PROXYSQL40=1implies bothPROXYSQL31=1andPROXYSQLFFTO=1andPROXYSQLTSDB=1. Use conditional compilation with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Class names usePascalCasewith protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session).
Member variables usesnake_case.
Constants and macros useUPPER_SNAKE_CASE.
Use C++17; conditional compilation for feature tiers via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Use RAII for resource management; use jemalloc for memory allocation.
Use pthread mutexes for synchronization; usestd::atomic<>for counters.
Files:
include/DNS_Cache.hpplib/DNS_Cache.cpptest/tap/tests/unit/aurora_monitor_decision_unit-t.cppinclude/MySQL_Monitor.hppinclude/AuroraMonitorDecision.htest/tap/tests/unit/dns_cache_unit-t.cpplib/MySQL_Monitor.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards in headers use
#ifndef __CLASS_*_Hformat (e.g.,#ifndef __MYSQL_PROTOCOL_H).
Files:
include/DNS_Cache.hppinclude/MySQL_Monitor.hppinclude/AuroraMonitorDecision.h
{lib,src}/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
GenAI/MCP/RAG/LLM features live entirely in
plugins/genai/and load as a.soat runtime viadlopen. Do not guard withPROXYSQLGENAIin core code — that flag no longer guards any core code as of Step 7 of the GenAI plugin carve-out.
Files:
lib/DNS_Cache.cpplib/MySQL_Monitor.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppintest/tap/tests/. Test binaries are built via pattern rulemake <testname>-twhich compiles<testname>-t.cppinto<testname>-t. Register new tests ingroups.json.
Files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpptest/tap/tests/unit/dns_cache_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must use
test_globals.handtest_init.hand link againstlibproxysql.avia the custom test harness defined indoc/agents/project-conventions.md.
Files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpptest/tap/tests/unit/dns_cache_unit-t.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-20T09:34:19.124Z
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:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpptest/tap/tests/unit/dns_cache_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/aurora_monitor_decision_unit-t.cpptest/tap/tests/unit/dns_cache_unit-t.cpp
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/dns_cache_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (7)
include/DNS_Cache.hpp (1)
86-95: LGTM!lib/DNS_Cache.cpp (1)
351-379: LGTM!test/tap/tests/unit/dns_cache_unit-t.cpp (1)
1-128: LGTM!include/AuroraMonitorDecision.h (1)
1-35: LGTM!test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp (1)
1-72: LGTM!include/MySQL_Monitor.hpp (1)
267-273: LGTM!lib/MySQL_Monitor.cpp (1)
401-403: LGTM!Also applies to: 1705-1707
After each successful Aurora monitor check (before evaluating results), compare the pooled connection's peer IP against the hostname's DNS resolution. If the hostname no longer resolves to the connected IP, trigger switchover response: destroy stale monitor connections, purge the connection pool with switchover timestamps to reject in-flight pre-switchover connections when returned, trigger DNS cache refresh, and shun backend servers to force proxy connection pool drain. When a switchover is detected, stale topology data from the old cluster is discarded (not stored in the history ring buffer) and the evaluate/ autopurge logic is skipped for that iteration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- dns_cache_unit-t: 18 assertions covering DNS_Cache::contains_ip() - aurora_monitor_decision_unit-t: 13 assertions covering should_reject_pooled_connection() timestamp logic Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tabacco
force-pushed
the
add-aurora-bg-support
branch
from
dcf2649 to
b4bbbb1
Compare
|
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/tap/tests/unit/dns_cache_unit-t.cpp (1)
10-11: 💤 Low valueConsider including DNS_Cache.hpp directly.
The test only uses
DNS_Cache, yet includesMySQL_Monitor.hppinstead of the header whereDNS_Cacheis declared (include/DNS_Cache.hppper the relevant code snippet). Including the minimal necessary header makes dependencies explicit and improves maintainability.📦 Suggested minimal include
`#include` "proxysql.h" -#include "MySQL_Monitor.hpp" +#include "DNS_Cache.hpp"🤖 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/tap/tests/unit/dns_cache_unit-t.cpp` around lines 10 - 11, The test currently includes MySQL_Monitor.hpp while only using DNS_Cache; replace or add the minimal header by including DNS_Cache.hpp instead of MySQL_Monitor.hpp so the test depends only on the DNS_Cache declaration; update the includes at the top of the test (currently referencing proxysql.h and MySQL_Monitor.hpp) to include DNS_Cache.hpp and remove the unnecessary MySQL_Monitor.hpp include to make the dependency explicit.
🤖 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.
Nitpick comments:
In `@test/tap/tests/unit/dns_cache_unit-t.cpp`:
- Around line 10-11: The test currently includes MySQL_Monitor.hpp while only
using DNS_Cache; replace or add the minimal header by including DNS_Cache.hpp
instead of MySQL_Monitor.hpp so the test depends only on the DNS_Cache
declaration; update the includes at the top of the test (currently referencing
proxysql.h and MySQL_Monitor.hpp) to include DNS_Cache.hpp and remove the
unnecessary MySQL_Monitor.hpp include to make the dependency explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8c28865-f106-431f-b4e6-73c0cd478602
📒 Files selected for processing (8)
include/AuroraMonitorDecision.hinclude/DNS_Cache.hppinclude/MySQL_Monitor.hpplib/DNS_Cache.cpplib/MySQL_Monitor.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/aurora_monitor_decision_unit-t.cpptest/tap/tests/unit/dns_cache_unit-t.cpp
✅ Files skipped from review due to trivial changes (1)
- include/DNS_Cache.hpp
🚧 Files skipped from review as they are similar to previous changes (5)
- include/MySQL_Monitor.hpp
- lib/DNS_Cache.cpp
- include/AuroraMonitorDecision.h
- test/tap/tests/unit/Makefile
- test/tap/tests/unit/aurora_monitor_decision_unit-t.cpp
👮 Files not reviewed due to content moderation or server errors (1)
- lib/MySQL_Monitor.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers are controlled via flags:PROXYSQL31=1for v3.1.x features (FFTO, TSDB),PROXYSQL40=1for v4.0.x features (plugin loader).PROXYSQL40=1implies bothPROXYSQL31=1andPROXYSQLFFTO=1andPROXYSQLTSDB=1. Use conditional compilation with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Class names usePascalCasewith protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session).
Member variables usesnake_case.
Constants and macros useUPPER_SNAKE_CASE.
Use C++17; conditional compilation for feature tiers via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Use RAII for resource management; use jemalloc for memory allocation.
Use pthread mutexes for synchronization; usestd::atomic<>for counters.
Files:
lib/MySQL_Monitor.cpptest/tap/tests/unit/dns_cache_unit-t.cpp
{lib,src}/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
GenAI/MCP/RAG/LLM features live entirely in
plugins/genai/and load as a.soat runtime viadlopen. Do not guard withPROXYSQLGENAIin core code — that flag no longer guards any core code as of Step 7 of the GenAI plugin carve-out.
Files:
lib/MySQL_Monitor.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppintest/tap/tests/. Test binaries are built via pattern rulemake <testname>-twhich compiles<testname>-t.cppinto<testname>-t. Register new tests ingroups.json.
Files:
test/tap/tests/unit/dns_cache_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must use
test_globals.handtest_init.hand link againstlibproxysql.avia the custom test harness defined indoc/agents/project-conventions.md.
Files:
test/tap/tests/unit/dns_cache_unit-t.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-20T09:34:19.124Z
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:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/dns_cache_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
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:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/dns_cache_unit-t.cpp
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/dns_cache_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🪛 GitHub Check: SonarCloud Code Analysis
lib/MySQL_Monitor.cpp
[failure] 401-401: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[failure] 6365-6365: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[failure] 6350-6350: Rewrite the code so that you no longer need this "delete".
test/tap/tests/unit/dns_cache_unit-t.cpp
[warning] 45-45: Make sure using this hardcoded IP address is safe here.
[warning] 60-60: Make sure using this hardcoded IP address is safe here.
[warning] 72-72: Make sure using this hardcoded IP address is safe here.
[warning] 82-82: Make sure using this hardcoded IP address is safe here.
[warning] 34-34: Make sure using this hardcoded IP address is safe here.
[warning] 51-51: Make sure using this hardcoded IP address is safe here.
[warning] 65-65: Make sure using this hardcoded IP address is safe here.
[warning] 36-36: Make sure using this hardcoded IP address is safe here.
[warning] 102-102: Make sure using this hardcoded IP address is safe here.
[warning] 74-74: Make sure using this hardcoded IP address is safe here.
[warning] 103-103: Make sure using this hardcoded IP address is safe here.
[warning] 20-20: Make sure using this hardcoded IP address is safe here.
[warning] 46-46: Make sure using this hardcoded IP address is safe here.
[warning] 24-24: Make sure using this hardcoded IP address is safe here.
[warning] 18-18: Make sure using this hardcoded IP address is safe here.
[warning] 107-107: Make sure using this hardcoded IP address is safe here.
[warning] 50-50: Make sure using this hardcoded IP address is safe here.
[warning] 18-18: Make sure using this hardcoded IP address is safe here.
[warning] 32-32: Make sure using this hardcoded IP address is safe here.
[warning] 67-67: Make sure using this hardcoded IP address is safe here.
[warning] 53-53: Make sure using this hardcoded IP address is safe here.
[warning] 22-22: Make sure using this hardcoded IP address is safe here.
🔇 Additional comments (1)
test/tap/tests/unit/dns_cache_unit-t.cpp (1)
1-128: LGTM!
2 participants
Summary
DNS_Cache::contains_ip()for efficient cache-level IP membership checksProblem
During an Aurora blue/green deployment switchover, the green cluster takes over the blue cluster's DNS hostnames. ProxySQL's Aurora monitor holds pooled TCP connections keyed by hostname — when DNS changes, these connections continue talking to the old (blue) cluster because the TCP sessions remain valid. The blue cluster still appears healthy (has a writer, reports normal topology), so the monitor never notices the switch. Writes fail and reads return stale data.
Approach
After each successful Aurora monitor query (but before evaluating the results), compare the pooled connection's actual peer IP (via
getpeername) against the DNS resolution for that hostname:getaddrinfo()callResponse actions:
evaluate_aws_aurora_results()for this iteration (stale data from old cluster)mysql_server_aws_aurora_failoversServers auto-recover on next successful ping to the new IP.
Note on blocking DNS
The confirmation
getaddrinfo()call is blocking and can stall the per-HG monitor thread if DNS is unreachable (OS-dependent timeout, typically 5-30s). This only fires when the DNS cache already disagrees with the peer IP, which should be rare outside an actual switchover event. During normal operation, the cache check passes and no syscall is made.Test plan
dns_cache_unit-t— 18 assertions coveringcontains_ip()(basic, unknown host, cache update simulating switchover, disabled/empty cache, IPv6, removal)proxysql/packaging:build-debian12-v4.0.0)monitor_health_unit-tstill passes (no regression)Summary by CodeRabbit
New Features
Tests