ref(difs): Clean up Objectstore write path after review

Simplify parallel multipart upload: use offset-based reads for correct
part ordering, return just storage_path instead of a redundant tuple,
and inline the retry helper. Restructure ProGuard clone to handle
file vs storage_path explicitly. Simplify maybe_renew_debug_files TTI
bump to warn-and-continue instead of skipping renewal. Remove tests
superseded by integration coverage.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: lcian

src/sentry/api/endpoints/debug_files.py

@@ -836,17 +836,18 @@ def _clone_proguard_debug_file_for_reupload(
     meta = build_proguard_reupload_dif_meta(debug_file, requested_debug_id)
     if debug_file.file is not None:
         dif, created = create_dif_from_id(project, meta, file=debug_file.file)
-    else:
-        # Objectstore streams are not seekable, but create_dif_from_id needs to
-        # read the file twice (checksum + upload). Buffer into a temp file.
+    if debug_file.storage_path is not None:
         source_fileobj = debug_file.getfile()
         try:
+            # Spool into a temporary file to get a seekable stream.
             with tempfile.SpooledTemporaryFile(max_size=5 * 1024 * 1024) as tmp:
                 shutil.copyfileobj(source_fileobj, tmp)
                 tmp.seek(0)
                 dif, created = create_dif_from_id(project, meta, fileobj=tmp)
         finally:
             source_fileobj.close()
+    else:
+        raise ValueError("ProjectDebugFile has neither file nor storage_path")
 
     if created:
         record_last_upload(project)

src/sentry/debug_files/debug_files.py

@@ -23,30 +23,31 @@ def maybe_renew_debug_files(debug_files: Sequence[ProjectDebugFile]) -> None:
         days=options.get("system.debug-files-renewal-age-threshold-days")
     )
 
+    # We first check if any file needs renewal, before going to the database.
+    needs_bump = any(dif.date_accessed <= threshold_date for dif in debug_files)
+    if not needs_bump:
+        return
+
     ids_to_renew = []
     for dif in debug_files:
         if dif.date_accessed > threshold_date:
             continue
+        ids_to_renew.append(dif.id)
 
-        # For Objectstore-backed files, issue a HEAD request to bump the TTI.
-        if dif.storage_path is not None:
+        # For Objectstore-backed files, issue a HEAD request to bump TTI.
+        if dif.storage_path is not None and dif.date_accessed <= threshold_date:
             try:
                 dif._get_objectstore_session().head(dif.storage_path)
             except Exception:
-                logger.exception(
-                    "debugfile.objectstore_tti_renewal_failed",
+                logger.warning(
+                    "Failed to bump TTI for debug file",
                     extra={
-                        "project_debug_file_id": dif.id,
+                        "dif_id": dif.id,
                         "project_id": dif.project_id,
                         "storage_path": dif.storage_path,
                     },
+                    exc_info=True,
                 )
-                continue
-
-        ids_to_renew.append(dif.id)
-
-    if not ids_to_renew:
-        return
 
     # Update `date_accessed` in the db.
     with metrics.timer("debug_files_renewal"):

src/sentry/demo_mode/tasks.py

@@ -270,8 +270,6 @@ def _sync_project_debug_file(
                 date_accessed=source_project_debug_file.date_accessed,
             )
     except IntegrityError as e:
-        if target_project is not None and target_storage_path is not None:
-            get_debug_files_session(target_org.id, target_project.id).delete(target_storage_path)
         sentry_sdk.capture_exception(e)
         return None
 

src/sentry/models/debugfile.py

@@ -44,6 +44,7 @@
 from sentry.models.files.utils import MAX_FILE_SIZE, clear_cached_files
 from sentry.objectstore import get_debug_files_session
 from sentry.utils import json, metrics
+from sentry.utils.concurrent import ContextPropagatingThreadPoolExecutor
 from sentry.utils.zip import safe_extract_zip
 
 if TYPE_CHECKING:
@@ -54,12 +55,11 @@
 logger = logging.getLogger(__name__)
 
 DIF_MIMETYPES = {v: k for k, v in KNOWN_DIF_FORMATS.items()}
-OBJECTSTORE_MULTIPART_PART_SIZE = 32 * 1024 * 1024
-OBJECTSTORE_MULTIPART_MAX_RETRIES = 3
-OBJECTSTORE_MULTIPART_MAX_WORKERS = 4
 
 _proguard_file_re = re.compile(r"/proguard/(?:mapping-)?(.*?)\.txt$")
 
+OBJECTSTORE_MULTIPART_PART_SIZE = 32 * 1024 * 1024  # 32 MiB
+
 
 class BadDif(Exception):
     pass
@@ -168,7 +168,6 @@ class Meta:
     __repr__ = sane_repr("object_name", "cpu_name", "debug_id")
 
     def get_checksum(self) -> str:
-        """Returns the SHA-1 checksum, reading from the denormalized column or the linked File."""
         if self.storage_path is not None:
             assert self.checksum is not None
             return self.checksum
@@ -178,7 +177,6 @@ def get_checksum(self) -> str:
         raise ValueError("ProjectDebugFile has neither file nor storage_path")
 
     def get_content_type(self) -> str:
-        """Returns the MIME content type, reading from the denormalized column or the linked File."""
         if self.storage_path is not None:
             assert self.content_type is not None
             return str(self.content_type)
@@ -187,7 +185,6 @@ def get_content_type(self) -> str:
         raise ValueError("ProjectDebugFile has neither file nor storage_path")
 
     def get_file_size(self) -> int:
-        """Returns the file size in bytes, reading from the denormalized column or the linked File."""
         if self.storage_path is not None:
             assert self.file_size is not None
             return int(self.file_size)
@@ -196,7 +193,6 @@ def get_file_size(self) -> int:
         raise ValueError("ProjectDebugFile has neither file nor storage_path")
 
     def get_date_created(self) -> datetime:
-        """Returns the creation timestamp, reading from the denormalized column or the linked File."""
         if self.storage_path is not None:
             assert self.date_created is not None
             return self.date_created
@@ -205,7 +201,6 @@ def get_date_created(self) -> datetime:
         raise ValueError("ProjectDebugFile has neither file nor storage_path")
 
     def get_headers(self) -> dict[str, str]:
-        """Returns file headers (currently just Content-Type), from the denormalized column or the linked File."""
         if self.storage_path is not None:
             assert self.content_type is not None
             return {"Content-Type": self.content_type}
@@ -264,14 +259,12 @@ def features(self) -> frozenset[str]:
         return frozenset((self.data or {}).get("features", []))
 
     def _get_objectstore_session(self) -> Session:
-        """Returns an Objectstore session scoped to this debug file's project."""
         from sentry.models.project import Project
 
         org_id = Project.objects.get_from_cache(id=self.project_id).organization_id
         return get_debug_files_session(org=org_id, project=self.project_id)
 
     def getfile(self) -> IO[bytes]:
-        """Returns a file-like object with the debug file contents, from Objectstore or the linked File."""
         if self.storage_path is not None:
             try:
                 response = self._get_objectstore_session().get(self.storage_path)
@@ -284,7 +277,6 @@ def getfile(self) -> IO[bytes]:
         raise ValueError("ProjectDebugFile has neither file nor storage_path")
 
     def save_to(self, path: str) -> None:
-        """Downloads the debug file contents to a local path, atomically via a temp file."""
         if self.storage_path is not None:
             path = os.path.abspath(path)
             base = os.path.dirname(path)
@@ -404,57 +396,48 @@ def create_dif_from_file(
     return create_dif_from_id(project, result[0], file=file)
 
 
-def _put_objectstore_part_with_retry(
-    upload: MultipartUpload, chunk: bytes, part_number: int
-) -> CompletePart:
-    """Uploads a single multipart part, retrying with exponential backoff on transient errors."""
-    for attempt in range(OBJECTSTORE_MULTIPART_MAX_RETRIES):
-        try:
-            return upload.put_part(chunk, part_number=part_number, content_length=len(chunk))
-        except (RequestError, HTTPError):
-            if attempt == OBJECTSTORE_MULTIPART_MAX_RETRIES - 1:
-                raise
-            time.sleep(2**attempt)
-
-    raise AssertionError("unreachable")
-
-
 def _upload_dif_to_objectstore(
     session: Session,
     fileobj: IO[bytes],
     content_type: str,
     file_size: int,
-) -> tuple[str, int]:
-    """Uploads a debug file to Objectstore via parallel multipart upload, aborting on failure.
-
-    Parts are uploaded concurrently with up to OBJECTSTORE_MULTIPART_MAX_WORKERS threads.
-    Each worker reads its chunk from fileobj under a lock, so memory usage is bounded to
-    roughly max_workers * OBJECTSTORE_MULTIPART_PART_SIZE.
-
-    Returns (storage_path, uploaded_size).
-    """
-    from sentry.utils.concurrent import ContextPropagatingThreadPoolExecutor
-
+) -> str:
+    """Uploads a debug file to Objectstore via parallel multipart upload, returning the key under which the file was uploaded."""
     upload = session.initiate_multipart_upload(content_type=content_type)
-    read_lock = threading.Lock()
+
+    lock = threading.Lock()
     num_parts = max(1, math.ceil(file_size / OBJECTSTORE_MULTIPART_PART_SIZE))
 
-    def _read_and_upload_part(part_number: int) -> CompletePart | None:
-        with read_lock:
+    def put_part_with_retry(
+        upload: MultipartUpload, chunk: bytes, part_number: int
+    ) -> CompletePart:
+        for attempt in range(3):
+            try:
+                return upload.put_part(chunk, part_number=part_number, content_length=len(chunk))
+            except (RequestError, HTTPError):
+                if attempt == 2:
+                    raise
+                time.sleep(2**attempt)
+        raise AssertionError("unreachable")
+
+    def read_and_put_part(part_number: int) -> CompletePart | None:
+        offset = (part_number - 1) * OBJECTSTORE_MULTIPART_PART_SIZE
+        with lock:
+            fileobj.seek(offset)
             chunk = fileobj.read(OBJECTSTORE_MULTIPART_PART_SIZE)
         if not chunk:
             return None
-        return _put_objectstore_part_with_retry(upload, chunk, part_number)
+        return put_part_with_retry(upload, chunk, part_number)
 
     try:
         with ContextPropagatingThreadPoolExecutor(
-            max_workers=OBJECTSTORE_MULTIPART_MAX_WORKERS
+            max_workers=4,
         ) as executor:
-            futures = [executor.submit(_read_and_upload_part, i + 1) for i in range(num_parts)]
+            futures = [executor.submit(read_and_put_part, i + 1) for i in range(num_parts)]
             parts = [part for f in futures if (part := f.result()) is not None]
 
         storage_path = upload.complete(parts)
-        return storage_path, file_size
+        return storage_path
     except Exception:
         try:
             upload.abort()
@@ -554,19 +537,17 @@ def create_dif_from_id(
         session = get_debug_files_session(project.organization_id, project.id)
         if file is not None:
             with file.getfile() as source_fileobj:
-                storage_path, uploaded_file_size = _upload_dif_to_objectstore(
+                storage_path = _upload_dif_to_objectstore(
                     session, source_fileobj, content_type, file_size
                 )
         elif fileobj is not None:
-            storage_path, uploaded_file_size = _upload_dif_to_objectstore(
-                session, fileobj, content_type, file_size
-            )
+            storage_path = _upload_dif_to_objectstore(session, fileobj, content_type, file_size)
         else:
             raise RuntimeError("missing file object")
 
         metrics.distribution(
             "storage.put.size",
-            uploaded_file_size,
+            file_size,
             tags={"usecase": "debug-files", "compression": "none"},
             unit="byte",
         )
@@ -575,7 +556,7 @@ def create_dif_from_id(
             file=None,
             storage_path=storage_path,
             content_type=content_type,
-            file_size=uploaded_file_size,
+            file_size=file_size,
             date_created=timezone.now(),
             checksum=checksum,
             debug_id=meta.debug_id,

tests/sentry/api/endpoints/test_debug_files.py

@@ -1,11 +1,9 @@
 import zipfile
 from io import BytesIO
-from unittest.mock import patch
 from uuid import uuid4
 
 from django.core.files.uploadedfile import SimpleUploadedFile
 from django.urls import reverse
-from django.utils import timezone
 
 from sentry.models.debugfile import ProjectDebugFile
 from sentry.models.files.file import File
@@ -157,36 +155,6 @@ def test_access_control(self) -> None:
         response = self.client.get(f"{url}?id={download_id}")
         assert response.status_code == 404
 
-    def test_download_closes_streaming_file(self) -> None:
-        class CloseTrackingBytesIO(BytesIO):
-            was_closed = False
-
-            def close(self) -> None:
-                self.was_closed = True
-                super().close()
-
-        debug_file = ProjectDebugFile.objects.create(
-            project_id=self.project.id,
-            file=None,
-            storage_path="debug-files/key",
-            content_type="text/x-proguard+plain",
-            file_size=len(PROGUARD_SOURCE),
-            date_created=timezone.now(),
-            checksum="e6d3c5185dac63eddfdc1a5edfffa32d46103b44",
-            object_name="proguard-mapping",
-            cpu_name="any",
-            debug_id=PROGUARD_UUID,
-            data={"features": ["mapping"]},
-        )
-        payload = CloseTrackingBytesIO(PROGUARD_SOURCE)
-
-        with patch.object(ProjectDebugFile, "getfile", return_value=payload):
-            response = self.client.get(f"{self.url}?id={debug_file.id}")
-            assert response.status_code == 200, response.content
-            assert close_streaming_response(response) == PROGUARD_SOURCE
-
-        assert payload.was_closed
-
     def test_dsyms_requests(self) -> None:
         response = self._upload_proguard(self.url, PROGUARD_UUID)
         assert response.status_code == 201, response.content