Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion migrations_lockfile.txt
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ replays: 0007_organizationmember_replay_access

seer: 0017_drop_old_fk_columns

sentry: 1114_extend_repository_url_length
sentry: 1115_projectdebugfile_add_objectstore_columns

social_auth: 0003_social_auth_json_field

Expand Down
1 change: 1 addition & 0 deletions src/sentry/api/endpoints/debug_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ def download(self, debug_file_id, project: Project):
raise Http404

try:
assert debug_file.file is not None
fp = debug_file.file.getfile()
response = StreamingHttpResponse(
iter(lambda: fp.read(4096), b""), content_type="application/octet-stream"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
from django.db import migrations, models

import sentry.db.models.fields.bounded
import sentry.db.models.fields.foreignkey

from sentry.new_migrations.migrations import CheckedMigration


class Migration(CheckedMigration):
is_post_deployment = False

dependencies = [
("sentry", "1114_extend_repository_url_length"),
]

operations = [
migrations.AlterField(
model_name="projectdebugfile",
name="file",
field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(
null=True, on_delete=models.PROTECT, to="sentry.file"
),
),
migrations.AddField(
model_name="projectdebugfile",
name="storage_path",
field=models.TextField(null=True),
),
migrations.AddField(
model_name="projectdebugfile",
name="content_type",
field=models.TextField(null=True),
),
migrations.AddField(
model_name="projectdebugfile",
name="file_size",
field=sentry.db.models.fields.bounded.BoundedBigIntegerField(null=True),
),
migrations.AddField(
model_name="projectdebugfile",
name="date_created",
field=models.DateTimeField(null=True),
),
]
19 changes: 17 additions & 2 deletions src/sentry/models/debugfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ def find_by_debug_ids(
class ProjectDebugFile(Model):
__relocation_scope__ = RelocationScope.Excluded

file = FlexibleForeignKey("sentry.File", on_delete=models.PROTECT)
# When the migration to Objectstore is complete, this can be removed.
file = FlexibleForeignKey("sentry.File", null=True, on_delete=models.PROTECT)
checksum = models.CharField(max_length=40, null=True, db_index=True)
object_name = models.TextField()
cpu_name = models.CharField(max_length=40)
Expand All @@ -129,6 +130,16 @@ class ProjectDebugFile(Model):
data = LegacyTextJSONField(default=dict, null=True)
date_accessed = models.DateTimeField(default=timezone.now, db_default=Now())

# The following fields are present if and only if the file is stored in Objectstore.
# Key of the file in Objectstore.
storage_path = models.TextField(null=True)
# Mirrors `file.headers["Content-Type"]` for files stored in Objectstore.
content_type = models.TextField(null=True)
# Mirrors `file.size` for files stored in Objectstore.
file_size = BoundedBigIntegerField(null=True)
# Mirrors `file.timestamp` for files stored in Objectstore.
date_created = models.DateTimeField(null=True)

objects: ClassVar[ProjectDebugFileManager] = ProjectDebugFileManager()

difcache: ClassVar[DIFCache]
Expand All @@ -145,6 +156,7 @@ class Meta:

@property
def file_format(self) -> str:
assert self.file is not None
ct = self.file.headers.get("Content-Type", "unknown").lower()
return KNOWN_DIF_FORMATS.get(ct, "unknown")

Comment on lines 156 to 162

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.

Bug: Serializing a DebugFile with a None file attribute (for Objectstore-backed DIFs) raises an AssertionError or AttributeError because the serializer does not handle the null case.
Severity: CRITICAL

Suggested Fix

Update DebugFileSerializer to handle cases where obj.file is None. Use obj.content_type and other direct attributes from the DebugFile model for Objectstore-backed DIFs instead of accessing them through the now-nullable file relation. Remove the assert self.file is not None from the file_format property and add logic to handle the None case.

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/models/debugfile.py#L156-L162

Potential issue: The `DebugFileSerializer` attempts to serialize debug information files
(DIFs) by accessing attributes of `obj.file`, such as `obj.file.headers` and
`obj.file.size`. It also calls the `obj.file_format` property. However, for DIFs backed
by Objectstore, the `file` attribute is `None`. The `file_format` property contains an
`assert self.file is not None`, which will raise an `AssertionError`. Direct access to
`obj.file` attributes will raise an `AttributeError`. This causes any API endpoint that
serializes these DIFs, such as the `DebugFilesEndpoint`, to crash.

Also affects:

  • src/sentry/models/debugfile.py:130~136

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

Expand Down Expand Up @@ -201,7 +213,8 @@ def delete(self, *args: Any, **kwargs: Any) -> tuple[int, dict[str, int]]:
# row behind, but no surviving ProjectDebugFile should point to a deleted
# File.
try:
self.file.delete()
if self.file is not None:
self.file.delete()
except ProtectedError:
pass

Expand All @@ -224,6 +237,7 @@ def clean_redundant_difs(project: Project, debug_id: str) -> None:
uuidmap_seen = False
il2cpp_seen = False
for i, dif in enumerate(difs):
assert dif.file is not None
mime_type = dif.file.headers.get("Content-Type")
if mime_type == DIF_MIMETYPES["bcsymbolmap"]:
if not bcsymbolmap_seen:
Comment on lines 237 to 243

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.

Bug: The clean_redundant_difs function unconditionally asserts dif.file is not None, which will fail and crash uploads if any existing DIF for the same debug_id is Objectstore-backed.
Severity: HIGH

Suggested Fix

Modify clean_redundant_difs to correctly handle ProjectDebugFile instances where file is None. Instead of asserting dif.file is not None to get the content type, use dif.content_type which is available on the model directly and supports both file-backed and Objectstore-backed DIFs.

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/models/debugfile.py#L237-L243

Potential issue: The function `clean_redundant_difs` is called after a new debug
information file (DIF) is uploaded. It queries for all existing `ProjectDebugFile`
records with the same `debug_id` and iterates through them. Inside the loop, it
unconditionally runs `assert dif.file is not None`. If a pre-existing DIF for that
`debug_id` is backed by Objectstore, its `file` attribute will be `None`, causing the
assertion to fail. This will crash the upload process for any new DIF that shares a
`debug_id` with an Objectstore-backed DIF.

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

Expand Down Expand Up @@ -733,6 +747,7 @@ def fetch_difs(
except OSError as e:
if e.errno != errno.ENOENT:
raise
assert dif.file is not None
dif.file.save_to(dif_path)
rv[debug_id] = dif_path

Expand Down
2 changes: 2 additions & 0 deletions tests/sentry/tasks/test_assemble.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ def test_dif(self) -> None:
project_id=self.project.id, checksum=total_checksum
).get()

assert dif.file is not None
assert dif.file.headers == {"Content-Type": "text/x-breakpad"}

def test_assemble_from_files(self) -> None:
Expand Down Expand Up @@ -203,6 +204,7 @@ def test_assemble_debug_id_override(self) -> None:
project_id=self.project.id, checksum=total_checksum
).get()

assert dif.file is not None
assert dif.file.headers == {"Content-Type": "text/x-breakpad"}
assert dif.debug_id == "67e9247c-814e-392b-a027-dbde6748fcbf-beef"

Expand Down
Loading