Skip to content

v4.0/MCP: LOAD MCP <X> FROM DISK (and its TO MEMORY alias) also installs to runtime and can start the listener #6171

Description

@renecannao

Summary

All three of the genai plugin's LOAD MCP <X> FROM DISK verbs copy disk.main. and then install into the runtime. Two of them additionally call mcp_start_listener_if_enabled(), which can construct and start the MCP listener.

This breaks the admin verb contract that the rest of ProxySQL — core and the mysqlx plugin alike — obeys:

Verb family Contract
LOAD <X> FROM DISK / LOAD <X> TO MEMORY disk → memory
LOAD <X> TO RUNTIME / LOAD <X> FROM MEMORY memory → runtime
SAVE <X> TO DISK / SAVE <X> FROM MEMORY memory → disk
SAVE <X> TO MEMORY / SAVE <X> FROM RUNTIME runtime → memory

Because LOAD MCP <X> TO MEMORY is registered as an alias of FROM DISK, the practical effect is that LOAD MCP PROFILES TO MEMORY applies configuration to the runtime, and LOAD MCP VARIABLES TO MEMORY can open the MCP port. TO MEMORY is the single verb in the admin vocabulary whose entire purpose is stage this without applying it.

Affected version / build

Branch v3.0, v4.0 tier (PROXYSQL40=1), verified at 5f9806ea528af5d9d7fffd3900f2810bc6279d41.

Detail

Verb Current behaviour Expected
LOAD MCP VARIABLES FROM DISK (plugin_commands.cpp:240) disk→main, mcp_load_variables_from_admindb(), mcp_start_listener_if_enabled() disk→main
LOAD MCP PROFILES FROM DISK (plugin_commands.cpp:434) disk→main, mcp_load_target_auth_map_from_admindb(), mcp_start_listener_if_enabled() disk→main
LOAD MCP QUERY RULES FROM DISK (plugin_commands.cpp:479) disk→main, mcp_load_query_rules_to_runtime() disk→main

Alias registrations that widen the blast radius (plugin_commands.cpp:555, :571):

reg("LOAD MCP VARIABLES FROM DISK", ..., { "LOAD MCP VARIABLES TO MEMORY", "LOAD MCP VARIABLES TO MEM" });
reg("LOAD MCP PROFILES FROM DISK",  ..., { "LOAD MCP PROFILES TO MEMORY" });

For contrast, the two neighbours that get it right:

  • mysqlxload_users_from_disk() calls disk_to_memory() and nothing else (plugins/mysqlx/src/mysqlx_admin_schema.cpp:323); same for routes, backend endpoints and variables.
  • coreFlushCommandWrapper(..., "disk_to_memory") is a pure table copy (lib/Admin_Handler.cpp:571).

Why this matters beyond tidiness

  1. No way to stage config. An operator cannot pull the on-disk MCP configuration into main. to review or edit it before applying. Loading it is applying it. Every other ProxySQL module supports the staged workflow.
  2. A listener can be started as a side effect. On a node where MCP has been deliberately stopped at runtime while mcp-enabled=true remains on disk, LOAD MCP VARIABLES FROM DISK — or its TO MEMORY alias — silently reopens the MCP port. The MCP endpoints expose backend query execution, so this is not a cosmetic difference.
  3. It hides the real cause of v4.0: plugin-registered config tables are never restored from disk at startup (MCP profiles and query rules lost on restart) #6167. Because FROM DISK also applies to runtime, the natural workaround for the missing startup restore appeared to "just work", which made the underlying startup gap harder to see for what it was.

Reproduction

-- Park the listener at runtime while leaving it enabled on disk.
SET mcp-enabled=true;
LOAD MCP VARIABLES TO RUNTIME;
SAVE MCP VARIABLES TO DISK;
SET mcp-enabled=false;
LOAD MCP VARIABLES TO RUNTIME;      -- listener stops; port closed

-- Stage the on-disk config for review. Should touch main. only.
LOAD MCP VARIABLES TO MEMORY;

-- Observed: the MCP port is listening again.
-- Expected: still closed; only `LOAD MCP VARIABLES TO RUNTIME` should reopen it.

Same shape for profiles:

INSERT INTO mcp_target_profiles (target_id, protocol, hostgroup_id, auth_profile_id, active)
VALUES ('staged','mysql',10,'a1',1);
SAVE MCP PROFILES TO DISK;
DELETE FROM mcp_target_profiles WHERE target_id='staged';
LOAD MCP PROFILES TO RUNTIME;       -- runtime has no 'staged'

LOAD MCP PROFILES TO MEMORY;        -- should only repopulate main.
-- Observed: 'staged' is live on the MCP query endpoint.

Suggested fix

Remove the runtime install and the mcp_start_listener_if_enabled() call from all three *_from_disk callbacks, leaving them as pure disk.main. copies. LOAD MCP <X> TO RUNTIME remains the only verb that applies configuration, and the only one that may start or restart the listener.

Migration note: the test suite depends on the current behaviour

This is a behaviour change to shipped verbs, and the TAP suite has grown to rely on it — several tests use LOAD MCP VARIABLES FROM DISK as a "restore the runtime" idiom, in helpers literally named restore_mcp_runtime(). Each of these needs an explicit LOAD MCP ... TO RUNTIME appended:

  • test/tap/tests/mcp_show_connections_commands_inmemory-t.cpp:116 (restore_mcp_runtime)
  • test/tap/tests/mcp_mysql_concurrency_stress-t.cpp:216 (restore_mcp_runtime)
  • test/tap/tests/mcp_pgsql_concurrency_stress-t.cpp:212 (restore_mcp_runtime)
  • test/tap/tests/mcp_mixed_mysql_pgsql_concurrency_stress-t.cpp:300 (restore_mcp_runtime)
  • test/tap/tests/mcp_query_rules-t.cpp:233
  • test/tap/tests/mcp_query_run_sql_readonly-t.cpp:226
  • test/tap/tests/mcp_query_run_sql_readonly_bypass-t.cpp:324
  • test/tap/tests/mcp_show_queries_topk-t.cpp:406
  • test/tap/tests/mcp_stats_refresh-t.cpp:357
  • test/tap/tests/test_stats_mcp_tables-t.cpp:186
  • test/tap/tests/mcp_module-t.cpp:332, :507
  • test/tap/tests/mcp_rules_testing/mcp_test_helpers.sh:240 (restore_mcp_group_baseline)

That every one of these call sites reads as "restore the runtime" is itself evidence the current semantics are surprising: the suite adopted the shortcut precisely because the verb did more than its name says.

Test coverage to add

  • FROM DISK / TO MEMORY repopulates main. and leaves the runtime untouched: seed disk, change the runtime, LOAD ... TO MEMORY, assert main. matches disk while runtime_mcp_* still reflects the pre-load state.
  • LOAD MCP VARIABLES TO MEMORY does not start the listener when mcp-enabled=true on disk and the runtime listener is stopped.
  • LOAD ... TO RUNTIME after FROM DISK does apply, i.e. the staged workflow works end to end.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions