Skip to content

mypy(workflow-engine): Prepare for detector.project to be nullable - #120744

Merged
leeandher merged 3 commits into
masterfrom
leanderrodrigues/iswf-3123-typing-fixes-before-migration
Jul 28, 2026
Merged

mypy(workflow-engine): Prepare for detector.project to be nullable#120744
leeandher merged 3 commits into
masterfrom
leanderrodrigues/iswf-3123-typing-fixes-before-migration

Conversation

@leeandher

Copy link
Copy Markdown
Member

sorry for all the codeowners pings 🙏

we're looking to support a new detector for All Projects, but the current model (Detector) has a FK to Project. We can't use a random project without having issues with things like cascade deletions and project transfers so we're opting to make the column nullable.

That will introduce a lot of mypy issues though, so before that migration we're addressing those here.

Tech Spec - https://app.notion.com/p/sentry/Spec-All-Projects-Default-Alert-Migration-8e48b10e4b5d821fb1c581cc65e65c32?source=copy_link
Linear Ticket - https://linear.app/getsentry/issue/ISWF-3123/create-migration-to-support-all-projects

@linear-code

linear-code Bot commented Jul 28, 2026

Copy link
Copy Markdown

ISWF-3123

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 28, 2026
@leeandher
leeandher marked this pull request as ready for review July 28, 2026 15:52
@leeandher
leeandher requested review from a team as code owners July 28, 2026 15:52
Comment thread src/sentry/notifications/notification_action/types.py
Comment thread src/sentry/workflow_engine/endpoints/organization_open_periods.py
Comment thread src/sentry/api/serializers/models/rule.py
cursor[bot]

This comment was marked as outdated.

Comment thread src/sentry/workflow_engine/endpoints/validators/utils.py
Comment thread src/sentry/api/serializers/models/rule.py
@leeandher
leeandher force-pushed the leanderrodrigues/iswf-3123-typing-fixes-before-migration branch from 50406d9 to 1f65dfc Compare July 28, 2026 17:04
@leeandher
leeandher requested a review from a team as a code owner July 28, 2026 17:04
Comment thread src/sentry/workflow_engine/endpoints/organization_open_periods.py

@saponifi3d saponifi3d left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - just an unrelated note to keep in mind for the future. i also confirmed that there shouldn't be any other call sites. 🚢

@github-actions

This comment was marked as outdated.

Comment thread src/sentry/workflow_engine/models/detector.py Outdated
@leeandher
leeandher force-pushed the leanderrodrigues/iswf-3123-typing-fixes-before-migration branch from 1f65dfc to 0191043 Compare July 28, 2026 18:01
@leeandher
leeandher removed the request for review from a team July 28, 2026 18:02
Comment on lines 155 to +156
def get_org_from_detector(detector: Detector) -> tuple[Organization]:
return (detector.project.organization,)
return (detector.linked_project.organization,)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache invalidation crashes for non-project-scoped detectors

get_org_from_detector now uses detector.linked_project, which raises ValueError when project is None. This function is registered as a post_save/post_delete signal handler for all Detector models, so saving or deleting any non-project-scoped detector (introduced by this PR) will propagate an unhandled exception through Django signals.

Evidence
  • Detector.linked_project raises ValueError("Detector is not project-scoped") when self.project is None (src/sentry/workflow_engine/models/detector.py:123-124).
  • @cache_func_for_models([(Detector, get_org_from_detector)]) registers get_org_from_detector as the arg_getter for post_save and post_delete signals on every Detector instance (src/sentry/utils/function_cache.py:66-97).
  • clear_cache_for_cached_func calls arg_getter(instance) without any try/except (src/sentry/utils/function_cache.py:47), so the ValueError propagates directly.
  • The PR explicitly introduces detectors with nullable project ("All Projects"), meaning uptime code will now hit this crash path whenever one of those detectors is saved or deleted.
  • The cached function get_active_auto_monitor_count_for_org only cares about uptime detectors with project__organization, so a project=None detector is irrelevant to its cache, but the signal still fires unconditionally.

Identified by Warden · sentry-backend-bugs · 3CL-M2W

@leeandher
leeandher merged commit 403652f into master Jul 28, 2026
86 checks passed
@leeandher
leeandher deleted the leanderrodrigues/iswf-3123-typing-fixes-before-migration branch July 28, 2026 21:40
leeandher added a commit that referenced this pull request Jul 28, 2026
)

The cascade deletions cause issues with these caching functions. The
test failures can be [seen in the migration
PR](#120687 (comment)).
I'm not too familiar with these utilities but from my understanding:
- It's causing failures because the cascade deletion order has changed
- with the NOT NULL key to Project: the Detector was always deleted
before the Project, meaning Detector's post_delete fires, and that
Project is still in the DB, so detector.project.organization (on the
in-memory Detector) still works.
- with the nulllable FK to Project: Django does not ensure that
Detector's post_delete happens before the Project row is removed. When
the Project is removed first, the caching function hits the DoesNotExist
error.

I consulted claude about this ordering change, who added:
> the reason the ordering changes is specific to how Django's cascade
collector works. It tracks dependencies between models to determine
deletion order, but it only registers a dependency when the FK is NOT
NULL (line 136 of django/db/models/deletion.py: if source is not None
and not nullable). And the CASCADE handler passes nullable=field.null
(line 27). So making the FK nullable literally removes the ordering
constraint that kept Detector-before-Project, and the deletion order
becomes arbitrary.

Previous PR: #120744
Next PR: #120687
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants