Skip to content

fix(redis): namespace warmup circuit-breaker keys with REDIS_KEY_PREFIX - #13328

Merged
diegosouzapw merged 3 commits into
diegosouzapw:release/v3.8.51from
datrixlab:fix/warmup-cb-redis-key-prefix
Sep 17, 2026
Merged

diegosouzapw merged 3 commits into
diegosouzapw:release/v3.8.51from
datrixlab:fix/warmup-cb-redis-key-prefix

Conversation

@datrixlab

Copy link
Copy Markdown
Contributor

Summary

  • REDIS_KEY_PREFIX is documented as the namespace for all OmniRoute Redis keys (.env.example, docs/ops/REDIS_PRODUCTION_CONFIG.md). The rate limiter, auth cache and quota store honor it. The warmup circuit breaker does not: redisCircuitBreakerStore.ts hardcodes const KEY_PREFIX = "omniroute:warmup:cb:", and its client in circuitBreakerFactory.ts is created without a keyPrefix.
  • With a custom prefix on a shared Redis, those keys still land in the default namespace:
    • outside a key-pattern ACL such as ~tenantA:*;
    • shared between two instances that set different prefixes precisely to stay apart, so one instance's forbidden/backoff state for a connection id is read by the other.
  • The feat(redis): add configurable key namespace prefix #11042 review flagged this as MANDATORY: "There is a third ioredis client at src/lib/warmupScheduler/circuitBreakerFactory.ts:92 … whose store keys … are hardcoded to omniroute:warmup:cb: … Either derive that prefix from REDIS_KEY_PREFIX … with a matching test … or narrow the doc claim." The PR merged without either.
  • Fix: derive the prefix the way redisQuotaStore.ts does (${process.env.REDIS_KEY_PREFIX?.trim() || "omniroute:"}warmup:cb:). I kept it in the store constant rather than adding an ioredis keyPrefix to the client, so the key is built in one place and cannot be prefixed twice. With REDIS_KEY_PREFIX unset or blank, the key is byte-identical to before.
  • docs/ops/REDIS_PRODUCTION_CONFIG.md, docs/reference/ENVIRONMENT.md and .env.example now list the warmup circuit breaker as the fourth workload. The ops doc's "old keys expire via TTL / LRU" note gains one exception: a forbidden connection's key is PERSISTed, so it will not age out.

Related Issues

Validation

  • Change type: other (Redis / warmup scheduler)
  • Focused tests: the new test, the five existing tests/unit/lib/warmupScheduler/* Redis/factory tests, quota-redis-store.test.ts, rate-limiter-redis-optional.test.ts (22 pass, 1 skipped: the real-Redis integration test that needs RUN_QUOTA_REDIS_INT=1)
  • eslint on the changed code; node scripts/check/check-env-doc-sync.mjs passes
  • Based on the current release/v3.8.51
  • Production-code change includes a new automated test

Tests Added Or Updated

  • tests/unit/lib/warmupScheduler/redisCircuitBreakerStorePrefix.test.ts (new). The test runs a real recordResult() → isInBackoff() → get() round trip against a key-recording fake Redis:
    • With REDIS_KEY_PREFIX=tenantA:, the only key written is tenantA:warmup:cb:conn-1.
    • Guard: with the variable unset or blank, the key is still omniroute:warmup:cb:conn-1.

With the source change reverted, the first test fails and the guard passes. The existing prefix tests for the rate limiter and quota store only check source text; this one checks the key actually written.

Coverage Notes

  • src/lib/warmupScheduler/redisCircuitBreakerStore.ts: the prefix is used on every Redis call in the store, and the round trip exercises hset, expire, hget and hgetall.

Reviewer Notes

  • The default namespace is unchanged, so deployments without REDIS_KEY_PREFIX see no change.
  • Deployments that already set a custom prefix will start with empty warmup circuit state in Redis. A connection that is currently marked forbidden gets one more warmup attempt, fails with 403 again, and is re-marked. The old omniroute:warmup:cb:* keys for forbidden connections were PERSISTed, so they do not expire by themselves; the ops doc now says so, next to its existing note that changing the prefix orphans old keys.
  • docs/ops/REDIS_PRODUCTION_CONFIG.md did not pass prettier --check on the base branch either (table alignment); the diff there only touches the lines about the workloads.

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 with a custom
prefix on a shared Redis its keys still landed in the default namespace:
outside a key-pattern ACL such as `~tenantA:*`, and shared between
instances that set different prefixes to keep apart. The diegosouzapw#11042 review
flagged this client as the one the prefix missed.

Derive the prefix the way redisQuotaStore does. With REDIS_KEY_PREFIX
unset or blank the key is byte-identical to before. The ops doc,
ENVIRONMENT.md and .env.example now list the fourth workload, and the ops
doc notes that a forbidden connection's key is persisted without a TTL,
so it does not age out after a prefix change.
@diegosouzapw

Copy link
Copy Markdown
Owner

Good follow-through on the #11042 review's mandatory item that never got
addressed — deriving the warmup circuit-breaker key prefix the same way
redisQuotaStore.ts already does, with the default-case byte-for-byte
compatibility preserved. Ran your new prefix test at this PR's head: 2/2 pass,
including the round-trip against a key-recording fake Redis proving both the
prefixed and unprefixed cases. This looks merge-ready.

Conflict in docs/reference/ENVIRONMENT.md was additive: the release tip
inserted APP_BIND_HOST/QDRANT_BIND_HOST/BIFROST_BIND_HOST rows just above
the REDIS_KEY_PREFIX row this branch edits. Kept both sides.
@diegosouzapw
diegosouzapw merged commit 5acac80 into diegosouzapw:release/v3.8.51 Sep 17, 2026
3 of 7 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…IX (diegosouzapw#13328)

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 (120a92f) 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants