fix: base64 body encoding and client close in cache layer - #41
Conversation
- M1: Use base64 encoding for response body in cache entries instead of lossy decode(errors='replace'), with backward-compatible read path - L5: Make _PoolState.clear() async and close the AsyncRedis client before resetting state to prevent connection leaks - H4: Widen except clause from (json.JSONDecodeError, KeyError) to Exception in cache-hit path - Add tests for base64 round-trip, pool client close, and update existing _PoolState test to async
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
==========================================
+ Coverage 96.81% 97.25% +0.43%
==========================================
Files 12 12
Lines 1257 1383 +126
==========================================
+ Hits 1217 1345 +128
+ Misses 40 38 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Add test_build_hit_response_decodes_base64_body to directly exercise the base64 decode path in _build_hit_response (line 313) - Add test_build_hit_response_304_not_modified to cover the 304 path - Resolves codecov missing coverage on PR redis#41
…che miss. Main's replay path hardcoded media_type="application/json"; that's gone, so text/csv, XML, HTML and your own headers survive. • An explicit cacheability allowlist, with visible refusals. Text representations only — media types, +json/+xml suffixes, UTF-8/ASCII charsets. Anything else is served whole but not stored, marked X-Redis-Cache: BYPASS (absent on main) and logged once per route. _storage_refusal holds the rules in one place. • Header handling follows RFC 9111/9110 explicitly. A denylist for what an entry must not keep — library-owned, hop-by-hop, proxy, framing, and Date/Set-Cookie by policy — plus a restricted 304 set and an 8 KiB header-block cap. About 11 new module constants and 12 new helpers in cache.py (+588 lines). • Vary is stored and replayed, and Vary: * is refused (0 mentions on main, 8 now). Lookup still never reads request headers, so per-variant routes need a custom key_builder. • Docs and tests extended
|
Hey @emiliano-go , had to revert most of the change, but I will use it as a basis of a new fix. Close client on pool state clearThis is a false-positive - verified it manually and there is no leak. Measured server-side with Redis.init lines 103–105 carry the comment "auto_close_connection_pool only has an effect if connection_pool is None… if connection_pool is not None, the user owns it - and line 196 hard-sets self.auto_close_connection_pool = False in the injected-pool branch, ignoring the parameter. Binary-safe body encodingThere is a genuine problem here in how headers are being handled; and how binary is being handled. At some point during development I seem to recall I had some check for that, but it seems I lost it and now the code behaves wrong. However IMHO this SDK should not handle binary payloads at all. Check my latest change, especially the guide section I added, for argumentation why. We can reconsider later. |
|
Thanks Tihomir; both accepted. Client-close dropped, and I agree on keeping the SDK to text payloads only (the existing allowlist and UTF-8 refusal already cover the lossy-decode case). I'll get |
3 participants
Summary
Fixes two cache layer issues: lossy binary body encoding and connection leak on shutdown.
Changes
Binary-safe body encoding
_write_cache_entrystored response bodies usingdecode(errors="replace"). This silently corrupts binary responses (images, protobuf, msgpack). The replacement character makes round-trip lossy.Now uses base64 encoding with an "encoding" field in the cache entry. The read path checks for this field. Entries without it use the legacy str/encode path for backward compatibility.
Close client on pool state clear
_PoolState.clear()set_async_client = Nonewithout callingaclose(). This leaks the connection pool on shutdown.Now
clear()is async and callsaclose()before resetting state. The lifespan handler awaits it.Files changed
src/redis_fastapi/cache.py: base64 encode body in write path, decode in read path based on encoding fieldsrc/redis_fastapi/deps.py: make_PoolState.clear()async, callaclose()src/redis_fastapi/lifespan.py: awaitps.clear()tests/unit/test_adversarial.py: 3 new tests, update existing_PoolStatetest to asyncTest results
nox passes across Python 3.10-3.14. 407 tests, 93.42% coverage.
Related