[zuozhubinge]

Description

Fixes #4003.

RedisStoragePlugin in mooncake-transfer-engine/src/transfer_metadata_plugin.cpp
held a single long-lived redisContext and never rebuilt it. hiredis latches any
I/O error into the context and rejects every subsequent command, so one
disconnect permanently disabled metadata publishing and lookup for the rest of the
process lifetime: get() / set() / remove() returned false forever, segment
registration and lookup failed, and only restarting the process recovered.

We hit this in production with a managed Redis reached through a TCP load balancer
that reclaims connections idle for more than ~4000s. The connection is only touched
on segment (un)registration and on the first lookup of a given remote segment, so it
sits idle for hours; with no TCP keepalive nothing is sent during that window and the
load balancer silently reclaims the socket. Redis itself stayed healthy the whole
time — the broken state lived entirely in the client's cached context, so restarting
Redis did not help.

What changed

1. Reconnect and retry once on connection-level failure (execLocked). Only null
   replies are retried: a REDIS_REPLY_ERROR means the server answered and rejected
   the command, so retrying cannot help. GET/SET/DEL are idempotent, so the retry is
   safe.
2. AUTH/SELECT moved into the connect path so they are replayed on every
   reconnect, and a construction-time failure is no longer terminal. Previously, a
   Redis that was unreachable at startup left client_ null forever.
3. Connect timeout, command timeout and TCP keepalive. There was previously no
   timeout at all, so a slow or blackholed Redis could block the caller indefinitely
   while holding access_client_mutex_. The 3s values match the Redis client already
   used by Mooncake Store HA
   (mooncake-store/src/ha/common/redis/redis_connection.cpp). Keepalive matters
   because this connection is idle for long stretches, and the command timeout is what
   makes recovery work when a middlebox drops the flow without resetting it.
4. getWithStatus() routed through the same retry path. [Bugfix][TransferEngine] Invalidate stale remote segments in syncSegmentCache #3072 added that override
   so syncSegmentCache() can tell an absent key from a backend outage, but it issued
   its own redisCommand() with if (!client_) return kUnavailable; and no reconnect.
   Rebasing onto current main without touching it would have left [Bug]: Redis metadata connection never recovers after a disconnect, permanently breaking segment registration #4003 reproducible
   through that entry point, and syncSegmentCache() calls it periodically. get() now
   delegates to it, which also removes the duplicated GET handling. A null reply after
   execLocked() means the backend is unreachable, so it maps to kUnavailable and
   never to kNotFound, preserving the invariant [Bugfix][TransferEngine] Invalidate stale remote segments in syncSegmentCache #3072 added.
5. CI now executes the Redis regression tests. The job that runs ctest configured
   the tree without -DUSE_REDIS=ON, so the suite was compiled but never run; the job
   that enables USE_REDIS builds without running ctest. Adding the flag is enough —
   dependencies.sh already installs libhiredis-dev on that runner and the top-level
   CMakeLists.txt calls enable_testing().

No public interface changes. MetadataStoragePlugin::Create() could already return a
plugin with a dead connection, and no caller inspects connectivity, so behaviour for
callers is unchanged apart from now recovering.

One observable change for operators: get() now logs a nil reply at WARNING instead of
ERROR. An absent key is an expected outcome when resolving a segment that is not
registered yet, so the previous level reported a normal lookup miss as an error. The
message text is unchanged (RedisStoragePlugin: unable to get <key> from <uri>), so
anything matching on the text still matches, but alerting that keys on ERROR level will
no longer fire for lookup misses. The connection-level failure path — which is what
actually indicates a problem — is unaffected.

Module

• Transfer Engine (mooncake-transfer-engine)
• Mooncake Store (mooncake-store)
• Reshard (mooncake-reshard)
• Mooncake EP (mooncake-ep)
• Mooncake PG (mooncake-pg)
• Integration (mooncake-integration)
• P2P Store (mooncake-p2p-store)
• Python Wheel (mooncake-wheel)
• Common (mooncake-common)
• Mooncake RL (mooncake-rl)
• CI/CD
• Docs
• Other

