Skip to content

feat(cells) Make organization avatar URL cell compatible#115689

Merged
markstory merged 5 commits into
masterfrom
feat-org-avatar-cell
May 20, 2026
Merged

feat(cells) Make organization avatar URL cell compatible#115689
markstory merged 5 commits into
masterfrom
feat-org-avatar-cell

Conversation

@markstory
Copy link
Copy Markdown
Member

I was reviewing the URL exception list and saw that this URL needed an org scoped endpoint added. I've added a new URL path for that includes the organization's slug, and moved the URL generation into model methods so that the absolute_url() interface from BaseAvatar is provided by both the cell + control models.

Refs INFRENG-319

I was reviewing the URL exception list and saw that this URL needed
an org scoped endpoint added. I've added a new URL path for that
includes the organization's slug, and moved the URL generation into
model methods so that the `absolute_url()` interface from `BaseAvatar`
is provided by both the cell + control models.

Refs INFRENG-319
@markstory markstory requested a review from a team as a code owner May 15, 2026 21:32
@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 15, 2026

INFRENG-319

@markstory markstory requested a review from a team May 15, 2026 21:32
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 15, 2026
Comment thread src/sentry/web/urls.py
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 2 potential issues.

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 692efbc. Configure here.

Comment thread src/sentry/web/urls.py Outdated
Comment thread src/sentry/web/urls.py
markstory added 2 commits May 19, 2026 15:35
We have a bunch of middleware that does path checks and that logic runs
before django has done URL route matching. The previous URL of
`/organizations/:slug/avatar` is incompatible with the static prefix
checks done for cookies, so I chose to go back to `/organization-avatar`
as the prefix.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: I think we can remove this attribute, looks like OrganizationAvatar.absolute_url uses reverse(), and never reads this attribute, and likewise AvatarBase.absolute_url does read this but it gets overridden here

locality_url, f"/{OrganizationAvatar.url_path}/{avatar_replica.avatar_ident}/"
),
"avatarUrl": avatar_replica.absolute_url(),
}
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.

OrganizationAvatarReplica.absolute_url() raises DoesNotExist if OrganizationMapping is missing

Calling avatar_replica.absolute_url() in the serializer will propagate an unhandled OrganizationMapping.DoesNotExist if the mapping row hasn't been replicated yet or was deleted, crashing the endpoint — wrap the call in a try/except or fall back to None for avatarUrl.

Evidence
  • avatar_replica.absolute_url() (in organizationavatarreplica.py, line 33) calls OrganizationMapping.objects.get_from_cache(organization_id=self.organization_id).
  • get_from_cache ultimately calls .get(**kwargs) on a cache miss (confirmed in db/models/manager/base.py lines 314, 323).
  • If the OrganizationMapping row does not exist, Django raises OrganizationMapping.DoesNotExist.
  • Neither absolute_url() nor the serializer's serialize() method catches this exception.
  • An OrganizationAvatarReplica can exist before its corresponding OrganizationMapping has been fully replicated across silos, making this a real race condition.

Identified by Warden sentry-backend-bugs · 7UC-E35

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If the mapping record is gone the org is too.

def absolute_url(self) -> str:
"""
Provide a consistent interface with OrganizationAvatar to simplify serializers.
"""
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.

OrganizationMapping.objects.get_from_cache() raises unhandled DoesNotExist in absolute_url

If the OrganizationMapping for this avatar's organization_id has been deleted or not yet created, get_from_cache raises OrganizationMapping.DoesNotExist, which propagates unhandled through the serializer and causes a 500.

Evidence
  • get_from_cache in BaseManager (base.py:283) is a thin caching wrapper around self.get(**kwargs) — it does not catch DoesNotExist.
  • absolute_url calls OrganizationMapping.objects.get_from_cache(organization_id=self.organization_id) with no try/except.
  • The serializer (organization.py:387) calls avatar_replica.absolute_url() unconditionally when an OrganizationAvatarReplica exists.
  • An org can have a replica row while its mapping is concurrently deleted or not yet replicated, triggering the exception during serialization.

Identified by Warden sentry-backend-bugs · HXD-U5J

@@ -58,3 +60,9 @@ def handle_async_deletion(
control_replica_service.delete_organization_avatar_replica(
organization_id=shard_identifier,
)
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.

Organization.objects.get_from_cache() in absolute_url() raises DoesNotExist if organization is deleted

If the organization has been deleted while its avatar still exists, Organization.objects.get_from_cache(id=self.organization_id) will raise Organization.DoesNotExist, crashing any serialization that calls absolute_url() (e.g. the organization serializer at lines 387 and 545).

Evidence
  • absolute_url() calls Organization.objects.get_from_cache(id=self.organization_id) with no try/except.
  • get_from_cache ultimately calls .get(**kwargs) (base.py line 344) which raises DoesNotExist when the record is absent.
  • The serializer (organization.py lines 387, 545) calls avatar_replica.absolute_url() / attrs["avatar"].absolute_url() without guarding against this exception.
  • OrganizationAvatar is a ReplicatedCellModel, so the avatar replica can outlive the organization if deletion is not perfectly ordered.

Identified by Warden sentry-backend-bugs · ENG-WHM

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If the org is gone so is the avatar.

@markstory markstory merged commit 430862f into master May 20, 2026
83 checks passed
@markstory markstory deleted the feat-org-avatar-cell branch May 20, 2026 20:45
JonasBa pushed a commit that referenced this pull request May 21, 2026
I was reviewing the URL exception list and saw that this URL needed an
org scoped endpoint added. I've added a new URL path for that includes
the organization's slug, and moved the URL generation into model methods
so that the `absolute_url()` interface from `BaseAvatar` is provided by
both the cell + control models.

Refs INFRENG-319
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.

3 participants