Uh oh!
There was an error while loading. Please reload this page.
feat(cache): Redis backend (single-node + integration tests) - #22
Conversation
Per spec §3.5 / plan §4.14. New aisix-cache::RedisCache that implements the existing Cache trait against a single-node Redis instance via redis::aio::ConnectionManager (transparent reconnect, no per-request handshake). - JSON-serialise ChatResponse for storage, namespaced under configurable prefix (default 'aisix:cache:<fingerprint>'). - TTL applied per-entry via SET EX, defaults to 5m to match MemoryCache. - All errors map to CacheError::Backend; the proxy treats backend failures as cache misses and proceeds to the upstream. Bootstrap (aisix-server) now switches on cfg.cache.backend: - memory → MemoryCache (default) - redis → RedisCache::connect(cfg.cache.redis.url) - qdrant → falls back to MemoryCache with a WARN until that PR lands Tests: - 4 hermetic unit tests (TTL flooring, prefix join, invalid-URL error) - 3 integration tests in tests/redis_integration.rs that round-trip against a real Redis instance. Skipped silently when AISIX_REDIS_URL is unset; CI sets it to redis://127.0.0.1:6379 against a redis:7 service container. CI: - rust-unit job now spins a redis:7-alpine service and exports AISIX_REDIS_URL. cargo llvm-cov runs with --all-features so the redis backend code paths land in the coverage gate. Cluster + Sentinel modes drop in under the same ConnectionManager pattern in a follow-up — the redis crate already has them on its workspace feature list.
There was a problem hiding this comment.
Pull request overview
Adds a Redis-backed implementation of the aisix-cache::Cache trait and wires the server/CI to support running Redis-backed caching (including live integration tests in CI).
Changes:
- Implement
RedisCache(feature-gated) with JSONChatResponsestorage and per-entry TTL viaSET EX. - Add Redis backend selection in
aisix-serverbootstrap config wiring. - Add live Redis integration tests and update CI to run them with a Redis service +
--all-featurescoverage run.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
crates/aisix-server/src/main.rs | Selects cache backend (memory/redis/qdrant fallback) based on config and connects to Redis when configured. |
crates/aisix-server/Cargo.toml | Enables aisix-cache’s redis feature for the server binary. |
crates/aisix-cache/src/redis.rs | New RedisCache backend implementation + unit tests. |
crates/aisix-cache/src/lib.rs | Feature-gated module/export wiring for RedisCache. |
.github/workflows/ci.yml | Adds Redis service + env var for integration tests and runs coverage with --all-features. |
crates/aisix-cache/tests/redis_integration.rs | New opt-in (env var) integration tests against a live Redis instance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tracing::info!(target: "aisix::cache", backend = "redis", "connecting cache backend"); | ||
| let redis = RedisCache::connect(&url) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("redis cache connect failed (url={url}): {e}"))?; |
There was a problem hiding this comment.
The error context includes the full Redis URL (url={url}), which may contain credentials (e.g. redis://:password@host:6379). Consider redacting userinfo before formatting/logging (or omit the URL entirely) to avoid leaking secrets into logs and error reports.
| .map_err(|e| anyhow::anyhow!("redis cache connect failed (url={url}): {e}"))?; | |
| .map_err(|e| anyhow::anyhow!("redis cache connect failed: {e}"))?; |
| /// Override the per-entry TTL. Caps at `u64::MAX / 1000` to stay | ||
| /// inside Redis's `EX` second range. | ||
| pub fn with_ttl(mut self, ttl: Duration) -> Self { | ||
| self.ttl_secs = ttl.as_secs().max(1); |
There was a problem hiding this comment.
with_ttl's doc comment says the TTL is capped to stay within Redis EX range, but the implementation only floors to 1s (ttl.as_secs().max(1)) and does not cap. Either implement the described cap (and document the actual limit) or remove/adjust the comment so it matches behavior.
| #[test] | ||
| fn full_key_concatenates_prefix() { | ||
| // Construct a RedisCache without actually opening a connection by | ||
| // using a dummy ConnectionManager via mem::forget? No — we'd leak. | ||
| // Easier: test the prefix logic via a free function. | ||
| assert_eq!(prefix_join("aisix:cache", "ab12"), "aisix:cache:ab12"); | ||
| assert_eq!(prefix_join("", "x"), ":x"); | ||
| } | ||
| fn prefix_join(prefix: &str, key: &str) -> String { | ||
| format!("{prefix}:{key}") | ||
| } |
There was a problem hiding this comment.
The full_key_concatenates_prefix test doesn't exercise RedisCache::full_key; it reimplements the formatting in a local prefix_join, so a future change to full_key could break behavior without failing this test. Consider extracting the key-join logic into a shared helper used by both full_key and the tests (or expose a small pure function) so the unit test actually covers production code.
| #[cfg(feature = "redis")] | ||
| mod redis; | ||
| pub use cache::{Cache, CacheError, CacheOutcome}; | ||
| pub use key::CacheKey; | ||
| pub use memory::{MemoryCache, DEFAULT_CAPACITY, DEFAULT_TTL}; | ||
| #[cfg(feature = "redis")] | ||
| pub use redis::{ | ||
| RedisCache, DEFAULT_PREFIX as REDIS_DEFAULT_PREFIX, DEFAULT_TTL as REDIS_DEFAULT_TTL, | ||
| }; |
There was a problem hiding this comment.
The crate-level docs still say the Redis backend "lands in a follow-up PR" even though this PR adds RedisCache and exports it behind the redis feature. Please update the module/docs to reflect the current set of backends so users aren't misled about Redis availability.
| //! End-to-end Redis tests against a live Redis instance. | ||
| //! | ||
| //! Runs only when `AISIX_REDIS_URL` is set (e.g. on CI which spins | ||
| //! `redis:7-alpine` as a service). The unit test module in | ||
| //! `src/redis.rs` handles hermetic checks; this file proves the | ||
| //! request → upstream → cache round-trip actually round-trips. |
There was a problem hiding this comment.
The module docs claim these tests prove a "request → upstream → cache" round-trip, but the tests interact with RedisCache directly (no proxy request / upstream call involved). Suggest rewording the comment to describe what is actually validated here (RedisCache put/get/TTL behavior against a live Redis), or add a higher-level integration test if end-to-end proxy behavior is intended.
| //! End-to-end Redis tests against a live Redis instance. | |
| //! | |
| //! Runs only when `AISIX_REDIS_URL` is set (e.g. on CI which spins | |
| //! `redis:7-alpine` as a service). The unit test module in | |
| //! `src/redis.rs` handles hermetic checks; this file proves the | |
| //! request → upstream → cache round-trip actually round-trips. | |
| //! Redis integration tests against a live Redis instance. | |
| //! | |
| //! Runs only when `AISIX_REDIS_URL` is set (e.g. on CI which spins | |
| //! `redis:7-alpine` as a service). The unit test module in | |
| //! `src/redis.rs` handles hermetic checks; this file validates | |
| //! `RedisCache` put/get/miss/TTL behavior against a real Redis server. |
| services: | ||
| redis: | ||
| image: redis:7-alpine | ||
| ports: ["6379:6379"] | ||
| env: | ||
| # Picked up by crates/aisix-cache/tests/redis_integration.rs. | ||
| # The tests no-op when this is unset (local dev), so absence is safe. | ||
| AISIX_REDIS_URL: redis://127.0.0.1:6379 |
There was a problem hiding this comment.
The Redis service is started without a health check or explicit wait. GitHub Actions will start the container, but tests can still race Redis readiness and become flaky. Consider adding options with a --health-cmd (e.g. redis-cli ping) + retries, or add a small step before tests that polls redis-cli ping until it succeeds.
Uh oh!
There was an error while loading. Please reload this page.
The /v1/messages path dispatched to the upstream without any guardrail check and without the budget pre-check, so prompts bypassed content/DLP policy (rate limits were already enforced). Translate the Anthropic body into the internal ChatFormat and run the resolved input guardrail chain before dispatch (a Block short-circuits), and run the cp-api budget pre-check — mirroring /v1/chat/completions. Applies to both the Anthropic passthrough and cross-provider dispatch paths. Output guardrails on the verbatim Anthropic byte-passthrough stream are not yet enforced (the passthrough forwards raw bytes without building a ChatResponse); that remains as follow-up. Part of #448 (finding #22, input + budget)
…eaming) Thread the resolved guardrail chain (as Arc) through the /v1/messages dispatch paths and run output guardrails on the response: - Non-streaming: cross-provider checks the bridge ChatResponse; passthrough extracts response text (content blocks + raw content array for tool_use) into a synthetic ChatResponse. - Streaming: both the cross-provider SSE encoder path and the verbatim Anthropic byte-passthrough accumulate assistant text and run the guardrail at end-of-stream. Bytes are forwarded live (matching /v1/chat/completions and LiteLLM's streaming guardrail), so a block is signalled with a terminal Anthropic `error` (content_filter) event. Completes the output side of #448#22; with this and the earlier input + budget work, /v1/messages no longer bypasses the guardrail/quota pipeline. The remaining findings (#6 count_tokens, #2/#13 reasoning_content, #24 guardrail-vs-rate-limit ordering) are accepted as standard behavior (LiteLLM has the same gap). Fixes#448
Summary
New `aisix-cache::RedisCache` implements the existing `Cache` trait
against a single-node Redis via `redis::aio::ConnectionManager`.
(default prefix `aisix:cache`).
in-memory backend.
miss and proceeds to upstream (no failure leaks to client).
Bootstrap wiring
```yaml
cache:
backend: redis
redis:
url: "redis://127.0.0.1:6379"
```
The server bootstrap now matches on `cfg.cache.backend`:
Tests
error path)
against a real Redis. Tests no-op silently when `AISIX_REDIS_URL`
is unset (so local `cargo test` stays hermetic). CI now sets it to
`redis://127.0.0.1:6379` against a `redis:7-alpine` service container
in the `rust-unit` job.
CI changes
in the coverage gate.
Test plan
from baseline)
Follow-ups
crate already supports both via the same ConnectionManager pattern).
first).
🤖 Generated with Claude Code