[PM-42157] fix: register separate memory caches for icons and change-password URIs - #8226
Open
deniswe wants to merge 1 commit into
Open
[PM-42157] fix: register separate memory caches for icons and change-password URIs#8226deniswe wants to merge 1 commit into
deniswe wants to merge 1 commit into
Conversation
AddMemoryCache registers IMemoryCache with TryAdd, so calling it twice created one shared cache rather than two. The second options callback also won, silently discarding IconsSettings.CacheSizeLimit. Register each cache as its own keyed singleton so both honour their configured size limits independently. Fixes bitwarden#7701
Collaborator
|
Thank you for your contribution! We've added this to our internal Community PR board for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
bitwarden-bot
changed the title
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
2 participants
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.
AddMemoryCache registers IMemoryCache with TryAdd, so calling it twice created one shared cache rather than two. The second options callback also won, silently discarding IconsSettings.CacheSizeLimit. Register each cache as its own keyed singleton so both honour their configured size limits independently.
Fixes #7701
🎟️ Tracking
#7701 (PM-37996)
#7701
📔 Objective
Startup.ConfigureServicescalledservices.AddMemoryCache(...)twice, intending one cache forIconsControllerand one forChangePasswordUriController. BecauseAddMemoryCacheregistersIMemoryCacheviaTryAdd, the second call registers no second cache — it only appends anotheroptions callback. Both controllers therefore shared a single cache whose
SizeLimitcame fromwhichever value was configured last (
ChangePasswordUriSettings.CacheSizeLimit), silentlydiscarding
IconsSettings.CacheSizeLimit. Cached icons were then evicted under a size budgetnever intended for them, matching the reported symptom of icons disappearing.
This registers the two caches as separate keyed singletons, so each honours its own configured
CacheSizeLimitand neither evicts the other's entries. Keyed registration follows the existingpattern in the codebase (e.g.
[FromKeyedServices(OrganizationReportCacheConstants.CacheName)]).The registration moved into an
AddCachesextension method alongside the existingConfigureHttpClients/AddHtmlParsing/AddServices, which also makes it directly testable.Added tests cover that the two caches are distinct instances, do not share entries, and each
applies its own size limit.
Note for reviewers: deployments that configured only
IconsSettings.CacheSizeLimitpreviously ended up with no effective limit at all; that limit now takes effect, so their icon
cache will begin evicting. The default configuration (
nullfor both) is unchanged.