fix: re-read settings when CACHEKIT_MASTER_KEY appears + correct missing-key env var name#200
fix: re-read settings when CACHEKIT_MASTER_KEY appears + correct missing-key env var name#20027Bslash6 wants to merge 2 commits into
Conversation
…ing-key env var name #195: get_settings() cached the config once; if first built before CACHEKIT_MASTER_KEY was in the environment (e.g. an import-time cache decorator before secrets load), it froze master_key=None forever — encryption then silently never activated. get_settings() now re-reads once the key appears (under the existing lock; idempotent, no churn after). #194: the missing-key error and a docstring named REDIS_CACHE_MASTER_KEY, which is never read; corrected both to CACHEKIT_MASTER_KEY (the only honored var), matching config/nested.py. Closes #194. Closes #195.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCorrects ChangesMaster key env var fix and singleton self-heal
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cachekit/config/singleton.py`:
- Around line 49-60: The code has a race condition where another thread calling
reset_settings() between the unlocked check on line 55 and the locked check on
line 59 could set _settings_instance to None, causing an AttributeError when
trying to access _settings_instance.master_key. Capture _settings_instance into
a local variable before the initial unlocked check, then use that cached
reference throughout the condition to prevent the instance from becoming None
between the two checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd606741-8805-4d1f-81ab-56cd841ff3cf
📒 Files selected for processing (4)
src/cachekit/cache_handler.pysrc/cachekit/config/singleton.pysrc/cachekit/serializers/encryption_wrapper.pytests/unit/test_encryption_config_resolution.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ncurrent-reset AttributeError) Addresses CodeRabbit on #200: the #195 self-heal dereferenced _settings_instance.master_key in the unlocked fast path after a non-None check. A concurrent reset_settings() between the two could null the global and AttributeError (or return None). Snapshot the global to a local for the unlocked checks; re-read it under the lock (rebuilding if a peer reset or it is still keyless).
Summary
Two encryption-config correctness fixes.
Closes #195. Closes #194.
#195 — keyless settings singleton froze permanently
get_settings()builtCachekitConfigonce and cached it process-wide. If the first build happened beforeCACHEKIT_MASTER_KEYwas in the environment — e.g. an import-time@cache/@cache.iodecorator evaluated before the app loads its secrets (python-dotenv, cloud secret managers) — it frozemaster_key=Noneforever. Setting the env var afterward had no effect: with auto-encryption that means data is cached plaintext though a key is present, and with explicit@cache.secureit surfaces as #194's confusing "master key required".Fix:
get_settings()now re-reads once the key appears (master_key is NoneandCACHEKIT_MASTER_KEYnow set → rebuild, under the existing lock). Idempotent — after the rebuildmaster_keyis set, so it never fires again (no per-call churn).#194 — missing-key error named a nonexistent env var
The error and a
master_keydocstring told users to setREDIS_CACHE_MASTER_KEY, which is never read; the only honored variable isCACHEKIT_MASTER_KEY. Corrected both strings (encryption_wrapper.py+cache_handler.py), matching the already-correct sibling message inconfig/nested.py.Scope / known residual
The self-heal fixes the singleton and any handler built after the key loads. A
CacheSerializationHandleris built eagerly when@cacheis applied (decoration/import) and freezes its encryption decision at__init__; a handler frozen keyless at import is not retroactively unfrozen by this change. Fully closing that (auto-encryption + import-time decorator + secrets-after-import) needs lazy encryption resolution in the handler — invasive, security-critical, deferred to its own PR. Note@cache.secure(explicit) already fails loud in that window, so only the auto-encryption path is silent.Tests
New
tests/unit/test_encryption_config_resolution.py: get_settings() re-reads when the key appears (the #195 repro), stays stable once loaded, and the missing-key error namesCACHEKIT_MASTER_KEY. Full suite: 1866 passed, basedpyright 0 errors.Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation & Error Handling
CACHEKIT_MASTER_KEYwhen the master key is missing.