Skip to content

feat(cells): Add the rpc methods for project key mapping#110589

Merged
lynnagara merged 10 commits intomasterfrom
add-projectkey-mapping-rpc-services
Mar 16, 2026
Merged

feat(cells): Add the rpc methods for project key mapping#110589
lynnagara merged 10 commits intomasterfrom
add-projectkey-mapping-rpc-services

Conversation

@lynnagara
Copy link
Member

@lynnagara lynnagara commented Mar 12, 2026

3 new rpc methods are added:

  1. ControlReplicaService.upsert_project_key_mapping - handles insert or update of project key
  2. ControlReplicaService.delete_project_key_mapping - project key deletion
  3. RegionReplicaService.delete_project_key - delete the project key on the cell silo in case an integrity error was encountered in control (e.g. duplicate public key in 2 cells)

These will be needed by #110231 as we will start syncing these mappings from the cell to the control silo

These will be needed by #110231
as we will start syncing these mapping from the cell to the control silo
@lynnagara lynnagara requested a review from a team March 12, 2026 21:47
@lynnagara lynnagara requested a review from a team as a code owner March 12, 2026 21:47
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 12, 2026
Comment on lines +404 to +406
region_replica_service.delete_project_key(
project_key_id=project_key.id, cell_name=project_key.cell_name
)
Copy link
Member

Choose a reason for hiding this comment

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

We could also have this RPC method return a result, that the outbox handler in the cell can use to do a local delete. That would save us having ping-pong outboxes. As the delete in the region is going to open another connection back to control to push the delete outbox back.

Copy link
Member Author

Choose a reason for hiding this comment

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

i had it reraise the integrity error and removed the method going back the other way, now the caller to upsert will have to handle that

Copy link
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.

Autofix Details

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

  • ✅ Fixed: Duplicate class name RpcProjectKey causes ambiguity
    • Renamed the hybrid-cloud model to RpcProjectKeyMapping and updated replica service imports/type annotations to remove the name collision with the existing project key RPC model.

Create PR

Or push these changes by commenting:

@cursor push e2dddb6d71
Preview (e2dddb6d71)
diff --git a/src/sentry/hybridcloud/services/project_key_mapping/model.py b/src/sentry/hybridcloud/services/project_key_mapping/model.py
--- a/src/sentry/hybridcloud/services/project_key_mapping/model.py
+++ b/src/sentry/hybridcloud/services/project_key_mapping/model.py
@@ -1,7 +1,7 @@
 from sentry.hybridcloud.rpc import RpcModel
 
 
-class RpcProjectKey(RpcModel):
+class RpcProjectKeyMapping(RpcModel):
     id: int
     public_key: str
     cell_name: str

diff --git a/src/sentry/hybridcloud/services/replica/impl.py b/src/sentry/hybridcloud/services/replica/impl.py
--- a/src/sentry/hybridcloud/services/replica/impl.py
+++ b/src/sentry/hybridcloud/services/replica/impl.py
@@ -23,7 +23,7 @@
 from sentry.hybridcloud.services.control_organization_provisioning import (
     RpcOrganizationSlugReservation,
 )
