Skip to content

Commit d4ac7c8

Browse files
fix(tests): Fix flakes from cross-worker FLUSHALL and recycled project IDs (#128317)
This fixes three flaky backend tests that fail on master and then pass on retry. There are two root causes, and each one has its own commit. **FLUSHALL from other xdist workers.** The xdist workers, and the per-worker Snuba instances, each use their own DB on one Redis server. `test_circuit_breaker2.py`, `utils/sentry_apps/test_webhooks.py` and `test_artifact_bundles.py` called `flushall()` on the single-host client, and FLUSHALL deletes every DB on the server. That wipes the state of the tests that other workers are running at the same moment: - The snowflake counters reset in the middle of a test. The worker then generates IDs that it already used, and five collisions in a row raise `MaxSnowflakeRetryError`. Tests that create a lot of orgs or projects hit this most, for example `CursoredSchedulerTest`, `test_organization_trace::test_with_missing_group` and `test_backfill_group_action_log`. - The reprocessing state is deleted while an event is reprocessed. `test_minidump_full::test_reprocessing_objectstore` then gets the unsymbolicated event back and its snapshot does not match. These tests now use `flushdb()`. The test plugin also makes `flushall()` on a single-host Redis client raise, so a new test cannot bring the problem back. Prefixed redis-cluster clients still allow it, because they already delete only their own worker's keys. **Recycled snowflake project IDs.** `test_replay_data_export_no_replay_data` expects Snuba to have no replays, but it never reset the replays dataset. Redis is flushed after each test, so when the test runs in the same second as the test before it, `default_project` gets the same ID. The replay that the earlier test stored then gets exported. The test now uses the `replay_store` fixture, which drops the dataset. Not covered: the `assert None is not None` failure in `test_reprocessing_objectstore` and the Kafka `_MAX_POLL_EXCEEDED` in `test_full_minidump_invalid_extra`. I could not reproduce either one locally. <!-- junior-request-attribution:start --> via @joshuarli. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:agent%3A4fe45e7cb5955bf5a36ba1ff0d1eb304 --> -- [View Junior Session](https://junior-prod.sentry.dev/conversations/agent%3A4fe45e7cb5955bf5a36ba1ff0d1eb304) [[Sentry]](https://sentry.sentry.io/explore/conversations/agent%3A4fe45e7cb5955bf5a36ba1ff0d1eb304/?project=4510944073809921) <!-- junior-session-footer:end --> --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: Joshua Li <14209781+joshuarli@users.noreply.github.com>
1 parent 3065324 commit d4ac7c8

6 files changed

Lines changed: 46 additions & 11 deletions

File tree

‎src/sentry/testutils/pytest/sentry.py‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
from datetime import datetime
1111
from hashlib import sha256
1212
from pathlib import Path
13-
from typing import TypeVar
13+
from typing import Any, TypeVar
1414
from unittest import mock
1515

1616
import pytest
@@ -149,6 +149,27 @@ def pytest_ignore_collect(collection_path: Path, config: pytest.Config) -> bool
149149
return True if rel not in _COLLECT_ALLOWED_FILES else None
150150

151151

152+
def _forbid_single_host_flushall() -> None:
153+
# The xdist workers and the per-worker Snuba instances share one Redis server and
154+
# each use their own DB on it. FLUSHALL deletes every DB, which resets the snowflake
155+
# ID counters of the other workers in the middle of their tests. They then generate
156+
# IDs that they already used and fail with MaxSnowflakeRetryError.
157+
from redis.client import Redis
158+
from rediscluster import RedisCluster
159+
160+
flushall = Redis.flushall
161+
162+
def guarded_flushall(self: Redis, *args: Any, **kwargs: Any) -> Any:
163+
# Prefixed cluster clients only delete the keys of this worker.
164+
if isinstance(self, RedisCluster):
165+
return flushall(self, *args, **kwargs)
166+
raise AssertionError(
167+
"FLUSHALL deletes the Redis DBs of all xdist workers. Use flushdb() instead."
168+
)
169+
170+
Redis.flushall = guarded_flushall # type: ignore[method-assign]
171+
172+
152173
def pytest_configure(config: pytest.Config) -> None:
153174
import warnings
154175

@@ -169,6 +190,8 @@ def pytest_configure(config: pytest.Config) -> None:
169190
config.addinivalue_line("markers", "migrations: requires --migrations")
170191
config.addinivalue_line("markers", "symbolicator: test requires access to symbolicator")
171192

193+
_forbid_single_host_flushall()
194+
172195
if sys.platform == "darwin" and shutil.which("colima"):
173196
# This is the only way other than pytest --basetemp to change
174197
# the temproot. We'd like to keep invocations to just "pytest".

‎tests/sentry/debug_files/test_artifact_bundles.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ def get_indexed_files(project, release_name="", dist_name="", distinct=False):
105105
class ArtifactLookupTest(TestCase):
106106
def clear_cache(self):
107107
redis_client = get_redis_cluster_for_artifact_bundles()
108-
redis_client.flushall()
108+
redis_client.flushdb()
109109

110110
def test_indexing_artifacts(self) -> None:
111111
self.clear_cache()
@@ -215,7 +215,7 @@ def test_indexing_artifacts_with_cached_indexing_state(self) -> None:
215215

216216
class GetCachedBundlesIndexingStateTest(TestCase):
217217
def setUp(self) -> None:
218-
get_redis_cluster_for_artifact_bundles().flushall()
218+
get_redis_cluster_for_artifact_bundles().flushdb()
219219

220220
def create_bundles(self, count: int) -> None:
221221
for _ in range(count):

‎tests/sentry/replays/test_data_export.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,10 @@ def test_replay_data_export_no_replay_projects( # type: ignore[no-untyped-def]
120120
@pytest.mark.snuba
121121
@requires_snuba
122122
def test_replay_data_export_no_replay_data( # type: ignore[no-untyped-def]
123-
default_organization, default_project
123+
default_organization, default_project, replay_store
124124
) -> None:
125+
# `replay_store` drops the replays written by earlier tests. `default_project` can
126+
# get the same snowflake ID as the project of an earlier test.
125127
# Setting has_replays flag because the export will skip projects it assumes do not have
126128
# replays.
127129
default_project.flags.has_replays = True

‎tests/sentry/testutils/pytest/test_sentry.py‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,10 @@
77
from collections import Counter
88
from pathlib import Path
99

10+
import pytest
11+
1012
import sentry
11-
from sentry.utils import json
13+
from sentry.utils import json, redis
1214

1315
SCRIPTS = Path(sentry.__file__).parents[2] / ".github" / "workflows" / "scripts"
1416
SHARDS = 4
@@ -96,3 +98,11 @@ def test_filtered_run_spreads_selected_tests_over_every_shard(tmp_path: Path) ->
9698
)
9799
assert len(unsharded) == 20
98100
assert sharded == unsharded
101+
102+
103+
def test_flushall_on_single_host_redis_is_rejected() -> None:
104+
client = redis.redis_clusters.get("default")
105+
with pytest.raises(AssertionError, match="flushdb"):
106+
client.flushall()
107+
with pytest.raises(AssertionError, match="flushdb"):
108+
client.pipeline().flushall()

‎tests/sentry/utils/sentry_apps/test_webhooks.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ def setUp(self):
244244
organization=self.organization, slug=self.sentry_app.slug
245245
)
246246
client = redis.redis_clusters.get(settings.SENTRY_RATE_LIMIT_REDIS_CLUSTER)
247-
client.flushall()
247+
client.flushdb()
248248

