feat(objectstore): enable token generator in objectstore client

[matt-codecov]

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

[sentry]

Bug: The create_client function mutates a shared dictionary from options.get("token_generator") using .pop(). Subsequent calls in the same process will cause a KeyError.
_{Severity: CRITICAL}

🔍 Detailed Analysis

The objectstore.config option is registered with FLAG_NOSTORE, which causes options.get() to return the same dictionary instance on every call. The create_client function, used by both get_attachments_session and get_preprod_session, mutates the dictionary from options.get("token_generator") by using .pop() to extract keys. The first time create_client is called in a process, it removes the keys. A subsequent call in the same process will find the keys missing, leading to a KeyError and a crash. This occurs when both session types are used and token_generator is configured.

💡 Suggested Fix

To prevent mutating the shared dictionary, create a copy of the options before accessing its keys. Change signing_key_options := options.get("token_generator") to signing_key_options = options.get("token_generator").copy(). Alternatively, use .get() instead of .pop().

🤖 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/objectstore/__init__.py#L58-L59

Potential issue: The `objectstore.config` option is registered with `FLAG_NOSTORE`,
which causes `options.get()` to return the same dictionary instance on every call. The
`create_client` function, used by both `get_attachments_session` and
`get_preprod_session`, mutates the dictionary from `options.get("token_generator")` by
using `.pop()` to extract keys. The first time `create_client` is called in a process,
it removes the keys. A subsequent call in the same process will find the keys missing,
leading to a `KeyError` and a crash. This occurs when both session types are used and
`token_generator` is configured.

_{Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8374223}

[cursor]

Dictionary mutation corrupts shared config for subsequent clients

High Severity

The use of .pop() on signing_key_options mutates the configuration dictionary in place. Since the options system returns cached values, and create_client() is called separately for both _ATTACHMENTS_CLIENT and _OBJECTSTORE_CLIENT, the first call successfully extracts kid and secret_key, but subsequent calls find these keys missing (already popped). This means only the first initialized client will have a functioning TokenGenerator; the second client silently fails to configure authentication. Using .get() instead of .pop() would avoid mutating the shared config.

🔬 Verification Test

Why verification test was not possible: This bug requires the Sentry options store infrastructure and the objectstore-client library to be properly configured. However, the bug can be verified by code inspection:

1. options_store.get("objectstore.config") returns a cached dictionary (confirmed by reading src/sentry/options/store.py lines 99-122 which shows caching behavior)
2. signing_key_options.pop("kid") and signing_key_options.pop("secret_key") mutate this cached dictionary
3. Two separate clients (_ATTACHMENTS_CLIENT and _OBJECTSTORE_CLIENT) both call create_client()
4. If get_attachments_session() is called first, it pops the keys; when get_preprod_session() is called later, the keys are gone from the cached config
5. The second client's token_generator will be None because signing_key_options.pop("kid") will raise KeyError (or return None if .pop() had a default, but it doesn't)

The pattern of using .pop() on configuration dictionaries that may be cached/shared is a known anti-pattern that causes exactly this type of intermittent, order-dependent bug.

 

[lcian]

It seems that the comment from cursor is correct and we shouldn't use pop (unless options.get actually returns a copy of the dict, in that case pop should be fine as well).
On a side note, I don't know why we're creating 2 separate clients, we should just create a single 1 as long as that works, I can fix that in a separate PR.
Other than that LGTM

[getsantry]

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