From f2197dd19365dc633db241d798f4ea1059cd9fbb Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Tue, 25 Nov 2025 22:16:19 -0500 Subject: [PATCH 1/5] fix --- .../preprod/vcs/status_checks/size/tasks.py | 77 +++++++-- .../size/test_status_checks_tasks.py | 163 +++++++++++++++++- 2 files changed, 226 insertions(+), 14 deletions(-) diff --git a/src/sentry/preprod/vcs/status_checks/size/tasks.py b/src/sentry/preprod/vcs/status_checks/size/tasks.py index 3d62fab9112733..cdf6d0566ddd16 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -3,6 +3,7 @@ import logging from abc import ABC, abstractmethod from datetime import datetime +from enum import StrEnum from typing import Any from sentry.constants import ObjectStatus @@ -129,19 +130,24 @@ def create_preprod_status_check_task(preprod_artifact_id: int) -> None: if GITHUB_STATUS_CHECK_STATUS_MAPPING[status] == GitHubCheckStatus.COMPLETED: completed_at = preprod_artifact.date_updated - check_id = provider.create_status_check( - repo=commit_comparison.head_repo_name, - sha=commit_comparison.head_sha, - status=status, - title=title, - subtitle=subtitle, - text=None, # TODO(telkins): add text field support - summary=summary, - external_id=str(preprod_artifact.id), - target_url=target_url, - started_at=preprod_artifact.date_added, - completed_at=completed_at, - ) + try: + check_id = provider.create_status_check( + repo=commit_comparison.head_repo_name, + sha=commit_comparison.head_sha, + status=status, + title=title, + subtitle=subtitle, + text=None, # TODO(telkins): add text field support + summary=summary, + external_id=str(preprod_artifact.id), + target_url=target_url, + started_at=preprod_artifact.date_added, + completed_at=completed_at, + ) + except Exception as e: + _update_posted_status_check(preprod_artifact, success=False, error=e) + raise + if check_id is None: logger.error( "preprod.status_checks.create.failed", @@ -151,8 +157,11 @@ def create_preprod_status_check_task(preprod_artifact_id: int) -> None: "organization_slug": preprod_artifact.project.organization.slug, }, ) + _update_posted_status_check(preprod_artifact, success=False) return + _update_posted_status_check(preprod_artifact, success=True, check_id=check_id) + logger.info( "preprod.status_checks.create.success", extra={ @@ -165,6 +174,48 @@ def create_preprod_status_check_task(preprod_artifact_id: int) -> None: ) +def _update_posted_status_check( + preprod_artifact: PreprodArtifact, + success: bool, + check_id: str | None = None, + error: Exception | None = None, +) -> None: + """Update the posted_status_check field in the artifact's extras.""" + extras = preprod_artifact.extras or {} + + posted_status_check: dict[str, Any] = {"success": success} + if success and check_id: + posted_status_check["check_id"] = check_id + if not success: + posted_status_check["error_type"] = _get_error_type(error).value + + extras["posted_status_check"] = posted_status_check + preprod_artifact.extras = extras + preprod_artifact.save(update_fields=["extras"]) + + +def _get_error_type(error: Exception | None) -> StatusCheckErrorType: + """Determine the error type from an exception.""" + if error is None: + return StatusCheckErrorType.UNKNOWN + if isinstance(error, IntegrationConfigurationError): + return StatusCheckErrorType.INTEGRATION_ERROR + if isinstance(error, ApiError): + return StatusCheckErrorType.API_ERROR + return StatusCheckErrorType.UNKNOWN + + +class StatusCheckErrorType(StrEnum): + """Error types for status check creation failures.""" + + UNKNOWN = "unknown" + """An unknown error occurred (e.g., API returned null check_id).""" + API_ERROR = "api_error" + """A retryable API error (5xx, rate limit, transient issues).""" + INTEGRATION_ERROR = "integration_error" + """An integration configuration error (permissions, invalid request, etc.).""" + + def _compute_overall_status( artifacts: list[PreprodArtifact], size_metrics_map: dict[int, list[PreprodArtifactSizeMetrics]] ) -> StatusCheckStatus: diff --git a/tests/sentry/preprod/vcs/status_checks/size/test_status_checks_tasks.py b/tests/sentry/preprod/vcs/status_checks/size/test_status_checks_tasks.py index d21673a18f1af4..272206283a619b 100644 --- a/tests/sentry/preprod/vcs/status_checks/size/test_status_checks_tasks.py +++ b/tests/sentry/preprod/vcs/status_checks/size/test_status_checks_tasks.py @@ -11,7 +11,10 @@ from sentry.models.commitcomparison import CommitComparison from sentry.models.repository import Repository from sentry.preprod.models import PreprodArtifact, PreprodArtifactSizeMetrics -from sentry.preprod.vcs.status_checks.size.tasks import create_preprod_status_check_task +from sentry.preprod.vcs.status_checks.size.tasks import ( + StatusCheckErrorType, + create_preprod_status_check_task, +) from sentry.shared_integrations.exceptions import IntegrationConfigurationError from sentry.testutils.cases import TestCase from sentry.testutils.silo import region_silo_test @@ -988,3 +991,161 @@ def test_sibling_deduplication_with_same_app_id_different_platforms(self): assert ( ios_artifact.id not in sibling_ids_from_ios_new ), "Old iOS artifact should be deduplicated (not the triggering artifact)" + + def test_posted_status_check_success(self): + """Test that successful status check posts are recorded in artifact extras.""" + preprod_artifact = self._create_preprod_artifact( + state=PreprodArtifact.ArtifactState.PROCESSED + ) + + PreprodArtifactSizeMetrics.objects.create( + preprod_artifact=preprod_artifact, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + state=PreprodArtifactSizeMetrics.SizeAnalysisState.COMPLETED, + min_download_size=1024 * 1024, + max_download_size=1024 * 1024, + min_install_size=2 * 1024 * 1024, + max_install_size=2 * 1024 * 1024, + ) + + _, mock_provider, client_patch, provider_patch = self._create_working_status_check_setup( + preprod_artifact + ) + mock_provider.create_status_check.return_value = "check_12345" + + with client_patch, provider_patch: + with self.tasks(): + create_preprod_status_check_task(preprod_artifact.id) + + preprod_artifact.refresh_from_db() + assert preprod_artifact.extras is not None + assert "posted_status_check" in preprod_artifact.extras + assert preprod_artifact.extras["posted_status_check"]["success"] is True + assert preprod_artifact.extras["posted_status_check"]["check_id"] == "check_12345" + + def test_posted_status_check_failure_null_check_id(self): + """Test that failed status check posts (null check_id) are recorded in artifact extras.""" + preprod_artifact = self._create_preprod_artifact( + state=PreprodArtifact.ArtifactState.PROCESSED + ) + + PreprodArtifactSizeMetrics.objects.create( + preprod_artifact=preprod_artifact, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + state=PreprodArtifactSizeMetrics.SizeAnalysisState.COMPLETED, + min_download_size=1024 * 1024, + max_download_size=1024 * 1024, + min_install_size=2 * 1024 * 1024, + max_install_size=2 * 1024 * 1024, + ) + + _, mock_provider, client_patch, provider_patch = self._create_working_status_check_setup( + preprod_artifact + ) + mock_provider.create_status_check.return_value = None # Simulate API returning no check_id + + with client_patch, provider_patch: + with self.tasks(): + create_preprod_status_check_task(preprod_artifact.id) + + preprod_artifact.refresh_from_db() + assert preprod_artifact.extras is not None + assert "posted_status_check" in preprod_artifact.extras + assert preprod_artifact.extras["posted_status_check"]["success"] is False + assert ( + preprod_artifact.extras["posted_status_check"]["error_type"] + == StatusCheckErrorType.UNKNOWN.value + ) + + @responses.activate + def test_posted_status_check_failure_integration_error(self): + """Test that integration errors during status check creation are recorded in artifact extras.""" + preprod_artifact = self._create_preprod_artifact( + state=PreprodArtifact.ArtifactState.PROCESSED + ) + + PreprodArtifactSizeMetrics.objects.create( + preprod_artifact=preprod_artifact, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + state=PreprodArtifactSizeMetrics.SizeAnalysisState.COMPLETED, + min_download_size=1024 * 1024, + max_download_size=1024 * 1024, + min_install_size=2 * 1024 * 1024, + max_install_size=2 * 1024 * 1024, + ) + + integration = self.create_integration( + organization=self.organization, + external_id="test-integration-error", + provider="github", + metadata={"access_token": "test_token", "expires_at": "2099-01-01T00:00:00Z"}, + ) + + Repository.objects.create( + organization_id=self.organization.id, + name="owner/repo", + provider="integrations:github", + integration_id=integration.id, + ) + + responses.add( + responses.POST, + "https://api.github.com/repos/owner/repo/check-runs", + status=403, + json={ + "message": "Resource not accessible by integration", + "documentation_url": "https://docs.github.com/rest/checks/runs#create-a-check-run", + }, + ) + + with self.tasks(): + try: + create_preprod_status_check_task(preprod_artifact.id) + except IntegrationConfigurationError: + pass # Expected + + preprod_artifact.refresh_from_db() + assert preprod_artifact.extras is not None + assert "posted_status_check" in preprod_artifact.extras + assert preprod_artifact.extras["posted_status_check"]["success"] is False + assert ( + preprod_artifact.extras["posted_status_check"]["error_type"] + == StatusCheckErrorType.INTEGRATION_ERROR.value + ) + + def test_posted_status_check_preserves_existing_extras(self): + """Test that recording status check result preserves other fields in extras.""" + preprod_artifact = self._create_preprod_artifact( + state=PreprodArtifact.ArtifactState.PROCESSED + ) + preprod_artifact.extras = {"existing_field": "existing_value", "another_field": 123} + preprod_artifact.save() + + PreprodArtifactSizeMetrics.objects.create( + preprod_artifact=preprod_artifact, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + state=PreprodArtifactSizeMetrics.SizeAnalysisState.COMPLETED, + min_download_size=1024 * 1024, + max_download_size=1024 * 1024, + min_install_size=2 * 1024 * 1024, + max_install_size=2 * 1024 * 1024, + ) + + _, mock_provider, client_patch, provider_patch = self._create_working_status_check_setup( + preprod_artifact + ) + mock_provider.create_status_check.return_value = "check_67890" + + with client_patch, provider_patch: + with self.tasks(): + create_preprod_status_check_task(preprod_artifact.id) + + preprod_artifact.refresh_from_db() + assert preprod_artifact.extras is not None + # Verify existing fields are preserved + assert preprod_artifact.extras["existing_field"] == "existing_value" + assert preprod_artifact.extras["another_field"] == 123 + # Verify new field is added + assert "posted_status_check" in preprod_artifact.extras + assert preprod_artifact.extras["posted_status_check"]["success"] is True + assert preprod_artifact.extras["posted_status_check"]["check_id"] == "check_67890" From 8a83890e17129a09b5b4f59f2c5a8a413d6ca06f Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Tue, 25 Nov 2025 22:44:38 -0500 Subject: [PATCH 2/5] fix --- src/sentry/preprod/vcs/status_checks/size/tasks.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/preprod/vcs/status_checks/size/tasks.py b/src/sentry/preprod/vcs/status_checks/size/tasks.py index cdf6d0566ddd16..457aaab6292fcf 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -181,6 +181,7 @@ def _update_posted_status_check( error: Exception | None = None, ) -> None: """Update the posted_status_check field in the artifact's extras.""" + preprod_artifact.refresh_from_db(fields=["extras"]) extras = preprod_artifact.extras or {} posted_status_check: dict[str, Any] = {"success": success} From e52129a3d532c2e06a2164830713a07861f8a815 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Wed, 26 Nov 2025 11:25:20 -0500 Subject: [PATCH 3/5] transaction --- .../preprod/vcs/status_checks/size/tasks.py | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/sentry/preprod/vcs/status_checks/size/tasks.py b/src/sentry/preprod/vcs/status_checks/size/tasks.py index 457aaab6292fcf..18b4eb608b22f7 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -6,6 +6,8 @@ from enum import StrEnum from typing import Any +from django.db import router, transaction + from sentry.constants import ObjectStatus from sentry.integrations.base import IntegrationInstallation from sentry.integrations.github.status_check import GitHubCheckConclusion, GitHubCheckStatus @@ -181,18 +183,19 @@ def _update_posted_status_check( error: Exception | None = None, ) -> None: """Update the posted_status_check field in the artifact's extras.""" - preprod_artifact.refresh_from_db(fields=["extras"]) - extras = preprod_artifact.extras or {} - - posted_status_check: dict[str, Any] = {"success": success} - if success and check_id: - posted_status_check["check_id"] = check_id - if not success: - posted_status_check["error_type"] = _get_error_type(error).value - - extras["posted_status_check"] = posted_status_check - preprod_artifact.extras = extras - preprod_artifact.save(update_fields=["extras"]) + with transaction.atomic(router.db_for_write(PreprodArtifact)): + artifact = PreprodArtifact.objects.select_for_update().get(id=preprod_artifact.id) + extras = artifact.extras or {} + + posted_status_check: dict[str, Any] = {"success": success} + if success and check_id: + posted_status_check["check_id"] = check_id + if not success: + posted_status_check["error_type"] = _get_error_type(error).value + + extras["posted_status_check"] = posted_status_check + artifact.extras = extras + artifact.save(update_fields=["extras"]) def _get_error_type(error: Exception | None) -> StatusCheckErrorType: From 17bbe6397abf58902201de6419c755a146a01d54 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Wed, 26 Nov 2025 11:32:56 -0500 Subject: [PATCH 4/5] store dict --- .../preprod/vcs/status_checks/size/tasks.py | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/sentry/preprod/vcs/status_checks/size/tasks.py b/src/sentry/preprod/vcs/status_checks/size/tasks.py index 18b4eb608b22f7..955ec97845d0b6 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -147,7 +147,7 @@ def create_preprod_status_check_task(preprod_artifact_id: int) -> None: completed_at=completed_at, ) except Exception as e: - _update_posted_status_check(preprod_artifact, success=False, error=e) + _update_posted_status_check(preprod_artifact, check_type="size", success=False, error=e) raise if check_id is None: @@ -159,10 +159,12 @@ def create_preprod_status_check_task(preprod_artifact_id: int) -> None: "organization_slug": preprod_artifact.project.organization.slug, }, ) - _update_posted_status_check(preprod_artifact, success=False) + _update_posted_status_check(preprod_artifact, check_type="size", success=False) return - _update_posted_status_check(preprod_artifact, success=True, check_id=check_id) + _update_posted_status_check( + preprod_artifact, check_type="size", success=True, check_id=check_id + ) logger.info( "preprod.status_checks.create.success", @@ -178,22 +180,26 @@ def create_preprod_status_check_task(preprod_artifact_id: int) -> None: def _update_posted_status_check( preprod_artifact: PreprodArtifact, + check_type: str, success: bool, check_id: str | None = None, error: Exception | None = None, ) -> None: - """Update the posted_status_check field in the artifact's extras.""" + """Update the posted_status_checks field in the artifact's extras.""" with transaction.atomic(router.db_for_write(PreprodArtifact)): artifact = PreprodArtifact.objects.select_for_update().get(id=preprod_artifact.id) extras = artifact.extras or {} - posted_status_check: dict[str, Any] = {"success": success} + posted_status_checks = extras.get("posted_status_checks", {}) + + check_result: dict[str, Any] = {"success": success} if success and check_id: - posted_status_check["check_id"] = check_id + check_result["check_id"] = check_id if not success: - posted_status_check["error_type"] = _get_error_type(error).value + check_result["error_type"] = _get_error_type(error).value - extras["posted_status_check"] = posted_status_check + posted_status_checks[check_type] = check_result + extras["posted_status_checks"] = posted_status_checks artifact.extras = extras artifact.save(update_fields=["extras"]) From 34d09ed4dd6843611d060278281daa9f1d1334f6 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Wed, 26 Nov 2025 11:45:21 -0500 Subject: [PATCH 5/5] tests --- .../size/test_status_checks_tasks.py | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/tests/sentry/preprod/vcs/status_checks/size/test_status_checks_tasks.py b/tests/sentry/preprod/vcs/status_checks/size/test_status_checks_tasks.py index 272206283a619b..53e22548a79105 100644 --- a/tests/sentry/preprod/vcs/status_checks/size/test_status_checks_tasks.py +++ b/tests/sentry/preprod/vcs/status_checks/size/test_status_checks_tasks.py @@ -1019,9 +1019,10 @@ def test_posted_status_check_success(self): preprod_artifact.refresh_from_db() assert preprod_artifact.extras is not None - assert "posted_status_check" in preprod_artifact.extras - assert preprod_artifact.extras["posted_status_check"]["success"] is True - assert preprod_artifact.extras["posted_status_check"]["check_id"] == "check_12345" + assert "posted_status_checks" in preprod_artifact.extras + assert "size" in preprod_artifact.extras["posted_status_checks"] + assert preprod_artifact.extras["posted_status_checks"]["size"]["success"] is True + assert preprod_artifact.extras["posted_status_checks"]["size"]["check_id"] == "check_12345" def test_posted_status_check_failure_null_check_id(self): """Test that failed status check posts (null check_id) are recorded in artifact extras.""" @@ -1050,10 +1051,11 @@ def test_posted_status_check_failure_null_check_id(self): preprod_artifact.refresh_from_db() assert preprod_artifact.extras is not None - assert "posted_status_check" in preprod_artifact.extras - assert preprod_artifact.extras["posted_status_check"]["success"] is False + assert "posted_status_checks" in preprod_artifact.extras + assert "size" in preprod_artifact.extras["posted_status_checks"] + assert preprod_artifact.extras["posted_status_checks"]["size"]["success"] is False assert ( - preprod_artifact.extras["posted_status_check"]["error_type"] + preprod_artifact.extras["posted_status_checks"]["size"]["error_type"] == StatusCheckErrorType.UNKNOWN.value ) @@ -1106,10 +1108,11 @@ def test_posted_status_check_failure_integration_error(self): preprod_artifact.refresh_from_db() assert preprod_artifact.extras is not None - assert "posted_status_check" in preprod_artifact.extras - assert preprod_artifact.extras["posted_status_check"]["success"] is False + assert "posted_status_checks" in preprod_artifact.extras + assert "size" in preprod_artifact.extras["posted_status_checks"] + assert preprod_artifact.extras["posted_status_checks"]["size"]["success"] is False assert ( - preprod_artifact.extras["posted_status_check"]["error_type"] + preprod_artifact.extras["posted_status_checks"]["size"]["error_type"] == StatusCheckErrorType.INTEGRATION_ERROR.value ) @@ -1146,6 +1149,7 @@ def test_posted_status_check_preserves_existing_extras(self): assert preprod_artifact.extras["existing_field"] == "existing_value" assert preprod_artifact.extras["another_field"] == 123 # Verify new field is added - assert "posted_status_check" in preprod_artifact.extras - assert preprod_artifact.extras["posted_status_check"]["success"] is True - assert preprod_artifact.extras["posted_status_check"]["check_id"] == "check_67890" + assert "posted_status_checks" in preprod_artifact.extras + assert "size" in preprod_artifact.extras["posted_status_checks"] + assert preprod_artifact.extras["posted_status_checks"]["size"]["success"] is True + assert preprod_artifact.extras["posted_status_checks"]["size"]["check_id"] == "check_67890"