From c3b708cec17dbcad9b487eb5eec999234345794d Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Fri, 7 Nov 2025 15:11:37 -0500 Subject: [PATCH 1/5] fix --- .../preprod/vcs/status_checks/size/tasks.py | 29 ++++- .../size/test_status_checks_tasks.py | 111 ++++++++++++++++++ 2 files changed, 139 insertions(+), 1 deletion(-) diff --git a/src/sentry/preprod/vcs/status_checks/size/tasks.py b/src/sentry/preprod/vcs/status_checks/size/tasks.py index 8dc40980600a16..102478f1657276 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -24,6 +24,7 @@ from sentry.preprod.models import PreprodArtifact, PreprodArtifactSizeMetrics from sentry.preprod.url_utils import get_preprod_artifact_url from sentry.preprod.vcs.status_checks.size.templates import format_status_check_messages +from sentry.shared_integrations.exceptions import ApiForbiddenError, IntegrationConfigurationError from sentry.silo.base import SiloMode from sentry.tasks.base import instrumented_task from sentry.taskworker.namespaces import integrations_tasks @@ -36,7 +37,7 @@ name="sentry.preprod.tasks.create_preprod_status_check", namespace=integrations_tasks, processing_deadline_duration=30, - retry=Retry(times=3), + retry=Retry(times=3, ignore=(IntegrationConfigurationError,)), silo_mode=SiloMode.REGION, ) def create_preprod_status_check_task(preprod_artifact_id: int) -> None: @@ -351,6 +352,32 @@ def create_status_check( response = self.client.create_check_run(repo=repo, data=check_data) check_id = response.get("id") return str(check_id) if check_id else None + except ApiForbiddenError as e: + lifecycle.record_failure(e) + + # Check if this is a permission error + error_message = str(e).lower() + if ( + "resource not accessible" in error_message + or "insufficient" in error_message + or "permission" in error_message + ): + logger.exception( + "preprod.status_checks.create.insufficient_permissions", + extra={ + "organization_id": self.organization_id, + "integration_id": self.integration_id, + "repo": repo, + }, + ) + raise IntegrationConfigurationError( + "GitHub App lacks permissions to create check runs. " + "Please ensure the app has the required permissions and that " + "the organization has accepted any updated permissions." + ) from e + + # For other 403 errors, re-raise the original exception + raise except Exception as e: lifecycle.record_failure(e) return None 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 19c6b69f12e038..e61dcbc076d70a 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 @@ -3,11 +3,14 @@ import uuid from unittest.mock import Mock, patch +import responses + from sentry.integrations.source_code_management.status_check import StatusCheckStatus 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.shared_integrations.exceptions import ApiForbiddenError, IntegrationConfigurationError from sentry.testutils.cases import TestCase from sentry.testutils.silo import region_silo_test @@ -445,3 +448,111 @@ def test_create_preprod_status_check_task_mixed_states_monorepo(self): assert "com.example.uploading" in summary assert "com.example.failed" in summary assert "Upload timeout" in summary + + @responses.activate + def test_create_preprod_status_check_task_github_permission_error(self): + """Test task handles GitHub permission errors without retrying.""" + 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="123", + 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) + assert False, "Expected IntegrationConfigurationError to be raised" + except IntegrationConfigurationError as e: + assert "GitHub App lacks permissions" in str(e) + assert "required permissions" in str(e) + + # Verify no retries due to ignore policy + assert len(responses.calls) == 1 + assert ( + responses.calls[0].request.url == "https://api.github.com/repos/owner/repo/check-runs" + ) + + @responses.activate + def test_create_preprod_status_check_task_github_non_permission_403(self): + """Test task re-raises non-permission 403 errors (for potential retry).""" + 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="456", + 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, + ) + + # 403 error that's NOT permission-related (should be re-raised, not converted) + responses.add( + responses.POST, + "https://api.github.com/repos/owner/repo/check-runs", + status=403, + json={ + "message": "Repository is temporarily unavailable", + }, + ) + + with self.tasks(): + try: + create_preprod_status_check_task(preprod_artifact.id) + assert False, "Expected ApiForbiddenError to be raised" + except ApiForbiddenError as e: + assert "temporarily unavailable" in str(e) + + assert len(responses.calls) == 1 + assert ( + responses.calls[0].request.url == "https://api.github.com/repos/owner/repo/check-runs" + ) From d1859e938765969dd1a9c779f75a20363f6fd05e Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Fri, 7 Nov 2025 15:28:01 -0500 Subject: [PATCH 2/5] fix --- .../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 102478f1657276..05bee7c3a3881f 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -354,8 +354,8 @@ def create_status_check( return str(check_id) if check_id else None except ApiForbiddenError as e: lifecycle.record_failure(e) - - # Check if this is a permission error + # 403 errors are typically not transient (permission or configuration issues) + # so we convert them to IntegrationConfigurationError to prevent retries error_message = str(e).lower() if ( "resource not accessible" in error_message @@ -375,12 +375,18 @@ def create_status_check( "Please ensure the app has the required permissions and that " "the organization has accepted any updated permissions." ) from e - - # For other 403 errors, re-raise the original exception - raise - except Exception as e: - lifecycle.record_failure(e) - return None + else: + logger.exception( + "preprod.status_checks.create.forbidden", + extra={ + "organization_id": self.organization_id, + "integration_id": self.integration_id, + "repo": repo, + }, + ) + raise IntegrationConfigurationError( + f"GitHub API returned 403 Forbidden when creating check run: {e}" + ) from e GITHUB_STATUS_CHECK_STATUS_MAPPING: dict[StatusCheckStatus, GitHubCheckStatus] = { From 5350e6eed068081f98b4196cf0dd926fe2c5af47 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Fri, 7 Nov 2025 15:45:52 -0500 Subject: [PATCH 3/5] fix --- .../preprod/vcs/status_checks/size/tasks.py | 60 +++++++++++-------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/src/sentry/preprod/vcs/status_checks/size/tasks.py b/src/sentry/preprod/vcs/status_checks/size/tasks.py index 05bee7c3a3881f..5243e1048e4e45 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -24,7 +24,7 @@ from sentry.preprod.models import PreprodArtifact, PreprodArtifactSizeMetrics from sentry.preprod.url_utils import get_preprod_artifact_url from sentry.preprod.vcs.status_checks.size.templates import format_status_check_messages -from sentry.shared_integrations.exceptions import ApiForbiddenError, IntegrationConfigurationError +from sentry.shared_integrations.exceptions import ApiError, IntegrationConfigurationError from sentry.silo.base import SiloMode from sentry.tasks.base import instrumented_task from sentry.taskworker.namespaces import integrations_tasks @@ -352,42 +352,50 @@ def create_status_check( response = self.client.create_check_run(repo=repo, data=check_data) check_id = response.get("id") return str(check_id) if check_id else None - except ApiForbiddenError as e: + except ApiError as e: lifecycle.record_failure(e) - # 403 errors are typically not transient (permission or configuration issues) - # so we convert them to IntegrationConfigurationError to prevent retries - error_message = str(e).lower() - if ( - "resource not accessible" in error_message - or "insufficient" in error_message - or "permission" in error_message - ): - logger.exception( - "preprod.status_checks.create.insufficient_permissions", - extra={ - "organization_id": self.organization_id, - "integration_id": self.integration_id, - "repo": repo, - }, - ) - raise IntegrationConfigurationError( - "GitHub App lacks permissions to create check runs. " - "Please ensure the app has the required permissions and that " - "the organization has accepted any updated permissions." - ) from e - else: + + # 4xx client errors (except 429) are not transient (our fault or configuration issues) + # Convert them to IntegrationConfigurationError to prevent retries + # 429 rate limits, 5xx server errors, and other transient issues will bubble up and be retriable. + if e.code and 400 <= e.code < 500 and e.code != 429: + if e.code == 403: + error_message = str(e).lower() + if ( + "resource not accessible" in error_message + or "insufficient" in error_message + or "permission" in error_message + ): + logger.exception( + "preprod.status_checks.create.insufficient_permissions", + extra={ + "organization_id": self.organization_id, + "integration_id": self.integration_id, + "repo": repo, + }, + ) + raise IntegrationConfigurationError( + "GitHub App lacks permissions to create check runs. " + "Please ensure the app has the required permissions and that " + "the organization has accepted any updated permissions." + ) from e + logger.exception( - "preprod.status_checks.create.forbidden", + "preprod.status_checks.create.client_error", extra={ "organization_id": self.organization_id, "integration_id": self.integration_id, "repo": repo, + "status_code": e.code, }, ) raise IntegrationConfigurationError( - f"GitHub API returned 403 Forbidden when creating check run: {e}" + f"GitHub API returned {e.code} client error when creating check run" ) from e + # For 5xx or other errors, re-raise to allow retries + raise + GITHUB_STATUS_CHECK_STATUS_MAPPING: dict[StatusCheckStatus, GitHubCheckStatus] = { StatusCheckStatus.ACTION_REQUIRED: GitHubCheckStatus.COMPLETED, From 493ae32510f4ea99c6760be32b77319e9bb1d109 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Fri, 7 Nov 2025 16:10:26 -0500 Subject: [PATCH 4/5] fix --- .../preprod/vcs/status_checks/size/tasks.py | 52 +++++++++---------- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/src/sentry/preprod/vcs/status_checks/size/tasks.py b/src/sentry/preprod/vcs/status_checks/size/tasks.py index 5243e1048e4e45..8efd3e2f90b5d9 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -354,32 +354,30 @@ def create_status_check( return str(check_id) if check_id else None except ApiError as e: lifecycle.record_failure(e) - - # 4xx client errors (except 429) are not transient (our fault or configuration issues) - # Convert them to IntegrationConfigurationError to prevent retries - # 429 rate limits, 5xx server errors, and other transient issues will bubble up and be retriable. - if e.code and 400 <= e.code < 500 and e.code != 429: - if e.code == 403: - error_message = str(e).lower() - if ( - "resource not accessible" in error_message - or "insufficient" in error_message - or "permission" in error_message - ): - logger.exception( - "preprod.status_checks.create.insufficient_permissions", - extra={ - "organization_id": self.organization_id, - "integration_id": self.integration_id, - "repo": repo, - }, - ) - raise IntegrationConfigurationError( - "GitHub App lacks permissions to create check runs. " - "Please ensure the app has the required permissions and that " - "the organization has accepted any updated permissions." - ) from e - + # Only convert specific permission 403s as IntegrationConfigurationError + # GitHub can return 403 for various reasons (rate limits, temporary issues, permissions) + if e.code == 403: + error_message = str(e).lower() + if ( + "resource not accessible" in error_message + or "insufficient" in error_message + or "permission" in error_message + ): + logger.exception( + "preprod.status_checks.create.insufficient_permissions", + extra={ + "organization_id": self.organization_id, + "integration_id": self.integration_id, + "repo": repo, + "error_message": str(e), + }, + ) + raise IntegrationConfigurationError( + "GitHub App lacks permissions to create check runs. " + "Please ensure the app has the required permissions and that " + "the organization has accepted any updated permissions." + ) from e + elif e.code and 400 <= e.code < 500 and e.code != 429: logger.exception( "preprod.status_checks.create.client_error", extra={ @@ -393,7 +391,7 @@ def create_status_check( f"GitHub API returned {e.code} client error when creating check run" ) from e - # For 5xx or other errors, re-raise to allow retries + # For non-permission 403s, 429s, 5xx, and other error raise From dea1b5555d4ef366d19a6935b191174c32527094 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Mon, 10 Nov 2025 12:20:26 -0500 Subject: [PATCH 5/5] tests --- .../size/test_status_checks_tasks.py | 115 +++++++++++++++++- 1 file changed, 111 insertions(+), 4 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 e61dcbc076d70a..31ca4416b01f5b 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 @@ -10,7 +10,7 @@ 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.shared_integrations.exceptions import ApiForbiddenError, IntegrationConfigurationError +from sentry.shared_integrations.exceptions import IntegrationConfigurationError from sentry.testutils.cases import TestCase from sentry.testutils.silo import region_silo_test @@ -506,7 +506,7 @@ def test_create_preprod_status_check_task_github_permission_error(self): @responses.activate def test_create_preprod_status_check_task_github_non_permission_403(self): - """Test task re-raises non-permission 403 errors (for potential retry).""" + """Test task re-raises non-permission 403 errors (allows retry for transient issues).""" preprod_artifact = self._create_preprod_artifact( state=PreprodArtifact.ArtifactState.PROCESSED ) @@ -535,7 +535,7 @@ def test_create_preprod_status_check_task_github_non_permission_403(self): integration_id=integration.id, ) - # 403 error that's NOT permission-related (should be re-raised, not converted) + # 403 error that's NOT permission-related (should re-raise to allow retry) responses.add( responses.POST, "https://api.github.com/repos/owner/repo/check-runs", @@ -546,13 +546,120 @@ def test_create_preprod_status_check_task_github_non_permission_403(self): ) with self.tasks(): + # Should re-raise ApiForbiddenError (not convert to IntegrationConfigurationError) + # This allows the task system to retry in case it's a transient issue try: create_preprod_status_check_task(preprod_artifact.id) assert False, "Expected ApiForbiddenError to be raised" - except ApiForbiddenError as e: + except Exception as e: + assert e.__class__.__name__ == "ApiForbiddenError" assert "temporarily unavailable" in str(e) assert len(responses.calls) == 1 assert ( responses.calls[0].request.url == "https://api.github.com/repos/owner/repo/check-runs" ) + + @responses.activate + def test_create_preprod_status_check_task_github_400_error(self): + """Test task converts 400 errors to IntegrationConfigurationError (no retry).""" + 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="789", + 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=400, + json={ + "message": "Invalid request", + "errors": [{"field": "head_sha", "code": "invalid"}], + }, + ) + + with self.tasks(): + try: + create_preprod_status_check_task(preprod_artifact.id) + assert False, "Expected IntegrationConfigurationError to be raised" + except IntegrationConfigurationError as e: + assert "400 client error" in str(e) + + # Verify no retries + assert len(responses.calls) == 1 + + @responses.activate + def test_create_preprod_status_check_task_github_429_error(self): + """Test task allows 429 rate limit errors to retry""" + 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="999", + 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=429, + json={ + "message": "API rate limit exceeded", + "documentation_url": "https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting", + }, + ) + + with self.tasks(): + # 429 should be re-raised as ApiRateLimitedError (not converted to IntegrationConfigurationError) + # This allows the task system to retry with backoff + try: + create_preprod_status_check_task(preprod_artifact.id) + assert False, "Expected ApiRateLimitedError to be raised" + except Exception as e: + assert e.__class__.__name__ == "ApiRateLimitedError" + assert "rate limit" in str(e).lower() + + assert len(responses.calls) == 1