impr(workflow engine): Add caching for detector lookup by data source

[Christinarlong]

Okay basically we're caching the bulk query and adapting the single detector query in the subscription processor work with that.

[saponifi3d]

let me know if it'd help to hop on a call / discuss any of these comments too! this whole area is pretty confusing, even for those with context. haha

[saponifi3d]

up to you on flattening the cache stuff and pulling it off of the detector model, i would recommend it as it will allow us to DRY up the code around purging / updating the cache.

[saponifi3d]

love the change with the transaction!

mind adding some tests to make sure all the invalidation stuff is working as expected?

One specific test case i'd like to include is changing a detector's enabled attribute to false and save that to see if it correctly invalidates the cache. (this is a code path that exists with Uptime, they have some code that will disable a detector based on billing information).

Another test case would be on the detector endpoint; have a hot cache then trigger a successful PUT / POST, and then ensure the cache is correctly invalidated.

[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.}

[sentry]

The cache storage mechanism doesn't have a version or cache invalidation strategy for the Detector objects themselves. While individual cache entries are invalidated correctly by invalidate_detectors_by_data_source_cache(), there's a risk that stale Detector model instances (with outdated attributes like name, enabled, etc.) could be cached and returned. Consider implementing cache versioning or model change hooks (e.g., via Django signals on Detector.save()) to ensure consistency, especially since detector attributes can be modified directly via .update() calls without always triggering the cache invalidation callbacks.
_{Severity: MEDIUM}

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/workflow_engine/processors/data_source.py#L14-L30

Potential issue: The cache storage mechanism doesn't have a version or cache
invalidation strategy for the `Detector` objects themselves. While individual cache
entries are invalidated correctly by `invalidate_detectors_by_data_source_cache()`,
there's a risk that stale `Detector` model instances (with outdated attributes like
`name`, `enabled`, etc.) could be cached and returned. Consider implementing cache
versioning or model change hooks (e.g., via Django signals on Detector.save()) to ensure
consistency, especially since detector attributes can be modified directly via
`.update()` calls without always triggering the cache invalidation callbacks.

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

[sentry]

In delete_uptime_detector() at line 585-598, the cache is invalidated synchronously (not via transaction.on_commit()). This differs from all other functions in this file that use transaction.on_commit(). If the subsequent delete_uptime_subscription() or RegionScheduledDeletion.schedule() calls modify database state, the synchronous invalidation could occur before the transaction commits, creating a brief window where the cache is cleared but the detector still exists in the database. For consistency and safety, this should use transaction.on_commit() like the other functions.
_{Severity: HIGH}

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/uptime/subscriptions/subscriptions.py#L585-L598

Potential issue: In `delete_uptime_detector()` at line 585-598, the cache is invalidated
synchronously (not via `transaction.on_commit()`). This differs from all other functions
in this file that use `transaction.on_commit()`. If the subsequent
`delete_uptime_subscription()` or `RegionScheduledDeletion.schedule()` calls modify
database state, the synchronous invalidation could occur before the transaction commits,
creating a brief window where the cache is cleared but the detector still exists in the
database. For consistency and safety, this should use `transaction.on_commit()` like the
other functions.

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

[sentry]

The _invalidate_cache_by_detector() method (lines 145-149) is called in both update() (line 214) and delete() (line 236). However, in update() it's called INSIDE the atomic transaction block, while in other locations like uptime subscriptions it's called via transaction.on_commit(). This inconsistency could cause cache invalidation to occur before the transaction commits. For consistency with other parts of the codebase and to avoid race conditions, consider wrapping the _invalidate_cache_by_detector() call in update() with a transaction.on_commit() callback instead of calling it synchronously.
_{Severity: HIGH}

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/workflow_engine/endpoints/validators/base/detector.py#L145-L214

Potential issue: The `_invalidate_cache_by_detector()` method (lines 145-149) is called
in both `update()` (line 214) and `delete()` (line 236). However, in `update()` it's
called INSIDE the atomic transaction block, while in other locations like uptime
subscriptions it's called via `transaction.on_commit()`. This inconsistency could cause
cache invalidation to occur before the transaction commits. For consistency with other
parts of the codebase and to avoid race conditions, consider wrapping the
`_invalidate_cache_by_detector()` call in `update()` with a `transaction.on_commit()`
callback instead of calling it synchronously.

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

[sentry]

In _update_workflow_engine_models() at line 23-30, the code defines a nested function invalidate_cache() inside a closure and then schedules it via transaction.on_commit(). This is correct, but ensure the signal handler context doesn't already have an active transaction that might not commit as expected. Django signals can be tricky with transactions. Document or verify that this signal is always called within a request-response cycle where transaction behavior is predictable.
_{Severity: MEDIUM}

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/receivers/rule_snooze.py#L23-L30

Potential issue: In `_update_workflow_engine_models()` at line 23-30, the code defines a
nested function `invalidate_cache()` inside a closure and then schedules it via
`transaction.on_commit()`. This is correct, but ensure the signal handler context
doesn't already have an active transaction that might not commit as expected. Django
signals can be tricky with transactions. Document or verify that this signal is always
called within a request-response cycle where transaction behavior is predictable.

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

[sentry]

In test_put_invalidates_cache() at line 1071-1110, the test verifies that the cache is invalidated after a PUT request. However, it only tests one data source. If a detector has multiple data sources (allowed by the system depending on configuration), ensure the cache is invalidated for ALL of them. Consider adding a test case with multiple data sources to verify comprehensive coverage.
_{Severity: MEDIUM}

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:
tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py#L1071-L1110

Potential issue: In `test_put_invalidates_cache()` at line 1071-1110, the test verifies
that the cache is invalidated after a PUT request. However, it only tests one data
source. If a detector has multiple data sources (allowed by the system depending on
configuration), ensure the cache is invalidated for ALL of them. Consider adding a test
case with multiple data sources to verify comprehensive coverage.

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