fix: address PR review feedback

Resolves in-scope review items on PR #5651.  Items deemed out of scope
for this PR (O(N) admin dispatch lookup, per-plugin mutex, worker
backpressure, runtime-reload DDL semantics, E2E tests going through
ProxySQL rather than directly to MySQL) are tracked elsewhere; items
that were already fixed in earlier commits on this branch (Admin_Handler
MYSQLX alias vectors, Admin_Bootstrap PROXYSQL40 gating) are
acknowledged as stale.

Changes:

* .github/workflows/CI-mysqlx.yml: add `include/` and
  `src/proxysql_global.cpp` to both `sparse-checkout` blocks.  The
  plugin Makefile discovers the repo root via
  `src/proxysql_global.cpp` and includes headers from `include/`;
  without these, `make` inside `plugins/mysqlx` climbs up to `/` or
  fails on missing includes.  (coderabbit: "Checkout the files
  required by the plugin Makefile.")

* .github/workflows/CI-mysqlx.yml: `fail-on-cache-miss` now depends on
  `github.event_name == 'workflow_run'`.  Strict behavior is retained
  for the cache-driven pipeline that CI-trigger populates; manual
  `workflow_dispatch` runs no longer fail when the SHA-specific cache
  doesn't exist yet.  (coderabbit: "Don't make manual runs depend on a
  pre-existing build cache.")

* plugins/mysqlx/Makefile: replace the unbounded
  `while [ ! -f ./src/proxysql_global.cpp ]; do cd ..; done` with a
  bounded upward walk (up to 12 levels) that fails fast with a clear
  error message when the marker file is missing — e.g. on a sparse
  checkout that omits `src/proxysql_global.cpp`.  Build output no
  longer appears to hang indefinitely.  (coderabbit: "Prevent root
  discovery from hanging on sparse or invalid checkouts.")

* doc/PLUGIN_API.md: update startup sequence from the old
  three-phase (load → init → start) description to the four-phase
  chassis lifecycle (load → register_schemas → admin materialize →
  init → start).  Document the `register_schemas` descriptor field
  and the `abi_version` / `PROXYSQL_PLUGIN_ABI_VERSION` contract.
  Clarify that `stop()` pairs with `init()` (not `start()`) for
  teardown symmetry.  (coderabbit: "Update plugin lifecycle
  documentation to reflect four-phase model and ABI versioning.")

* plugins/mysqlx/include/mysqlx_session.h: default-initialize every
  field in `MysqlxCredentials`.  `bool x_enabled` was indeterminate
  for a default-constructed instance, which would let a lookup miss
  produce a value that silently authenticates.  In-class initializers
  give every field a deterministic default.  (coderabbit: "Default-
  initialize credential fields used by auth decisions.")

* plugins/mysqlx/src/mysqlx_backend_session.cpp: close
  `backend_fd_` when `authenticate_backend()` returns false.
  Previously `connect()` stored the newly-opened fd in `backend_fd_`
  and returned the auth result; a retry on the same
  `MysqlxBackendSession` would then overwrite `backend_fd_` and
  leak the previous socket.  (coderabbit: "Close/reset `backend_fd_`
  when authentication fails.")

* lib/Admin_Bootstrap.cpp: `materialize_plugin_tables()` now runs DDL
  only for the subset of plugin-declared tables that were freshly
  inserted into `tables_defs_*` (the rest are already materialized).
  DDL failures are startup-fatal: on `execute()` returning false,
  log the plugin kind + table name + SQL and `exit(EXIT_FAILURE)`.
  A partially materialized schema produces opaque runtime errors from
  the plugin later on, so aborting startup is the right default.
  (gemini + coderabbit: "Execute DDL only for newly materialized
  plugin tables" / "add error checking".)

Not changed in this commit (deferred / out of scope / stale):
* CI-mysqlx E2E tests going through ProxySQL (requires plumbing
  listener + admin config in the CI workflow).
* O(N) admin dispatch scan → hash map.
* Descriptor build-id hash for libstdc++ compat detection.
* Per-plugin mutex on mutable plugin context.
* Worker client-fd enqueue backpressure.
* Stale plan-doc feedback on docs/superpowers/plans/*.md.

Verification (clean tree):
* `unset PROXYSQL*; make cleanall && make debug -j$(nproc) &&
  make build_tap_test_debug -j$(nproc)` -- 0 errors.
* `PROXYSQLGENAI=1 make cleanall && PROXYSQLGENAI=1 make
  debug -j$(nproc) && PROXYSQLGENAI=1 make build_tap_test_debug
  -j$(nproc)` -- 0 errors.
* Plugin unit tests (PROXYSQL40): 48 + 52 + 26 + 46 + 10 = 182 pass.

.github/workflows/CI-mysqlx.yml

@@ -20,6 +20,8 @@ jobs:
         uses: actions/checkout@v4
         with:
           sparse-checkout: |
+            include
+            src/proxysql_global.cpp
             test/tap
             plugins/mysqlx
             test/scripts
@@ -31,7 +33,10 @@ jobs:
             src/proxysql
             test/
           key: build-${{ github.event.workflow_run && github.event.workflow_run.head_sha || github.sha }}
-          fail-on-cache-miss: true
+          # Strict cache dependency when CI-trigger populated the cache
+          # upstream; lenient on manual workflow_dispatch so a missing cache
+          # doesn't block an ad-hoc run.
+          fail-on-cache-miss: ${{ github.event_name == 'workflow_run' }}
 
       - name: Build mysqlx plugin
         run: |
@@ -58,6 +63,8 @@ jobs:
         uses: actions/checkout@v4
         with:
           sparse-checkout: |
+            include
+            src/proxysql_global.cpp
             test/tap
             plugins/mysqlx
             test/scripts
@@ -69,7 +76,10 @@ jobs:
             src/proxysql
             test/
           key: build-${{ github.event.workflow_run && github.event.workflow_run.head_sha || github.sha }}
-          fail-on-cache-miss: true
+          # Strict cache dependency when CI-trigger populated the cache
+          # upstream; lenient on manual workflow_dispatch so a missing cache
+          # doesn't block an ad-hoc run.
+          fail-on-cache-miss: ${{ github.event_name == 'workflow_run' }}
 
       - name: Build mysqlx plugin
         run: |

doc/PLUGIN_API.md

@@ -41,16 +41,37 @@ startup phase before the database takes precedence).
 
 ### Startup Sequence
 
-1. ProxySQL parses `proxysql.cnf` and populates the `plugins` list.
-2. Admin module initializes (creates SQLite databases).
-3. For each plugin path, ProxySQL calls `dlopen()` to load the `.so`.
-4. ProxySQL resolves the `proxysql_plugin_descriptor_v1` symbol.
-5. The plugin's `init()` callback is called, receiving `ProxySQL_PluginServices`.
-   The plugin registers its tables and commands during this call.
-6. ProxySQL creates the registered SQLite tables.
-7. The plugin's `start()` callback is called. The plugin should start its
-   threads, open listener sockets, and load runtime configuration.
-8. ProxySQL is ready. The plugin is live.
+ProxySQL uses a **four-phase** plugin lifecycle.  Every phase but Phase B
+is mandatory; Phase B is optional via the `register_schemas` descriptor
+field and only enabled when the plugin declares ABI version 2.
+
+1. **Phase A — load.**  ProxySQL parses `proxysql.cnf` and populates the
+   `plugins` list.  For each plugin path, ProxySQL calls `dlopen()`,
+   resolves the `proxysql_plugin_descriptor_v1` symbol, and validates
+   the descriptor (`abi_version`, `name`, callback pointers).
+2. **Phase B — register_schemas (optional, ABI 2 only).**  If the
+   descriptor wires `register_schemas`, the loader invokes it with a
+   `ProxySQL_PluginServices` whose `register_table` /
+   `register_command` / `register_command_alias` entries are LIVE but
+   whose DB-handle getters (`get_admindb`, `get_configdb`,
+   `get_statsdb`) are non-null stubs that return `nullptr`.  The plugin
+   declares the tables it owns and (optionally) its admin commands; it
+   MUST NOT touch DB handles here.  Plugins that leave
+   `register_schemas` null (or that declare ABI 1) skip this phase
+   entirely and do all their setup in Phase D.
+3. **Phase C — admin materialization.**  The admin module initializes
+   and materializes the SQLite schemas collected during Phase B
+   (`merge_plugin_tables` + `CREATE TABLE`).  On DDL failure ProxySQL
+   aborts startup.
+4. **Phase D — init.**  The plugin's `init()` callback is called,
+   receiving a fully live `ProxySQL_PluginServices` (DB handles now
+   valid).  Plugins that opted out of Phase B register their tables
+   AND commands here; plugins that used Phase B only finish their
+   context setup.
+5. **Phase E — start.**  The plugin's `start()` callback is called.
+   The plugin should start its threads, open listener sockets, and
+   load runtime configuration.  After this returns, ProxySQL is ready
+   and the plugin is live.
 
 ### Shutdown Sequence
 
@@ -80,26 +101,37 @@ All types are defined in `include/ProxySQL_Plugin.h`:
 ```cpp
 struct ProxySQL_PluginDescriptor {
     const char *name;                         // Human-readable plugin name
-    uint32_t abi_version;                     // Must be 1
+    uint32_t abi_version;                     // PROXYSQL_PLUGIN_ABI_VERSION (1 or 2)
     proxysql_plugin_init_cb init;             // bool (*)(ProxySQL_PluginServices *)
     proxysql_plugin_start_cb start;           // bool (*)()
     proxysql_plugin_stop_cb stop;             // bool (*)()
     proxysql_plugin_status_json_cb status_json;  // const char *(*)()
+    proxysql_plugin_register_schemas_cb register_schemas; // ABI 2 only, optional
 };
 ```
 
-| Field          | Type     | Description                                               |
-|----------------|----------|-----------------------------------------------------------|
-| `name`         | `const char*` | Plugin identifier, used in logging.                  |
-| `abi_version`  | `uint32_t`    | Must be `1`.                                         |
-| `init`         | callback      | Called once at startup. Receives `ProxySQL_PluginServices`. Register tables and commands here. |
-| `start`        | callback      | Called after `init`. Start threads, open sockets, load config. |
-| `stop`         | callback      | Called on shutdown. Stop threads, release resources.  |
-| `status_json`  | callback      | Return a JSON string describing plugin status.        |
+| Field              | Type          | Description                                               |
+|--------------------|---------------|-----------------------------------------------------------|
+| `name`             | `const char*` | Plugin identifier, used in logging.                        |
+| `abi_version`      | `uint32_t`    | Set from `PROXYSQL_PLUGIN_ABI_VERSION`.  Value `1` = pre-chassis descriptor (six fields).  Value `2` = PROXYSQL40 descriptor (adds `register_schemas`).  A v3/v3.1 ProxySQL core rejects `abi_version > 1`. |
+| `init`             | callback      | Phase D — called with live services; register tables and commands here (or finish context setup if `register_schemas` already did it). |
+| `start`            | callback      | Phase E — start threads, open sockets, load config.        |
+| `stop`             | callback      | Called on shutdown.  Pairs with `init`, not `start`: if `init` returned true and `start` later failed, `stop` is still called so the plugin can release resources it allocated in `init`. |
+| `status_json`      | callback      | Return a static JSON string describing plugin status.      |
+| `register_schemas` | callback      | Phase B (ABI 2 only).  Optional; leave null to skip Phase B entirely.  Services passed here have `register_table` / `register_command` / `register_command_alias` LIVE but DB-handle getters returning `nullptr`. |
 
 All callbacks return `bool` (except `status_json` which returns `const char*`).
-Return `true` on success, `false` on failure. A `false` return from `init` or
-`start` causes ProxySQL to exit.
+Return `true` on success, `false` on failure. A `false` return from
+`register_schemas`, `init`, or `start` causes ProxySQL to exit.
+
+#### ABI version
+
+`include/ProxySQL_Plugin.h` exposes `PROXYSQL_PLUGIN_ABI_VERSION` (2 under
+PROXYSQL40, undefined in pre-chassis builds — the descriptor is then a
+legacy six-field struct with `abi_version = 1`).  Plugins MUST assign
+`abi_version` from this macro rather than hard-coding a literal; the
+core's loader uses it to detect layout skew and reject plugins built
+for an unsupported ABI.  See `ProxySQL_Plugin.h` for the exact rules.
 
 ### The Entry Point
 

lib/Admin_Bootstrap.cpp

@@ -1352,7 +1352,17 @@ void ProxySQL_Admin::materialize_plugin_tables() {
 	ProxySQL_PluginManager* plugin_manager = proxysql_get_plugin_manager();
 	if (!plugin_manager) return;
 
-	auto merge_new = [this](std::vector<table_def_t *>* target, const std::vector<ProxySQL_PluginTableDef>& defs) {
+	// merge_new returns the subset of `defs` that were actually added
+	// (skipping entries whose table_name already exists in `target`).
+	// The caller runs DDL for only this subset so a repeat invocation
+	// would not re-execute CREATE TABLE statements for tables that
+	// already exist in the SQLite database.
+	auto merge_new = [this](
+		std::vector<table_def_t *>* target,
+		const std::vector<ProxySQL_PluginTableDef>& defs
+	) -> std::vector<const ProxySQL_PluginTableDef*> {
+		std::vector<const ProxySQL_PluginTableDef*> added;
+		added.reserve(defs.size());
 		for (const auto& def : defs) {
 			bool exists = false;
 			for (const auto* existing : *target) {
@@ -1363,23 +1373,34 @@ void ProxySQL_Admin::materialize_plugin_tables() {
 			}
 			if (!exists) {
 				insert_into_tables_defs(target, def.table_name, def.table_def);
+				added.push_back(&def);
 			}
 		}
+		return added;
 	};
 
-	merge_new(tables_defs_admin, plugin_manager->tables(ProxySQL_PluginDBKind::admin_db));
-	merge_new(tables_defs_config, plugin_manager->tables(ProxySQL_PluginDBKind::config_db));
-	merge_new(tables_defs_stats, plugin_manager->tables(ProxySQL_PluginDBKind::stats_db));
-
-	for (const auto& def : plugin_manager->tables(ProxySQL_PluginDBKind::admin_db)) {
-		admindb->execute(def.table_def);
-	}
-	for (const auto& def : plugin_manager->tables(ProxySQL_PluginDBKind::config_db)) {
-		configdb->execute(def.table_def);
-	}
-	for (const auto& def : plugin_manager->tables(ProxySQL_PluginDBKind::stats_db)) {
-		statsdb->execute(def.table_def);
-	}
+	const auto new_admin  = merge_new(tables_defs_admin,  plugin_manager->tables(ProxySQL_PluginDBKind::admin_db));
+	const auto new_config = merge_new(tables_defs_config, plugin_manager->tables(ProxySQL_PluginDBKind::config_db));
+	const auto new_stats  = merge_new(tables_defs_stats,  plugin_manager->tables(ProxySQL_PluginDBKind::stats_db));
+
+	// Plugin-declared DDL failures are startup-fatal: a partially
+	// materialized schema would only surface later as opaque runtime
+	// errors from the plugin itself.  Log the plugin, table name, and
+	// SQLite message, then abort.
+	auto run_ddl = [](SQLite3DB* db, const std::vector<const ProxySQL_PluginTableDef*>& defs, const char* kind) {
+		for (const auto* def : defs) {
+			if (!db->execute(def->table_def)) {
+				proxy_error("Plugin %s-db DDL failed for table %s: %s\n",
+				            kind,
+				            def->table_name ? def->table_name : "(null)",
+				            def->table_def ? def->table_def : "(null)");
+				exit(EXIT_FAILURE);
+			}
+		}
+	};
+	run_ddl(admindb,  new_admin,  "admin");
+	run_ddl(configdb, new_config, "config");
+	run_ddl(statsdb,  new_stats,  "stats");
 
 	// admindb<->configdb("disk") and admindb<->statsdb("stats") are
 	// already attached by init_sqlite3db() above this function in the

plugins/mysqlx/Makefile

@@ -1,6 +1,14 @@
 #!/bin/make -f
 
-PROXYSQL_PATH := $(shell while [ ! -f ./src/proxysql_global.cpp ]; do cd ..; done; pwd)
+# Walk upward until we find the repo root (src/proxysql_global.cpp).  A hop
+# counter stops the climb at / on a sparse checkout that omits the marker
+# file -- otherwise we'd loop forever.  Fails fast with a clear message so
+# build output doesn't look like it's hanging.
+PROXYSQL_PATH := $(shell set -e; d="$$PWD"; for _ in 1 2 3 4 5 6 7 8 9 10 11 12; do if [ -f "$$d/src/proxysql_global.cpp" ]; then echo "$$d"; exit 0; fi; if [ "$$d" = "/" ]; then break; fi; d="$$(dirname "$$d")"; done; echo "ERROR: plugins/mysqlx/Makefile: could not find src/proxysql_global.cpp (repo root) from $$PWD; sparse checkout missing include/ and src/proxysql_global.cpp?" >&2; exit 1)
+
+ifneq (,$(findstring ERROR,$(PROXYSQL_PATH)))
+$(error $(PROXYSQL_PATH))
+endif
 
 include $(PROXYSQL_PATH)/include/makefiles_vars.mk
 include $(PROXYSQL_PATH)/include/makefiles_paths.mk

plugins/mysqlx/include/mysqlx_session.h

@@ -12,10 +12,12 @@
 class Mysqlx_Thread;
 
 struct MysqlxCredentials {
-	std::string password_hash;
-	bool x_enabled;
-	std::string allowed_auth;
-	std::string backend_password;
+	std::string password_hash {};
+	// Default to false so default-constructed (e.g. lookup-miss) values
+	// never authenticate by accident via an indeterminate bool.
+	bool x_enabled { false };
+	std::string allowed_auth {};
+	std::string backend_password {};
 };
 
 typedef std::function<MysqlxCredentials(const std::string& username)> MysqlxCredentialLookup;

plugins/mysqlx/src/mysqlx_backend_session.cpp

@@ -83,7 +83,15 @@ bool MysqlxBackendSession::connect(const MysqlxResolvedIdentity& identity,
 	std::string backend_user = identity.backend_username;
 	std::string backend_pass = identity.backend_password;
 
-	return authenticate_backend(backend_user, backend_pass, err);
+	if (!authenticate_backend(backend_user, backend_pass, err)) {
+		// Auth failure: close the socket we just opened so a later retry
+		// on this MysqlxBackendSession doesn't overwrite backend_fd_ and
+		// leak the previous fd.
+		close(backend_fd_);
+		backend_fd_ = -1;
+		return false;
+	}
+	return true;
 }
 
 bool MysqlxBackendSession::authenticate_backend(const std::string& username,