Caching twice: any regular cache mechanism + HttpContext.Items on top of that

I think "caching twice" might be a red herring. What I see is additional logic in the extension method that leverages a database call and then caches the result of database call + additional logic. You haven't described where these two functions exist within the overall architecture of the application, but I suspect the repository and extension method exist in different functional layers; this can justify the separation.

Beyond that, nobody outside of your team can really answer this question. Sometimes you need a good sit-down with version control history to infer why something was done.

My intuition says the database caching might have been introduced after the extension method. I recommend inspecting the other areas of the application that call the repository. You might find caching was necessary in those other areas because they didn't have access to an IDisplayHelper object, or simply didn't need one, yet they still needed this theme information. Who knows? The database caching might have been introduced first before the extension method, and perhaps someone did a little premature optimizing. This is all pure conjecture on my part.

In general, multiple layers of caching can be justified if they end up caching different kinds of data. For example, NHibernate, an ORM for .NET, has two layers of caching: (1) entity caching at the session level, and (2) a cache of individual values at the session factory level. Each cache serves a different purpose. The first speeds up retrieval of entities for a single session; the second provides a means to hydrate multiple instances of the same entity across sessions without hitting the database again.

There are valid reasons for the code you see, but the whole point of caching information is to realize a speed boost. If you disable the cache in the IDisplayHelper and it doesn't seem to negatively impact performance, you could consider removing that cache. As for why those two caches where introduced? That isn't something strangers on the internet can tell you; more than likely, you'll need to consult version control or your teammates to understand this.

There are a number of things about the GetThemeVariables method which I think we can say point to it being incorrect.

0. static method

1. Using the service locator pattern, widely frowned upon, should be using DI, the displayhelper is passed in, why not all the services?

2. If services.GetService can return null for the storeContext it could also return null for the repo, thus throwing an exception.

3. Using HttpContext.Items as a cache at all isn't great. The primary use should be communication between layers. But here the cached data doesn't seem to be unique to the request, but to the store. So caching it per request is inefficient.

4. Calling Await() rather than returning a Task and awaiting it.

I'm guessing; but it looks like this method has been hacked in because someone has wanted to use the theme at the bottom of a call chain and realised they don't have access to the services they need to be able to retrieve it.

Rather than refactor, pass the needed vars or services in through that chain, or add the variables to the HttpContect.Items, or even displayHelper.HttpContext at some higher level in the code, they have just made a static method they can call from anywhere and uses service location to hopefully work. There is no thought given to caching or lack of caching.

If you refactored the code properly then you wouldn't have this method at all.

Assumptions about components form stronger dependencies/coupling and make reasoning about the product harder (because a component can no longer be considered in isolation).

The decision to cache data was probably made twice to weaken two kinds of dependencies.