feat(control) Add the ability to cache multi-parameter RPC calls by markstory · Pull Request #123077 · getsentry/sentry

markstory · 2026-08-28T22:03:19Z

Currently RPC caching can only be applied to methods with a singular integer parameter. I'd like to be able to cache integration RPC calls as they are high volume, and integration data changes infrequently.

The highest volume integration queries use multiple parameters. With this change we can introduce new service wrappers that enable our highest volume integration RPC calls to be cached.

Refs INFRENG-568

Currently RPC caching can only be applied to methods with a singular integer parameter. I'd like to be able to cache integration RPC calls as they are high volume, and integration data changes infrequently. The highest volume integration queries use multiple parameters. With this change we can introduce new service wrappers that enable our highest volume integration RPC calls to be cached.

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit 6201326. Configure here.}

linear-code · 2026-08-28T22:09:39Z

INFRENG-568

markstory · 2026-08-28T22:19:17Z

+        if len(arg_str) + len(self.base_key) > MAX_CACHE_KEY_LENGTH or _UNSAFE_KEY_CHARS.search(
+            arg_str
+        ):
+            arg_str = hashlib.md5(arg_str.encode("utf-8")).hexdigest()


This isn't being used in a security context. It is just a cache key

sentry · 2026-08-28T22:21:01Z

+# We can go as low as 16 bytes of md5 without risking collisions
+MAX_BASE_KEY_LENGTH = MAX_CACHE_KEY_LENGTH - 16


Bug: The MAX_BASE_KEY_LENGTH calculation incorrectly reserves 16 chars for a 32-char MD5 hexdigest, causing hash truncation and increasing the risk of cache collisions with long base keys.
_{Severity: MEDIUM}

Suggested Fix

Update the MAX_BASE_KEY_LENGTH calculation to reserve enough space for the full 32-character hash and the colon separator. The constant should be set to MAX_CACHE_KEY_LENGTH - 33.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: src/sentry/hybridcloud/rpc/caching/service.py#L51-L52 Potential issue: The constant `MAX_BASE_KEY_LENGTH` is calculated as `MAX_CACHE_KEY_LENGTH - 16`, incorrectly assuming 16 characters are needed for an MD5 hash. The `hashlib.md5(...).hexdigest()` function actually returns a 32-character string. When a `base_key` of maximum length (48 characters) is used with parameters that trigger hashing, the resulting cache key is constructed and then truncated to 64 characters. This action truncates the 32-character hash down to its first 15 characters, significantly increasing the probability of hash collisions and potentially causing silent cache corruption where incorrect data is returned.

sentry-warden · 2026-08-28T22:23:56Z

+MAX_BASE_KEY_LENGTH = MAX_CACHE_KEY_LENGTH - 16
+
+# Memcached rejects whitespace and control characters in keys, so any parameter
+# encoding containing them has to be hashed rather than used verbatim.
+_UNSAFE_KEY_CHARS = re.compile(r"[^\x21-\x7e]")
+
+
+class SiloCacheBackedCallable(Generic[*_Params, _R]):
    """
    Get a single record from cache or wrapped function.

    When cache read returns no data, the wrapped function will be
    invoked. The result of the wrapped function is then stored in cache.

-    Ideal for 'get by id' style methods
+    Ideal for 'get by id' style methods.
+
+    Cache keys vary based on the parameters, so all parameters must be JSON
+    serializable. Parameters that don't fit in the remaining key budget, or that
+    contain characters memcached rejects, are hashed instead.
    """

    silo_mode: SiloMode
    base_key: str
-    cb: Callable[[int], _R | None]
+    cb: Callable[[*_Params], _R | None]
    type_: type[_R]
    timeout: int | None

    def __init__(
        self,
        base_key: str,
        silo_mode: SiloMode,
-        cb: Callable[[int], _R | None],
+        cb: Callable[[*_Params], _R | None],
        t: type[_R],
        timeout: int | None = None,
    ):
+        if len(base_key) > MAX_BASE_KEY_LENGTH:
+            raise ValueError(
+                f"base_key {base_key!r} is {len(base_key)} characters; it must be at most "
+                f"{MAX_BASE_KEY_LENGTH} so that hashed parameters fit within the "
+                f"{MAX_CACHE_KEY_LENGTH} character CacheVersion.key column"
+            )
        self.base_key = base_key
        self.silo_mode = silo_mode
        self.cb = cb
        self.type_ = t
        self.timeout = timeout