-from sentry.hybridcloud.services.project_key_mapping import RpcProjectKey
+from sentry.hybridcloud.services.project_key_mapping import RpcProjectKeyMapping
 from sentry.hybridcloud.services.replica.service import (
     ControlReplicaService,
     RegionReplicaService,
@@ -377,7 +377,7 @@
 
         handle_replication(Team, destination)
 
-    def upsert_project_key_mapping(self, *, project_key: RpcProjectKey) -> None:
+    def upsert_project_key_mapping(self, *, project_key: RpcProjectKeyMapping) -> None:
         try:
             with transaction.atomic(router.db_for_write(ProjectKeyMapping)):
                 rows_updated = ProjectKeyMapping.objects.filter(

diff --git a/src/sentry/hybridcloud/services/replica/service.py b/src/sentry/hybridcloud/services/replica/service.py
--- a/src/sentry/hybridcloud/services/replica/service.py
+++ b/src/sentry/hybridcloud/services/replica/service.py
@@ -7,7 +7,7 @@
 from sentry.hybridcloud.services.control_organization_provisioning import (
     RpcOrganizationSlugReservation,
 )
-from sentry.hybridcloud.services.project_key_mapping import RpcProjectKey
+from sentry.hybridcloud.services.project_key_mapping import RpcProjectKeyMapping
 from sentry.notifications.services import RpcExternalActor
 from sentry.organizations.services.organization import RpcOrganizationMemberTeam, RpcTeam
 from sentry.silo.base import SiloMode
@@ -41,7 +41,7 @@
 
     @rpc_method
     @abc.abstractmethod
-    def upsert_project_key_mapping(self, *, project_key: RpcProjectKey) -> None:
+    def upsert_project_key_mapping(self, *, project_key: RpcProjectKeyMapping) -> None:
         pass
 
     @rpc_method

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


def upsert_project_key_mapping(self, *, project_key: RpcProjectKeyMapping) -> None:
try:
with transaction.atomic(router.db_for_write(ProjectKeyMapping)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: The upsert_project_key_mapping method is missing the enforce_constraints() wrapper, causing IntegrityError to be uncaught in nested transactions and bypassing local error handling.
Severity: MEDIUM

Suggested Fix

Wrap the transaction.atomic() call with the enforce_constraints() context manager. This ensures database constraints are checked immediately, allowing the try...except IntegrityError block to function correctly even within a nested transaction. The line should be changed to with enforce_constraints(transaction.atomic(router.db_for_write(ProjectKeyMapping))):.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/hybridcloud/services/replica/impl.py#L382

Potential issue: The `upsert_project_key_mapping` method uses `transaction.atomic()` to
wrap a database operation and attempts to catch `IntegrityError`. However, it is missing
the `enforce_constraints()` wrapper. In a nested transaction scenario, Django defers
constraint checks to the end of the outermost transaction. This causes the
`IntegrityError` to be raised outside of the local `try...except` block, bypassing the
intended error logging and handling. This behavior is inconsistent with other methods in
the same file that correctly use `enforce_constraints()` to handle database integrity
errors within nested transactions.

handle_replication(Team, destination)

def upsert_project_key_mapping(self, *, project_key: RpcProjectKeyMapping) -> bool:
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: The upsert_project_key_mapping method is missing the enforce_constraints wrapper, which will cause IntegrityError to be uncaught in nested transactions, crashing the replication process.
Severity: HIGH

Suggested Fix

Wrap the transaction.atomic(...) call inside the upsert_project_key_mapping method with the enforce_constraints context manager. This will ensure database constraints are checked upon exiting the transaction, allowing the IntegrityError to be caught and handled locally.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/hybridcloud/services/replica/impl.py#L381

Potential issue: The `upsert_project_key_mapping` method uses `transaction.atomic` to
wrap an `update_or_create` call and attempts to catch `IntegrityError`. However, it
lacks the `enforce_constraints` context manager. When this method is called within an
outer transaction, which is a common scenario in the outbox processing framework, the
inner transaction will not check for constraint violations upon exit. As a result, any
`IntegrityError` will propagate past the local `except` block and crash the outer
transaction, preventing the intended graceful handling of duplicate key conflicts.

Copy link
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 is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment on lines +382 to +388
with transaction.atomic(router.db_for_write(ProjectKeyMapping)):
ProjectKeyMapping.objects.update_or_create(
project_key_id=project_key.id,
cell_name=project_key.cell_name,
defaults={"public_key": project_key.public_key},
)
except IntegrityError:
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: The upsert_project_key_mapping function is missing the enforce_constraints wrapper, which will cause IntegrityError to be unhandled in nested transactions, leading to a crash.
Severity: CRITICAL

Suggested Fix

Wrap the transaction.atomic(router.db_for_write(ProjectKeyMapping)) context manager with enforce_constraints(...) to ensure database constraints are checked within the inner transaction, allowing the IntegrityError to be caught as intended.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/hybridcloud/services/replica/impl.py#L382-L388

Potential issue: The `upsert_project_key_mapping` function is designed to catch
`IntegrityError` from unique constraint violations and return `False`. However, it lacks
the `enforce_constraints` wrapper around its `transaction.atomic()` block. When this
function is called within an outer transaction, a standard pattern in the replication
pipeline, any `IntegrityError` will not be caught by the local `except` block. Instead,
it will propagate to the outer transaction, causing an unhandled exception and a crash,
thereby bypassing the intended error handling mechanism. This wrapper is used
consistently in other similar methods in the codebase for this exact purpose.

@lynnagara lynnagara merged commit b0b072c into master Mar 16, 2026
78 checks passed
@lynnagara lynnagara deleted the add-projectkey-mapping-rpc-services branch March 16, 2026 18:05
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.

2 participants