test: harden Redis-backed test isolation under pytest-xdist (#546)#636
Open
vishal-bala wants to merge 1 commit into
Open
test: harden Redis-backed test isolation under pytest-xdist (#546)#636vishal-bala wants to merge 1 commit into
vishal-bala wants to merge 1 commit into
Conversation
Eliminates same-worker state contamination and cross-worker cluster
collisions that caused order-dependent, retry-passing integration test
failures (e.g. test_hybrid_query_with_geo_filter returning 0 results).
Converts worker-scoped and fixed Redis resource names to per-test unique
names via the redis_test_name fixture, and tightens recreate/cleanup so
stale state cannot survive an interrupted or co-located test:
- Recreate group: test_hybrid, test_aggregation, test_search_results,
test_svs_integration and the shared conftest index fixtures now use
per-test names and create(overwrite=True, drop=True). test_hybrid and
test_aggregation previously shared user_index_{worker_id}/v1_{worker_id}.
- Fixed-name collisions: from_existing_complex ("test"),
no_proactive_module_validation ("my_index"), embedcache warning tests
("test_cache"), and the shared "doc" prefix in
test_no_proactive_module_checks now use per-test names with guaranteed
teardown.
- llmcache: the two tests sharing float64_cache_{worker_id} now use
unique names and clean up.
- Cluster tests (shared hard-coded localhost:7001 across workers):
per-test names, drop=True, and try/finally cleanup.
The SemanticRouter {name}:route_config leak is tracked separately in #634.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🛡️ Jit Security Scan Results✅ No security findings were detected in this PR
Security scan by Jit
|
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
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.
Motivation
The integration suite runs under
pytest -n auto, and now and then a single matrix job fails on a test that passes everywhere else — and passes again on rerun. The most recent example wastest_hybrid_query_with_geo_filterreturning 0 results instead of 3 on exactly one Python/redis-py/Redis combination. These failures read as version-specific, but they aren't: they're a test-isolation problem wearing a disguise.The key fact is how the suite isolates workers. Each xdist worker gets its own Redis container, so two tests can only interfere when they run on the same worker and reach for the same Redis key names. For that to actually corrupt a result, two things have to line up:
overwrite=Truebut nodrop=True, leaving stale documents under a reused prefix.When both hold, one test sees another's leftovers — empty results, inflated counts, dtype mismatches — and which test loses depends entirely on the order the scheduler picked. That's the retry-passing, pseudo-random signature.
#543 introduced a
redis_test_namehelper that mints per-test-unique names and began migrating fixtures onto it. This PR finishes that migration for the modules that can genuinely cause flakiness, and tightens their recreate/cleanup so leftovers can't survive.What changed
Recreate group — per-test names +
create(overwrite=True, drop=True):test_hybrid.pyandtest_aggregation.py(these two shared the identicaluser_index_{worker_id}/v1_{worker_id}— the direct cause of the geo-filter failure), plustest_search_results.py,test_svs_integration.py, and the sharedconftest.pyindex fixtures.Fixed-name collisions — per-test names + guaranteed teardown:
from_existing_complex(hard-coded"test") andno_proactive_module_validation("my_index") in the sync + async search-index suites.test_embedcache_warnings.pytests sharing"test_cache"."doc"prefix /"test-index"name acrosstest_no_proactive_module_checks.py.Raw
worker_idcollision — unique names + cleanup:test_llmcache.pytests that both createdfloat64_cache_{worker_id}and never cleaned up.Cluster tests — the one place cross-worker collisions are real, since every cluster test points at a single hard-coded
redis://localhost:7001:test_redis_cluster_support.pyandtest_cluster_pipelining.pynow use per-test names,drop=True, andtry/finallycleanup.Scope and follow-ups
SemanticRouter.delete()leak of its{name}:route_configkey is a product-code gap, not a test-only fix, so it's split into SemanticRouter.delete() leaves orphaned {name}:route_config key in Redis #634 (which also owns the dependenttest_key_separator_handling.pycleanup).Testing
Honest caveat for reviewers: this was developed in an environment without Docker, so I could not run the integration suite.
black,isort, andmypypass. Please runmake testand a repeatedpytest -n autopass to confirm the flake is gone before merging — this is a test-only change, so behavior is fully validated by the suite itself.Note
Low Risk
Changes are confined to the test suite; production library behavior is unchanged.
Overview
Hardens integration tests so parallel
pytest-xdistworkers and shared Redis/cluster endpoints do not leave stale keys or collide on index/cache names.Fixtures and tests now use
redis_test_name(per-test unique index/prefix/cache names) instead ofworker_id-only or hard-coded names like"test"/"my_index"/"test_cache". Sharedconftestindex fixtures (flat_index,hnsw_index, async variants) and modules such astest_hybrid,test_aggregation,test_search_results, andtest_svs_integrationadopt that naming and callcreate(overwrite=True, drop=True)so documents under a reused prefix are cleared before load.Teardown is tightened with
try/finally,delete(drop=True), and cacheclear()/aclear()on failure paths (e.g.from_existing_complex, module-validation tests,test_llmcachedtype caches,test_embedcache_warnings). Cluster tests use per-test names anddrop=Truebecause all workers sharelocalhost:7001.Reviewed by Cursor Bugbot for commit c4e6b33. Bugbot is set up for automated code reviews on this repo. Configure here.