Type of Change

• Bug fix
• New feature
• Refactor
• Breaking change
• Documentation update
• Performance improvement
• Other

How Has This Been Tested?

Added mooncake-transfer-engine/tests/transfer_metadata_redis_plugin_test.cpp, which
drives the plugin through the public MetadataStoragePlugin::Create("redis://...")
factory against an in-process RESP fake server. A real Redis cannot be scripted to
drop a connection at an exact moment, which is precisely the condition under test, and
the fake keeps the test hermetic (no Redis server required in CI).

Cases covered:

• set/get/remove round trip
• set recovers after the server drops the connection
• get recovers after the server drops the connection
• getWithStatus recovers after the server drops the connection
• getWithStatus reports kNotFound for an absent key while the server is up, and
  kUnavailable — never kNotFound — once the server is gone
• remove recovers after the server drops the connection
• recovery when Redis is unavailable at construction time and comes up later
• AUTH and SELECT are replayed on reconnect
• operations still fail, and fail fast, when Redis stays down

Test commands:

cmake -S mooncake-transfer-engine -B build -G Ninja \
  -DUSE_REDIS=ON -DUSE_HTTP=ON -DUSE_ETCD=OFF -DBUILD_UNIT_TESTS=ON
cmake --build build -j"$(nproc)" --target transfer_metadata_redis_plugin_test
./build/tests/transfer_metadata_redis_plugin_test

# and again under ASan + UBSan
cmake -S mooncake-transfer-engine -B build-asan -G Ninja \
  -DUSE_REDIS=ON -DUSE_HTTP=ON -DUSE_ETCD=OFF -DBUILD_UNIT_TESTS=ON \
  -DCMAKE_BUILD_TYPE=Debug \
  -DCMAKE_CXX_FLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer -g"
cmake --build build-asan -j"$(nproc)" --target transfer_metadata_redis_plugin_test
./build-asan/tests/transfer_metadata_redis_plugin_test

The binary is invoked directly above because enable_testing() is only called from the
top-level CMakeLists.txt, so a standalone Transfer Engine configure builds the test
target but registers no CTest tests. In CI the suite now runs through ctest from the
repository-root configure, because this PR adds -DUSE_REDIS=ON to the job that runs
it.

Test results:

• Unit tests pass — 9/9, 167 ms total
• Integration tests pass (if applicable)
• Manual testing done — see production validation below

[==========] Running 9 tests from 1 test suite.
[       OK ] RedisStoragePluginTest.SetGetRemoveRoundTrip (0 ms)
[       OK ] RedisStoragePluginTest.SetSucceedsAfterServerDropsConnection (21 ms)
[       OK ] RedisStoragePluginTest.GetSucceedsAfterServerDropsConnection (21 ms)
[       OK ] RedisStoragePluginTest.GetWithStatusRecoversAfterServerDropsConnection (21 ms)
[       OK ] RedisStoragePluginTest.GetWithStatusDistinguishesAbsentKeyFromOutage (20 ms)
[       OK ] RedisStoragePluginTest.RemoveSucceedsAfterServerDropsConnection (21 ms)
[       OK ] RedisStoragePluginTest.RecoversWhenRedisIsDownAtConstruction (20 ms)
[       OK ] RedisStoragePluginTest.AuthAndSelectAreReplayedOnReconnect (21 ms)
[       OK ] RedisStoragePluginTest.FailsWhenRedisStaysDown (20 ms)
[  PASSED  ] 9 tests.

The run emits the expected recovery diagnostics, e.g. RedisStoragePlugin: SET mooncake/node-1 failed on 127.0.0.1:17885: Server closed the connection; reconnecting and retrying once, followed by a successful retry.

The same 9 cases also pass under -fsanitize=address,undefined (190 ms, no
sanitizer reports and no LeakSanitizer findings at exit), which covers the
redisContext and redisReply lifetimes on the reconnect path.

These tests exercise the reconnect path via a peer that closes or refuses the
connection, so they cover the latched-error case rather than the silent-blackhole
case; the command timeout is therefore not exercised by them, which is also why the
whole suite finishes in 167 ms with no stalls.

