fix(mysqlx): address reviewer feedback (transaction safety, fail-closed TLS, hostgroup refresh, status string)

Four reviewer-flagged issues from CodeRabbit + Gemini on the
consolidated mysqlx stack (originally raised on PRs #5704, #5707,
#5709), all addressed in one place since this is the consolidation
branch:

1. SQLite transaction safety in stats projections.
   `MysqlxStatsStore::flush_to_sqlite` and
   `mysqlx_populate_stats_processlist` previously did a bare DELETE
   followed by a per-row INSERT loop. A transient SQLite error in
   any INSERT left the projection table empty, which is far more
   misleading to operators than stale-but-recent rows. Wrap both in
   BEGIN/DELETE/INSERT/COMMIT with explicit ROLLBACK on any failure;
   on rollback, the previous projection stays in place.

2. Fail-closed when backend TLS is required but no SSL_CTX is
   available. The earlier code in handler_connecting_server had
   `if (desired_backend_tls) { if (ctx) { /* set up TLS */ } }` —
   the inner `if (ctx)` was a guard with no else-branch, so a
   missing SSL_CTX silently produced a plaintext backend connection
   in TLS-required and AsClient-on-TLS-frontend scenarios. Reject
   the connect with X-Protocol error 2026 ("Backend TLS required
   but no SSL context configured on this worker") instead. Caught
   by CodeRabbit on PR #5707.

3. Stale destination_hostgroup in stats rows. `MysqlxStatsStore::
   get_or_create` only set `destination_hostgroup` on first insert.
   If a route was rebound to a different hostgroup via LOAD MYSQLX
   ROUTES TO RUNTIME, subsequent traffic continued reporting the
   first-seen hostgroup forever. Refresh the hostgroup field on
   every lookup; counters are not reset (only metadata is updated).

4. session_status_to_string missing X_PASSTHROUGH_FORWARD case.
   `Mysqlx_Thread::session_status_to_string` enumerated every
   MysqlxSession::Status except the new X_PASSTHROUGH_FORWARD,
   which fell through to the "UNKNOWN" default. The new state was
   added in the TLS passthrough work (commit 4d8af5a02) but the
   stringifier was not updated. Add the explicit case.

All four are local fixes with no test changes required — existing
unit tests still pass under ASAN (mysqlx_session 87/87,
mysqlx_message_dispatch 114/114, mysqlx_stats 26/26,
mysqlx_thread 25/25, mysqlx_concurrent 6/6, mysqlx_admin_schema
25/25, mysqlx_backend_auth 58/58, mysqlx_tls 44/44,
mysqlx_config_store 33/33). The pre-existing
mysqlx_admin_commands_unit-t test 24 failure
("mysqlx_variables has 4 rows after save") predates this branch.
feature/mysqlx-stack-consolidated
Rene Cannao 2 months ago
parent 352455d79c
commit e4908a1df6

@ -299,19 +299,24 @@ uint64_t monotonic_time_ms_local() {
void mysqlx_populate_stats_processlist(SQLite3DB& statsdb) {
MysqlxPluginContext& ctx = mysqlx_context();
// Always wipe the projection table — empty thread pool or empty
// session list both mean "no active sessions" and the operator
// must see that, not stale rows from the last refresh.
statsdb.execute("DELETE FROM stats_mysqlx_processlist");
if (ctx.threads.empty()) return;
uint64_t now_ms = monotonic_time_ms_local();
std::vector<MysqlxSessionSnapshot> rows;
for (const auto& thr : ctx.threads) {
if (thr) thr->snapshot_sessions_for_stats(rows, now_ms);
}
// Atomic rebuild: DELETE + INSERTs run in a single transaction so a
// failure in any INSERT rolls everything back, leaving the previous
// projection in place rather than a half-populated (or empty) table.
// Reviewer feedback (CodeRabbit / Gemini on PR #5704) flagged that the
// previous bare DELETE-then-loop-INSERTs would leave operators staring
// at "no sessions" right after a transient SQLite error, which is far
// more misleading than "stale-but-recent" data.
if (!statsdb.execute("BEGIN")) return;
if (!statsdb.execute("DELETE FROM stats_mysqlx_processlist")) {
statsdb.execute("ROLLBACK");
return;
}
for (const auto& r : rows) {
std::string sql = "INSERT INTO stats_mysqlx_processlist "
"(username, route, worker_id, backend_host, backend_port, "
@ -328,8 +333,12 @@ void mysqlx_populate_stats_processlist(SQLite3DB& statsdb) {
sql += ", "; sql += std::to_string(r.bytes_out);
sql += ", "; sql += std::to_string(r.session_age_ms);
sql += ")";
statsdb.execute(sql.c_str());
if (!statsdb.execute(sql.c_str())) {
statsdb.execute("ROLLBACK");
return;
}
}
statsdb.execute("COMMIT");
}
// Default visibility is required because the plugin .so is built with

@ -1799,12 +1799,26 @@ void MysqlxSession::handler_connecting_server() {
// above) using mysqlx_tls_backend_mode + per-endpoint
// use_ssl override + frontend TLS state. Replicate the decision
// here onto the freshly-allocated MysqlxConnection.
//
// Fail-closed contract: if backend TLS is desired but no SSL_CTX
// is available on this worker, refuse the connection rather than
// silently downgrading to plaintext. The earlier code had `if
// (desired_backend_tls) { if (ctx) { ... } }` — the inner ctx
// check was a guard, but the missing else-branch meant a
// missing/misconfigured SSL_CTX silently produced a plaintext
// connection in TLS-required and AsClient-on-TLS-frontend
// scenarios. CodeRabbit flagged this on PR #5707; fixed here.
if (desired_backend_tls) {
if (thread_ptr_ && thread_ptr_->get_ssl_ctx()) {
backend_conn_->set_backend_tls_required(true);
backend_conn_->set_ssl_ctx(thread_ptr_->get_ssl_ctx());
backend_conn_->set_backend_tls_fallback_allowed(tls_fallback_allowed);
SSL_CTX* ssl_ctx = thread_ptr_ ? thread_ptr_->get_ssl_ctx() : nullptr;
if (ssl_ctx == nullptr) {
send_error(2026, "Backend TLS required but no SSL context configured on this worker");
delete backend_conn_; backend_conn_ = nullptr;
status_ = X_SESSION_CLOSING; healthy = false;
return;
}
backend_conn_->set_backend_tls_required(true);
backend_conn_->set_ssl_ctx(ssl_ctx);
backend_conn_->set_backend_tls_fallback_allowed(tls_fallback_allowed);
}
if (identity_) {

@ -38,6 +38,12 @@ MysqlxRouteStats& MysqlxStatsStore::get_or_create(const std::string& route_name,
inserted->second.destination_hostgroup = destination_hostgroup;
return inserted->second;
}
// Refresh the metadata so a route whose destination_hostgroup was
// rebound (e.g. via LOAD MYSQLX ROUTES TO RUNTIME pointing the same
// route at a different hostgroup) reports the current target, not
// the hostgroup we first saw at the route's first traffic event.
// Counters are NOT reset — only metadata is refreshed.
it->second.destination_hostgroup = destination_hostgroup;
return it->second;
}
@ -97,7 +103,16 @@ std::optional<std::pair<std::string, int>> MysqlxStatsStore::get_last_conn_err_f
void MysqlxStatsStore::flush_to_sqlite(SQLite3DB& statsdb) {
std::lock_guard<std::mutex> lock(mutex_);
statsdb.execute("DELETE FROM stats_mysqlx_routes");
// Atomic rebuild: DELETE + INSERTs run in a single transaction so a
// transient SQLite error during the loop rolls back to the previous
// projection rather than leaving an empty stats_mysqlx_routes — which
// would mislead operators into thinking no traffic flowed. Same shape
// as mysqlx_populate_stats_processlist in mysqlx_plugin.cpp.
if (!statsdb.execute("BEGIN")) return;
if (!statsdb.execute("DELETE FROM stats_mysqlx_routes")) {
statsdb.execute("ROLLBACK");
return;
}
for (const auto& [name, stats] : route_stats_) {
// Build with std::string so a long, escaped route name can never silently
@ -120,6 +135,10 @@ void MysqlxStatsStore::flush_to_sqlite(SQLite3DB& statsdb) {
sql += ", ";
sql += std::to_string(stats.bytes_recv.load(std::memory_order_relaxed));
sql += ")";
statsdb.execute(sql.c_str());
if (!statsdb.execute(sql.c_str())) {
statsdb.execute("ROLLBACK");
return;
}
}
statsdb.execute("COMMIT");
}

@ -444,6 +444,7 @@ const char* session_status_to_string(MysqlxSession::Status s) {
case MysqlxSession::X_SESSION_CLOSING: return "X_SESSION_CLOSING";
case MysqlxSession::X_SESSION_CLOSED: return "X_SESSION_CLOSED";
case MysqlxSession::X_SESSION_RESET_WAITING: return "X_SESSION_RESET_WAITING";
case MysqlxSession::X_PASSTHROUGH_FORWARD: return "X_PASSTHROUGH_FORWARD";
}
return "UNKNOWN";
}

Loading…
Cancel
Save