Feature: Implement Redis Caching for Agent Outputs #68
Conversation
WalkthroughImplements a singleton Redis client with set/get/delete cache methods, adds Redis settings to application config, and adds the Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant RC as RedisClient
participant R as Redis Server
App->>RC: import redis_client / call method
RC->>RC: __new__() checks/returns singleton
alt first instantiation
RC->>RC: _initialize_redis_client()
RC->>R: CONNECT (host,port,db,password) / enable decode_responses
R-->>RC: PONG / error
note right of RC `#FFDDAA`: Logs success or warning on failure
end
App->>RC: set_cache(key,value,ttl)
RC->>RC: verify client
RC->>R: SETEX key ttl value
R-->>RC: OK / error
RC->>App: return/log result
App->>RC: get_cache(key)
RC->>RC: verify client
RC->>R: GET key
R-->>RC: value / nil
RC->>App: return value or None
App->>RC: delete_cache(key)
RC->>RC: verify client
RC->>R: DEL key
R-->>RC: integer / error
RC->>App: return/log result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
backend/app/cache/redis_client.py (2)
27-38: Return success status and use logging.Silent failure without a return value makes it impossible for callers to know if the write succeeded. Consider returning a boolean and replacing
print()with proper logging.- def set_cache(self, key: str, value: str, ttl: int = 3600): + def set_cache(self, key: str, value: str, ttl: int = 3600) -> bool: """ Sets a key-value pair in Redis cache with an optional time-to-live (TTL). :param key: The key to store the value under. :param value: The value to store. :param ttl: Time-to-live in seconds. Defaults to 1 hour. + :return: True if successful, False otherwise. """ if self.client: try: self.client.setex(key, ttl, value) - except Exception as e: - print(f"Error setting cache for key {key}: {e}") + return True + except redis.RedisError as e: + logger.error(f"Error setting cache for key {key}: {e}") + return False
40-62: Use specific exception types and proper logging.Catching broad
Exceptioncan mask unrelated bugs. Useredis.RedisErrorfor Redis-specific errors. Replaceprint()with logging throughout. Fordelete_cache, consider returning a boolean to indicate success.def get_cache(self, key: str): if self.client: try: return self.client.get(key) - except Exception as e: - print(f"Error getting cache for key {key}: {e}") + except redis.RedisError as e: + logger.error(f"Error getting cache for key {key}: {e}") return None - def delete_cache(self, key: str): + def delete_cache(self, key: str) -> bool: if self.client: try: self.client.delete(key) - except Exception as e: - print(f"Error deleting cache for key {key}: {e}") + return True + except redis.RedisError as e: + logger.error(f"Error deleting cache for key {key}: {e}") + return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/cache/redis_client.py(1 hunks)backend/app/core/config.py(1 hunks)requirements.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/cache/redis_client.py (1)
backend/tests/test_routes.py (1)
client(16-18)
🪛 Ruff (0.14.5)
backend/app/cache/redis_client.py
23-23: Do not catch blind exception: Exception
(BLE001)
37-37: Do not catch blind exception: Exception
(BLE001)
49-49: Do not catch blind exception: Exception
(BLE001)
61-61: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
backend/app/core/config.py (1)
15-18: LGTM!Redis configuration follows standard defaults and integrates well with the existing pydantic-settings pattern. The optional password field is correctly typed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/app/cache/redis_client.py (3)
8-18: Singleton & lifetime behaviour are solid but only attempt connection onceThe double‑checked locking singleton looks correct and thread‑safe, and the module‑level
redis_client = RedisClient()gives a simple import surface.Be aware that if Redis is unavailable at process start,
_initialize_redis_clientreturnsNoneand the sameNoneclient is reused for the lifetime of that process; there is no built‑in reconnection on later cache operations. If you expect Redis to come and go (e.g., transient outages, delayed start in dev), consider either:
- adding a lightweight re‑init path (e.g., if
self.client is Noneinset_cache/get_cache, try_initialize_redis_client()once), or- exposing an explicit
reconnect()/refresh()helper you can call during app startup health checks.Also applies to: 72-72
20-33: Narrow exception type in init and align with Ruff TRY300/BLE001Catching bare
Exceptionhere will also swallow programmer/configuration bugs (TypeError, ValueError, etc.), and Ruff flags this (BLE001); at the same time, TRY300 complains about thereturninside thetry. You can address both by:
- catching
redis.RedisError(or a smaller subset likeredis.ConnectionError,redis.TimeoutError) for graceful degradation, and- using a
try/except/elsewith thereturnin theelseblock.
redis.RedisErroris the documented base class for all redis‑py exceptions. (redis.io)
Also, newer docs favorredis.Redis(...)overredis.StrictRedis(...)(the latter is just an alias kept for backwards compatibility). (pypi.org)Example refactor:
- def _initialize_redis_client(self): - try: - client = redis.StrictRedis( + def _initialize_redis_client(self): + try: + client = redis.Redis( host=settings.REDIS_HOST, port=settings.REDIS_PORT, db=settings.REDIS_DB, password=settings.REDIS_PASSWORD, decode_responses=True ) - client.ping() - return client - except Exception as e: - logger.warning(f"Redis unavailable, caching disabled: {e}") - return None + client.ping() + except redis.RedisError: + logger.warning("Redis unavailable, caching disabled", exc_info=True) + return None + else: + return clientThis preserves your “disable cache if Redis is down” behavior, while avoiding swallowing unrelated exceptions and satisfying Ruff.
35-70: Cache ops: avoid bareException, catch Redis‑specific errors insteadThe “best effort” behavior (no raise, just log and continue) makes sense for a cache layer, but the broad
except Exceptioninset_cache,get_cache, anddelete_cacheboth hides programmer errors and trips Ruff BLE001.Consider narrowing the caught type to Redis‑specific errors and, if desired, letting unexpected errors bubble:
def set_cache(self, key: str, value: str, ttl: int = 3600): @@ - if self.client: - try: - self.client.setex(key, ttl, value) - except Exception as e: - logger.warning(f"Error setting cache for key {key}: {e}") + if not self.client: + return + try: + self.client.setex(key, ttl, value) + except redis.RedisError: + logger.warning("Error setting cache for key %s", key, exc_info=True) @@ - if self.client: - try: - return self.client.get(key) - except Exception as e: - logger.warning(f"Error getting cache for key {key}: {e}") - return None + if not self.client: + return None + try: + return self.client.get(key) + except redis.RedisError: + logger.warning("Error getting cache for key %s", key, exc_info=True) + return None @@ - if self.client: - try: - self.client.delete(key) - except Exception as e: - logger.warning(f"Error deleting cache for key {key}: {e}") + if not self.client: + return + try: + self.client.delete(key) + except redis.RedisError: + logger.warning("Error deleting cache for key %s", key, exc_info=True)This keeps the graceful fallback but makes it easier to notice non‑Redis bugs and aligns with Ruff’s guidance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/cache/redis_client.py(1 hunks)requirements.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🪛 Ruff (0.14.5)
backend/app/cache/redis_client.py
30-30: Consider moving this statement to an else block
(TRY300)
31-31: Do not catch blind exception: Exception
(BLE001)
45-45: Do not catch blind exception: Exception
(BLE001)
57-57: Do not catch blind exception: Exception
(BLE001)
69-69: Do not catch blind exception: Exception
(BLE001)
Overview: This PR introduces Redis for temporary caching of agent outputs and intermediate results.
Changes
app/cache/redis_client.pyto encapsulate Redis client functionality.set_cache(key, value, ttl)for easy interaction with the cache.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.