249249
def _make_event(self, install=None):
250250
return AppPlatformEvent(

‎tests/sentry/utils/test_circuit_breaker2.py‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ def setUp(self) -> None:
129129
)
130130

131131
# Clear all existing keys from redis
132-
self.breaker.redis_pipeline.flushall()
132+
self.breaker.redis_pipeline.flushdb()
133133
self.breaker.redis_pipeline.execute()
134134

135135
def test_sets_default_values(self) -> None:
@@ -261,7 +261,7 @@ def setUp(self) -> None:
261261
)
262262

263263
# Clear all existing keys from redis
264-
self.breaker.redis_pipeline.flushall()
264+
self.breaker.redis_pipeline.flushdb()
265265
self.breaker.redis_pipeline.execute()
266266

267267
def test_increments_error_count(self) -> None:
@@ -435,7 +435,7 @@ def setUp(self) -> None:
435435
)
436436

437437
# Clear all existing keys from redis
438-
self.breaker.redis_pipeline.flushall()
438+
self.breaker.redis_pipeline.flushdb()
439439
self.breaker.redis_pipeline.execute()
440440

441441
def test_allows_request_in_non_broken_state_with_quota_remaining(self) -> None:
@@ -516,7 +516,7 @@ def setUp(self) -> None:
516516
)
517517

518518
# Clear all existing keys from redis
519-
self.breaker.redis_pipeline.flushall()
519+
self.breaker.redis_pipeline.flushdb()
520520
self.breaker.redis_pipeline.execute()
521521

522522
def test_sets_rate_based_defaults(self) -> None:
@@ -689,7 +689,7 @@ def test_record_success_noop_when_not_rate_based(self) -> None:
689689
count_breaker = MockCircuitBreaker(
690690
"count_only", DEFAULT_CONFIG, CountBasedTripStrategy.from_config(DEFAULT_CONFIG)
691691
)
692-
count_breaker.redis_pipeline.flushall()
692+
count_breaker.redis_pipeline.flushdb()
693693
count_breaker.redis_pipeline.execute()
694694

695695
assert isinstance(count_breaker.trip_strategy, CountBasedTripStrategy)

0 commit comments

Comments
 (0)