[Lang-Akshay]

Problem

The plugin manager, including the external OPA plugin, was rebuilt for every tool and every 30 seconds. Each rebuild re-initialized all plugins. Tracked in #6532.

Changes

• Tools of a team without tool-specific bindings share one manager, cached under <team_id>::*. Wildcard bindings apply to it.
• A tool with its own bindings keeps its own manager and never receives the shared one.
• Concurrent cold builds keep one manager. The loser shuts down before any request receives it.
• PLUGIN_MANAGER_CACHE_TTL sets the cache lifetime. Default is 300 seconds (was a fixed 30). 0 disables expiry.
• The invalidation listener rebuilds managers after each Redis subscribe, because pub/sub does not replay missed messages.

Security

The manager holds no per-request state. Per-request identity is passed as hook arguments. Bound tools never run with the shared manager's configuration.

Closes #6532

[msureshkumar88]

Thank you for the PR. The goal is right: initialising an external plugin such as OPA on a cold context is expensive, and one slot per tool is wasteful. The diff is focused. It has no schema change, no Alembic change, and no unrelated edits. The tests are readable.

I request changes. Fix 2 can run a tool without its bindings and can shut down a manager that is in use. Fix 1 removes the only path that recovers from a missed invalidation. I reproduced F1 and F2 against the real TenantPluginManagerFactory with a fake manager and fake bindings.

Scope note: the issue reporter's triage could not reproduce a per-request rebuild. Context IDs are stable (team::tool.name, server_id, or __global__), so a 30 s TTL explains one rebuild per context per 30 s, not one per request. A failed _build_manager caches nothing (gateway_plugin_manager.py:250-257), so an external plugin that keeps failing init would rebuild on every request. Please confirm the real cause with a log sample from the reporter.

severity | path:line | problem | fix

blocking | gateway_plugin_manager.py:157-166 | F1. A tool with its own bindings gets the team's unbound manager. The canonical-slot fast path runs after the per-tool key misses and before any bindings lookup. On a cold worker, unbound tool A fills T::__team__. Tool B, which has an OPA binding, then hits that slot and runs with base config. Repro: B config is BASE when A runs first and MERGED:OPA when B runs first. The outcome depends on request order after a restart or scale-out. With a TTL set, an expired bound tool also falls through to the canonical slot (line 152 pops, line 166 returns). The expired manager is never shut down because line 168 is skipped. | Delete the fast path. Add an alias map context_id -> cache_key, filled by _build_manager. get_manager reads the alias first. A tool with no alias runs get_config_from_db once, then reuses the shared slot without re-initialising plugins.

blocking | gateway_plugin_manager.py:204-241 | F2. Concurrent cold requests shut down a manager that is in use. _inflight is keyed by context_id. Unbound tools A and C have different keys but one cache_key. Both run initialize(). The second store replaces the first and calls old.shutdown() on the manager already returned to request A. Repro: two builds, and the manager returned to A is shut down. With on_error: ignore, a policy plugin then fails open (CWE-636). The existing check at line 214 runs before the slow initialize(), so it does not close the window. | After initialize(), take _lock and re-read cache_key. If a live entry exists, shut down the new manager and return the existing one. Alternatively, key _inflight by the resolved cache_key.

blocking | gateway_plugin_manager.py:94, config.py:1395 | F3. TTL=0 removes the only path that recovers from a missed invalidation. (a) Without Redis, _publish_invalidation returns False (plugins/__init__.py:248), so peer workers never see binding changes. Before: stale at most 30 s. After: stale until restart. (b) The pub/sub listener does not resync after reconnect (plugins/__init__.py:495-537), and Redis pub/sub does not replay. (c) Mode overrides expire in Redis after _PLUGIN_MODE_TTL_SECONDS = 86400. A manager built under an override keeps it after expiry. A plugin disabled for a day stays disabled until restart. This changes a default without documentation, which is a consumer-contract change. | Keep a bounded default (for example 300 s). Make refresh cheap and off the request path: rebuild only when a fingerprint changes (bindings updated_at plus Redis mode values), or refresh in the background and swap. Call invalidate_all() after each listener (re)subscribe.

functionally-impacting | gateway_plugin_manager.py:209-212 | F4. Fix 2 has no effect for teams that use wildcard (*) bindings. get_bindings_for_tool merges * rows, so config is never None for such a team. Every tool keeps its own slot and its own OPA init. | Pick the shared slot when the tool has no specific rows: not any(b.tool_name == tool_name for b in bindings). The shared config then equals the wildcard-only config.

functionally-impacting | gateway_plugin_manager.py:46 | F5. The sentinel __team__ is a valid tool name (validation_tool_name_pattern allows it). A tool named __team__ with bindings loads those bindings into the shared slot. Every unbound tool in the team inherits them (CWE-706). | Use a key outside the tool-name alphabet, such as T::*. get_bindings_for_tool(db, team, "*") already returns the wildcard rows.

functionally-impacting | tests/unit/mcpgateway/plugins/test_gateway_plugin_manager.py | F6. Tests mock _build_manager and get_config_from_db, and test_get_manager_fast_path_finds_team_slot asserts the F1 behaviour. Missing: bound tool after the shared slot exists; concurrent cold start; wildcard team; tool named __team__; TTL > 0 expiry of a bound tool; invalidate_team and invalidate_all rebuilding the shared slot; one test with real get_bindings_for_tool on SQLite. AGENTS.md also requires a live-gateway test: add one under tests/live_gateway/plugins/ that makes N calls, asserts one Plugin manager initialized log per context, and checks that a binding applies when an unbound tool ran first. | Add the tests, and replace the fast-path test.

functionally-impacting | docs, .env.example | F7. PLUGIN_MANAGER_CACHE_TTL is not in .env.example, docs/docs/architecture/plugins.md, or the Helm values. The default change needs a release note. | Document the variable and the default.

suggestion | gateway_plugin_manager.py:148-166 | F8. Every get_manager call takes the global lock, and the PR adds a split and f-string under it for each per-tool miss. The hit path needs no lock on the event loop. | Read _managers and the alias map without the lock. Lock only to build.

suggestion | gateway_plugin_manager.py:250-257 | F9. No negative caching for failed builds. A failing external plugin pays the full init on every request. | Add a short failure cooldown (5-10 s). Log once per cooldown.

suggestion | gateway_plugin_manager.py | F10. Add an INFO log or counter for builds and slot reuse so the issue's acceptance criterion can be checked in production.

minor | gateway_plugin_manager.py:239,248,256 | Shutdown-failure warnings omit exc_info=True. | Add it.

minor | gateway_plugin_manager.py:42-46,134-144,157-159,194-198,204-207 | Five comment and docstring blocks restate one design, and the tests use "Fix A/B" labels. | Keep one docstring. Drop the labels.

minor | PR body | The body lists test counts. AGENTS.md says no test plans in PRs. | Remove the line.

Blast radius if merged as is: deployments without Redis, or with a Redis interruption, keep stale binding and mode policy until restart (F3). Teams with mixed bound and unbound tools can run bound tools without their policy after a cold start (F1). Concurrent cold starts can break an in-flight request (F2).