fix: Remove get_cached() and use get() everywhere for cross-worker visibility - #6388
Draft
mattf wants to merge 1 commit into
Draft
fix: Remove get_cached() and use get() everywhere for cross-worker visibility#6388mattf wants to merge 1 commit into
mattf wants to merge 1 commit into
Conversation
…sibility Remove get_cached() entirely from the DistributionRegistry protocol and implementations. All callers now use get() which implements cache-then-DB fallback semantics. The old get_cached() was a cache-only accessor that returned None when a routing table lookup missed the in-memory dict. In single-process deployments this was fine, but in multi-worker uvicorn deployments each process has its own cache that is only updated by _ensure_initialized() at startup, register(), update(), and delete() -- all within a single process. A cache miss in worker A after worker B created a resource would always return None instead of falling back to the DB where worker B had persisted the object. This is the exact bug the register() override already worked around: its doc comment explicitly says 'use super().get() (DB read) rather than self.get_cached() so that in multi-worker deployments, where each process has its own in-memory cache, we always read the authoritative stored object'. The same logic applies to all callers. get_all() already had TTL-based cache refresh on every call, and that's used for list endpoints. But the routing path (get_provider_impl()) and the metrics helper (_get_provider_id()) both called get_cached(), creating a silent failure mode where freshly created resources were invisible until the next TTL refresh or a restart. By removing get_cached() and using get() everywhere (which checks cache first, falls back to DB on miss, and updates the cache), we get immediate cross-worker visibility for all operations with only a modest additional DB round-trip cost on first access per resource (and that access gets cached for subsequent calls). Changes: - Remove get_cached() from DistributionRegistry protocol - Remove get_cached() from DiskDistributionRegistry (was raising NotImplementedError) - Remove get_cached() from CachedDiskDistributionRegistry (was just a dict get) - Update get_provider_impl() in common.py to use get() (was using get_cached()) - Update _get_provider_id() in vector_io.py to be async and use get() (was sync and using get_cached()) -- all 6 call sites are in async methods and already awaited - Update test mocks and test assertions that referenced get_cached() Signed-off-by: Matthew Farrellee <matt@cs.wisc.edu>
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
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.
Remove get_cached() entirely from the DistributionRegistry protocol and implementations. All callers now use get() which implements cache-then-DB fallback semantics.
The old get_cached() was a cache-only accessor that returned None when a routing table lookup missed the in-memory dict. In single-process deployments this was fine, but in multi-worker uvicorn deployments each process has its own cache that is only updated by _ensure_initialized() at startup, register(), update(), and delete() -- all within a single process. A cache miss in worker A after worker B created a resource would always return None instead of falling back to the DB where worker B had persisted the object.
This is the exact bug the register() override already worked around: its doc comment explicitly says 'use super().get() (DB read) rather than self.get_cached() so that in multi-worker deployments, where each process has its own in-memory cache, we always read the authoritative stored object'. The same logic applies to all callers.
get_all() already had TTL-based cache refresh on every call, and that's used for list endpoints. But the routing path (get_provider_impl()) and the metrics helper (_get_provider_id()) both called get_cached(), creating a silent failure mode where freshly created resources were invisible until the next TTL refresh or a restart.
By removing get_cached() and using get() everywhere (which checks cache first, falls back to DB on miss, and updates the cache), we get immediate cross-worker visibility for all operations with only a modest additional DB round-trip cost on first access per resource (and that access gets cached for subsequent calls).
Changes:
closes #5008