Skip to content

Read, admin and health paths treat server error replies as connection loss (read-side counterpart of #105) #123

Description

@bigsamich

RedisAdapter version or revision

main at ff2d5b8. Unchanged at the #108 → #109 → #111 stack head 0c3b0e6; #108 adds getStreamSnapshot() with the same pattern.

Redis version and deployment mode

Redis 7.4.11 standalone (TCP).

Observed behavior

#109 separates server rejection from transport failure for writes. Every other path still treats a ReplyError from a healthy server as a lost connection:

  • RedisConnection::xrange/xrevrange/ping/del/exists/rename/copy/publish/hexpire return their failure value for any sw::redis::Error, ReplyError included.
  • Their callers pass that result to reconnect(). This covers every get* helper (RedisAdapterTempl.hpp), Add owned stream subscriptions and safe payload decoding #108's getStreamSnapshot(), connected() (reconnect(_redis.ping())), rename/del/exists/publish, copy(), and petWatchdog().

So on a healthy server:

  • getSingleValue/getSingleList on a key holding a string, or on a key outside the user's ACL pattern, return RA_NOT_CONNECTED, and getValues/getLists return an empty list.
  • getStreamSnapshot() on such a key reports connected=false, id="0-0".
  • Each such call launches the full reconnect: a new client and pool (3 new server connections, 4 with an ACL user) and a stop/restart of every reader bucket.
  • A 10 Hz poll of a wrong-type key produces 10 reader restarts and about 30 new TCP connections per second, plus one LOG_ERR per read. On loopback this caused no measurable subscriber loss, only churn and false outage reports.
  • rename() of a missing key, NOPERM on del/exists/copy/publish, and a HEXPIRE refused by ACL in the watchdog (+7 reconnects in 5 s) do the same.
  • Under MISCONF, Redis refuses PING. One connected() probe therefore triggers a reconnect whose own PING fails, and together with A failed reconnect() replaces the working client with nothing; subscriptions stall and the first write after recovery fails #117 every read then returns RA_NOT_CONNECTED while the server is still serving reads.

At the stack head, reads and writes of the same wrong-type key are now classified inconsistently: the write returns RA_REJECTED with no reconnect, and the read returns RA_NOT_CONNECTED plus a reconnect.

Expected: as #105 states for writes, "do not reconnect for server-side ReplyError"; report rejection distinctly from unavailable transport.

Reproduction

control.set("{S8}:wrong", "not-a-stream");
int v;
auto t = adapter.getSingleValue<int>("wrong", v);   // err()==1 (RA_NOT_CONNECTED)
auto s = adapter.getStreamSnapshot("wrong");        // connected=false, id="0-0"
// INFO stats total_connections_received: +3 per call; every reader bucket restarted

Build and runtime environment

  • Ubuntu 24.04, gcc 13.3
  • pinned redis-plus-plus 1.3.15 and hiredis 1.3.0
  • a private Redis 7.4.11 in a container

Suggested fix

Mirror #109 on the read side:

  • Give the read/admin helpers and ping() a status (a trailing CommandStatus* as Separate rejected stream writes from transport failures #109 did for xtrim, or result structs). Catch sw::redis::ReplyError separately as Rejected, and keep other errors as Unavailable.
  • Call reconnect(0) only for Unavailable. Return RA_REJECTED from getSingleValue/getSingleList on Rejected.
  • Give StreamSnapshot a rejection flag or status, and never return a valid replay cursor ("0-0") on failure.
  • Let connected() distinguish "server refuses commands" from "no transport".
  • Add the read counterpart of write_error_test.cpp: set a wrong-type key, call getSingleValue/getStreamSnapshot, and assert that no reconnect occurred and that connected() stays true.

Related: #105, #109, #108 (getStreamSnapshot), #117.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions