fix: let a connection open when CLIENT SETINFO is denied

[vishal-bala]

RedisVL announces itself on connect with CLIENT SETINFO LIB-NAME and, if that is refused, sends the same string through ECHO. The ECHO call was unguarded, so NoPermissionError escaped while the connection was being created, before any index operation could be attempted:

NoPermissionError: User <name> has no permissions to run the 'echo' command

An application role assembled from @read/@write hits this. CLIENT SETINFO is tagged @connection and @slow, ECHO is @connection and @fast, and neither is in @read or @write — so a rule built up from those categories never grants either.

Subtracting @dangerous is not what denies them. Measured on Redis 8.4.5, +@all -@dangerous permits both, so the shape that fails is a hand-written +@read +@write rule rather than a broad rule with exclusions — which also means Redis Cloud's predefined Read-Write shape is unaffected, while a @read-shaped Read-Only rule would be affected. Note too that +@read +@write +@slow permits CLIENT SETINFO while still denying ECHO, so the fallback was never a reliable second chance.

Why the fallback is deleted rather than guarded

It was added in 934d269 (#155) as a telemetry breadcrumb for servers older than Redis 7.2, where CLIENT SETINFO does not exist, so the library name would at least appear in MONITOR or the slowlog. It earns nothing today:

• It fires only when CLIENT SETINFO errors, and the ACL rule that denies one denies the other.
• Its argument reaches nothing that reads lib-name — CLIENT LIST and CLIENT INFO take that field from SETINFO alone, and an O(1) ECHO only enters the slowlog with slowlog-log-slower-than near zero.
• redis-py already covers the old-server case one layer down, sending its own CLIENT SETINFO during the connection handshake under try/except ResponseError: pass.

The hasattr(client, "echo") guard goes with it, along with the comment claiming RedisCluster has no echo; both redis.cluster.RedisCluster and redis.asyncio.cluster.RedisCluster expose it.

What is kept, deliberately

The four duplicated blocks become one sync and one async helper. The explicit client_setinfo call stays: redis-py's handshake reports its own name, and this overwrites it with the composed redis-py(redisvl_v…;<wrapper>) string that adoption metrics read. The helper docstring says so, so it does not read as redundant with the handshake.

except ResponseError also stays narrow. In the connection-factory path this is the first command on a freshly created connection and therefore the de-facto connectivity check, so broadening to RedisError would swallow ConnectionError and defer a real failure to some later command. AuthenticationError subclasses ConnectionError, not ResponseError, so WRONGPASS and NOAUTH keep propagating either way.

Tests

tests/unit/test_client_identification.py — 16 cases over both twins. Mutation-checked: restoring the ECHO fallback fails six unit cases plus the integration test, and broadening the except to Exception fails two.

Both refusals are covered, since only one is a permission problem: a plain ResponseError stands for a pre-7.2 server, which is the case that lets the fallback go. Identification is asserted on all three URL shapes — sentinel, cluster, standalone — because it sits after that fan-out and moving it into one branch otherwise goes unnoticed; live cluster tests need --run-cluster-tests and never run in CI.

One integration test opens a connection under +@read +@write, with the premise pinned: CLIENT SETINFO must raise NoPermissionError for that user, so the test cannot go vacuous if Redis ever grants it to that role.

ACL user setup moves into an acl_user fixture, reused by the existing -@admin test. Rules are applied after reset because ACL SETUSER is additive and usernames are derived from the test's node id; the user is dropped before the connections it authenticated; and the fixture skips on deployments that reject ACL SETUSER.

Docs

docs/user_guide/installation.md said a credential permitted to run neither command fails at connection time. That is no longer true. The replacement names +client|setinfo for anyone who wants RedisVL attributed in CLIENT LIST, and is explicit that this labels only the connection RedisVL opens — redis-py labels the rest of the pool as plain redis-py.

It also gains a cluster caveat found while verifying this fix: CLUSTER SLOTS is tagged @slow only, so RedisCluster.from_url cannot discover the topology under a +@read +@write rule and the connection fails before identification is attempted, reported as the misleading Redis Cluster cannot be connected. Such a deployment needs +cluster|slots as well.

Not in scope

The rest of the ACL documentation pass. Four statements in installation.md are stale for a different reason — they predate a create_index=False opt-out for the extension constructors — and are corrected alongside it in a follow-up PR.

Identification still reaches only one connection: the explicit call labels whichever pooled connection it borrows, not the rest of the pool or any reconnect, and on a cluster redis-py routes it to the default node alone. Passing the composed name in as redis-py's lib_name/driver_info would fix that, but redis>=5.0,<8.0 straddles the deprecation of lib_name in favour of driver_info and needs version-conditional handling.

---

Note

Low Risk
Narrow change to cosmetic client identification on connect; real connectivity and auth errors still propagate, with broad unit and integration coverage.

Overview
Fixes connection setup for ACL roles built from +@read +@write, where CLIENT SETINFO (and the old ECHO fallback) were denied and could abort connect before any index work.

Connection behavior: Duplicated identification logic is consolidated into _identify_client / _aidentify_client, which still call CLIENT SETINFO LIB-NAME for adoption metrics but ignore ResponseError (ACL denial or pre-7.2 servers) and log at debug. The ECHO fallback is removed so a denied ECHO no longer surfaces as NoPermissionError. ConnectionError on that first command is not swallowed.

Tests: New acl_user fixture for temporary ACL users; integration test that +@read +@write can connect without identification permission; large test_client_identification.py unit suite (sync/async, URL shapes, no ECHO).

Docs: installation.md now states connect succeeds without CLIENT SETINFO, documents optional +client|setinfo, and adds cluster +cluster|slots for +@read +@write roles.

^{Reviewed by Cursor Bugbot for commit ce519f7. Bugbot is set up for automated code reviews on this repo. Configure here.}

[nkanu17]

lgtm!