fix(startup): lazy-import ioredis in rateLimiter to fix MCP ERR_MODULE_NOT_FOUND (#6559)

CHANGELOG.md

@@ -14,6 +14,7 @@ _Living section — bullets land here as PRs merge into `release/v3.8.47` (paral
 
 ### 🐛 Bug Fixes
 
+- **fix(startup):** `omniroute --mcp` crashed at Node ESM link time with `ERR_MODULE_NOT_FOUND` for `ioredis` on installs where the published MCP bundle didn't happen to have `ioredis` rescued from a parent `node_modules` ([#6559](https://github.com/diegosouzapw/OmniRoute/issues/6559)) — `src/shared/utils/rateLimiter.ts` had a top-level static `import Redis from "ioredis"`; that module is only ever reached via a lazy `await import(...)` several call-sites deep in the MCP tool chain, but esbuild's `--packages=external` bundling of the MCP server (`scripts/build/prepublish.ts` Step 8.5) still hoisted rateLimiter.ts's own static import into a real top-level ESM import in the compiled `dist/open-sse/mcp-server/server.js`, forcing Node to resolve `ioredis` at module-link time — before any `--mcp` startup code runs — and `ioredis` is not guaranteed to ship in the MCP-only bundle's `node_modules`. `getRedisClient()` now lazily imports `ioredis` on first use (matching the established soft-dependency pattern in `src/lib/quota/redisQuotaStore.ts`) while still throwing synchronously when Redis isn't configured. Regression guard: `tests/unit/build/mcp-bundle-no-eager-ioredis.test.ts` (bundles the real MCP server entrypoint with the exact publish-time esbuild flags and asserts no top-level static `ioredis` import remains, while the pre-existing lazy `await import("ioredis")` in `redisQuotaStore.ts` stays intact).
 - **fix(auth):** an API key restricted via `allowedModels`/`allowedCombos` could bypass that restriction entirely over the Codex Responses-over-WebSocket bridge ([#6564](https://github.com/diegosouzapw/OmniRoute/issues/6564)) — `prepare()` in `src/app/api/internal/codex-responses-ws/route.ts` authenticated the WS bridge's API key (`authenticate()`/`authorizeWebSocketHandshake()`) and honored `allowedConnections`, but never called `enforceApiKeyPolicy()`, the same model/combo policy gate the HTTP `/v1/responses` path enforces via `handleChat()` — so a key scoped to e.g. `combo/model-1.0` could still reach a direct Codex model like `gpt-5.5` through this transport, as long as an eligible Codex OAuth connection existed. The bridge's WS auth token arrives via query params (`api_key`/`token`/`access_token`), not a normal `Authorization` header, so a new `enforceCodexWsApiKeyPolicy()` builds an equivalent `Request` carrying an explicit `Authorization: Bearer <apiKey>` header and calls `enforceApiKeyPolicy()` against the CLIENT-requested model, before any Codex-specific model remapping or credential selection. Regression guard: `tests/unit/codex-ws-policy-enforcement-6564.test.ts` (a model-restricted key is rejected 403 before reaching credential selection; a combo-restricted key is rejected 403 requesting a disallowed combo; a key that DOES allow the requested model still proceeds past policy).
 - **fix(security):** loopback-gate `/api/middleware/*` so a leaked JWT over a tunnel can't install or trigger a middleware hook — middleware hooks compile + run arbitrary JS via `new vm.Script` on the request hot path (`src/lib/middleware/registry.ts`), the same RCE class as the already-gated `/api/plugins/*`; `/api/middleware/` is now in `LOCAL_ONLY_API_PREFIXES` so loopback enforcement runs unconditionally before any auth check (Hard Rules #15 + #17). Regression guard: `tests/unit/route-guard-middleware-local-only.test.ts`. ([#6541](https://github.com/diegosouzapw/OmniRoute/pull/6541)) — see PR. (thanks @developerjillur)
 - **fix(startup):** AgentBridge's MITM server no longer fails to start with `ROUTER_API_KEY is required` on a normal install ([#6403](https://github.com/diegosouzapw/OmniRoute/issues/6403)) — `POST /api/tools/agent-bridge/server` resolved the spawned MITM child's router key from only an explicit `apiKey` body field (never sent by the AgentBridge UI — the schema has no such field) and the `ROUTER_API_KEY` env var (unset by default), so `startMitm()` always received `""` and the child hard-exited, even though OmniRoute already had a usable API key in its own DB. A new `resolveRouterApiKey()` now falls back to `pickApiKeyForInternalUse()` (the same DB-backed selector the combo-health-check / cloud-sync internal probes use), resolving in order: explicit key → `ROUTER_API_KEY` env → an existing DB key. Regression guard: `tests/unit/agentbridge-mitm-router-key-6403.test.ts`.

src/lib/db/apiKeys.ts

@@ -237,7 +237,7 @@ async function deleteRedisAuthCacheEntry(keyHash: unknown): Promise<void> {
   try {
     const { getRedisClient, isRedisConfigured } = await import("@/shared/utils/rateLimiter");
     if (!isRedisConfigured()) return;
-    const redis = getRedisClient();
+    const redis = await getRedisClient();
     await redis.del(`auth:api_key:${keyHash}`);
   } catch {
     // Redis is an optimization for auth caching; SQLite remains authoritative.
@@ -1126,7 +1126,7 @@ export async function validateApiKey(key: string | null | undefined) {
     try {
       const { getRedisClient, isRedisConfigured } = await import("@/shared/utils/rateLimiter");
       if (isRedisConfigured()) {
-        const redis = getRedisClient();
+        const redis = await getRedisClient();
         const redisKey = `auth:api_key:${hashedKey}`;
         const redisData = await redis.get(redisKey);
         if (redisData) {
@@ -1179,7 +1179,7 @@ export async function validateApiKey(key: string | null | undefined) {
     try {
       const { getRedisClient, isRedisConfigured } = await import("@/shared/utils/rateLimiter");
       if (isRedisConfigured()) {
-        const redis = getRedisClient();
+        const redis = await getRedisClient();
         const redisKey = `auth:api_key:${hashedKey}`;
         await redis.set(
           redisKey,

src/shared/utils/rateLimiter.ts

@@ -1,4 +1,4 @@
-import Redis from "ioredis";
+import type Redis from "ioredis";
 
 // Redis is optional. When REDIS_URL is unset, use a process-local fallback
 // instead of probing localhost on every API request.
@@ -7,7 +7,17 @@ if (process.env.NODE_ENV === "production" && !REDIS_URL) {
   console.warn("[REDIS] REDIS_URL is not set in production. Using in-memory rate limiting.");
 }
 
-let redisClient: Redis | null = null;
+// #6559 — `ioredis` must stay a LAZY dependency here. This module is bundled
+// (via esbuild --packages=external) into the MCP server output, and esbuild
+// hoists any static top-level `import ... from "ioredis"` into a real
+// top-level ESM import in the compiled bundle — even though this module is
+// only ever reached through a dynamic `await import(...)` several call-sites
+// deep (apiKeys.ts -> mcpCallerIdentity.ts -> compressionTools.ts -> server.ts).
+// A static import forces Node to resolve `ioredis` at module-link time,
+// before any `--mcp` startup code runs, and `ioredis` is not guaranteed to
+// ship in the MCP-only bundle's node_modules. Mirrors the established
+// soft-dependency pattern in src/lib/quota/redisQuotaStore.ts.
+let redisClientPromise: Promise<Redis> | null = null;
 
 export function isRedisConfigured(): boolean {
   return REDIS_URL.length > 0;
@@ -43,29 +53,41 @@ export function _createRedisLogThrottleForTests() {
   return createRedisLogThrottle();
 }
 
-export function getRedisClient() {
+/**
+ * Return the singleton Redis client, creating it (lazily importing `ioredis`)
+ * on first call. Throws SYNCHRONOUSLY (not a rejected Promise) when Redis is
+ * not configured — callers rely on this to fail fast without an `await`.
+ * Otherwise returns a Promise that resolves once the client is constructed.
+ */
+export function getRedisClient(): Promise<Redis> {
   if (!isRedisConfigured()) {
     throw new Error("Redis is not configured");
   }
 
-  if (!redisClient) {
-    redisClient = new Redis(REDIS_URL, {
-      maxRetriesPerRequest: 3,
-      enableReadyCheck: false,
-      retryStrategy(times) {
-        return Math.min(times * 50, 2000); // Exponential backoff
-      },
-    });
-    redisClient.on("error", (err) => {
-      // Throttle: log once per error-state change instead of on every retry (#4878).
-      if (redisLogThrottle.shouldLog(err.message)) {
-        console.error("[REDIS] Error:", err.message);
-      }
-    });
-    // A successful connection resets the throttle so the next failure logs again.
-    redisClient.on("ready", () => redisLogThrottle.reset());
+  if (!redisClientPromise) {
+    redisClientPromise = (async () => {
+      // Lazy dynamic import — see the #6559 note above the singleton declaration.
+      const mod = await import("ioredis");
+      const RedisCtor = (mod.default ?? mod) as typeof Redis;
+      const client = new RedisCtor(REDIS_URL, {
+        maxRetriesPerRequest: 3,
+        enableReadyCheck: false,
+        retryStrategy(times) {
+          return Math.min(times * 50, 2000); // Exponential backoff
+        },
+      });
+      client.on("error", (err) => {
+        // Throttle: log once per error-state change instead of on every retry (#4878).
+        if (redisLogThrottle.shouldLog(err.message)) {
+          console.error("[REDIS] Error:", err.message);
+        }
+      });
+      // A successful connection resets the throttle so the next failure logs again.
+      client.on("ready", () => redisLogThrottle.reset());
+      return client;
+    })();
   }
-  return redisClient;
+  return redisClientPromise;
 }
 
 export interface RateLimitRule {
@@ -216,7 +238,7 @@ export async function checkRateLimit(
     return checkInMemoryRateLimit(FALLBACK_MEMORY_STORE, keyId, rules);
   }
 
-  const redis = getRedisClient();
+  const redis = await getRedisClient();
 
   const args: (string | number)[] = [Math.floor(Date.now() / 1000)];
 

tests/unit/build/mcp-bundle-no-eager-ioredis.test.ts

@@ -0,0 +1,67 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+import { execFileSync } from "node:child_process";
+import { mkdtempSync, readFileSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+
+// #6559 — `omniroute --mcp` crashed at Node ESM link time with
+// ERR_MODULE_NOT_FOUND for 'ioredis'.
+//
+// Root cause: src/shared/utils/rateLimiter.ts had a top-level static
+// `import Redis from "ioredis"`. esbuild's `--packages=external` bundling of
+// the MCP server (scripts/build/prepublish.ts Step 8.5) hoists that into a
+// real top-level ESM import in the bundled output, even though rateLimiter.ts
+// is only ever reached via a dynamic `await import(...)` several call-sites
+// deep. `ioredis` is not guaranteed to ship in the MCP-only bundle's
+// node_modules, so the eager import can crash at link time before any
+// `--mcp` startup code runs.
+//
+// This test reproduces the exact bundling step used at publish time and
+// asserts the compiled output never contains a top-level static `ioredis`
+// import — it must stay a lazy `await import("ioredis")`, matching the
+// established soft-dependency pattern in src/lib/quota/redisQuotaStore.ts.
+
+const repoRoot = process.cwd();
+
+test("MCP server bundle has no top-level static import of ioredis", () => {
+  const outDir = mkdtempSync(join(tmpdir(), "mcp-bundle-ioredis-"));
+  const outFile = join(outDir, "server.js");
+
+  try {
+    execFileSync(
+      "npx",
+      [
+        "esbuild",
+        "open-sse/mcp-server/server.ts",
+        "--bundle",
+        "--platform=node",
+        "--packages=external",
+        "--format=esm",
+        `--outfile=${outFile}`,
+      ],
+      { cwd: repoRoot, stdio: "pipe" }
+    );
+
+    const bundled = readFileSync(outFile, "utf8");
+
+    // A static/hoisted ESM import resolves at module-link time and would
+    // crash the MCP server before startup if ioredis isn't in dist/node_modules.
+    assert.doesNotMatch(
+      bundled,
+      /^import\s+.*["']ioredis["'];?\s*$/m,
+      "MCP bundle must not eagerly (statically) import 'ioredis' at the top level — " +
+        "it must stay a lazy `await import(\"ioredis\")` (see src/lib/quota/redisQuotaStore.ts)"
+    );
+
+    // The lazy dynamic import from redisQuotaStore.ts must still be present —
+    // proves the assertion above isn't vacuously true (e.g. ioredis missing entirely).
+    assert.match(
+      bundled,
+      /await import\(\s*["']ioredis["']\s*\)/,
+      "expected the existing lazy dynamic import of ioredis to remain in the bundle"
+    );
+  } finally {
+    rmSync(outDir, { recursive: true, force: true });
+  }
+});