Production validation: the reconnect, timeout and keepalive changes have 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
described in #4003, where idle connections are reclaimed after roughly 4000s. Before the
patch the failure was fully deterministic there: SGLang (Store client) was restarted
roughly every 4 hours, and every restart reproduced it. The two intervals are different
things — the load balancer reclaims the connection about 4000s after the last Redis
access, but the dead context is only discovered at the next metadata operation, and a
restart is what forces those segment registrations and lookups to happen. Once
discovered, metadata publishing and lookup stayed broken for the remaining lifetime of
that process. After the patch the same restart cadence has not reproduced it once over
the 5-day run; a reclaimed connection now shows up only as a single warning followed by
a successful retry:

RedisStoragePlugin: SET mooncake/ram/<segment> failed on <host>:6379: Server closed
the connection; reconnecting and retrying once

To be precise about what that run covers: the deployed build predates getWithStatus()
(added by #3072), so the routing of that entry point through the same retry path is
covered by the unit tests above rather than by the production run.

CI coverage: the job that runs ctest previously configured without -DUSE_REDIS=ON,
so these tests were compiled but never executed. This PR adds the flag to that job, so
transfer_metadata_redis_plugin_test now runs in CI. It needs no Redis server.

Checklist

• I have performed a self-review of my own code
• I have formatted my code using ./scripts/code_format.sh
• I have run pre-commit on the files changed in this PR and all hooks pass
• I have updated the documentation (if applicable) — no user-facing interface
  change; happy to document the timeout values if they should be configurable
• I have added tests to prove my changes are effective
• For changes >500 LOC: I have filed an RFC issue — non-test changes are ~245
  lines in a single file and this is a bug fix rather than an architectural
  change, so no RFC was filed. Issue [Bug]: Redis metadata connection never recovers after a disconnect, permanently breaking segment registration #4003 describes the bug.

Known limitations, called out deliberately

• No reconnect backoff. While Redis is unreachable, every operation now attempts a
  fresh connection instead of failing immediately, and it does so while holding
  access_client_mutex_. Worst case for a blackholed Redis is roughly command timeout
  plus reconnect plus command timeout. This is still bounded, whereas before this PR
  the same situation could block forever with no timeout at all, but if maintainers
  prefer a cooldown (skip reconnect attempts for N ms after a failure) I am glad to
  add one.
• SIGPIPE. Writing to a socket the peer has closed can raise SIGPIPE on Linux, and
  no Mooncake code installs SIG_IGN. This exposure is unchanged by this PR — the
  pre-existing code issued the same write on the same dead socket — so it is not
  addressed here, but it seems worth a separate look.
• set/remove do not inspect reply->type. A server-level error such as
  -READONLY during a failover is still reported as success. This is pre-existing
  behaviour, left untouched to keep the diff focused, and tracked separately as
  requested.

AI Assistance Disclosure

• No AI tools were used
• AI tools were used (specify below)

AI assistance was used for code review, test scaffolding, and drafting this
description. The bug was diagnosed from a production incident on our side. I have
reviewed every changed line and can explain and defend the change end-to-end.

[zuozhubinge]

Note: #4015 covers the same bug and was opened first. I've left a comment there comparing the two implementations — #4042 additionally adds connect/command timeouts, TCP keepalive, and hermetic tests that actually drop a connection (also clean under ASan/UBSan), and has been validated for 5 days in a PD-disaggregated production deployment. Deferring to maintainers on how to combine the two.

[zuozhubinge]

CI note: one real failure, not two — CI Gate is just the aggregator (needs every job group, if: always(), 3s), so it is red because of the job below.

Failing job: test-wheel-ubuntu (ubuntu-22.04, 3.12), step Run all PG CPU tests with latest PyTorch, test test_pg_init_functional.TestMooncakePGInitFunctionalCPU.test_destroy_and_reinit (AssertionError: 1 != 10, 47 tests, 1 failure, 2 skipped).

This looks like a mooncake-pg control-plane race on destroy + re-init, not something this PR can reach:

• The expected allreduce result is 10 (1+2+3+4 over a world of 4) and the observed value is 1, i.e. after re-init the rank's group contained only itself — the group view never converged.
• Immediately before the failure: [AGENT] applyGroupView group=mooncake_pg_1 NOT FOUND in groups_ (epoch=6), (epoch=7), (epoch=14) and [AGENT] Ignored stale GroupView for group=mooncake_pg_1 epoch=10 (agent.cpp:164/:177) — out-of-order GroupView epochs across the destroy/re-init boundary.
• The wheel build does not pass -DUSE_REDIS=ON, so the entire RedisStoragePlugin block this PR modifies sits behind #ifdef USE_REDIS and is compiled out in that job; the new test target is behind if(USE_REDIS) in tests/CMakeLists.txt too.
• The transfer_metadata.cpp:1295] Local segment descriptor not found lines in that log appear in every passing test as well (normal P2P-handshake noise), so they are unrelated to the failure.
• The same job also skips test_inplace_rejoin_collective / test_inplace_rejoin_p2p because the fault-injection preload shim fails to build with glog/logging.h: No such file or directory — another pre-existing environment gap in that job.

The sibling test-wheel-ubuntu (ubuntu-24.04, 3.10) job, CTest unit tests (Python 3.12) and the other 20+ jobs all pass. Could a maintainer re-run that job? Happy to dig further if it reproduces.

[he-yufeng]

Read the full diff and built + ran the new test suite locally on this branch (mc-dev container): 7/7 pass, and the log lines show the exact failure mode being exercised (server drops connection mid-run, reconnect, retry succeeds; down at construction recovers; auth + SELECT replayed on the fresh socket; stays-down fails clean).

The shape is the right one for this bug class. hiredis latching every later command onto a dead context is exactly why a single idle-timeout used to be a permanent outage, and rebuilding on demand with one retry over idempotent ops is the minimal correct answer. The things I specifically checked:

• The retry only fires on a null reply (connection-level), never on REDIS_REPLY_ERROR, so a server-side rejection can't be silently re-executed.
• AUTH and SELECT are replayed on every reconnect, not just at construction, which is required for a managed Redis behind a balancer.
• SO_KEEPALIVE failure is a warning, not fatal; the connection still works, it just can't defend against middlebox idle drops by itself. Reasonable degradation.
• The constructor no longer hard-fails when Redis is briefly down at startup; every operation re-attempts. That changes the startup contract in a good way and the RecoversWhenRedisIsDownAtConstruction test pins it.
• The timeout constants match the store-side coordinator client you reference (redis_connection.cpp:20-21), confirmed against main.

One non-blocking observation: the get() nil reply now logs at WARNING where it used to be ERROR, and the comment explains why (absent key is an expected lookup miss). Worth a line in the PR description since it changes log semantics for existing users.

Nice work, and the in-process RESP fake is the right call for this test.

[zuozhubinge]

Thanks for the thorough read, and for building and running the suite locally.

Added the log-semantics note to the description as suggested: get() logs a nil reply at WARNING instead of ERROR, since an absent key is an expected outcome when resolving a segment that is not registered yet. The message text is unchanged, so text-based matching still works, but ERROR-level alerting will no longer fire on lookup misses; the connection-level failure path is untouched.

Two things still open that I cannot do from a fork: the one red job is the unrelated mooncake-pg test_destroy_and_reinit failure described above and needs a re-run, and the review requirement is not satisfied yet. @alogfans could you take a look when you have a moment, since this path is Transfer Engine?

[alogfans]

I agree that this is the more complete fix for #4003. Before merging, please:

1. Update the branch to the latest main and rerun CI.
2. Rerun the failed PG test_destroy_and_reinit job; it appears unrelated to this Redis path, but the required checks should be green.
3. Ensure transfer_metadata_redis_plugin_test is actually executed in CI with USE_REDIS=ON, rather than only compiled.

The existing behavior where SET/DEL treat REDIS_REPLY_ERROR as success should also be tracked as a follow-up, especially for READONLY responses during failover.

