feat(plugins): restrict non-hook plugin initialization to a single worker across instances

[gandhipratik203]

📌 Summary

Adds an opt-in Redis backend for primary-worker election so a single primary can be elected across multiple gateway instances/replicas, not just per host. The default remains filelock (per-host), so existing single-host deployments are unchanged.

Related to #5139 (cross-instance follow-up to the per-host is_primary_worker() work).

💡 What it adds

mcpgateway/services/leader_election.py — a PrimaryWorkerElector with two backends behind the existing is_primary_worker() API:

• filelock (passive, default): one primary per host. The OS releases the lock on process exit; no background task.
• redis (active): one primary across all instances sharing a Redis. Lease via SET NX EX, renewed by an atomic compare-and-renew Lua heartbeat, released by an if-owner Lua, with a follower loop that re-acquires on lease expiry.

Wiring:

• is_primary_worker() reads the elector's cached flag for the redis backend and fails closed if the elector isn't started — a redis deployment never silently degrades to per-host scope.
• The app lifespan starts the elector before plugin initialization (awaiting the initial election) and stops it on shutdown. Only the redis backend needs an elector; the filelock path stays lazy.

⚙️ Config (conservative defaults)

Setting	Default
PRIMARY_WORKER_ELECTION_BACKEND	filelock
PRIMARY_WORKER_REDIS_KEY	mcpgw:primary_worker
PRIMARY_WORKER_LEASE_TTL	15
PRIMARY_WORKER_HEARTBEAT_INTERVAL	5
PRIMARY_WORKER_REDIS_UNAVAILABLE_POLICY	fail_closed (or filelock_fallback)

No behavior change is triggered by CACHE_TYPE; the backend is explicit.

🧪 Verification

Check	Status
Unit tests (fakeredis: single-primary, renew, release, follower takeover, fail-closed, filelock-fallback) + backend switch	✅ 19 passed
Real 2-worker gateway boot, redis backend	✅ exactly 1 primary
Real 2-worker gateway boot, filelock backend (regression)	✅ exactly 1 primary
ruff / mypy on new files	✅ clean

🐳 Multi-instance verification (manual)

Proves the cross-instance guarantee on a real multi-replica stack: two gateway containers sharing one Redis elect exactly one primary — something a per-host file lock cannot do. make test-primary-worker-multiinstance automates this; the steps below let a reviewer drive it by hand and inspect the live election in Redis.

Everything needed ships with the PR — the marker plugin (plugins/primary_worker_multiinstance/marker.py, a minimal non-hook example gated on is_primary_worker()) and its config (plugins/primary_worker_multiinstance_config.yaml) are committed. The plugin is opt-in (loaded only via that config), so it never runs in a normal deployment. Its SADD host:pid to a Redis set is e2e observability (so we can count primaries across containers), not a representative side effect — a real plugin puts its own work after the guard.

Fast path:

make docker                             # bake this branch's election code into the image
make test-primary-worker-multiinstance  # 2 replicas + redis; asserts exactly one primary

Detailed step-by-step (click to expand)

Prerequisites

• Docker + docker compose, run from the repo root on this branch.

How the pieces reach the container (why step 1 is required)

Two layers arrive by different routes:

• Gateway core — is_primary_worker(), the Redis elector, and the lifespan wiring — is compiled into the image. It must be rebuilt from this branch (make docker), or a stale image would load the plugin but run the old (filelock-only) election.
• The marker plugin + config is bind-mounted at runtime from your checked-out ./plugins (docker-compose.yml: ${PLUGINS_DIR:-./plugins}:/app/plugins:ro), so it comes from your working tree automatically — no rebuild needed for it.

If you've set PLUGINS_DIR in your .env, unset it for this test so the default ./plugins (containing the committed marker) is mounted.

1. Build the image from this branch

make docker

