Skip to content

ref(gitlab): Wrap status sync delete+create in transaction.atomic()#112490

Open
iamrajjoshi wants to merge 1 commit intoraj/gitlab-feat-par/status-sync-outboundfrom
ref/gitlab-status-sync-transaction-safety
Open

ref(gitlab): Wrap status sync delete+create in transaction.atomic()#112490
iamrajjoshi wants to merge 1 commit intoraj/gitlab-feat-par/status-sync-outboundfrom
ref/gitlab-status-sync-transaction-safety

Conversation

@iamrajjoshi
Copy link
Copy Markdown
Collaborator

Summary

  • Wraps the IntegrationExternalProject delete-then-recreate sequence in update_organization_config with transaction.atomic() to prevent data loss if a create() fails mid-loop
  • Without this, a partial failure leaves the org with no external project records and status sync silently broken

Followup to #107216 per @GabeVillalobos's review.

Test plan

  • Existing test_update_organization_config_status_sync, test_update_organization_config_invalid_status, and test_update_organization_config_missing_status tests cover the happy path and error cases

Ensure the IntegrationExternalProject delete-then-recreate sequence in
update_organization_config is atomic. Without this, a partial failure
during the create loop would leave the org with no external project
records and status sync silently broken.

Co-Authored-By: Claude <noreply@anthropic.com>
@iamrajjoshi iamrajjoshi requested review from a team as code owners April 8, 2026 17:45
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 8, 2026
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Atomic transaction targets wrong database connection
    • Added router import and specified using=router.db_for_write(IntegrationExternalProject) to ensure the transaction.atomic() block targets the control database where the @control_silo_model is routed.

Create PR

Or push these changes by commenting:

@cursor push 8fa1afb848
Preview (8fa1afb848)
diff --git a/src/sentry/integrations/gitlab/integration.py b/src/sentry/integrations/gitlab/integration.py
--- a/src/sentry/integrations/gitlab/integration.py
+++ b/src/sentry/integrations/gitlab/integration.py
@@ -6,7 +6,7 @@
 from urllib.parse import urlparse
 
 from django import forms
-from django.db import transaction
+from django.db import router, transaction
 from django.http.request import HttpRequest
 from django.http.response import HttpResponseBase
 from django.urls import reverse
@@ -356,7 +356,7 @@
 
             data["sync_status_forward"] = bool(project_mappings)
 
-            with transaction.atomic():
+            with transaction.atomic(using=router.db_for_write(IntegrationExternalProject)):
                 IntegrationExternalProject.objects.filter(
                     organization_integration_id=self.org_integration.id
                 ).delete()

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ece1db2. Configure here.

if statuses["on_unresolve"] not in valid_statuses:
raise IntegrationError(
f"Invalid unresolve status: {statuses['on_unresolve']}. Must be 'opened' or 'closed'."
with transaction.atomic():
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.

Atomic transaction targets wrong database connection

High Severity

The transaction.atomic() block defaults to the default database, but IntegrationExternalProject is a @control_silo_model routed to the control database. This mismatch means the delete and create operations are not atomic. If a create fails, the preceding delete won't roll back, leading to data loss. The router import is also missing.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit ece1db2. Configure here.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

Backend Test Failures

Failures on 6b613cc in this run:

tests/sentry/integrations/gitlab/test_integration.py::GitlabIssueSyncTest::test_update_organization_config_invalid_statuslog
[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/sentry/integrations/gitlab/test_integration.py:1451: in test_update_organization_config_invalid_status
    installation.update_organization_config(data)
src/sentry/integrations/gitlab/integration.py:359: in update_organization_config
    with transaction.atomic():
src/sentry/testutils/pytest/stale_database_reads.py:120: in atomic
    return old_atomic(*args, **kwargs)
src/sentry/silo/patches/silo_aware_transaction_patch.py:40: in siloed_atomic
    validate_transaction_using_for_silo_mode(using)
src/sentry/silo/patches/silo_aware_transaction_patch.py:95: in validate_transaction_using_for_silo_mode
    raise TransactionMissingDBException("'using' must be specified when creating a transaction")
E   sentry.silo.patches.silo_aware_transaction_patch.TransactionMissingDBException: 'using' must be specified when creating a transaction
tests/sentry/integrations/gitlab/test_integration.py::GitlabIssueSyncTest::test_update_organization_config_status_synclog
[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/sentry/integrations/gitlab/test_integration.py:1409: in test_update_organization_config_status_sync
    installation.update_organization_config(data)
src/sentry/integrations/gitlab/integration.py:359: in update_organization_config
    with transaction.atomic():
src/sentry/testutils/pytest/stale_database_reads.py:120: in atomic
    return old_atomic(*args, **kwargs)
src/sentry/silo/patches/silo_aware_transaction_patch.py:40: in siloed_atomic
    validate_transaction_using_for_silo_mode(using)
src/sentry/silo/patches/silo_aware_transaction_patch.py:95: in validate_transaction_using_for_silo_mode
    raise TransactionMissingDBException("'using' must be specified when creating a transaction")
E   sentry.silo.patches.silo_aware_transaction_patch.TransactionMissingDBException: 'using' must be specified when creating a transaction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant