[Bugfix][TransferEngine] Reconnect Redis context after I/O error (#4003)

[XFDG]

Description

Fixes #4003.

RedisStoragePlugin holds a single long-lived redisContext and never rebuilds it. hiredis latches any I/O error into the context (ctx->err != 0) and then rejects every subsequent command — so a single disconnect (e.g. a load-balancer idle timeout) permanently disables metadata publishing and lookup for the rest of the process lifetime:

get()/set()/remove() return false forever
redisCommand() returns nullptr on every call

Root Cause

RedisStoragePlugin never stored host / port and had no reconnect path. After any REDIS_ERR_IO, the stale context was reused indefinitely.

Fix

• Store host_, port_, username_, password_, db_index_ at construction time.
• Add reconnect(): redisFree → redisConnect → optional AUTH → optional SELECT. Returns false on failure (never crashes).
• Add ensureConnected(): checks ctx->err before each operation; calls reconnect() if non-zero.
• get(), set(), remove(): call ensureConnected() first; on nullptr reply, do one reconnect() + retry (handles mid-flight drops).

Testing

• RedisStoragePluginTest/BasicRoundTrip: SET/GET/DEL round-trip; skipped via GTEST_SKIP() when MC_REDIS_TEST_SERVER is unreachable.
• RedisStoragePluginTest/PluginWorksAfterInitialConnectionError: verifies graceful false (no crash) when server is unreachable from start.

[zuozhubinge]

Thanks @XFDG — I filed #4003 and opened #4042 for the same bug before noticing this
PR was already open (mine went up about 18 hours later; per AGENTS.md I should have
checked for an existing PR first, apologies for the overlap).

The two fixes share the same core: remember the connection parameters, rebuild the
context, retry once. Rather than duplicate the work, here are the three things #4042
has that this PR does not, so maintainers can decide how they would like the two
combined.

1. Connect and command timeouts. This is the part that matters most for the
incident in #4003. Our metadata Redis sits behind a TCP load balancer that reclaims
idle connections. When an L4 load balancer drops the flow silently rather than
resetting it, ctx->err is still 0, so ensureConnected() passes, the command is
issued, and the caller blocks indefinitely while holding access_client_mutex_ —
stalling every other metadata operation in the process. The redisConnect() on the
reconnect path has the same exposure. #4042 sets a 3s connect timeout and a 3s command
timeout, matching the values already used by
mooncake-store/src/ha/common/redis/redis_connection.cpp.

2. TCP keepalive. The plugin only touches Redis on segment (un)registration and on
the first lookup of a given remote segment, so in a steady-state cluster the connection
is idle for hours and nothing is sent during that window — which is exactly why the
load balancer reclaims it. redisEnableKeepAlive() keeps the flow alive so the drop
largely stops happening, instead of only being recovered from afterwards.

3. Tests that actually drop a connection, with no external Redis. The tests here
call GTEST_SKIP() when MC_REDIS_TEST_SERVER is unreachable, so they will be skipped
in CI, and neither case exercises a mid-flight disconnect — the condition the bug is
actually about. A real Redis also cannot be scripted to drop a connection at an exact
moment. #4042 adds an in-process RESP fake that closes established connections on
demand, giving 7 hermetic, deterministic cases: round trip; set, get and remove
each recovering after a drop; recovery when Redis is down at construction time and
comes up later; AUTH/SELECT replayed on reconnect; and fail-fast when Redis stays down.
7/7 in 126 ms, no server required. Because the fake needs no server, the same 7 cases
also run clean under ASan + UBSan (no reports, no LeakSanitizer findings), which is
what gives confidence about the redisContext and redisReply lifetimes on the
reconnect path — a suite that skips without a live Redis cannot provide that.

This exact patch is running in production. It has been cherry-picked onto
v0.3.11.post1 and run for 5 days in a prefill/decode disaggregated deployment on 8 H800
nodes, against the same managed Redis behind an L4 load balancer as in #4003. Before the
patch the failure was deterministic there: SGLang (Store client) was restarted roughly
every 4 hours and every restart reproduced it, because a restart is what forces the first
segment registration and lookup after a long idle window — which is when the reclaimed
connection gets discovered. Over the 5-day run with the patch, the same restart cadence
has not reproduced it once. Worth noting that what has been validated is the combination
— reconnect plus the timeouts and keepalive — so landing the reconnect alone would ship
the part that has not been exercised against a real load balancer.

Credit where due: checking ctx->err up front in ensureConnected() is a bit
cleaner than reacting to the null reply, which is what #4042 does. In practice the two
converge — hiredis returns early on a latched context (redisBufferWrite() bails on
c->err before touching the socket), so the reactive path costs no extra round trip,
just one redundant warning log per drop — but the proactive check states the intent
more explicitly and is worth keeping.

Also worth flagging for whichever PR lands: the CI job that runs ctest configures
without -DUSE_REDIS=ON, and the job that does enable USE_REDIS builds but does not
run ctest — so neither PR's tests execute in CI today. Adding the flag to the ctest
job would fix that, either here or separately.

@alogfans could you advise which route you prefer? Happy to do the work either way.

[alogfans]

Thanks for working on this. After comparing this PR with #4042, I prefer moving forward with #4042.

The main blocker here is that construction-time failures are still permanent: the authenticated constructor returns before saving the credentials when client_ is null, and ensureConnected() also returns false for a null client instead of attempting reconnect(). The current tests only verify graceful failure and do not exercise recovery after Redis becomes available or after an established connection is dropped.

#4042 also covers connect/command timeouts, TCP keepalive, AUTH/SELECT replay, and deterministic disconnect tests. These are important for the silent load-balancer timeout scenario described in #4003.

Thank you for providing the initial implementation and helping clarify the required reconnect behavior.

[XFDG]

Thanks @alogfans for the review and for pointing out the construction-time failure gap. I'll defer to #4042 which handles that case correctly. Closing this PR.