2. Define a compose shortcut (zsh-safe: a function, since unquoted $VAR doesn't word-split in zsh)

dc() { docker compose -p mcpgw-pw-e2e -f docker-compose.yml "$@"; }

3. Bring up Redis + 2 gateway replicas (redis backend)

REDIS_URL is already wired to the redis service inside compose; you only set the backend, the plugin config, and a small worker count:

GUNICORN_WORKERS=3 \
PRIMARY_WORKER_ELECTION_BACKEND=redis \
PLUGINS_CONFIG_FILE=plugins/primary_worker_multiinstance_config.yaml \
  dc up -d --scale gateway=2 gateway

This also starts the postgres → pgbouncer → migration dependencies automatically.

4. Wait until both gateways are healthy

dc ps gateway     # re-run until BOTH show "(healthy)" — ~30–60s

Give it ~3s more after both are healthy so each replica finishes initialize().

5. Assert exactly one primary across the two containers

The marker plugin adds <host>:<pid> to a shared Redis set only when is_primary_worker() is true. A set (not a list) keeps the count correct even if initialize() runs more than once:

dc exec -T redis redis-cli SCARD    mcpgw:primary_worker:e2e:markers   # => 1   ✅
dc exec -T redis redis-cli SMEMBERS mcpgw:primary_worker:e2e:markers   # => one host:pid

Pass = SCARD is 1. Across all 6 workers (2 replicas × 3), exactly one is primary.

6. Inspect the lease and heartbeat (optional)

dc exec -T redis redis-cli GET mcpgw:primary_worker    # => the primary's lease (instance id)
dc exec -T redis redis-cli TTL mcpgw:primary_worker    # => ~15, refreshed by the heartbeat
# watch it renew (TTL bounces back toward 15, never hits 0):
for i in 1 2 3 4 5; do dc exec -T redis redis-cli TTL mcpgw:primary_worker; sleep 3; done

7. Contrast — filelock backend gives 2 (the gap this PR closes)

dc down --remove-orphans -v          # clean slate (wipes the redis volume/set)

GUNICORN_WORKERS=3 \
PRIMARY_WORKER_ELECTION_BACKEND=filelock \
PLUGINS_CONFIG_FILE=plugins/primary_worker_multiinstance_config.yaml \
  dc up -d --scale gateway=2 gateway

dc ps gateway                        # wait for both healthy
dc exec -T redis redis-cli SCARD mcpgw:primary_worker:e2e:markers   # => 2  (one primary PER container)

A per-host file lock elects one primary per container, so the set has 2 members — exactly what the redis backend fixes.

8. Tear down

dc down --remove-orphans -v

Expected results

Backend	SCARD mcpgw:primary_worker:e2e:markers	Meaning
redis	1	one primary across all containers (the new capability)
filelock	2	one primary per host (cannot dedup across containers)

Troubleshooting / gotchas

• Bind for 0.0.0.0:6379 failed: port is already allocated — another Compose stack is publishing Redis on 6379. Find it with docker ps --filter publish=6379, then docker compose -p <that-project> -f docker-compose.yml down -v.
• Don't drop GUNICORN_WORKERS=3 — the compose default is 24 workers per container.
• Re-running in place (without down -v) reuses existing containers and the existing Redis set — no new election. Either dc down -v first, or clear the set (dc exec -T redis redis-cli DEL mcpgw:primary_worker:e2e:markers) and add --force-recreate to the up command.
• Count distinct primaries, not lines — the marker is a Redis set, so repeated initialize() calls dedup; SCARD is the authoritative count.
• zsh — unquoted $VAR doesn't word-split; use the dc() function above (or ${=VAR}), not C="docker compose …"; $C ….

Requires Docker and brings up the standard compose stack (~1–2 min); not part of make test.

📓 Notes

• Election under the redis backend is best-effort under network partitions; keep side effects idempotent. Documented in the plugin lifecycle docs.
• Public contract unchanged: is_primary_worker() -> bool. Leadership-change callbacks are intentionally deferred.
• GatewayService is not refactored here — this is additive; its leader election can migrate onto the shared elector later.

[msureshkumar88]

Nice work here — the opt-in design (default stays filelock, redis backend fails closed, lifespan wiring is clean) and the test suite (fakeredis-based unit tests + backend-aware e2e + the docker-compose multi-instance script) are a strong foundation. Requesting changes for one correctness issue that goes to the core guarantee this PR is built around.

Blocking

filelock_fallback doesn't reset _is_primary on a mid-heartbeat Redis error — can produce two simultaneous primaries.

In leader_election.py, _maintain():

except Exception as exc:
    if self._policy == "fail_closed":
        self._is_primary = False
    logger.warning("election maintenance error pid=%d: %s", os.getpid(), exc)

_handle_redis_unavailable() (the code that actually does _acquire_filelock()) is only invoked from _start_redis() on the initial connect. If a node is already primary and Redis becomes unreachable mid-heartbeat under filelock_fallback, _is_primary stays True — the except block only demotes for fail_closed. The lease then expires unrenewed, another instance's follower loop wins it via SET NX EX, and now two processes both report primary. That's the exact split-brain this PR exists to prevent.

It also doesn't match the docs — .env.example/configuration.md describe filelock_fallback as "Redis down → per-host" without qualifying "startup only," so a reader would reasonably expect fallback to kick in here too.

Suggested fix: route the maintenance-loop except block through _handle_redis_unavailable(exc) for both policies (uniform handling), or explicitly scope filelock_fallback to startup-only in the docs/setting description and steer long-running deployments toward fail_closed. Given the stated intent ("fail_closed preserves the global guarantee"), the former seems like what was intended.

Would also be good to add a unit test for this case — test_redis_maintenance_error_fail_closed exists, but there's no equivalent for filelock_fallback + mid-heartbeat error, which is exactly the gap that let this through.

Suggestions (non-blocking)

• Duplication: _default_lock_path() in leader_election.py reimplements _lock_path() from mcpgateway/utils/primary_worker.py (same port-scoped temp-file logic, same PRIMARY_WORKER_LOCK_PATH override — the docstring even says "mirrors mcpgateway.utils.primary_worker"). Exporting and reusing the existing helper would avoid keeping two copies in sync by hand.
• Falsy-default pattern: PrimaryWorkerElector.__init__ uses param or settings.xxx for lease_ttl, heartbeat_interval, redis_key, etc. An explicit 0 or "" would silently fall back to the settings default rather than being honored. Not exploitable today since nothing passes 0, but param is None would be more robust and matches the pattern already tested elsewhere (test_empty_string_override_falls_back_to_default).
• Shutdown robustness: stop() only swallows asyncio.CancelledError when awaiting the cancelled maintenance task; any other exception there would propagate out of stop_primary_worker_elector() and could interrupt the rest of the main.py shutdown sequence. Low likelihood given the loop's own internal except Exception, but wrapping it the same best-effort way as the redis-release block right below would make shutdown unconditionally safe.

Minor / non-code

• No issue number referenced in the PR body or commits — worth linking a tracking issue if one exists, per the repo's commit/PR conventions.

Everything else looks solid: no breaking change (default backend unchanged), no dead code, docs are consistent across .env.example/configuration.md/lifecycle.md, and the implementation matches the PR description closely.

[gandhipratik203]

Fixed the filelock_fallback split-brain in 256c5b0ee: maintenance-loop errors now route through _handle_redis_unavailable() for both policies (fail_closed demotes, filelock_fallback re-elects/demotes), so no stale primary survives an error. Made _acquire_filelock() idempotent for the per-heartbeat calls, and added the regression test (pre-hold lock → non-primary) plus the lock-free case.

[gandhipratik203]

Non-blocking suggestions also addressed in 2a0885f9f: deduped the lock-path helper (reuse _lock_path from primary_worker), switched the init defaults to is None so explicit 0/"" are honored, and made stop() best-effort for non-CancelledError task errors. leader_election.py stays at 100% coverage.

[msureshkumar88]

Security hardening — possible credential leak in logs (CWE-532)

mcpgateway/services/leader_election.py, lines 194, 197, and 222 log the raw exception string when Redis is unreachable:

logger.warning("redis unavailable (%s); falling back to per-host filelock", exc)   # line 194
logger.warning("redis unavailable (%s); failing closed (non-primary)", exc)         # line 197
logger.warning("election maintenance error pid=%d: %s", os.getpid(), exc)           # line 222

If REDIS_URL (or PRIMARY_WORKER_REDIS_URL) is configured with embedded credentials (redis://user:pass@host:port), some redis.asyncio connection-error types stringify the connection target, so str(exc) could end up putting the password in the application log. Given fail_closed is the default policy, these lines are also the ones most likely to fire in a misconfigured or degraded environment — exactly when someone will be grepping logs.

Suggested fix: log type(exc).__name__ (and message only if you've confirmed it never embeds the DSN) instead of the raw exception, e.g.:

logger.warning("redis unavailable (%s); failing closed (non-primary)", type(exc).__name__)

Everything else in this PR looks solid — fail-closed default, atomic Lua ops (parameterized via KEYS/ARGV, no injection risk), strong test coverage. Requesting changes only for this log-redaction item; happy to approve once addressed.

[gandhipratik203]

Fixed in 2b712a383: all elector warnings now log type(exc).__name__ instead of str(exc), so a redis.asyncio connection error can't leak the REDIS_URL password into the log. Covered the three flagged lines plus the stop-cleanup and task-cancel paths (same class). Added a regression test that raises a ConnectionError with an embedded DSN mid-heartbeat and asserts the secret never reaches the log while the exception type still does.

[msureshkumar88]

Confirmed the log-redaction fix from the prior round: mcpgateway/services/leader_election.py now logs type(exc).__name__ instead of str(exc) at all three flagged sites (plus two more I hadn't caught, in the stop() cancel/cleanup paths), with an explicit # CWE-532 marker. Verified directly against the PR head commit (2b712a383). Resolving my prior request-changes.

Independent e2e verification

Ran a real (non-mocked) multi-container verification of the PR's core claim — built the gateway image from this branch and brought up the actual docker-compose stack (Postgres → PgBouncer → migration → Redis → 2 gateway replicas), not fakeredis/unit mocks.

Config:

• Image: mcpgateway/mcpgateway:latest built via make docker from this branch's HEAD
• Compose project mcpgw-pr5430-verify, docker-compose.yml + a host-port-only override (postgres/pgbouncer host ports remapped to avoid colliding with an already-running local stack — gateway reaches both via internal service DNS, not host ports, so this has no functional effect on the test)
• PLUGINS_CONFIG_FILE=plugins/primary_worker_multiinstance_config.yaml (the marker plugin shipped in this PR)
• GUNICORN_WORKERS=3, --scale gateway=2 (6 workers total across 2 containers)

Commands:

make docker
docker compose -p mcpgw-pr5430-verify -f docker-compose.yml -f <port-override>.yml up -d redis
docker compose ... exec -T redis redis-cli DEL mcpgw:primary_worker:e2e:markers

GUNICORN_WORKERS=3 PRIMARY_WORKER_ELECTION_BACKEND=redis \
PLUGINS_CONFIG_FILE=plugins/primary_worker_multiinstance_config.yaml \
  docker compose ... up -d --scale gateway=2 gateway
docker compose ... exec -T redis redis-cli SCARD mcpgw:primary_worker:e2e:markers
docker compose ... exec -T redis redis-cli SMEMBERS mcpgw:primary_worker:e2e:markers

# repeat with PRIMARY_WORKER_ELECTION_BACKEND=filelock

Output:

Backend	Containers × workers	Distinct primaries (SCARD)	Members
redis	2×3 = 6	1	6aa536f92127:31
filelock	2×3 = 6	2	08f6adfa639b:32, 175d64465a29:33

RESULT: PASS (redis backend -> 1 primaries, expected 1)
RESULT: PASS (filelock backend -> 2 primaries, expected 2)

Confirms the PR's central claim on a real multi-container boot: the redis backend dedups election across replicas to exactly one primary, while filelock is confirmed limited to per-container election — the exact gap this PR closes.

One non-blocking note: the branch currently shows as having merge conflicts with main — worth a rebase before merge, but unrelated to the code review itself.

Approving — nice work carrying this through several rounds (split-brain fix, idempotent start/stop, heartbeat/TTL validation, and now the log redaction). Solid design and test coverage throughout.

[ja8zyjits]

🚨 Blocking Issues

Test Coverage Gaps (High Priority)

1. Missing startup Redis unavailable tests

   • File: mcpgateway/services/leader_election.py:189-196
   • Need: test_redis_unavailable_at_startup_fail_closed(), test_redis_unavailable_at_startup_filelock_fallback()
   • Current: Only tests mid-heartbeat failures

2. No integration test for elector-before-plugins ordering

   • Files: mcpgateway/main.py:1598-1603, mcpgateway/utils/primary_worker.py:72-78
   • Need: Mock plugin initialize(), assert elector starts first
   • Gap: Critical ordering guarantee never tested

3. E2E marker content not validated

   • File: tests/live_gateway/plugins/test_primary_worker_e2e.py:42-48
   • Current: Only checks count (len(lines) == 1)
   • Fix: Add assert re.match(r'^pid=\d+ t=\d+\.\d+$', lines[0])

Medium Priority

4. Concurrent election race test missing

   • File: mcpgateway/services/leader_election.py:189-196
   • Need: test_redis_concurrent_start_elects_one_primary() with asyncio.gather()

5. Filelock unwritable path not tested

   • File: mcpgateway/services/leader_election.py:163-173
   • Need: Test /root/x.lock as non-root

6. stop() Redis release failure not tested

   • File: mcpgateway/services/leader_election.py:133-145
   • Need: test_stop_swallows_redis_release_error()

7. Boundary test: heartbeat == lease_ttl/2

   • File: mcpgateway/config.py:2571-2576
   • Need: Test exact boundary (heartbeat_interval=7.5, lease_ttl=15)

Code Quality

8. start() idempotency race condition
   • File: mcpgateway/services/leader_election.py:115-116
   • Issue: Two concurrent start() calls could both pass if self._started check
   • Fix: Add lock or atomic flag
   • Severity: Medium (unlikely in single-threaded lifespan, but documented as idempotent)

---

⚠️ Warnings

Advisory Architecture Notes

• Redis client per-elector (not pooled)

  • File: mcpgateway/services/leader_election.py:189-191
  • Impact: Low for single-elector; consider shared pool if connection count becomes issue
  • Acceptable for initial implementation

• Module-level singleton pattern

  • File: mcpgateway/services/leader_election.py:238-262
  • Recommendation: Add docstring clarifying idempotency and lifecycle
  • Current implementation safe

Low Severity Code Issues

• stop() lease release timing (1ms window, edge case)
• _acquire_filelock() idempotency logic subtle (add docstring example)
• stop() swallows task errors (acceptable for shutdown robustness)
• _filelock never released in fallback (OS cleanup, acceptable)
• is_primary_worker() fail-closed logging (could add warning)

Test Gaps (Low Priority)

• Cross-instance E2E manual (not in CI)
• No performance/load tests (100+ electors)
• No SSRF test for untrusted REDIS_URL (operator-controlled)
• No Lua injection test (mitigated by KEYS/ARGV arrays)

---

[gandhipratik203]

Thanks for the thorough pass. Added three of these as targeted tests in baba55121:

• Concurrent election (Add testing documentation #4) — test_redis_concurrent_start_elects_one_primary: 5 electors start() concurrently via asyncio.gather, exactly one wins (SET NX is atomic).
• stop() release-error (Update testing docs, copilot docs and minor fix to servers/ also offering a servers (no /) endpoint #6) — test_stop_swallows_redis_release_error: the lease-release eval raises during stop(), asserted swallowed.
• Boundary (Use async context manager for initializing gateway #7) — test_primary_worker_heartbeat_warns_at_exact_boundary: heartbeat == lease_ttl/2 warns (fields are int, so 7/14).

On the rest:

• Configure Renovate #1 (startup Redis-unavailable tests) — these already exist: test_redis_unavailable_fail_closed and test_redis_unavailable_filelock_fallback drive the startup path (_BoomRedis.set raises inside start() → _handle_redis_unavailable). Same coverage, different names.
• Docker Compose scripts and docs #8 (start() race) — the elector runs in the single-threaded asyncio loop and start() is called exactly once in the app lifespan, so two concurrent start()s don't occur; the if self._started guard is sufficient there. Happy to add an asyncio.Lock if a concurrent caller is ever introduced.
• Open Source release of MCP Context Forge - MCP Gateway #2 / Update Makefile and pyproject.toml with packaging steps #3 / Add postgresql to container build #5 — noted as low-priority: the elector-before-plugins ordering is structurally guaranteed (awaited before plugin init), the e2e's count assertion is the actual guarantee, and the unwritable-path branch is a trivial fail-closed. Can follow up if you feel strongly.

leader_election.py stays at 100% coverage. The advisory items (pooling, SSRF on the operator-controlled REDIS_URL, Lua injection mitigated by KEYS/ARGV) already look acknowledged as acceptable — no changes there.

[ja8zyjits]

LGTM