ref(cells): clean up rpc method signatures#111025
Open
Conversation
region_name is no longer passed
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
33
to
39
| def clear_key( | ||
| self, | ||
| *, | ||
| cell_name: str | None = None, # TODO(cells): make required when all callers are updated | ||
| region_name: str | None = None, # TODO(cells): remove when all callers are updated | ||
| cell_name: str, | ||
| key: str, | ||
| ) -> int: | ||
| pass |
Contributor
There was a problem hiding this comment.
Bug: The clear_key method signature changed, but several callers were not updated, which will cause a TypeError at runtime.
Severity: HIGH
Suggested Fix
Update all call sites of CellCachingService.clear_key() to remove the region_name keyword argument, aligning them with the new method signature.
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/rpc/caching/service.py#L33-L39
Potential issue: The `clear_key()` method in `CellCachingService` was updated to remove
the `region_name` parameter from its signature. However, multiple callers of this method
were not updated to reflect this change. Specifically, code paths related to user async
deletion/replication and Sentry App installation replication still attempt to call
`clear_key()` with the `region_name` keyword argument. This will cause a `TypeError` at
runtime when these operations are performed, leading to failures in these background
tasks.
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
33
to
39
| def clear_key( | ||
| self, | ||
| *, | ||
| cell_name: str | None = None, # TODO(cells): make required when all callers are updated | ||
| region_name: str | None = None, # TODO(cells): remove when all callers are updated | ||
| cell_name: str, | ||
| key: str, | ||
| ) -> int: | ||
| pass |
Contributor
There was a problem hiding this comment.
Bug: The clear_key method signature changed, but its callers were not updated. They still pass the removed region_name parameter, which will cause a TypeError.
Severity: HIGH
Suggested Fix
Update all call sites of cell_caching_service.clear_key() to stop passing the region_name argument and instead pass the required cell_name argument. For example, change calls from clear_key(..., region_name=cell_name) to clear_key(..., cell_name=cell_name).
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/rpc/caching/service.py#L33-L39
Potential issue: The `clear_key` method in `CellCachingService` was refactored to remove
the `region_name` parameter and make `cell_name` a required keyword argument. However,
multiple callers in `user.py`, `sentry_app_installation.py`, and `sentry_apps.py` were
not updated to match this new signature. These call sites still attempt to pass
`region_name`, which will result in a `TypeError: clear_key() got an unexpected keyword
argument 'region_name'` at runtime. This will break cache invalidation logic for users
and Sentry App installations during async deletion, replication, and other tasks.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Backend Test FailuresFailures on
|
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| organization_slug_reservation_id: int, | ||
| cell_name: str | None = None, | ||
| region_name: str | None = None, | ||
| cell_name, |
Contributor
There was a problem hiding this comment.
Missing type annotation on cell_name parameter
Low Severity
In delete_replicated_org_slug_reservation, cell_name is missing its : str type annotation. Every other method in DatabaseBackedCellReplicaService annotates it as cell_name: str, and the abstract definition in service.py also declares cell_name: str. This appears to be an accidental omission during the signature cleanup.
Contributor
Backend Test FailuresFailures on
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
region_name is no longer passed