From c3b708cec17dbcad9b487eb5eec999234345794d Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Fri, 7 Nov 2025 15:11:37 -0500 Subject: [PATCH 01/11] 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 02/11] 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 03/11] 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 f774eb7ce7596e57152c9a858852625315dd6581 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Fri, 7 Nov 2025 15:59:40 -0500 Subject: [PATCH 04/11] truncate --- .../preprod/vcs/status_checks/size/tasks.py | 52 +++++++++++++++++-- 1 file changed, 49 insertions(+), 3 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..310c4381b164ce 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -322,19 +322,44 @@ def create_status_check( ) return None + truncated_text = _truncate_to_byte_limit(text, GITHUB_MAX_TEXT_FIELD_LENGTH) + truncated_summary = _truncate_to_byte_limit(summary, GITHUB_MAX_SUMMARY_FIELD_LENGTH) + + if text and len(truncated_text) != len(text): + logger.warning( + "preprod.status_checks.create.text_truncated", + extra={ + "original_bytes": len(text.encode("utf-8")), + "truncated_bytes": len(truncated_text.encode("utf-8")), + "organization_id": self.organization_id, + "organization_slug": self.organization_slug, + }, + ) + + if summary and len(truncated_summary) != len(summary): + logger.warning( + "preprod.status_checks.create.summary_truncated", + extra={ + "original_bytes": len(summary.encode("utf-8")), + "truncated_bytes": len(truncated_summary.encode("utf-8")), + "organization_id": self.organization_id, + "organization_slug": self.organization_slug, + }, + ) + check_data: dict[str, Any] = { "name": title, "head_sha": sha, "external_id": external_id, "output": { "title": subtitle, - "summary": summary, + "summary": truncated_summary, }, "status": mapped_status.value, } - if text: - check_data["output"]["text"] = text + if truncated_text: + check_data["output"]["text"] = truncated_text if mapped_conclusion: check_data["conclusion"] = mapped_conclusion.value @@ -397,6 +422,27 @@ def create_status_check( raise +GITHUB_MAX_SUMMARY_FIELD_LENGTH = 65535 +GITHUB_MAX_TEXT_FIELD_LENGTH = 65535 + + +def _truncate_to_byte_limit(text: str | None, byte_limit: int) -> str | None: + """Truncate text to fit within byte limit while ensuring valid UTF-8.""" + if not text: + return text + + encoded = text.encode("utf-8") + if len(encoded) <= byte_limit: + return text + + # Truncate to byte_limit - 10 (a bit of wiggle room) to make room for "..." + # Note: this can break formatting you have and is more of a catch-all, + # broken formatting is better than silently erroring for the user. + # Templating logic itself should try to more contextually trim the content if possible. + truncated = encoded[: byte_limit - 10].decode("utf-8", errors="ignore") + return truncated + "..." + + GITHUB_STATUS_CHECK_STATUS_MAPPING: dict[StatusCheckStatus, GitHubCheckStatus] = { StatusCheckStatus.ACTION_REQUIRED: GitHubCheckStatus.COMPLETED, StatusCheckStatus.IN_PROGRESS: GitHubCheckStatus.IN_PROGRESS, From 493ae32510f4ea99c6760be32b77319e9bb1d109 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Fri, 7 Nov 2025 16:10:26 -0500 Subject: [PATCH 05/11] 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 acc2cb61aa01872576c443f4e03d6eb313c8eef0 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Fri, 7 Nov 2025 16:21:49 -0500 Subject: [PATCH 06/11] oops --- src/sentry/preprod/vcs/status_checks/size/tasks.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/sentry/preprod/vcs/status_checks/size/tasks.py b/src/sentry/preprod/vcs/status_checks/size/tasks.py index 310c4381b164ce..2732b8d32aaf9c 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -87,6 +87,7 @@ def create_preprod_status_check_task(preprod_artifact_id: int) -> None: client, commit_comparison.provider, preprod_artifact.project.organization_id, + preprod_artifact.project.organization.slug, repository.integration_id, ) if not provider: @@ -247,10 +248,16 @@ def _get_status_check_client( def _get_status_check_provider( - client: StatusCheckClient, provider: str | None, organization_id: int, integration_id: int + client: StatusCheckClient, + provider: str | None, + organization_id: int, + organization_slug: str, + integration_id: int, ) -> _StatusCheckProvider | None: if provider == IntegrationProviderSlug.GITHUB: - return _GitHubStatusCheckProvider(client, provider, organization_id, integration_id) + return _GitHubStatusCheckProvider( + client, provider, organization_id, organization_slug, integration_id + ) else: return None @@ -266,11 +273,13 @@ def __init__( client: StatusCheckClient, provider_key: str, organization_id: int, + organization_slug: str, integration_id: int, ): self.client = client self.provider_key = provider_key self.organization_id = organization_id + self.organization_slug = organization_slug self.integration_id = integration_id def _create_scm_interaction_event(self) -> SCMIntegrationInteractionEvent: From 65dab2ca5bb4c21b07dcb17e2f75b5ac3a920924 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Fri, 7 Nov 2025 16:33:58 -0500 Subject: [PATCH 07/11] types --- src/sentry/preprod/vcs/status_checks/size/tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/preprod/vcs/status_checks/size/tasks.py b/src/sentry/preprod/vcs/status_checks/size/tasks.py index 2732b8d32aaf9c..cb24bb8e995518 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -334,7 +334,7 @@ def create_status_check( truncated_text = _truncate_to_byte_limit(text, GITHUB_MAX_TEXT_FIELD_LENGTH) truncated_summary = _truncate_to_byte_limit(summary, GITHUB_MAX_SUMMARY_FIELD_LENGTH) - if text and len(truncated_text) != len(text): + if text and truncated_text and len(truncated_text) != len(text): logger.warning( "preprod.status_checks.create.text_truncated", extra={ @@ -345,7 +345,7 @@ def create_status_check( }, ) - if summary and len(truncated_summary) != len(summary): + if summary and truncated_summary and len(truncated_summary) != len(summary): logger.warning( "preprod.status_checks.create.summary_truncated", extra={ From fca94cc43a2e79ac15a5886496b057510351626f Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Fri, 7 Nov 2025 16:43:31 -0500 Subject: [PATCH 08/11] fix --- src/sentry/preprod/vcs/status_checks/size/tasks.py | 9 ++++++++- 1 file changed, 8 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 cb24bb8e995518..62b7bba8a00d8f 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -440,15 +440,22 @@ def _truncate_to_byte_limit(text: str | None, byte_limit: int) -> str | None: if not text: return text + TRUNCATE_AMOUNT = 10 + encoded = text.encode("utf-8") if len(encoded) <= byte_limit: return text + if byte_limit <= TRUNCATE_AMOUNT: + # This shouldn't happen, but just in case. + truncated = encoded[:byte_limit].decode("utf-8", errors="ignore") + return truncated + # Truncate to byte_limit - 10 (a bit of wiggle room) to make room for "..." # Note: this can break formatting you have and is more of a catch-all, # broken formatting is better than silently erroring for the user. # Templating logic itself should try to more contextually trim the content if possible. - truncated = encoded[: byte_limit - 10].decode("utf-8", errors="ignore") + truncated = encoded[: byte_limit - TRUNCATE_AMOUNT].decode("utf-8", errors="ignore") return truncated + "..." From dea1b5555d4ef366d19a6935b191174c32527094 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Mon, 10 Nov 2025 12:20:26 -0500 Subject: [PATCH 09/11] 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 From a3fa46292001073bfea86ae4150dbe803d56e578 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Mon, 10 Nov 2025 12:23:01 -0500 Subject: [PATCH 10/11] tests --- .../size/test_status_checks_tasks.py | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) 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 31ca4416b01f5b..439ce11c2870e3 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 @@ -13,6 +13,7 @@ from sentry.shared_integrations.exceptions import IntegrationConfigurationError from sentry.testutils.cases import TestCase from sentry.testutils.silo import region_silo_test +from sentry.utils import json @region_silo_test @@ -663,3 +664,66 @@ def test_create_preprod_status_check_task_github_429_error(self): assert "rate limit" in str(e).lower() assert len(responses.calls) == 1 + + @responses.activate + def test_create_preprod_status_check_task_truncates_long_summary(self): + """Test task truncates summary when it exceeds GitHub's byte limit.""" + commit_comparison = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha="a" * 40, + base_sha="b" * 40, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + ) + + artifacts = [] + for i in range(150): + long_app_id = f"com.example.very.long.app.identifier.number.{i}" + "x" * 200 + artifact = PreprodArtifact.objects.create( + project=self.project, + state=PreprodArtifact.ArtifactState.FAILED, + app_id=long_app_id, + error_message=f"This is a very long error message that will contribute to the summary size. Error #{i}: " + + "y" * 500, + commit_comparison=commit_comparison, + ) + artifacts.append(artifact) + + integration = self.create_integration( + organization=self.organization, + external_id="test-truncation", + 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=201, + json={"id": 12345, "status": "completed"}, + ) + + with self.tasks(): + create_preprod_status_check_task(artifacts[0].id) + + assert len(responses.calls) == 1 + request_body = responses.calls[0].request.body + + payload = json.loads(request_body) + summary = payload["output"]["summary"] + + assert summary is not None + summary_bytes = len(summary.encode("utf-8")) + + assert summary_bytes <= 65535, f"Summary has {summary_bytes} bytes, exceeds GitHub limit" + assert summary.endswith("..."), "Truncated summary should end with '...'" From f50d6d7d34d7b9c3e45c932da8d168ab63a86956 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Mon, 10 Nov 2025 12:50:12 -0500 Subject: [PATCH 11/11] link --- 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 c5341914de1822..32f0ff8de27bad 100644 --- a/src/sentry/preprod/vcs/status_checks/size/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/size/tasks.py @@ -429,6 +429,7 @@ def create_status_check( raise +# See: https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28#create-a-check-run GITHUB_MAX_SUMMARY_FIELD_LENGTH = 65535 GITHUB_MAX_TEXT_FIELD_LENGTH = 65535