fix(swr): LAB-381 panel fast-follow — contextvar propagation, CWE-532 redaction, orphan cut - #235
Conversation
…action, orphan cut, dedup Panel FIX-FIRST verdict on merged #228 head (Ray-approved); gates v0.14.0. Must-fix: - [MAJ] Sync background revalidation now runs under a copy_context() snapshot captured in-request, so contextvar-based tenant extraction (ContextVarExtractor + encryption) works off-thread exactly as the async path does via create_task. Without this the daemon thread hit an unset var -> fail-closed ValueError -> silently swallowed -> every stale window degraded to a sync miss at evict_at. AC test proves the revalidation actually executes AND stores through the real encryption stack. - [MIN/CWE-532] The three SWR debug logs emit redact_cache_key() digests; the revalidation thread name is static ('cachekit-swr-revalidate', no key slice). Regression test forces the debug line and asserts no raw key. Should-fix (Ray agreed the recommendations): - Cut orphaned CachekitIOBackend.get_with_freshness_async + its tests (the handler runs sync backend methods in the thread pool; the async variant had no callers — LAB-388 trust-bug pattern). Dropped the dead refresh_ttl params from both operation-handler freshness getters. - Backed-mode SWR gets its own _L2_SWR_MAX_CONCURRENT_REFRESHES constant; _swr_* -> _l2_swr_* rename disentangles it from the _l1_swr_* machinery. - Fail-closed propagation tests through BOTH freshness getters (DecryptionAuthenticationError raises; poisoned entry retained) so an except-reorder can't demote to fail-open with green tests. - Dedup, net deletion in source: 4x decrypt-failure tail -> _handle_l2_read_error[_async]; 4x L1-put idiom -> _put_l1 closure. Docs pass: the contextvar fix CHANGES documented behavior — cache() docstring + configuration.md now state contextvars are snapshotted and visible to the recompute (other request-scoped resources still are not). Gates: 2197 passed (7 new tests), ruff clean, basedpyright 0 errors. Co-authored-by: multica-agent <github@multica.ai>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…E-532) The crypto re-review of the fast-follow verified LAB-108 preservation, contextvar isolation, and the rename/dedup, and found two remaining redaction gaps on the SWR surface: - StandardCacheHandler.get_with_freshness[_async] + the operation-handler freshness getters logged raw cache_key at ERROR/WARNING on backend errors — fires on routine outages, tenant-bearing keys in prod logs. - The L1-only SWR debug lines (+ shared TTL-refresh done-callback) in wrapper.py still emitted raw keys. All now use redact_cache_key(). NOTE: the pre-existing raw-key logging convention in the non-SWR getters (plain get/get_buffer, sets) is deliberately out of scope for this release-gating PR — repo-wide sweep belongs in its own issue. 2197 tests green; ruff clean. Co-authored-by: multica-agent <github@multica.ai>
27Bslash6
changed the title
# Conflicts: # src/cachekit/decorators/wrapper.py
1 participant
LAB-381: SWR panel fast-follow — contextvar propagation, CWE-532 redaction, orphan cut, dedup
Fast-follow to merged #228, implementing the expert panel's FIX-FIRST findings (Ray-approved on LAB-381). Blocks release-please #234 (v0.14.0) — hold that until this merges so SWR doesn't ship with the items below.
Must-fix
contextvars.copy_context()snapshot captured in-request, matching whatasyncio.create_taskgives the async path. Previously,@cache+ encryption +ContextVarExtractorhit an unset tenant var off-thread → fail-closedValueError→ swallowed → background refresh silently no-oped and every stale window degraded to a sync miss atevict_at. The AC test proves revalidation executes AND stores through the real encryption stack with a contextvar-only tenant.redact_cache_key()digests; the revalidation thread name is static (cachekit-swr-revalidate). Regression test forces the debug line and asserts no raw key in logs or thread names.Should-fix (Ray agreed the recommendations)
CachekitIOBackend.get_with_freshness_async+ its test class removed (the handler layer runs sync backend methods in the thread pool; the async variant had zero callers — the LAB-388 trust-bug pattern). Deadrefresh_ttlparams dropped from both operation-handler freshness getters._L2_SWR_MAX_CONCURRENT_REFRESHES;_swr_*→_l2_swr_*so it no longer shares names or a constant with the_l1_swr_*machinery in the same file.get_cached_value_asynccontract):DecryptionAuthenticationErrorraises, poisoned entry retained as evidence — an except-clause reorder can't demote to fail-open with green tests._handle_l2_read_error[_async]; the 4× L1-put idiom →_put_l1.Docs gate
The contextvar fix changes documented behavior: the
cache()docstring andconfiguration.mdnow state that the background recompute sees a snapshot of the caller'scontextvars(contextvar-based tenant extraction works) while other request-scoped resources remain unavailable.Verification
2197 tests pass (7 new: contextvar AC through real encryption, CWE-532 log/thread assertions, 2× fail-closed propagation), ruff clean, basedpyright 0 errors. Source diff is net-negative outside tests.