ref(cells): Remove usage of Region.category#109840
Conversation
"category" will be removed from Region/Cell, and will only be a property of locality. Now there are no more callers to Region.category. It still exists as `_category` as it's still (temporarily) needed for the 1:1 cell/locality shim.
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| snowflake_id=config_value["snowflake_id"], | ||
| category=RegionCategory(config_value["category"]), | ||
| _category=RegionCategory(config_value["category"]), | ||
| address=config_value["address"], | ||
| visible=config_value.get("visible", True), |
There was a problem hiding this comment.
Bug: In TestEnvRegionDirectory, swap_state() doesn't update the _cell_to_locality cache, causing get_cells() to return stale data for tests that dynamically change region categories.
Severity: MEDIUM
Suggested Fix
Update TestEnvRegionDirectory.swap_state() to rebuild the _cell_to_locality mapping after the regions are updated. This ensures that any subsequent calls to get_cells() will use the correct, up-to-date locality information.
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/types/region.py#L219-L222
Potential issue: The `TestEnvRegionDirectory` class, used for testing, does not
correctly handle dynamic region updates. When `swap_state()` is called to change region
properties, it updates the list of `regions` but fails to rebuild the internal
`_cell_to_locality` cache. The newly refactored `get_cells()` method relies on this
cache to filter regions by category. As a result, if a test swaps a region's category
(e.g., from `MULTI_TENANT` to `SINGLE_TENANT`) and then calls `get_cells()` or a
function that uses it like `find_all_multitenant_region_names()`, it will receive stale
data based on the initial configuration, potentially causing test unreliability.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
| r | ||
| for r in self.regions | ||
| if (loc := self._cell_to_locality.get(r.name)) is not None and loc.category == category | ||
| ) |
There was a problem hiding this comment.
Stale _cell_to_locality in test directory after state swap
High Severity
RegionDirectory.get_cells(category) now looks up localities via self._cell_to_locality, but TestEnvRegionDirectory does not override get_cells and its swap_state method only updates _tmp_state — it never rebuilds _cell_to_locality. The initial _cell_to_locality maps only the default test region (e.g. "testregion123456"), so after swapping in different regions like "us", "eu", "acme", those names won't be found in _cell_to_locality, silently excluding them from filtered results. This breaks find_all_multitenant_region_names() and get_cell_names(category) in any test that uses override_regions or swap_state.
Additional Locations (1)
There was a problem hiding this comment.
Bots are right that get_cells() is not implemented on the TestEnvRegionDirectory
2 participants
"category" will be removed from Region/Cell, and will only be a property of locality.
Now there are no more callers to Region.category.
It still exists as
_categoryas it's still (temporarily) needed for the 1:1 cell/locality shim.