Once the branch is updated and the Redis regression tests run in CI, I’m comfortable approving this PR and treating #4015 as superseded.

[zuozhubinge]

Thanks — all four points are addressed. Pushed as three commits so the new work is
separable from the original fix.

1. Updated to latest main. Rebased onto 662cd44d (40 commits), and refreshed again
onto 6cc1a100 afterwards. CI runs on each new head.

2. The PG job. The force-push started a fresh run, so test_destroy_and_reinit gets
re-executed without needing a manual re-run.

3. transfer_metadata_redis_plugin_test now executes in CI. 8097e889 adds
-DUSE_REDIS=ON to the job that runs ctest. That is all that was needed:
dependencies.sh already installs libhiredis-dev on that runner, the top-level
CMakeLists.txt calls enable_testing(), and the ctest invocation has no -R/-E
filter. The suite needs no Redis server — it drives an in-process RESP fake.

4. SET/DEL and REDIS_REPLY_ERROR. Filed as a follow-up: #4099. It covers the
silent-write-loss shape (-READONLY on a promoted replica, plus -OOM / -MISCONF /
-NOAUTH), why retrying is not the fix, and that get() currently sends error replies to
the JSON parser. I am happy to take it.

One change beyond your list, which the rebase forced — please look at f75036ea.

#3072 added RedisStoragePlugin::getWithStatus() so syncSegmentCache() can tell an
absent key from a backend outage. That override issues its own redisCommand() with
if (!client_) return kUnavailable; and no reconnect. The rebase merged cleanly with no
textual conflict, which is exactly what made it worth checking: a mechanical rebase would
have left #4003 reproducible through that entry point, and syncSegmentCache() calls it
periodically. Once hiredis latched an I/O error, every later getWithStatus() would have
returned kUnavailable for the rest of the process lifetime.

So getWithStatus() now goes through the same execLocked() retry path, and get()
delegates to it, which also removes the duplicated GET handling. A null reply after
execLocked() means the backend is unreachable, so it maps to kUnavailable and never to
kNotFound — the invariant #3072 was protecting. Two tests pin it:
GetWithStatusRecoversAfterServerDropsConnection, and
GetWithStatusDistinguishesAbsentKeyFromOutage, which asserts kNotFound for an absent
key while the server is up and kUnavailable once it is gone.

Local runs on the rebased branch: 9/9 in 167 ms, and 9/9 under
-fsanitize=address,undefined with no sanitizer reports and no LeakSanitizer findings.
Description updated with the new case list and results.

Two notes on accuracy while I was there: the 5-day production run covers the reconnect,
timeout and keepalive changes only — the deployed build predates getWithStatus(), so
that routing is covered by unit tests rather than by the production run. And the
description previously said the Redis tests were not executed in CI, which this PR now
changes; that paragraph is corrected.

@he-yufeng your approval predates these two commits — the new code is all in f75036ea
and 8097e889 if you want to re-check just those.

[zuozhubinge]

All checks are green on 8097e88.

Rebased again onto the latest main (6cc1a10), which also re-triggered CI. The Redis
suite is executed by ctest now, not just compiled:

Start  35: transfer_metadata_redis_plugin_test
32/201 Test  #35: transfer_metadata_redis_plugin_test ...   Passed    0.18 sec
100% tests passed, 0 tests failed out of 201

