Fix EXPIRE family double reply for incompatible options and session kill on braces in an option

[hexonal (hexonal)]

Symptom

EXPIRE, PEXPIRE, EXPIREAT and PEXPIREAT answer an incompatible option pair with the error and then with an integer, so one command produces two RESP replies and the expiry that was just refused is applied anyway.

Raw socket against a Debug build of unpatched main, each command pipelined with PING so the extra reply is visible (redis-cli hides it — it stops after the first reply and silently attributes the leftover to the next command):

SET k v ; EXPIRE k 100 NX XX ; PING
b'+OK\r\n-ERR NX and XX, GT or LT options at the same time are not compatible\r\n:1\r\n+PONG\r\n'
                                                                              ^^^^^^ second reply

SET k2 v ; EXPIRE k2 100 NX XX ; TTL k2
b'+OK\r\n-ERR ...not compatible\r\n:1\r\n:100\r\n'
                                        ^^^^^^^ the refused expiry was applied

SET k8 v ; EXPIRE k8 0 NX XX ; EXISTS k8
b'+OK\r\n-ERR ...not compatible\r\n:1\r\n:0\r\n'
                                        ^^^^^ the key was deleted

Same shape for GT LT (:0), XX XX (:0), and for PEXPIRE/EXPIREAT/PEXPIREAT. Expected in every case: the error alone, key untouched.

Two more problems in the same option block:

EXPIRE k 100 NX BAD ; PING
b'-ERR Unsupported option NX\r\n+PONG\r\n'     # names NX, the option that parsed fine

