Implement CLIENT REPLY ON|OFF|SKIP (#1626)#1801
Conversation
Adds per-session CLIENT REPLY mode support matching Redis semantics:
- ON (default): replies sent normally
- OFF: all replies suppressed until next ON
- SKIP: one-shot suppression of the next command's reply
Suppression is implemented in ProcessMessages by snapshotting the mode
and dcurr before ParseCommand, then rewinding dcurr at end-of-command
when suppressed. SendAndReset also discards mid-command flushes when
suppressed. CLIENT REPLY ON clears the flag inside its handler so its
own +OK survives. Snapshotting before ParseCommand ensures parse-time
errors are also gated.
Files:
- libs/server/Resp/Parser/RespCommand.cs: CLIENT_REPLY enum, parser
case, AofIndependentCommands entry
- libs/server/Resp/ClientCommands.cs: ClientReplyMode enum,
NetworkCLIENTREPLY handler
- libs/server/Resp/RespServerSession.cs: per-session state, snapshot/
rewind hook, SendAndReset discard gate
- libs/server/Resp/CmdStrings.cs: REPLY/SKIP u8 constants
- libs/resources/RespCommands{Info,Docs}.json: CLIENT|REPLY metadata
- playground/CommandInfoUpdater/SupportedCommand.cs: subcommand entry
- test/Garnet.test/ClientReplyTests.cs: 9 raw-TCP NUnit tests
- test/Garnet.test/RespCommandTests.cs: AofIndependent list update
Tests: 9/9 new ClientReply pass on net8.0 and net10.0;
RespCommandTests 49/49 pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds Redis-compatible support for CLIENT REPLY ON|OFF|SKIP in the RESP server layer, enabling per-connection reply suppression for fire-and-forget pipelines while preserving CLIENT REPLY ON’s +OK response semantics.
Changes:
- Added new
RespCommand.CLIENT_REPLYparsing/metadata entries and command-info updater registration. - Implemented per-session reply-mode state in
RespServerSessionand aNetworkCLIENTREPLYhandler for ON/OFF/SKIP. - Added NUnit raw-socket tests covering basic OFF/ON/SKIP behaviors and error cases.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Garnet.test/RespCommandTests.cs | Marks CLIENT_REPLY as AOF-independent in the test list. |
| test/Garnet.test/ClientReplyTests.cs | Adds new raw TCP tests for CLIENT REPLY semantics. |
| playground/CommandInfoUpdater/SupportedCommand.cs | Registers `CLIENT |
| libs/server/Resp/RespServerSession.cs | Adds per-session reply suppression state and integrates suppression into processing and flushing. |
| libs/server/Resp/Parser/RespCommand.cs | Adds CLIENT_REPLY enum value, AOF-independent classification, and parser mapping for CLIENT REPLY. |
| libs/server/Resp/CmdStrings.cs | Adds REPLY/SKIP token constants used by parsing/handler logic. |
| libs/server/Resp/ClientCommands.cs | Introduces ClientReplyMode and implements NetworkCLIENTREPLY. |
| libs/resources/RespCommandsInfo.json | Adds command-info metadata entry for `CLIENT |
| libs/resources/RespCommandsDocs.json | Adds command docs entry for `CLIENT |
Comments suppressed due to low confidence (1)
libs/server/Resp/RespServerSession.cs:1391
- When suppressCurrentReply is true, SendAndReset() always returns without making progress even if the current write can never fit in the fixed response buffer. Many write sites use
while (!TryWrite...) SendAndReset();; for a reply larger than the buffer this becomes an infinite loop under CLIENT REPLY OFF/SKIP. The non-suppression path explicitly throws when no progress is made; suppression should also guarantee forward progress (e.g., by chunk-discarding, or by preserving the same too-large detection/throw behavior while still honoring suppression semantics).
internal void SendAndReset()
{
// CLIENT REPLY OFF/SKIP: discard whatever was written into the current buffer
// without sending it. We deliberately do NOT rotate to a fresh buffer or call Send —
// we just rewind dcurr to the head so the suppressed bytes are dropped.
if (suppressCurrentReply)
{
dcurr = networkSender.GetResponseObjectHead();
return;
}
byte* d = networkSender.GetResponseObjectHead();
if ((int)(dcurr - d) > 0)
{
Send(d);
networkSender.GetResponseObject();
dcurr = networkSender.GetResponseObjectHead();
dend = networkSender.GetResponseObjectTail();
}
else
{
// Reaching here means that we retried SendAndReset without the RespWriteUtils.Write*
// method making any progress. This should only happen when the message being written is
// too large to fit in the response buffer.
GarnetException.Throw("Failed to write to response buffer", LogLevel.Critical);
}
| // CLIENT REPLY OFF/SKIP: discard whatever was written into the current buffer | ||
| // without sending it. We deliberately do NOT rotate to a fresh buffer or call Send — | ||
| // we just rewind dcurr to the head so the suppressed bytes are dropped. | ||
| if (suppressCurrentReply) | ||
| { | ||
| dcurr = networkSender.GetResponseObjectHead(); | ||
| return; | ||
| } | ||
|
|
||
| byte* d = networkSender.GetResponseObjectHead(); |
There was a problem hiding this comment.
Good catch — fixed in 3a034b8. Promoted cmdStartPtr from a local to a per-session field cmdReplyFloor so SendAndReset() can use it as the rewind floor:
- If the suppressed write made progress (
dcurr > cmdReplyFloor): rewind to the floor — bytes in[head, cmdReplyFloor)from prior non-suppressed commands stay intact. - Else if prior replies are queued (
cmdReplyFloor > head):Send([head, cmdReplyFloor)), rotate buffer, setcmdReplyFloor = new head, continue suppressing in the fresh buffer. - Else (no progress and nothing prior): throw — same fatal condition as the non-suppressed path. This also addresses the suppressed comment about infinite loops on oversized suppressed writes.
Also fixed SendAndReset(IMemoryOwner<byte>, int) which had the inverse problem — it would Send() the buffer contents mid-write, flushing this command's earlier bytes before the end-of-command rewind could discard them. Under suppression it now just disposes the payload; the prior bytes remain in the buffer for end-of-batch flush.
Added ClientReplyPipelinedSkipBetweenReplies and ClientReplyPipelinedOffOnOrdering to cover the pipeline ordering case.
| // Burn off a one-shot SKIP. CLIENT REPLY commands themselves never burn the skip — | ||
| // a SKIP issued while already in Skip mode just re-arms (does not stack), and a | ||
| // following SKIP/ON/OFF transitions the mode directly inside the handler. | ||
| if (modeAtStart == ClientReplyMode.Skip && cmd != RespCommand.CLIENT_REPLY && cmd != RespCommand.INVALID) |
There was a problem hiding this comment.
Agreed — fixed in 3a034b8. Dropped the cmd != INVALID guard; SKIP now burns for any fully-received command except CLIENT_REPLY itself. Partial input is still gated by the !commandReceived early-break above. Added ClientReplySkipBurnedByUnknownCommand to lock in the behavior.
| /// <summary> | ||
| /// Tests for the <c>CLIENT REPLY ON|OFF|SKIP</c> subcommand. | ||
| /// Uses a raw TCP socket because StackExchange.Redis cannot tolerate reply suppression. | ||
| /// </summary> |
There was a problem hiding this comment.
Fair point — the description was aspirational. In 3a034b8 I added two pipelining tests (ClientReplyPipelinedSkipBetweenReplies and ClientReplyPipelinedOffOnOrdering) that send multiple commands in a single TCP write and assert exact byte-level reply ordering. I removed the SUBSCRIBE/pub-sub claim from the PR description and noted it as a possible follow-up. Suppression is implemented at the generic response-buffer layer so it applies uniformly across reply types, but a targeted pub/sub test wasn't added here.
| /// <summary>Read until the buffer matches the expected string (or read timeout fires).</summary> | ||
| public string ReadExpected(string expected) | ||
| { | ||
| var sb = new StringBuilder(); | ||
| while (sb.Length < expected.Length) | ||
| { | ||
| var chunk = TryRead(); | ||
| if (chunk.Length == 0) break; | ||
| sb.Append(chunk); | ||
| } | ||
| return sb.ToString(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fixed in 3a034b8. ReadExpected now reads exactly N bytes one at a time (no over-read possible), and I added ReadSimpleReply() that reads up to and including the trailing CRLF for proper RESP simple-string / error parsing. The WrongArity test was switched to ReadSimpleReply so it doesn't leave error-message bytes in the socket buffer between assertions (this was caught immediately by the new exact-byte ReadExpected — the previous chunk-reading version happened to over-read by luck).
- SendAndReset: rewind to per-command floor instead of buffer head so
prior non-suppressed replies queued in the same batch are preserved.
When the suppressed write made no progress but prior replies exist,
flush [head, cmdReplyFloor) and rotate to a fresh buffer. When neither
prior bytes nor progress exist, throw — matches the non-suppressed
fatal path so an oversized suppressed write cannot infinite-loop.
- SendAndReset(IMemoryOwner): drop the payload under suppression instead
of flushing partial buffer contents that may contain this command's
earlier (about-to-be-rewound) bytes.
- Burn one-shot SKIP for INVALID/unknown commands too — matches Redis:
the SKIP target is the next command attempt regardless of dispatch
outcome. Partial input is still gated by !commandReceived.
- Tests:
- ReadExpected reads exactly N bytes (no over-read).
- Add ReadSimpleReply that reads up to and including CRLF for
proper RESP simple-string / error parsing.
- WrongArity test now uses ReadSimpleReply so it doesn't leave bytes
in the socket between assertions.
- Add ClientReplySkipBurnedByUnknownCommand: SKIP + unknown cmd
should suppress the error AND burn the skip.
- Add ClientReplyPipelinedSkipBetweenReplies: PING/SKIP/PING/PING in
one TCP write must yield exactly two PONGs (verifies prior reply
is not lost by the suppression rewind).
- Add ClientReplyPipelinedOffOnOrdering: PING/OFF/PING/ON in one
write must yield +PONG +OK.
12/12 ClientReplyTests pass on net8.0 and net10.0; 49/49
RespCommandTests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dotnet format gate requires no final newline per the repo .editorconfig. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The AllCommandsCovered ACL test enforces that every command in RespCommandsInfo.json has at least one ACL test case. Adds ClientReplyACLsAsync that runs 'CLIENT REPLY ON' via GarnetClient. Uses ON specifically because OFF/SKIP suppress the reply and would block the synchronous string-result helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #1626
Summary
Implements
CLIENT REPLY ON|OFF|SKIPmatching Redis semantics:CLIENT REPLY ON.CLIENT REPLYcommand (including unknown / parse-error commands), then auto-returns toON.Per Redis behavior,
CLIENT REPLY OFFandCLIENT REPLY SKIPthemselves return no reply;CLIENT REPLY ONreturns+OK.Implementation
Per-session state lives on
RespServerSession:clientReplyMode(On/Off/Skip)suppressCurrentReply(per-iteration flag)cmdReplyFloor(snapshot ofdcurrat the start of the current command — the rewind floor under suppression so prior queued replies in the same batch are preserved)In
ProcessMessages, beforeParseCommand, we snapshotmodeAtStartandcmdReplyFloor = dcurr, then setsuppressCurrentReply = (modeAtStart != On). This ensures parse-time errors (e.g. malformed / unknown command) are also gated.SendAndReset()(suppression path):dcurr > cmdReplyFloor): rewind tocmdReplyFloor— keeps[head, cmdReplyFloor)intact.cmdReplyFloor > head): flush[head, cmdReplyFloor), rotate buffer, setcmdReplyFloor = new head. Suppression continues in the fresh buffer.SendAndReset(IMemoryOwner<byte>, int)(suppression path): drop the payload outright (do not flush the buffer, which may contain this command's earlier bytes betweencmdReplyFlooranddcurr).End-of-command: if still flagged,
dcurr = cmdReplyFloorrewinds the accumulated reply.CLIENT REPLY ONclears the flag inside its own handler so its+OKsurvives. After dispatch, if mode wasSkipand the command wasn'tCLIENT REPLY, mode auto-resets toOn— this fires for unknown /INVALIDcommands too, matching Redis.Files
libs/server/Resp/Parser/RespCommand.csCLIENT_REPLYenum + parser case +AofIndependentCommandsentrylibs/server/Resp/ClientCommands.csClientReplyModeenum +NetworkCLIENTREPLYhandlerlibs/server/Resp/RespServerSession.csProcessMessages, suppression-awareSendAndResetoverloadslibs/server/Resp/CmdStrings.csREPLY/SKIPu8 constantslibs/resources/RespCommandsInfo.json,RespCommandsDocs.jsonCLIENT|REPLYmetadataplayground/CommandInfoUpdater/SupportedCommand.cstest/Garnet.test/RespCommandTests.csCLIENT_REPLYto AofIndependent listtest/Garnet.test/ClientReplyTests.cs(new)Testing
ClientReplyTests: 12/12 pass on bothnet8.0andnet10.0. Coverage:ClientReplyOnReturnsOK—ONreturns+OK.ClientReplyOffSuppressesResponses— OFF gates PING/SET/GET; ON resumes.ClientReplySkipSuppressesOnlyNextCommand— SKIP is one-shot.ClientReplyWrongArityReturnsError— wrong arity returns-ERR…(uses CRLF-awareReadSimpleReply).ClientReplyInvalidModeReturnsSyntaxError— unknown mode returns-ERR syntax error.ClientReplyCaseInsensitive— lower/mixed case modes accepted.ClientReplyOffPersistsAcrossManyCommands— 50 SETs under OFF, all execute, no reply bytes; verified out-of-band via a second connection.ClientReplyOffErrorAlsoSuppressed— error replies (parse + syntax) are suppressed under OFF.ClientReplySkipDoesNotStack— two consecutive SKIPs only suppress one following command.ClientReplySkipBurnedByUnknownCommand— SKIP followed by unknown command suppresses the error AND burns the SKIP (next command replies normally).ClientReplyPipelinedSkipBetweenReplies—PING / SKIP / PING / PINGpipelined in a single TCP write yields exactly two+PONGreplies. Verifies the prior reply isn't lost by the in-batch suppression rewind.ClientReplyPipelinedOffOnOrdering—PING / OFF / PING / ONpipelined yields+PONG +OKin order.RespCommandTestsclass: 49/49 pass (incl.AofIndependentCommandsTestand command info/docs metadata).~Clientfilter: 130/131 — single failure (RespRangeIndexTests.RIConcurrentMultiClientTest) reproduces on a clean tree, pre-existing flake unrelated to this change.Known limitation
Inside
MULTI/EXEC,CLIENT REPLYqueues through the existingNetworkSKIPpath and takes effect atEXECreplay time. This matches Redis semantics for v1.Pub/sub interaction is not specifically tested in this PR —
CLIENT REPLYis gated at the generic response buffer layer, so it applies uniformly to all reply types, but a targeted pub/sub test could be added in a follow-up.