From 5acac8021d18b92152b853babc16474bd4a5117a Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Thu, 17 Sep 2026 07:09:38 +0700 Subject: [PATCH] fix(redis): namespace warmup circuit-breaker keys with REDIS_KEY_PREFIX (#13328) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The warmup scheduler's circuit-breaker keys were written to Redis without `REDIS_KEY_PREFIX`, so they escaped OmniRoute's namespace and could collide with another app sharing the instance — the one Redis surface the prefix wasn't reaching. Probe: 2/2 pass in `tests/unit/lib/warmupScheduler/redisCircuitBreakerStorePrefix.test.ts`, covering both the prefixed case and the unset/blank case where keys must stay unchanged. **Batch validation** — boarded with the other 10 PRs of your batch into one worktree cut from `release/v3.8.51`; every PR verified as an ancestor of the combined HEAD before validating. - Focused tests across all 11 PRs: **104/104 pass** on the combined tree. - Gates on the combined tree: `check-changelog-integrity` PASS, `check-complexity` PASS, `check-cognitive-complexity` PASS, `typecheck:core` PASS, `check:open-sse-typecheck` PASS. - `check-file-size` is red, but reproduces with byte-identical line counts on the pure `release/v3.8.51` tip (`open-sse/handlers/imageGeneration.ts` 3304, `open-sse/services/combo/roundRobinCombo.ts` 1221, `open-sse/utils/stream.ts` 3115). Inherited base-red, nothing added by this batch — it is also why this PR's "Fast Quality Gates" check was red. **Reconciled** — this PR was `CONFLICTING`. The conflict was in `docs/reference/ENVIRONMENT.md` and purely additive: the release tip had inserted `APP_BIND_HOST` / `QDRANT_BIND_HOST` / `BIFROST_BIND_HOST` rows directly above the `REDIS_KEY_PREFIX` row you edited. Kept both sides — the tip's three new rows and your updated description naming the warmup circuit breaker — then merged the current release branch in (120a92f6) and re-ran your focused test on the reconciled tree: 2/2 pass. No line of your diff was dropped. Thanks, @datrixlab — you also updated `.env.example`, `docs/ops/REDIS_PRODUCTION_CONFIG.md` and `ENVIRONMENT.md` alongside the code, which is why the only thing left to do here was a mechanical conflict resolution. --- .env.example | 5 +- .../fixes/13328-warmup-cb-redis-key-prefix.md | 1 + docs/ops/REDIS_PRODUCTION_CONFIG.md | 16 +++-- docs/reference/ENVIRONMENT.md | 2 +- .../redisCircuitBreakerStore.ts | 3 +- .../redisCircuitBreakerStorePrefix.test.ts | 67 +++++++++++++++++++ 6 files changed, 84 insertions(+), 10 deletions(-) create mode 100644 changelog.d/fixes/13328-warmup-cb-redis-key-prefix.md create mode 100644 tests/unit/lib/warmupScheduler/redisCircuitBreakerStorePrefix.test.ts diff --git a/.env.example b/.env.example index 34517b3f44..28a9f04b8f 100644 --- a/.env.example +++ b/.env.example @@ -112,8 +112,9 @@ DISABLE_SQLITE_AUTO_BACKUP=false # Example: redis://localhost:6379 (or redis://redis:6379 in Docker) # REDIS_URL=redis://localhost:6379 # Namespace prefix for ALL OmniRoute Redis keys (rate limiter + auth cache + -# quota store). Prevents key collisions when OmniRoute shares a Redis instance -# with other apps (e.g. on 127.0.0.1:6379). Default when unset: omniroute: +# quota store + warmup circuit breaker). Prevents key collisions when OmniRoute +# shares a Redis instance with other apps (e.g. on 127.0.0.1:6379). Default when +# unset: omniroute: # REDIS_KEY_PREFIX=omniroute: # Host interface docker-compose publishes the Redis sidecar on. # Default: 127.0.0.1 (loopback only). The compose Redis runs WITHOUT diff --git a/changelog.d/fixes/13328-warmup-cb-redis-key-prefix.md b/changelog.d/fixes/13328-warmup-cb-redis-key-prefix.md new file mode 100644 index 0000000000..97c5c21b84 --- /dev/null +++ b/changelog.d/fixes/13328-warmup-cb-redis-key-prefix.md @@ -0,0 +1 @@ +- **fix(redis):** Warmup circuit-breaker keys now honor `REDIS_KEY_PREFIX` like every other OmniRoute Redis key, instead of always using `omniroute:warmup:cb:` ([#13328](https://github.com/diegosouzapw/OmniRoute/pull/13328)) diff --git a/docs/ops/REDIS_PRODUCTION_CONFIG.md b/docs/ops/REDIS_PRODUCTION_CONFIG.md index ada9ad4456..1fdba9d37a 100644 --- a/docs/ops/REDIS_PRODUCTION_CONFIG.md +++ b/docs/ops/REDIS_PRODUCTION_CONFIG.md @@ -9,7 +9,7 @@ lastUpdated: 2026-08-06 ## Overview Redis is an **optional, soft dependency** in OmniRoute — the application degrades gracefully (in-memory -fallbacks) when Redis is unavailable. In production, tuning Redis reduces latency for three distinct +fallbacks) when Redis is unavailable. In production, tuning Redis reduces latency for four distinct workloads: | Workload | Driver | Client Factory | Key Pattern | @@ -17,8 +17,9 @@ workloads: | Rate limiting | `rateLimiter.ts` | `getRedisClient()` — lazy `ioredis` singleton | `rl:*` Lua‑atomic rate limit windows | | Auth cache | `apiKeys.ts` | Reuses `rateLimiter`'s client | `auth:api_key:` with TTL | | Quota store | `redisQuotaStore.ts` | Separate `getRedisClient(url)` singleton | `quota:*` configurable per-instance | +| Warmup circuit breaker | `redisCircuitBreakerStore.ts` | Separate client in `circuitBreakerFactory.ts` | `warmup:cb:` | -All three workloads share one namespace prefix so OmniRoute can co-exist with other apps on a +All four workloads share one namespace prefix so OmniRoute can co-exist with other apps on a single Redis instance (e.g. `127.0.0.1:6379`). See [Key Namespacing](#key-namespacing). --- @@ -28,7 +29,7 @@ single Redis instance (e.g. `127.0.0.1:6379`). See [Key Namespacing](#key-namesp | Setting | Value | Where | |---|---|---| | `REDIS_URL` env var | `redis://redis:6379` (compose), optional | `rateLimiter.ts:5`, `.env.example` | -| `REDIS_KEY_PREFIX` env var | `omniroute:` (default) | `rateLimiter.ts`, `redisQuotaStore.ts`, `.env.example` | +| `REDIS_KEY_PREFIX` env var | `omniroute:` (default) | `rateLimiter.ts`, `redisQuotaStore.ts`, `redisCircuitBreakerStore.ts`, `.env.example` | | `QUOTA_STORE_REDIS_URL` env var | separate, can differ from `REDIS_URL` | `quota/storeFactory.ts` | | `QUOTA_STORE_DRIVER` | `"sqlite"` (default), `"redis"` optional | `quota/storeFactory.ts` | | ioredis `maxRetriesPerRequest` | `3` | `rateLimiter.ts` client creation | @@ -49,15 +50,18 @@ using the same Redis (this instance runs Redis on `127.0.0.1:6379` alongside oth Set `REDIS_KEY_PREFIX` to a non-empty string to prefix **every** OmniRoute key: ```bash -# .env — all OmniRoute keys become omniroute:rl:*, omniroute:auth:*, omniroute:quota:* +# .env — all OmniRoute keys become omniroute:rl:*, omniroute:auth:*, omniroute:quota:*, omniroute:warmup:cb:* REDIS_KEY_PREFIX=omniroute: ``` - **Default:** `omniroute:` (applied when `REDIS_KEY_PREFIX` is unset or blank). - **Applied to:** rate limiter + auth cache (shared `ioredis` client via `keyPrefix`) and the - quota store (`KEY_PREFIX = "${REDIS_KEY_PREFIX}quota"`). + quota store (`KEY_PREFIX = "${REDIS_KEY_PREFIX}quota"`) and the warmup circuit breaker + (`KEY_PREFIX = "${REDIS_KEY_PREFIX}warmup:cb:"`). - **Changing the prefix** when keys already exist in Redis orphans the old keys (they expire - via TTL / LRU). Safe to change; no migration needed. + via TTL / LRU). Safe to change; no migration needed. The one exception is a warmup + circuit-breaker key for a connection marked forbidden: it is persisted without a TTL, so + list leftovers with `redis-cli --scan --pattern 'warmup:cb:*'` and delete them. - **ioredis `keyPrefix`** automatically prepends the prefix on writes **and** strips it on reads, so application code never sees the prefix. diff --git a/docs/reference/ENVIRONMENT.md b/docs/reference/ENVIRONMENT.md index 0962d7f2f1..0cd8db6e45 100644 --- a/docs/reference/ENVIRONMENT.md +++ b/docs/reference/ENVIRONMENT.md @@ -1407,7 +1407,7 @@ Provider quota endpoints, network tunnels (Tailscale, Ngrok, MITM debug proxy), | `APP_BIND_HOST` | `127.0.0.1` | `docker-compose.yml`, `docker-compose.prod.yml` | Host interface docker-compose publishes the app's own dashboard/API/live-WS ports on (#12568). With `REQUIRE_API_KEY=false` shipping as the `.env.example` default, `0.0.0.0` exposes the anonymous `/v1` LLM proxy to the whole LAN/WAN — only widen once `REQUIRE_API_KEY=true` or a reverse proxy in front enforces its own auth. | | `QDRANT_BIND_HOST` | `127.0.0.1` | `docker-compose.yml` | Host interface docker-compose publishes the Qdrant memory sidecar on (#12578). Same LAN-exposure reasoning as `REDIS_BIND_HOST`. | | `BIFROST_BIND_HOST` | `127.0.0.1` | `docker-compose.yml` | Host interface docker-compose publishes the Bifrost router sidecar on (#12578). Same LAN-exposure reasoning as `REDIS_BIND_HOST`. | -| `REDIS_KEY_PREFIX` | `omniroute:` | `src/shared/utils/rateLimiter.ts` | Namespace prefix applied to every OmniRoute Redis key (rate limiter, auth cache, quota store). Prevents key collisions when the Redis instance is shared with other apps (#11042). | +| `REDIS_KEY_PREFIX` | `omniroute:` | `src/shared/utils/rateLimiter.ts` | Namespace prefix applied to every OmniRoute Redis key (rate limiter, auth cache, quota store, warmup circuit breaker). Prevents key collisions when the Redis instance is shared with other apps (#11042). | | `OMNIROUTE_INTERNAL_SERVICE_TOKEN` | _(unset — mechanism disabled)_ | `src/lib/api/internalServiceAuth.ts` | Shared secret for identity-preserving internal REST hops (#9260): OmniRoute components calling other local OmniRoute routes send it as `x-omniroute-internal-service-token` so the original caller identity is preserved. Compared with `timingSafeEqual`. | | `OMNIROUTE_INTERNAL_SERVICE_TOKEN_FILE` | _(unset)_ | `src/lib/api/internalServiceAuth.ts` | Secret-file variant of the internal service token: path to a file whose trimmed content is the token. Only consulted when the inline var is unset. | | `OPENROUTER_PROVIDER_STATS_ENABLED` | `true` | `src/lib/catalog/openrouterProviderStats.ts` | Enrich the dashboard providers list with OpenRouter weekly ranking stats (#9324). On by default; set `false` to skip the background fetch entirely (non-blocking, never fatal). | diff --git a/src/lib/warmupScheduler/redisCircuitBreakerStore.ts b/src/lib/warmupScheduler/redisCircuitBreakerStore.ts index 37c8452b6a..00d9ce0c2a 100644 --- a/src/lib/warmupScheduler/redisCircuitBreakerStore.ts +++ b/src/lib/warmupScheduler/redisCircuitBreakerStore.ts @@ -18,7 +18,8 @@ type RedisLike = { persist: (key: string) => Promise; }; -const KEY_PREFIX = "omniroute:warmup:cb:"; +// Same namespace as the rate limiter and quota store (REDIS_KEY_PREFIX, #11042). +const KEY_PREFIX = `${process.env.REDIS_KEY_PREFIX?.trim() || "omniroute:"}warmup:cb:`; export class RedisCircuitBreakerStore implements CircuitBreakerStore { constructor(private redis: RedisLike) {} diff --git a/tests/unit/lib/warmupScheduler/redisCircuitBreakerStorePrefix.test.ts b/tests/unit/lib/warmupScheduler/redisCircuitBreakerStorePrefix.test.ts new file mode 100644 index 0000000000..56c69f168f --- /dev/null +++ b/tests/unit/lib/warmupScheduler/redisCircuitBreakerStorePrefix.test.ts @@ -0,0 +1,67 @@ +/** + * REDIS_KEY_PREFIX is documented as the namespace for ALL OmniRoute Redis keys, and the rate + * limiter, auth cache and quota store honor it. The warmup circuit breaker hardcoded + * `omniroute:warmup:cb:`, so on a shared Redis with a custom prefix its keys still landed in + * the default namespace (outside a `~tenantA:*` key ACL, and colliding between instances). + * Raised as MANDATORY in the #11042 review. + * + * KEY_PREFIX is read at module load, so each case imports a fresh copy of the store. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +function makeKeyRecordingRedis() { + const hashes = new Map>(); + return { + keys: () => [...hashes.keys()], + redis: { + async hgetall(key: string) { + return hashes.get(key) ?? {}; + }, + async hset(key: string, fields: Record) { + hashes.set(key, { ...(hashes.get(key) ?? {}), ...fields }); + return 1; + }, + async hget(key: string, field: string) { + return hashes.get(key)?.[field] ?? null; + }, + async expire() { + return 1; + }, + async persist() { + return 1; + }, + }, + }; +} + +let importCount = 0; + +async function keysWrittenWith(prefix: string | undefined) { + const prev = process.env.REDIS_KEY_PREFIX; + if (prefix === undefined) delete process.env.REDIS_KEY_PREFIX; + else process.env.REDIS_KEY_PREFIX = prefix; + try { + const { RedisCircuitBreakerStore } = await import( + `../../../../src/lib/warmupScheduler/redisCircuitBreakerStore.ts?case=${importCount++}` + ); + const { keys, redis } = makeKeyRecordingRedis(); + const store = new RedisCircuitBreakerStore(redis); + await store.recordResult("conn-1", { success: false, failureKind: "network" }); + assert.equal(await store.isInBackoff("conn-1"), true); + assert.equal((await store.get("conn-1"))?.streak, 1); + return keys(); + } finally { + if (prev === undefined) delete process.env.REDIS_KEY_PREFIX; + else process.env.REDIS_KEY_PREFIX = prev; + } +} + +test("warmup circuit-breaker keys use REDIS_KEY_PREFIX", async () => { + assert.deepEqual(await keysWrittenWith("tenantA:"), ["tenantA:warmup:cb:conn-1"]); +}); + +test("warmup circuit-breaker keys are unchanged when REDIS_KEY_PREFIX is unset or blank", async () => { + assert.deepEqual(await keysWrittenWith(undefined), ["omniroute:warmup:cb:conn-1"]); + assert.deepEqual(await keysWrittenWith(" "), ["omniroute:warmup:cb:conn-1"]); +});