Include user in the cache key for explain vulnerability LLM cache
The following discussion from !127261 (merged) should be addressed:
@ghavenga The problem is that this can break in the future without any changes to the caching code. For example, it would be hard to catch this problem in a change that only affects the client code. > And I don't think there's a reliable and cost-effective way to test for this.
Our OpenAI LLM client already has a dependency on
user
, for example. If we did not include the user in that case, we might end up showing cached responses for users that should not have access to OpenAI, and vice versa.If you don't want to add
user.id
to the cache key, then can we add a warning comment in the client constructor? Maybe something like:def initialize(_user) # WARNING: The user argument is currently unused, and downstream code relies on this # for caching. When introducing user-variadic behaviour, make sure to review downstream # code and update cache keys to include the user id as needed.
What do you think?
Implementation Plan
- Update the cache to include the user_id - https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/lib/gitlab/llm/completions/explain_vulnerability.rb#L91
Edited by Michael Becker