[he-yufeng]

Description

Fixes #4099. RedisStoragePlugin::set()/remove() treated any non-null reply as success and never inspected reply->type, so a REDIS_REPLY_ERROR — the server deliberately rejecting the write — was reported to registerSegmentDesc() as a completed publish. The likely trigger is a failover: a promoted replica, or a load balancer briefly routing to one, answers -READONLY, which is a valid reply object, so the write was dropped with no signal anywhere. -OOM at maxmemory, -MISCONF and -NOAUTH take the same silent path.

set()/remove()/get() now check REDIS_REPLY_ERROR and log the server's error string, so -READONLY is distinguishable from a connection failure. getWithStatus() maps it to kUnavailable directly instead of letting the error text reach the JSON parser as a misleading parse error. The check mirrors the AUTH/SELECT handling already in the same class.

No retry is added on purpose: the server answered, so the command was received and rejected; only the reporting was wrong.

Module

• Transfer Engine (mooncake-transfer-engine)

Type of Change

• Bug fix

How Has This Been Tested?

There is no existing test harness for this plugin, so I built one off-tree for this PR: the current RedisStoragePlugin class is extracted verbatim into a test driver at build time (so the binary runs the actual PR text, not a copy), linked against the real hiredis, and driven against a minimal fake Redis server speaking RESP2 that answers -READONLY to every write.

Test commands:

# fake Redis answering -READONLY to every command, then driver with the plugin under test
python3 fake_redis.py <port> error &   # modes: error / ok / miss
./driver <port> error

Test results:

• Stock (unpatched) class on -READONLY: set()=true, remove()=true — the silent write loss, reproduced.
• Patched class on -READONLY: set()=false, remove()=false, get()=false, getWithStatus()=kUnavailable, and the log carries the server's error string (READONLY You can't write against a read only replica.).
• Patched class on normal replies: set()/remove()/get() succeed, getWithStatus()=kFound — no behavior change on the healthy path.
• Patched class on a nil GET: getWithStatus()=kNotFound preserved — authoritative absence still distinguishes from outage, as syncSegmentCache relies on.
• Unit tests pass (none exist for this plugin; see above)
• Manual testing done (described above)

Checklist

• I have performed a self-review of my own code
• I have formatted my code using ./scripts/code_format.sh (pre-commit Format staged C/C++ changes passed on the touched file)
• I have run pre-commit on the files changed in this PR and all hooks pass
• I have updated the documentation (not applicable; no user-facing docs describe the old behavior)
• I have added tests to prove my changes are effective (off-tree harness above; happy to upstream it if you want it in tests/)
• For changes >500 LOC: I have filed an RFC issue

AI Assistance Disclosure

• AI tools were used (specify below)

Prepared with AI assistance (Kimi); the failing-then-passing harness results above are from my local runs, and the submitter has reviewed every changed line.

[alogfans]

LGTM