EXPIRE k 100 { ; PING
b''                                            # no reply at all, connection closed

The second is a FormatException escaping the command handler. Server log:

crit: Session[0] ProcessMessages threw an exception: System.FormatException: Input string was not in
a correct format. Failure to parse near offset 24. Format item ends prematurely.
  at Garnet.server.RespServerSession.AbortWithErrorMessage(String format, Object[] args)
     in libs/server/Resp/Objects/ObjectStoreUtils.cs:line 101
  at Garnet.server.RespServerSession.NetworkEXPIRE[TGarnetApi](...)
     in libs/server/Resp/KeyAdminCommands.cs:line 390

RespServerSession.TryConsumeMessages's generic catch (Exception) (RespServerSession.cs:566) disposes the network sender without writing anything, so the whole pipelined batch — including the +OK for the preceding SET — is lost.

Root cause

Double reply. libs/server/Resp/KeyAdminCommands.cs:413-419. The else arm of the option-combination check writes the error inline and falls out of the if block. Execution continues to storageApi.EXPIRE (line ~440) with expireOption still holding whatever the first token parsed to, and then writes a second reply. Every path through that else produced two replies; there is no input for which it was correct.

Wrong token reported. KeyAdminCommands.cs:397 validates token 3 but formats parseState.GetString(2).

Session kill. Both call sites (lines 390 and 397) pre-format the message and pass the single resulting string to AbortWithErrorMessage. There is no string → ReadOnlySpan<byte> conversion, so that binds to the params object[] overload at libs/server/Resp/Objects/ObjectStoreUtils.cs:99, which runs string.Format a second time over the already-substituted text. A { or } in the option therefore throws.

Note the interaction: EXPIRE k 100 NX { is safe on main only because line 397 formats token 2 (NX). Correcting the index without also removing the second format would have extended the crash to that input.

Fix

• The else arm now return AbortWithErrorMessage("ERR NX and XX, GT or LT options at the same time are not compatible"u8).
• Both unsupported-option sites pass the token as an argument instead of pre-formatting, binding to the (string format, object arg0) overload at ObjectStoreUtils.cs:63, and line 397 now reports token 3.

return AbortWithErrorMessage(...) is what the five other validation failures in this same method already do (KeyAdminCommands.cs:370, 377, 382, 390, 397), and it is how the sibling option-conflict check is written for GEOADD in libs/server/Resp/Objects/SortedSetGeoCommands.cs:58-62.

The wire text is unchanged: RespWriteUtils.TryWriteError's ReadOnlySpan<byte> overload (libs/common/RespWriteUtils.cs:228) emits '-' + payload + CRLF exactly as the ReadOnlySpan<char> one (line 266) did for this ASCII-only literal.

One deliberate behaviour change: AbortWithErrorMessage sets commandErrorWritten, which the raw TryWriteError it replaces did not. This rejection now counts toward INFO commandstats failed_calls (RespServerSession.cs:684-690), consistent with every other validation failure in the method.

Tests

New RespTests.KeyExpireIncompatibleOptionsSingleReplyTest uses LightClientRequest and asserts on raw RESP. StackExchange.Redis cannot observe a duplicated or missing reply — it matches replies positionally and would absorb the extra one — so a raw-RESP client is required. Each command is followed by PING, because AssertEqualUpToExpectedLength only compares the expected prefix and would pass against a trailing :1 otherwise.

Fails on unpatched main:

• EXPIRE, PEXPIRE, EXPIREAT, PEXPIREAT with NX XX, and EXPIRE ... GT LT — expects -ERR ...not compatible\r\n+PONG\r\n; main returns the error plus :1/:0 before +PONG.
• TTL keyA -> :-1 after those five rejections (main: :100).
• EXISTS keyB -> :1 after EXPIRE keyB 0 NX XX (main: :0, key deleted).
• EXPIRE keyA 100 NX ZZ -> -ERR Unsupported option ZZ (main: ... NX).

Regression guards, green on main as well:

• EXPIRE keyA 100 XX GT -> :0 and EXPIRE keyA 100 LT XX -> :0 — accepted pairs still reach storage and still reply exactly once.
• EXPIRE keyA 100 NX -> :1 — single-option happy path.
• EXPIRE keyA 100 ZZ -> -ERR Unsupported option ZZ — first-token path, unchanged.
• EXPIRE keyA -1 NX XX -> -ERR invalid expire time, must be >= 0 — argument validation still precedes option validation.

Extended RespTests.KeyExpireBadOptionTests ([TestCase("EXPIRE")], [TestCase("PEXPIRE")]), three new assertions, all three fail on unpatched main:

• <cmd> foo 100 { → ERR Unsupported option {. On main the connection is dropped, so the expected RedisServerException never arrives.
• <cmd> foo 100 NX ZZ → ERR Unsupported option ZZ. main says NX.
• <cmd> foo 100 NX { → ERR Unsupported option {. main says NX.

That test has no EXPIREAT/PEXPIREAT cases and none were added; four-command coverage lives in the new raw-RESP test and covers the incompatible-pair path only.

Related gaps left unfixed

1. The unsupported-option token is echoed into the error message verbatim, control bytes included. This is already the case on main for the first option token, and this change makes the second token reachable the same way when it is the one that failed to parse (main always echoed the first). It is not a new capability — the first-token form is already reachable on main — but flagging it so it is not read as introduced here. Sanitising GenericErrUnsupportedOption is a separate change and is not attempted; I can share a reproduction privately if that is useful for prioritising it.

2. Which pairs are rejected is unchanged. Garnet rejects duplicates such as XX XX and NX NX, reports GT LT with the NX/XX message rather than a dedicated one, and accepts at most two option tokens — all narrower than Redis, which ORs repeated flags and rejects only NX-with-XX/GT/LT and GT-with-LT. This change only stops the second reply; the accept/reject set and the message text are byte-identical to main.

3. libs/server/Resp/PubSubCommands.cs:430,459,474 use the same AbortWithErrorMessage(string.Format(...)) shape and so also format twice, but their arguments are constant strings with no braces, so they cannot throw. Left alone.

[Copilot]

Pull request overview

Fixes RESP protocol correctness and session stability for the EXPIRE command family option parsing in Garnet’s RESP layer, ensuring invalid option combinations and unsupported options produce exactly one error reply and do not apply any expiry side effects.

Changes:

• Prevents EXPIRE family incompatible option pairs (e.g., NX XX, GT LT) from writing an inline error and then continuing to execute the storage operation (double reply + unintended expiry application).
• Fixes unsupported-option reporting to echo the correct token (including when the second option is invalid) and avoids FormatException/connection drops when options contain {/} by using the correct AbortWithErrorMessage overload.
• Adds targeted tests, including raw-RESP pipelining coverage via LightClientRequest, to assert “single reply” behavior and no side effects on rejected expiries.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
libs/server/Resp/KeyAdminCommands.cs	Fixes control flow to return on incompatible option pairs and routes unsupported-option errors through safe AbortWithErrorMessage overloads (correct token index, no double-format).
test/standalone/Garnet.test/RespTests.cs	Extends option error tests (brace + second-token invalid) and adds a raw-RESP regression test ensuring incompatible options yield a single reply and do not modify key TTL/existence.