Skip to content
Merged
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
50 changes: 49 additions & 1 deletion src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@
PreprodSnapshotMetrics,
)
from sentry.preprod.snapshots.tasks import compare_snapshots
from sentry.preprod.snapshots.utils import find_base_snapshot_artifact
from sentry.preprod.snapshots.utils import (
find_base_snapshot_artifact,
find_head_snapshot_artifacts_awaiting_base,
)
from sentry.preprod.url_utils import get_preprod_artifact_url
from sentry.preprod.vcs.status_checks.snapshots.tasks import (
create_preprod_snapshot_status_check_task,
Expand Down Expand Up @@ -639,6 +642,51 @@ def post(self, request: Request, project: Project) -> Response:
},
)

# Trigger comparisons for any head artifacts that were uploaded before this base.
# Handles possible out-of-order uploads where heads arrive before their base build.
if commit_comparison is not None:
try:
waiting_heads = find_head_snapshot_artifacts_awaiting_base(
organization_id=project.organization_id,
base_sha=commit_comparison.head_sha,
base_repo_name=commit_comparison.head_repo_name,
project_id=project.id,
app_id=artifact.app_id,
build_configuration=artifact.build_configuration,
)
for head_artifact in waiting_heads:
head_metrics = head_artifact.preprodsnapshotmetrics
logger.info(
"Found head artifact awaiting base, triggering snapshot comparison",
extra={
"head_artifact_id": head_artifact.id,
"base_artifact_id": artifact.id,
"base_sha": commit_comparison.head_sha,
},
)
try:
PreprodSnapshotComparison.objects.get_or_create(
head_snapshot_metrics=head_metrics,
base_snapshot_metrics=snapshot_metrics,
defaults={"state": PreprodSnapshotComparison.State.PENDING},
)
except IntegrityError:
pass

compare_snapshots.apply_async(
kwargs={
"project_id": project.id,
"org_id": project.organization_id,
"head_artifact_id": head_artifact.id,
"base_artifact_id": artifact.id,
},
)
Comment on lines +667 to +683
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.

nit: shall we extract this into a helper method since it's a copy pasta of code from line 606?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't think it cleans things up much and just adds more indeterminism to track down a shared helper, so I'm fine lightly duplicating to avoid redirection

except Exception:
logger.exception(
"Failed to trigger comparisons for head artifacts awaiting base",
extra={"base_artifact_id": artifact.id},
)

return Response(
{
"artifactId": str(artifact.id),
Expand Down
33 changes: 33 additions & 0 deletions src/sentry/preprod/snapshots/utils.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import annotations

from sentry.preprod.models import PreprodArtifact, PreprodBuildConfiguration
from sentry.preprod.snapshots.models import PreprodSnapshotComparison


def find_base_snapshot_artifact(
Expand All @@ -26,3 +27,35 @@ def find_base_snapshot_artifact(
.order_by("-date_added")
.first()
)


def find_head_snapshot_artifacts_awaiting_base(
organization_id: int,
base_sha: str,
base_repo_name: str,
project_id: int,
app_id: str | None,
build_configuration: PreprodBuildConfiguration | None,
) -> list[PreprodArtifact]:
"""Find head snapshot artifacts that were uploaded before their base was available.

When a base artifact is uploaded, its commit_comparison.head_sha is the SHA that waiting
head artifacts have as their commit_comparison.base_sha. This finds those heads so
comparisons can be triggered retroactively.
"""
return list(
PreprodArtifact.objects.filter(
commit_comparison__organization_id=organization_id,
commit_comparison__base_sha=base_sha,
commit_comparison__base_repo_name=base_repo_name,
project_id=project_id,
preprodsnapshotmetrics__isnull=False,
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.

shouldn't we also exclude builds that have size metrics in this query too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice I don't think there will ever be a case where snapshotmetrics and size metrics are both set

app_id=app_id,
build_configuration=build_configuration,
)
.exclude(
preprodsnapshotmetrics__snapshot_comparisons_head_metrics__state=PreprodSnapshotComparison.State.SUCCESS,
)
.select_related("preprodsnapshotmetrics")
.order_by("-date_added")
)
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

from sentry.models.commitcomparison import CommitComparison
from sentry.preprod.models import PreprodArtifact
from sentry.preprod.snapshots.models import PreprodSnapshotMetrics
from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics
from sentry.testutils.cases import APITestCase


Expand Down Expand Up @@ -250,6 +250,88 @@ def test_snapshot_invalid_sha_format(self) -> None:

assert response.status_code == 400

@patch("sentry.preprod.api.endpoints.preprod_artifact_snapshot.get_preprod_session")
@patch("sentry.preprod.api.endpoints.preprod_artifact_snapshot.compare_snapshots")
def test_base_upload_triggers_comparison_for_waiting_head(
self, mock_compare_snapshots, mock_get_session
) -> None:
"""
When a head snapshot is uploaded before its base, uploading the base should
retroactively trigger a comparison for the waiting head.
"""
head_sha = "a" * 40
base_sha = "b" * 40
repo_name = "owner/repo"
app_id = "com.example.app"

# Simulate a head artifact that was uploaded before its base was available.
# It has a commit_comparison with base_sha pointing to the not-yet-uploaded base.
head_commit_comparison = CommitComparison.objects.create(
organization_id=self.org.id,
head_repo_name=repo_name,
head_sha=head_sha,
base_sha=base_sha,
provider="github",
head_ref="feature-branch",
base_repo_name=repo_name,
)
head_artifact = PreprodArtifact.objects.create(
project=self.project,
state=PreprodArtifact.ArtifactState.UPLOADED,
app_id=app_id,
commit_comparison=head_commit_comparison,
)
head_metrics = PreprodSnapshotMetrics.objects.create(
preprod_artifact=head_artifact,
image_count=1,
extras={
"manifest_key": f"{self.org.id}/{self.project.id}/{head_artifact.id}/manifest.json"
},
)

# No comparison exists yet — the base was missing when the head was uploaded.
assert not PreprodSnapshotComparison.objects.filter(
head_snapshot_metrics=head_metrics
).exists()

# Upload the base snapshot. Its head_sha matches the head artifact's base_sha.
url = self._get_create_url()
data = {
"app_id": app_id,
"head_sha": base_sha,
"provider": "github",
"head_repo_name": repo_name,
"head_ref": "main",
"images": {
"img1": {"display_name": "Screen 1", "width": 375, "height": 812},
},
}

with self.feature("organizations:preprod-snapshots"):
response = self.client.post(url, data, format="json")

assert response.status_code == 200

base_artifact = PreprodArtifact.objects.get(id=response.data["artifactId"])
base_metrics = PreprodSnapshotMetrics.objects.get(preprod_artifact=base_artifact)

# A pending comparison record should have been created linking head to base.
comparison = PreprodSnapshotComparison.objects.get(
head_snapshot_metrics=head_metrics,
base_snapshot_metrics=base_metrics,
)
assert comparison.state == PreprodSnapshotComparison.State.PENDING

# The comparison task should have been queued for the waiting head.
mock_compare_snapshots.apply_async.assert_called_once_with(
kwargs={
"project_id": self.project.id,
"org_id": self.org.id,
"head_artifact_id": head_artifact.id,
"base_artifact_id": base_artifact.id,
}
)


class ProjectPreprodSnapshotGetTest(APITestCase):
def setUp(self) -> None:
Expand Down
Loading