-    def __call__(self, object_id: int) -> _R | None:
+    def __call__(self, *args: *_Params) -> _R | None:
        if (
            SiloMode.get_current_mode() != self.silo_mode
            and SiloMode.get_current_mode() != SiloMode.MONOLITH
        ):
-            return self.cb(object_id)
-        return self.get_one(object_id)
+            return self.cb(*args)
+        return self.get_one(*args)
+
+    def key_from(self, *args: *_Params) -> str:
+        """
+        Generate a cache key for this call.
+        Keys must fit within CacheVersion.key column, and not contain whitespace.
+        """
+        # Preserve compatibility with historical cache keys
+        if len(args) == 1:
+            arg_str = str(args[0])  # type: ignore[misc]
+        else:
+            arg_str = json.dumps(args)

-    def key_from(self, object_id: int) -> str:
-        return f"{self.base_key}:{object_id}"
+        # Replace parameters that would overflow the key column, or that memcached
+        # would reject, with a digest of the same parameters. Add 1 for the :
+        if len(arg_str) + len(self.base_key) + 1 > MAX_CACHE_KEY_LENGTH or _UNSAFE_KEY_CHARS.search(
+            arg_str
+        ):
+            arg_str = hashlib.md5(arg_str.encode("utf-8")).hexdigest()
+
+        return f"{self.base_key}:{arg_str}"[:MAX_CACHE_KEY_LENGTH]


Hashed multi-param cache keys truncate and can collide

MAX_BASE_KEY_LENGTH reserves only 16 chars for the digest, but key_from embeds a full 32-char md5 hex and then slices to 64, so long base_keys drop digest bytes and distinct params can share one CacheVersion key.

Evidence

MAX_BASE_KEY_LENGTH = MAX_CACHE_KEY_LENGTH - 16 (line 52) allows a 48-char base_key.

On overflow/unsafe params, key_from sets arg_str = hashlib.md5(...).hexdigest() (32 hex chars) then returns f"{self.base_key}:{arg_str}"[:MAX_CACHE_KEY_LENGTH] (lines 123–125).

With a max-length base key that is 48 + 1 + 32 = 81 chars before the slice, so only the first 15 of 32 digest chars are kept.

Tests construct exactly this max base key ("a" * MAX_BASE_KEY_LENGTH) and exercise hashed params, so the truncated path is reachable; distinct arg digests that share a 15-hex prefix map to one key and cross-invalidate or serve the wrong cached RPC result.

CacheVersionBase.key is varchar(64) and incr_version/get_or_create use that exact string, so truncation is not just cosmetic.

_{Identified by Warden · sentry-backend-bugs · JCC-376}

markstory requested a review from a team as a code owner August 28, 2026 22:03

github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 28, 2026

sentry Bot reviewed Aug 28, 2026

View reviewed changes

Comment thread src/sentry/hybridcloud/rpc/caching/service.py Outdated

cursor Bot reviewed Aug 28, 2026

View reviewed changes

Comment thread src/sentry/hybridcloud/rpc/caching/service.py

github-advanced-security Bot found potential problems Aug 28, 2026

View reviewed changes

off by one and mypy

5611194

sentry Bot reviewed Aug 28, 2026

View reviewed changes

sentry-warden Bot reviewed Aug 28, 2026

View reviewed changes

Navigation Menu

Uh oh!

Uh oh!

feat(control) Add the ability to cache multi-parameter RPC calls - #123077

feat(control) Add the ability to cache multi-parameter RPC calls#123077
markstory wants to merge 2 commits into
masterfrom
feat-rpc-caching-variadi

markstory commented Aug 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

linear-code Bot commented Aug 28, 2026

Uh oh!

markstory Aug 28, 2026

Uh oh!

sentry Bot Aug 28, 2026

Uh oh!

sentry-warden Bot Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		# We can go as low as 16 bytes of md5 without risking collisions
		MAX_BASE_KEY_LENGTH = MAX_CACHE_KEY_LENGTH - 16

Uh oh!

Uh oh!

Conversation

markstory commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

linear-code Bot commented Aug 28, 2026

Uh oh!

markstory Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

sentry Bot Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

sentry-warden Bot Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

markstory commented Aug 28, 2026 •

edited

Loading