From ce14afe80bce793875a759cb0633b66f750006c1 Mon Sep 17 00:00:00 2001 From: ngochuyinfo-crypto Date: Tue, 23 Jun 2026 20:15:41 +0700 Subject: [PATCH] fix(router): remove orphaned :route_config key on SemanticRouter.delete() SemanticRouter persists its config at `{name}:route_config` on init and on route mutations, but `delete()` only dropped the search index and left that JSON key stranded in Redis. A later `from_existing(name)` could then read stale config from a router that was supposedly deleted. `delete()` now also removes the `{name}:route_config` key so deletion fully removes router state. Tests: - Add `test_delete_removes_route_config_key` asserting the config key is gone after `delete()` and that `from_existing()` raises instead of reading stale config. - Wrap the fixed-name routers in `test_key_separator_handling.py` (`test_router_sep`, `router_sep_test`, `router_trailing_test`) in try/finally so they clean up their index and config key after the test. Fixes #634 --- redisvl/extensions/router/semantic.py | 3 +- .../test_key_separator_handling.py | 77 +++++++++++-------- tests/integration/test_semantic_router.py | 30 ++++++++ 3 files changed, 77 insertions(+), 33 deletions(-) diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index c6d336564..c40ac443e 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -577,8 +577,9 @@ def remove_route(self, route_name: str) -> None: self._update_router_state() def delete(self) -> None: - """Delete the semantic router index.""" + """Delete the semantic router index and its persisted config.""" self._index.delete(drop=True) + self._index.client.delete(f"{self.name}:route_config") # type: ignore def clear(self) -> None: """Flush all routes from the semantic router index.""" diff --git a/tests/integration/test_key_separator_handling.py b/tests/integration/test_key_separator_handling.py index f4e7836ff..7dd64533f 100644 --- a/tests/integration/test_key_separator_handling.py +++ b/tests/integration/test_key_separator_handling.py @@ -99,21 +99,26 @@ def test_semantic_router_uses_index_separator(self, redis_url): overwrite=True, ) - # Modify the index schema to use custom separator - router._index.schema.index.key_separator = "|" - router._index.schema.index.prefix = "router" + try: + # Modify the index schema to use custom separator + router._index.schema.index.key_separator = "|" + router._index.schema.index.prefix = "router" - # Check that route reference keys use the custom separator - route_key = router._route_ref_key(router._index, "test_route", "ref123") + # Check that route reference keys use the custom separator + route_key = router._route_ref_key(router._index, "test_route", "ref123") - # Should use custom separator - assert "|" in route_key, f"Route key doesn't use custom separator: {route_key}" - assert ( - route_key.count(":") == 0 - ), f"Route key uses default separator: {route_key}" - assert ( - route_key == "router|test_route|ref123" - ), f"Unexpected route key: {route_key}" + # Should use custom separator + assert ( + "|" in route_key + ), f"Route key doesn't use custom separator: {route_key}" + assert ( + route_key.count(":") == 0 + ), f"Route key uses default separator: {route_key}" + assert ( + route_key == "router|test_route|ref123" + ), f"Unexpected route key: {route_key}" + finally: + router.delete() def test_prefix_with_separator_and_custom_separator(self): """Test handling when prefix contains old separator and we use a new one.""" @@ -204,19 +209,22 @@ def test_router_respects_modified_key_separator(self, redis_url): overwrite=True, ) - # Test with different separators - for separator in [":", "-", "_", "|"]: - router._index.schema.index.key_separator = separator - router._index.schema.index.prefix = "routes" + try: + # Test with different separators + for separator in [":", "-", "_", "|"]: + router._index.schema.index.key_separator = separator + router._index.schema.index.prefix = "routes" - # Test internal key generation - route_key = router._route_ref_key(router._index, "route1", "ref1") + # Test internal key generation + route_key = router._route_ref_key(router._index, "route1", "ref1") - # Should use the configured separator - expected = f"routes{separator}route1{separator}ref1" - assert ( - route_key == expected - ), f"For sep '{separator}': Expected '{expected}' but got '{route_key}'" + # Should use the configured separator + expected = f"routes{separator}route1{separator}ref1" + assert ( + route_key == expected + ), f"For sep '{separator}': Expected '{expected}' but got '{route_key}'" + finally: + router.delete() def test_router_with_prefix_ending_in_separator(self, redis_url): """Test SemanticRouter when prefix ends with separator.""" @@ -231,16 +239,21 @@ def test_router_with_prefix_ending_in_separator(self, redis_url): overwrite=True, ) - # Modify to have prefix ending with separator - router._index.schema.index.prefix = "routes:" - router._index.schema.index.key_separator = ":" + try: + # Modify to have prefix ending with separator + router._index.schema.index.prefix = "routes:" + router._index.schema.index.key_separator = ":" - # Generate a route key - route_key = router._route_ref_key(router._index, "route1", "ref1") + # Generate a route key + route_key = router._route_ref_key(router._index, "route1", "ref1") - # Should not have double separator - assert "::" not in route_key, f"Route key has double separator: {route_key}" - assert route_key == "routes:route1:ref1", f"Unexpected route key: {route_key}" + # Should not have double separator + assert "::" not in route_key, f"Route key has double separator: {route_key}" + assert ( + route_key == "routes:route1:ref1" + ), f"Unexpected route key: {route_key}" + finally: + router.delete() class TestSearchIndexKeyConstruction: diff --git a/tests/integration/test_semantic_router.py b/tests/integration/test_semantic_router.py index 872cb531c..9cc7a88ce 100644 --- a/tests/integration/test_semantic_router.py +++ b/tests/integration/test_semantic_router.py @@ -241,6 +241,36 @@ def test_add_route_survives_from_existing( router.delete() +def test_delete_removes_route_config_key( + client, redis_url, routes, redis_test_name, hf_vectorizer +): + """router.delete() must also remove the {name}:route_config JSON key, + not just drop the search index (see issue #634).""" + skip_if_no_redis_search(client) + + router = SemanticRouter( + name=redis_test_name("test_delete_config"), + routes=routes, + routing_config=RoutingConfig(max_k=2), + redis_client=client, + overwrite=True, + vectorizer=hf_vectorizer, + ) + config_key = f"{router.name}:route_config" + + # Config key is persisted on init. + assert client.exists(config_key) == 1 + + router.delete() + + # After delete(), no orphaned config key remains. + assert client.exists(config_key) == 0 + + # from_existing() on a deleted router must not read stale config. + with pytest.raises(Exception): + SemanticRouter.from_existing(name=router.name, redis_client=client) + + def test_add_route_duplicate_raises(semantic_router): duplicate = Route( name="greeting",