Skip to content

fix(organizations): Add select_related('organization') to membership checks#116350

Open
sentry[bot] wants to merge 3 commits into
masterfrom
claude/fix-org-member-select-related-62f7e
Open

fix(organizations): Add select_related('organization') to membership checks#116350
sentry[bot] wants to merge 3 commits into
masterfrom
claude/fix-org-member-select-related-62f7e

Conversation

@sentry
Copy link
Copy Markdown
Contributor

@sentry sentry Bot commented May 27, 2026

Summary

Fixes SENTRY-14KX

Adds select_related("organization") to check_membership_by_id and check_membership_by_email in DatabaseBackedOrganizationService to prevent an expensive lazy-load query on sentry_organization.

Root Cause

check_membership_by_id fetches an OrganizationMember via .get() without select_related("organization"). When serialize_member() subsequently calls member.get_scopes(), it accesses self.organization (a ForeignKey), which triggers a lazy-load SELECT on the large sentry_organization table (9+ seconds in production).

The call chain:

  1. trigger_action task → get_organization_by_id RPC with a user_id
  2. get_organization_by_idcheck_membership_by_id
  3. check_membership_by_idserialize_membermember.get_scopes()
  4. get_scopes() accesses self.organization → lazy-load → slow unoptimized query

Fix

Add select_related("organization") to the OrganizationMember query in both check_membership_by_id and check_membership_by_email, so the Organization row is fetched via a JOIN in the same query, eliminating the lazy load entirely.

Changes

  • src/sentry/organizations/services/organization/impl.py:
    • check_membership_by_id: Added select_related("organization") to the .get() call
    • check_membership_by_email: Added select_related("organization") to the .get() call (same pattern, same fix)

…checks

Fixes SENTRY-14KX

check_membership_by_id and check_membership_by_email fetch an
OrganizationMember without select_related('organization'). When
serialize_member() then calls member.get_scopes(), it accesses
self.organization (a ForeignKey), triggering a lazy-load query on the
large sentry_organization table. In production this causes 9+ second
queries on the hot path for trigger_action and other callers that go
through get_organization_by_id with a user_id.

Adding select_related('organization') ensures the Organization row is
fetched in the same query, eliminating the expensive lazy load.
@sentry sentry Bot requested a review from a team as a code owner May 27, 2026 21:50
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 27, 2026
@markstory markstory self-assigned this May 28, 2026
@markstory markstory added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label May 28, 2026
Comment thread tests/sentry/core/endpoints/scim/test_scim_team_details.py
@github-actions github-actions Bot removed the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label May 28, 2026
assert len(core_queries) <= 4, (
f"Expected at most 4 core member queries, got {len(core_queries)}:\n"
# Exactly 5 core queries, none of which scale with member count
assert len(core_queries) <= 5, (
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Bug: The test assertion in test_replace_members_query_count was increased from 4 to 5 queries without justification, potentially masking an unrelated performance regression.
Severity: MEDIUM

Suggested Fix

Investigate the source of the fifth query in the test_replace_members_query_count test. The query count should not be increased without understanding and justifying the new query. The test assertion should be reverted to <= 4 and the underlying cause of the extra query should be fixed or explicitly acknowledged.

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/core/endpoints/scim/test_scim_team_details.py#L386

Potential issue: The pull request increases the query count assertion in
`test_replace_members_query_count` from `<= 4` to `<= 5`. However, the code changes that
add `select_related("organization")` are not executed in this test's code path. This
indicates the fifth query is unrelated to the intended optimization. This change masks a
potential performance regression or a previously undetected query in the SCIM team
member replacement flow, which could allow future performance issues to go unnoticed.

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

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

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

Fix All in Cursor

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

Reviewed by Cursor Bugbot for commit 1b51367. Configure here.

f"Expected at most 4 core member queries, got {len(core_queries)}:\n"
# Exactly 5 core queries, none of which scale with member count
assert len(core_queries) <= 5, (
f"Expected at most 5 core member queries, got {len(core_queries)}:\n"
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.

Test filter matches unrelated query due to substring collision

Low Severity

The assertion was bumped from 4 to 5 because the is_fetch_new filter uses "IN" in sql, which is a naive substring check that now falsely matches the check_membership_by_id query. Adding select_related("organization") introduces INNER JOIN into that query's SQL, and the string "IN" is a substring of "INNER". This causes the auth/permission query to be incorrectly classified as a "core replace-path query." The docstring still lists only 4 queries, making it inconsistent with the assertion, and the test is now weaker at catching real N+1 regressions since the threshold was raised to accommodate a false positive in the filter logic.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1b51367. Configure here.

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