The intermediate run had one red test, MasterServiceSSDSnapshotTest.EvictObject in
master_service_ssd_test_for_snapshot. It did not recur here. For the record it looks
unrelated to this PR: no C++ source under mooncake-store/ reads the USE_REDIS macro
(the store's own switch is STORE_USE_REDIS, off in that job), the flag adds exactly one
0.18s test to the run, the same test fails on #4092, and the suite has prior race fixes in
#3813 and #3549.

Item 4 from your list is already handled: I filed #4099 and @he-yufeng has fixed it in
#4107, including a reproduction with a fake Redis answering -READONLY. Thanks for the
quick turnaround on that.

One practical note on merge order, since #4107 and this PR both touch
transfer_metadata_plugin.cpp. #4107 is written against main, so it adds a
REDIS_REPLY_ERROR check in four places, including inside get(). Under this PR get()
no longer issues its own command — it delegates to getWithStatus() — so that particular
hunk has no landing place here and becomes redundant, while the getWithStatus() hunk
anchors on context this PR rewrote. Whichever lands second will need a small rebase. I am
happy to do that rebase either way: if #4107 goes first I will adapt this branch, and if
this one goes first I can send the REDIS_REPLY_ERROR checks as a follow-up on top so
@he-yufeng does not have to redo the work.

Nothing outstanding on my side otherwise — happy to make further changes if you spot
anything.

[alogfans]

LGTM

[zuozhubinge]

@stmatengss @ykwd @Aionw @Ann-1024 @luketong777

This is blocked only on code-owner review. All of @alogfans's merge conditions are met and CI is green — could one of you approve when you have a moment? Thanks.

[zuozhubinge]

@alogfans This is now blocked solely on .github code-owner approval for the one-line -DUSE_REDIS=ON CI enable — the change you asked for as condition #3. Your approval already covers the transfer-engine code, and CI is green.

Two ways forward, whichever you prefer:

1. One of the .github owners (@stmatengss @ykwd @Ann-1024 @luketong777 @Aionw) approves the CI hunk and this merges as-is; or
2. I split the ci.yml line into a separate follow-up PR so the reconnect fix can land under your approval now, and the CI-enable goes through the infra owners on its own.

Happy to do the split if that's easier — just let me know.

[zuozhubinge]

Went with option 2 to unblock this. I moved the one-line -DUSE_REDIS=ON CI enable out to a separate follow-up PR: #4274.

This PR now touches only mooncake-transfer-engine/ files, so it no longer requires .github code-owner review — @alogfans's approval covers the whole diff and the PR is now APPROVED. CI is re-running after the force-push (dropping the ci.yml commit); no code changed otherwise.

@alogfans once CI is green, could you merge this (or enable auto-merge)? I don't have merge rights on the repo. The CI-enable in #4274 can go through the .github owners on its own schedule — it no longer blocks the fix.

[zuozhubinge]

@Aionw done — folded the -DUSE_REDIS=ON CI enable back into this PR (commit 9ed89a42), and closed #4274. This PR now contains both the transfer-engine reconnect fix and the CI change in one place, as you preferred.

Since it touches .github/workflows/ci.yml again, it needs a .github code-owner review — could you take a look? CI is re-running with the flag, which exercises transfer_metadata_redis_plugin_test under ctest (verified green previously).

[zuozhubinge]

Gentle bump — green and idle since Sep 23.

This is now blocked solely on a .github code-owner review. The Transfer Engine code is already approved by @alogfans and all CI is green (incl. transfer_metadata_redis_plugin_test under -DUSE_REDIS=ON). The only remaining hunk needing a .github owner is the one-line enable in 9ed89a42 (.github/workflows/ci.yml, -DUSE_REDIS=ON) — the change you asked me to fold back in, @Aionw. Could you (or another .github owner: @stmatengss @ykwd @Ann-1024 @luketong777) approve it?

I don't have merge rights, so once that approval lands, could a maintainer merge or enable auto-merge? Thanks!

[zuozhubinge]

@Aionw @alogfans — gentle follow-up: this has been idle for six weeks (last change Sep 23) and four since the bump above. It's still green and merges cleanly into current main. The only thing blocking it is a .github code-owner approval for the one-line -DUSE_REDIS=ON in 9ed89a42.

That line is still needed. Current main doesn't run transfer_metadata_redis_plugin_test anywhere: the existing USE_REDIS=ON builds are in the build-only TE job and in the #4972 job with BUILD_UNIT_TESTS=OFF.

To stop holding up the reconnect fix: if no .github owner has a chance to approve by Monday, Nov 9, I'll move the CI line back out to a separate PR so this one can merge under @alogfans's existing approval. Either way works for me, just let me know.

[Aionw]

LGTM for ci side.