ci: pin RediSearch WORKERS to 0 to fix flaky redis:latest tests#641
Merged
Conversation
Redis 8.8 changed the default search WORKERS from 0 to a multithreaded background executor (to align with Valkey). With workers > 0, FT.SEARCH races against key/hash-field expiry and can return matched docs with nil field arrays instead of filtering them out, which intermittently fails the redis:latest CI matrix. Set --search-workers 0 on the test Redis container to run search in the foreground and avoid the race. This is already the default on the pinned 8.2/8.4 images, so it is a no-op there, and it is overridable via REDIS_SEARCH_WORKERS. The underlying server bug is being fixed upstream. 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
|
vishal-bala
added a commit
that referenced
this pull request
Jul 2, 2026
## Motivation `TextQuery`, `HybridQuery`, and `AggregateHybridQuery` default to `stopwords="english"` and lazily download the NLTK corpus the first time they need it. Under pytest-xdist, several workers can hit the missing corpus at the same moment and call `nltk.download()` concurrently — their unzips clobber each other in the shared `nltk_data` directory, and a worker then reads a half-written corpus. It surfaces only on Python 3.14 because there `sentence-transformers` is skipped, so nothing warms the corpus serially before the parallel tests start; on 3.10–3.13 the HuggingFace path fetches it first and the race never opens. It's intermittent (only some 3.14 jobs, across `redis:8.2` and `redis:latest` alike), which is the giveaway that Redis has nothing to do with it. ## Fix Two complementary changes, following the pattern the unit tests already use (they pass `stopwords=None` to avoid NLTK entirely): **Pre-download the corpus once, before the workers race for it.** A new session-scoped autouse fixture fetches `stopwords` a single time, serialized across xdist workers with a file lock, so exactly one process does the download and the rest find it already present. This covers every test that legitimately needs english stopwords — including the ones that assert on stopword filtering (`test_empty_query_string`, `test_hybrid_query_stopwords`) and the count-sensitive `TextQuery` tests in `test_query.py`, whose result counts depend on filtering and so must keep real stopwords. Adds `filelock` (already present transitively) as an explicit dev dependency. **Decouple the tests that don't actually need stopwords.** The hybrid tests in `test_aggregation.py` match text with an optional `~@field` clause, so the result set is driven by the vector and filters, not the text tokens — dropping stopwords there changes nothing about the assertions. Passing `stopwords=None` on those constructions removes NLTK from that file except the two tests that specifically exercise the feature, shrinking the surface that can ever race. ## Verification Ran the affected tests under `-n 8` with the stopwords corpus deleted beforehand to simulate a cold runner. The fixture downloads once, no worker sees a partial corpus, and all tests pass — including the count-sensitive `TextQuery` tests, confirming english-stopword filtering is still intact. Independent of the `redis:latest` WORKERS fix (#641); this addresses a separate, pre-existing flake. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Test-only and dev-dependency changes; no production library behavior is modified. > > **Overview** > Fixes intermittent CI failures when parallel pytest workers concurrently download the NLTK **stopwords** corpus into a shared `nltk_data` directory. > > A new session-scoped autouse fixture **`ensure_nltk_stopwords`** in `conftest.py` ensures the corpus exists once before tests run; under **pytest-xdist**, a **`filelock`** serializes the download so only one worker fetches it. **`filelock`** is added as an explicit dev dependency in **`pyproject.toml`** / **`uv.lock`**. > > Hybrid aggregation integration tests that do not assert on stopword behavior now pass **`stopwords=None`** on **`AggregateHybridQuery`** so those cases skip NLTK entirely; tests that exercise stopword filtering keep default or explicit stopword lists. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 17f5435. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
🚀 PR was released in |
applied-ai-release-bot
Bot
added
the
released
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
2 participants
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
Our
redis:latestCI matrix has been flaking intermittently, and we traced it to a real change in Redis 8.8. That release turned on a multithreaded background executor for search by default — theWORKERSsetting went from0to a nonzero value to align with Valkey. It's a deliberate performance improvement, but it exposed a race: when a document's key or an indexed hash field expires (via TTL orHPEXPIRE) at the exact moment anFT.SEARCHis running in the background, the server returns the matched document's id with anilin place of its field array instead of dropping the expired doc. Clients that reasonably assume every matched doc carries a field array then break, which is what our suite was hitting at random.This was confirmed on the Redis side: the same reproduction runs cleanly on 8.2/8.4/8.6 and only fails on 8.8, and forcing
WORKERS > 0on 8.6 reproduces it too. The underlying server bug is being fixed upstream — this change just stabilizes our CI in the meantime.Fix
We pin the search worker pool back to
0on the test Redis container, so searches run in the foreground and the expiry race can't occur. The setting lives in the sharedtests/docker-compose.ymlthat drives every local and CI run, so it applies uniformly across the matrix. Because0is already the default on the pinned 8.2/8.4 images, this is a no-op there and only changes behavior forredis:latest. It's also overridable through theREDIS_SEARCH_WORKERSenvironment variable, so we can flip multithreading back on to validate the upstream fix once it lands.🤖 Generated with Claude Code
Note
Low Risk
Test-only Docker Compose configuration with no application or production runtime impact.
Overview
Stabilizes CI for
redis:latestby forcing RediSearch to run searches on the main thread instead of Redis 8.8’s default multithreaded background executor.In
tests/docker-compose.yml,REDIS_ARGSgains--search-workers ${REDIS_SEARCH_WORKERS:-0}alongside the existing persistence flags. That avoids a race where backgroundFT.SEARCHcan return matched doc IDs withnilfield arrays when keys or hash fields expire mid-query. Pinned 8.2/8.4 images already default workers to0, so behavior is unchanged there; only newer images are affected unless you setREDIS_SEARCH_WORKERS.Reviewed by Cursor Bugbot for commit c08c456. Bugbot is set up for automated code reviews on this repo. Configure here.