fix(router): reject an empty reference id instead of matching every one - #718
Merged
Conversation
_make_filter_queries built `Tag("reference_id") == id` with no guard, and
Tag renders any falsy value as the match-all `*`. Both callers read the
first row of each query's results, so an empty id resolved to one
arbitrary reference: get_route_references returned a reference nobody
asked for, and delete_route_references deleted one and reported success.
Reproduced on Redis 8.4 against a four-reference router --
delete_route_references(reference_ids=[""]) removed a real reference key
and returned 1.
Raise ValueError at the shared chokepoint, which covers both public
methods and the path that derives ids by splitting stored keys.
Whitespace-only ids are left alone: they render a clause that matches
nothing, so they already fail safe.
vishal-bala
added a commit
that referenced
this pull request
Sep 9, 2026
…ache (#725) ## Motivation The LangCache integration suite fails semi-randomly in the Service Tests job, on pull requests that touch nothing related to LangCache, and a different test fails each time. A reproducible defect fails the same test every run, so the pattern itself points at a shared resource, not at any one branch: #716 saw `test_store_and_check_async` fail on `assert []`, its stored entry gone; #718 saw two different tests fail, and a third on re-run. Two things combine to cause it. `TestLangCacheSemanticCacheIntegrationWithAttributes` flushed the entire managed cache, through `delete()`, `clear()` and `aclear()`, and the suite runs under `pytest -n auto`, so a flush on one xdist worker wiped entries another worker had stored moments earlier. The fixtures then bind to a single `cache_id` from repo secrets, which every CI run reaches concurrently: pull requests, fork pull requests, pushes to main, and the nightly cron. Runs on separate branches therefore flushed each other. ## Changes ### Whole-cache flushes are removed, and blocked from returning `test_delete_and_clear_alias` is deleted and the trailing `aclear()` is removed from `test_async_delete_variants`. An autouse fixture patches `delete`, `adelete`, `clear` and `aclear` to fail the test, so the rule is now mechanical, not a convention recorded in a docstring. This trades away live coverage of the flush endpoint, which is a constraint and not a preference: no cache shared with other runs can safely be flushed, and a dedicated flushable `cache_id` would collide identically once two runs used it. The wrappers themselves are four one-line calls into the SDK and keep their mocked unit coverage; the flush HTTP path is deliberately untested. ### Every write is scoped and expiring A per-test `scope` token, from `uuid4().hex[:12]`, is threaded into every prompt, response and attribute value, so no test can observe or delete another's entries. Every entry carries a 60-second TTL, which lets the shared caches drain now that nothing flushes them. The TTL is passed per call, not set once on the fixtures, because `store()` ignores a constructor TTL entirely. That defect is filed separately, and the module docstring records it so the repetition is not tidied away into the fixtures. ### The TTL-expiry tests no longer depend on result ranking `num_results` is a client-side slice: `_build_search_kwargs` never sends `max_results`, and the installed SDK documents its own default as one result. Raising it therefore buys no headroom against a concurrent run's semantically identical prompt competing for that single slot. Filtering on a scope-unique attribute does, because the service can only return entries matching it. ```python # Before: reads back whichever single entry the service ranks first. hits = langcache_with_attrs.check(prompt=prompt, num_results=5) # After: the result set is provably this test's own. hits = langcache_with_attrs.check(prompt=prompt, attributes=metadata) ``` Both tests also move from a two-second TTL to five, with the sleep from three seconds to six. Two seconds had to cover a store round trip and a search round trip against a shared managed service, so the assertion that the entry exists was racing its own TTL. Secondary changes: - Delete-by-attribute assertions tighten from `>= 1` to the number of entries actually stored, which scope-unique attributes make knowable. - Scope tokens join punctuation-heavy attribute values with `_` rather than `-`, which survives percent-encoding and is a RediSearch text separator. - The unit-test SDK mock is autospecced, so a renamed method or a changed signature fails there instead of being auto-vivified. - Two unit tests were indented into another test and so were never collected. Dedenting them adds both to the run, and the file now collects 26, having also lost a name-checking test that autospec subsumes. - CONTRIBUTING.md gains a paragraph on namespacing writes to shared, stateful external services. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Test-only and documentation changes; no production LangCache client behavior is modified. > > **Overview** > Stops flaky **LangCache** integration failures caused by many workers and CI jobs sharing the same managed `cache_id`. > > **Whole-cache flush is removed from live tests** and enforced with an autouse fixture that makes `delete`/`clear` (sync and async) fail if called. Integration cases that flushed the cache are dropped; flush behavior stays covered only in **unit** mocks (now **autospecced** against the real SDK). Two previously nested unit tests are dedented so they actually run. > > **Writes are isolated**: per-test `scope` tokens in prompts/responses/attributes, **60s TTL** on routine stores, and assertions that look for *your* scoped row in `hits` (not `hits[0]`). TTL-expiry tests use **scoped attribute filters**, longer TTL/sleep, and tighter delete-by-attribute counts. > > **CONTRIBUTING.md** adds guidance to namespace data and never flush shared external services, pointing at the integration module docstring. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 68d378d. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
vishal-bala
marked this pull request as ready for review
September 10, 2026 07:38
limjoobin
approved these changes
Sep 11, 2026
limjoobin
left a comment
Contributor
There was a problem hiding this comment.
LGTM. This PR makes SemanticRouter reject an empty reference id instead of letting it render as the match-all *, which previously caused get_route_references and delete_route_references to silently read and delete an arbitrary reference the caller did not ask about.
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
SemanticRouter.delete_route_references(reference_ids=[""])deletes a reference the caller never named._make_filter_queriesbuildsTag("reference_id") == idwith no guard onid, andTagrenders any falsy value as the match-all*. Both callers then read the first row of each query's results, so a match-all resolves to one arbitrary reference:get_route_referencesreturns it, anddelete_route_referencesremoves it.Reproduced against Redis 8.4 with a four-reference router:
The blast radius is one reference per empty id rather than the whole route, because each query carries
LIMIT 0 10and the caller takes onlyr[0]. It is still a silent deletion of data the caller did not ask about, and the reported count of1makes it look like the requested delete succeeded.Changes
An empty reference id is rejected rather than matched
_make_filter_queriesraisesValueErroron a falsy id before constructing the expression. Rejecting rather than skipping is deliberate: an empty id in the list means the caller's own id-building went wrong, and quietly dropping it would hide that while still reporting success for the rest of the batch.Both public methods already raise
ValueErrorfor unusable arguments, so the failure mode is unchanged in kind. The guard sits at the shared chokepoint, which coversget_route_referencesanddelete_route_referencestogether, including the path that derives ids by splitting stored keys.Secondary changes
tests/unit/test_semantic_router_queries.py, a new hermetic file:_make_filter_queriesis a static method that touches neither Redis nor a vectorizer, so a regression this consequential should be caught by a test that always runs rather than one behind the router's integration fixtures.Notes
Whitespace-only ids are deliberately left alone.
Tag("reference_id") == " "renders@reference_id:{\ \ }, which matches nothing rather than everything, so it already fails safe. Widening the guard to cover it would change behaviour without fixing a defect.The guard was mutation-checked: removing it alone fails the three new unit cases and the integration test, and nothing else.
Release Notes
An empty reference id no longer resolves to a match-all filter.
SemanticRouter.get_route_referencesanddelete_route_referencesnow raiseValueErrorwhen passed one, instead of matching every reference in the index.This changes behaviour for callers that pass an empty id, which previously succeeded and now raises. Such a call was always operating on the wrong data: an empty id rendered as a match-all filter, so it resolved to one arbitrary reference.
get_route_referencesreturned a reference that was never requested, anddelete_route_referencesdeleted one and reported success. Code that swallowed the old result now sees an exception instead, which is the point.Calls that pass real reference ids are unaffected, as are whitespace-only ids, which have always matched nothing rather than everything.
Next Steps
Note
Medium Risk
Changes behavior for callers passing empty reference ids (now raises) and touches shared delete/lookup paths, but the fix prevents silent incorrect deletions and does not affect valid ids.
Overview
Fixes a data-loss bug where passing an empty reference id to
get_route_referencesordelete_route_referencescould match every reference (viaTag("reference_id") == ""→*) and then operate on the first arbitrary hit—including silently deleting a reference the caller never named._make_filter_queriesnow raisesValueErroron any falsy id (""orNone) before building filters, so both public APIs fail fast instead of succeeding on the wrong row. Rejecting the whole batch when one id is empty is intentional so callers cannot mask bad id construction.Tests: new hermetic unit coverage for filter construction plus an integration test that empty-id calls raise and leave Redis keys unchanged.
Reviewed by Cursor Bugbot for commit b038cb2. Bugbot is set up for automated code reviews on this repo. Configure here.