feat: auto-detect backend from environment variables#88
Open
feat: auto-detect backend from environment variables#88
Conversation
📝 WalkthroughWalkthroughDefaultBackendProvider now auto-detects the L2 backend from CACHEKIT_* or REDIS_URL env vars, can return None for L1-only mode, and raises ConfigurationError on ambiguous backend env combinations. Wrapper behavior changed: sync fails fast with a permanent BackendError when no L2, async executes original function (L1-only). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant Provider as DefaultBackendProvider
participant Env as Env Vars
participant Resolver as _resolve_provider()
participant Factory as Backend Factories
Client->>Provider: get_backend()
alt first call (_resolved = False)
Provider->>Resolver: _resolve_provider()
Resolver->>Env: inspect CACHEKIT_API_KEY, CACHEKIT_REDIS_URL, REDIS_URL, CACHEKIT_MEMCACHED_SERVERS, CACHEKIT_FILE_CACHE_DIR
alt CACHEKIT_API_KEY set
Resolver->>Factory: create CachekitIOBackend
Factory-->>Resolver: CachekitIO instance
else if CACHEKIT_REDIS_URL or REDIS_URL set
Resolver->>Factory: create RedisBackend
Factory-->>Resolver: Redis instance
else if CACHEKIT_MEMCACHED_SERVERS set
Resolver->>Factory: create MemcachedBackend
Factory-->>Resolver: Memcached instance
else if CACHEKIT_FILE_CACHE_DIR set
Resolver->>Factory: create FileBackend
Factory-->>Resolver: File instance
else no backend vars
Resolver-->>Provider: None
end
alt multiple conflicting vars
Resolver-->>Provider: raise ConfigurationError
end
Provider->>Provider: cache result, set _resolved = True
else cached (_resolved = True)
Provider->>Provider: return cached backend or None
end
Provider-->>Client: backend instance or None
sequenceDiagram
participant User as User Code
participant Sync as sync_wrapper
participant Async as async_wrapper
participant L1 as L1 Cache
participant L2 as L2 Backend / Provider
User->>Sync: call decorated function
Sync->>L2: resolve backend (get_backend)
alt backend is None (L1-only)
Sync->>Sync: raise BackendError(PERMANENT)
Sync-->>User: error
else backend resolved
Sync->>L1: check L1
alt L1 hit
L1-->>User: return value
else L1 miss
Sync->>L2: query L2
L2-->>Sync: value
Sync->>L1: store
Sync-->>User: return value
end
end
User->>Async: call decorated coroutine
Async->>L2: resolve backend (get_backend)
alt backend is None (L1-only)
Async->>L1: check L1
alt L1 hit
L1-->>User: return value
else L1 miss
Async->>User: execute original coroutine (skip L2)
User-->>Async: result
Async->>L1: store
Async-->>User: return value
end
else backend resolved
Async->>L1: check L1
alt L1 hit
L1-->>User: return value
else L1 miss
Async->>L2: query L2
L2-->>Async: value
Async->>L1: store
Async-->>User: return value
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cachekit/backends/provider.py (1)
149-164: Minor thread-safety consideration for concurrent initialization.The lazy initialization pattern (
if not self._resolved) is not thread-safe. Concurrent threads could race and both enter_resolve_provider(). Since_resolve_provider()is idempotent (reads env vars, creates provider), this is benign—at worst, duplicate providers are created momentarily.If strict single-initialization is required in the future, consider adding a lock:
♻️ Optional: thread-safe initialization
+import threading + class DefaultBackendProvider(BackendProviderInterface): + _init_lock = threading.Lock() + def get_backend(self): - if not self._resolved: - self._provider = self._resolve_provider() - self._resolved = True + if not self._resolved: + with self._init_lock: + if not self._resolved: # double-check + self._provider = self._resolve_provider() + self._resolved = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/backends/provider.py` around lines 149 - 164, The lazy init in get_backend() can race since it checks self._resolved and calls _resolve_provider() without synchronization; to make initialization thread-safe, add a lock (e.g., self._init_lock = threading.Lock()) and wrap the resolve sequence in get_backend() with the lock so only one thread runs self._resolve_provider() and sets self._provider/self._resolved, leaving other threads to read the already-set self._provider; update any constructor (or class init) to create the lock and ensure get_backend() uses it around the if not self._resolved / self._resolve_provider() block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cachekit/backends/provider.py`:
- Around line 149-164: The lazy init in get_backend() can race since it checks
self._resolved and calls _resolve_provider() without synchronization; to make
initialization thread-safe, add a lock (e.g., self._init_lock =
threading.Lock()) and wrap the resolve sequence in get_backend() with the lock
so only one thread runs self._resolve_provider() and sets
self._provider/self._resolved, leaving other threads to read the already-set
self._provider; update any constructor (or class init) to create the lock and
ensure get_backend() uses it around the if not self._resolved /
self._resolve_provider() block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7641c536-c1e2-402b-a7a5-6a7313b97b12
📒 Files selected for processing (4)
.secrets.baselinesrc/cachekit/backends/provider.pysrc/cachekit/decorators/wrapper.pytests/unit/backends/test_provider.py
27Bslash6
force-pushed
the
feat/auto-detect-backend-from-env
branch
from
b603be2 to
5dd48df
Compare
DefaultBackendProvider now resolves the cache backend from CACHEKIT_-prefixed environment variables instead of always defaulting to Redis. Raises ConfigurationError when multiple CACHEKIT_ backend vars are set concurrently. Priority: CACHEKIT_API_KEY > CACHEKIT_REDIS_URL > CACHEKIT_MEMCACHED_SERVERS > CACHEKIT_FILE_CACHE_DIR > REDIS_URL (12-factor fallback) > None (L1-only). Non-prefixed REDIS_URL never conflicts with CACHEKIT_ vars.
27Bslash6
force-pushed
the
feat/auto-detect-backend-from-env
branch
from
5dd48df to
31fc1c1
Compare
validate_encryption_config() checks the env var independently of the inline master_key param passed to @cache.secure. The test was flaky (68% fail rate on main) because it depended on whether CACHEKIT_MASTER_KEY happened to be set from a prior test's singleton cache.
The model-checking/kani-github-action composite action internally uses unpinned dtolnay/rust-toolchain@stable and actions/checkout@v3, which violates the org's SHA-pinning policy and blocks Security Deep runs. Replace all dtolnay/rust-toolchain uses with plain rustup commands (already the pattern in fuzzing/miri/sanitizer jobs) and replace the Kani composite action with cargo install kani-verifier + cargo kani.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/test_l1_only_mode.py (1)
236-253: Consider using pytest'smonkeypatchfixture for env var management.The manual
os.environsave/restore via try/finally is correct, but pytest'smonkeypatchfixture handles this more concisely and is consistent with idiomatic pytest usage (auto-restores on teardown, including on exceptions raised outside thetry).♻️ Proposed refactor
- def test_intent_presets_with_backend_none(self): + def test_intent_presets_with_backend_none(self, monkeypatch): @@ # Test `@cache.secure`(master_key="...", backend=None) # validate_encryption_config() checks CACHEKIT_MASTER_KEY env var # independently of the inline master_key param, so we must set it. secure_call_count = 0 - old_key = os.environ.get("CACHEKIT_MASTER_KEY") - os.environ["CACHEKIT_MASTER_KEY"] = "a" * 64 - try: - - `@cache.secure`(master_key="a" * 64, backend=None) - def secure_func() -> str: - nonlocal secure_call_count - secure_call_count += 1 - return "secure" - - assert secure_func() == "secure" - assert secure_func() == "secure" - assert secure_call_count == 1, f"@cache.secure L1 miss - called {secure_call_count} times" - finally: - if old_key is None: - os.environ.pop("CACHEKIT_MASTER_KEY", None) - else: - os.environ["CACHEKIT_MASTER_KEY"] = old_key + monkeypatch.setenv("CACHEKIT_MASTER_KEY", "a" * 64) + + `@cache.secure`(master_key="a" * 64, backend=None) + def secure_func() -> str: + nonlocal secure_call_count + secure_call_count += 1 + return "secure" + + assert secure_func() == "secure" + assert secure_func() == "secure" + assert secure_call_count == 1, f"@cache.secure L1 miss - called {secure_call_count} times"With this change, the
import osadded at line 17 can also be dropped if it's not used elsewhere in the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_l1_only_mode.py` around lines 236 - 253, Replace manual os.environ save/restore with pytest's monkeypatch: in the test that sets CACHEKIT_MASTER_KEY and defines secure_func (the block using os.environ.get("CACHEKIT_MASTER_KEY"), os.environ["CACHEKIT_MASTER_KEY"]="a"*64, and the try/finally), use the monkeypatch fixture to call monkeypatch.setenv("CACHEKIT_MASTER_KEY", "a"*64) before defining `@cache.secure-decorated` secure_func and remove the try/finally cleanup; also remove the now-unused import os at the top of the file if it is no longer referenced..github/workflows/security-deep.yml (1)
24-27: Pin the Kani verifier version for reproducible security runs.
cargo install --locked kani-verifierinstalls the latest publishedkani-verifierat workflow runtime. Since this is a scheduled daily security gate, pin a known-good Kani version and update it intentionally to avoid silent verifier changes.♻️ Example adjustment
- name: Install Kani verifier run: | - cargo install --locked kani-verifier + cargo install --locked kani-verifier --version "$KANI_VERSION" cargo kani setupDefine
KANI_VERSIONat the workflow or job level with the version validated for this repo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/security-deep.yml around lines 24 - 27, The workflow currently installs the latest kani-verifier at runtime; define and use a pinned KANI_VERSION variable and update the "Install Kani verifier" step to install that specific version (e.g., use KANI_VERSION when invoking cargo install --locked kani-verifier) and run cargo kani setup afterwards; add KANI_VERSION at workflow or job level so future updates are intentional and reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/security-deep.yml:
- Around line 24-27: The workflow currently installs the latest kani-verifier at
runtime; define and use a pinned KANI_VERSION variable and update the "Install
Kani verifier" step to install that specific version (e.g., use KANI_VERSION
when invoking cargo install --locked kani-verifier) and run cargo kani setup
afterwards; add KANI_VERSION at workflow or job level so future updates are
intentional and reproducible.
In `@tests/unit/test_l1_only_mode.py`:
- Around line 236-253: Replace manual os.environ save/restore with pytest's
monkeypatch: in the test that sets CACHEKIT_MASTER_KEY and defines secure_func
(the block using os.environ.get("CACHEKIT_MASTER_KEY"),
os.environ["CACHEKIT_MASTER_KEY"]="a"*64, and the try/finally), use the
monkeypatch fixture to call monkeypatch.setenv("CACHEKIT_MASTER_KEY", "a"*64)
before defining `@cache.secure-decorated` secure_func and remove the try/finally
cleanup; also remove the now-unused import os at the top of the file if it is no
longer referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5ebb031-57aa-4d50-a997-198c555d0202
📒 Files selected for processing (5)
.github/workflows/fuzz-smoke.yml.github/workflows/security-deep.yml.github/workflows/security-fast.yml.github/workflows/security-medium.ymltests/unit/test_l1_only_mode.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #87
DefaultBackendProvidernow auto-detects the cache backend fromCACHEKIT_-prefixed environment variables instead of always defaulting to RedisConfigurationErrorwhen multipleCACHEKIT_-prefixed backend vars are set concurrently (ambiguous config)REDIS_URLnever conflicts — it's a 12-factor fallback that yields to any explicitCACHEKIT_varPriority chain:
CACHEKIT_API_KEY→CACHEKIT_REDIS_URL→CACHEKIT_MEMCACHED_SERVERS→CACHEKIT_FILE_CACHE_DIR→REDIS_URL→ None (L1-only)Unchanged: explicit
backend=parameter andset_default_backend()always take precedence over env-var detection.Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores