Invalidate subject caches after deletion - #2359
Shubham Padkonde (Shubham-Padkonde) wants to merge 3 commits into
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved cache-race and TTL-expiry failures can leave stale data or make successful deletions fail.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates sync and async schema registry clients to invalidate subject schema and latest-result caches after successful soft or permanent deletion.
Changes:
- Updates deletion cache invalidation.
- Adds parameterized sync/async regression tests.
- Documents the fix in the changelog.
File summaries
| File | Summary |
|---|---|
tests/schema_registry/_sync/test_api_client.py |
Sync regression coverage for cache invalidation. |
tests/schema_registry/_async/test_api_client.py |
Async regression coverage for cache invalidation. |
src/confluent_kafka/schema_registry/_sync/schema_registry_client.py |
Critical (1 vote): in-flight reads can repopulate stale caches after deletion. Moderate (1 vote): TTL expiry can cause deletion invalidation to raise KeyError. |
src/confluent_kafka/schema_registry/_async/schema_registry_client.py |
Critical (1 vote): in-flight reads can repopulate stale caches after deletion. Moderate (1 vote): TTL expiry can cause deletion invalidation to raise KeyError. |
CHANGELOG.md |
Documents the customer-facing cache invalidation fix. |
Review details
Suppressed comments (2)
src/confluent_kafka/schema_registry/_async/schema_registry_client.py:1094
TTLCachecan expire a matching entry after the key snapshot is built but before this deletion runs; cachetools can raiseKeyErrorwhendeltargets an expired entry. That makes a successful subject deletion fail during cache invalidation for short TTLs. Usepop(cache_key, None)(or otherwise tolerate expiry) while holding the lock.
del self._latest_with_metadata_cache[cache_key]
src/confluent_kafka/schema_registry/_sync/schema_registry_client.py:1089
TTLCachecan expire a matching entry after the key snapshot is built but before this deletion runs; cachetools can raiseKeyErrorwhendeltargets an expired entry. That makes a successful subject deletion fail during cache invalidation for short TTLs. Usepop(cache_key, None)(or otherwise tolerate expiry) while holding the lock.
del self._latest_with_metadata_cache[cache_key]
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| self._cache.remove_by_subject(subject_name) | ||
| async with self._latest_lock: | ||
| self._latest_version_cache.pop(subject_name, None) | ||
| for cache_key in list(self._latest_with_metadata_cache): | ||
| if cache_key[0] == subject_name: |
There was a problem hiding this comment.
Good point, and it's real, but it isn't specific to deletion. Every cached read in this client (lookup_schema, get_latest_version, get_latest_with_metadata, get_version, …) releases the cache lock while its REST request is in flight. So the same window already exists for a read racing any other write, for example the registration invalidation from #2333. This PR only fixes the deterministic case: a completed delete left stale entries in the cache for every later read.
Closing the in-flight race properly needs a per-subject generation (or similar) checked before every cache write, across all read paths in both clients. That's a broader change to the cache design than this fix. I'm happy to do it as a follow-up if the maintainers want it.
| self._cache.remove_by_subject(subject_name) | ||
| with self._latest_lock: | ||
| self._latest_version_cache.pop(subject_name, None) | ||
| for cache_key in list(self._latest_with_metadata_cache): | ||
| if cache_key[0] == subject_name: |
There was a problem hiding this comment.
Same as the async thread above; the sync client is generated from the async one by unasync.
Reads of the subject schema cache released its lock during the REST request, so a read that started before delete_subject, delete_version or clear_caches could repopulate the cache with its stale response. Snapshot an invalidation generation before the request and skip the cache write when it changed, matching the latest-result caches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
What
A successful soft delete left registered schemas cached, so registering the same schema again returned the cached ID without recreating the subject. Both soft and permanent deletion also left latest-version and metadata-filtered latest results cached.
Clear the deleted subject's schema cache after either successful deletion, and remove its latest-cache entries under the existing lock. Preserve other subjects' entries and keep caches unchanged if DELETE fails. Update the async implementation, generated sync client/tests, and changelog.
Checklist
References
Related cache discussion: #1950. This does not address its global schema-by-ID cache issue. #2333 covers cache invalidation after registration; this change covers whole-subject deletion.
Test & Review
Added 40 parameterized regression cases across sync/async clients: soft/permanent deletion, LRU/TTL latest caches, subsequent reads and re-registration, unrelated subjects, and failed deletion.
Tested with Python 3.12 and the 2.15.1 native extension against this checkout's Python source. No live Kafka/Schema Registry integration tests or full project suite were run.
Prepared with Codex assistance.