Fix EXPIRE family emitting two replies for incompatible options

NetworkEXPIRE wrote the "NX and XX, GT or LT options at the same time are
not compatible" error and then fell through to storageApi.EXPIRE, so a
rejected command produced two RESP replies and applied the expiry it had
just refused.

Measured on a Debug build of 8b329e3 over a raw socket, for EXPIRE,
PEXPIRE, EXPIREAT and PEXPIREAT with "NX XX", and for EXPIRE with
"GT LT":

  SET keyA valueA ; EXPIRE keyA 100 NX XX
    -> -ERR ...not compatible\r\n:1\r\n   (two replies for one command)
  TTL keyA -> :100                        (the refused expiry was applied)

  SET keyB valueB ; EXPIRE keyB 0 NX XX
    -> -ERR ...not compatible\r\n:1\r\n
  EXISTS keyB -> :0                       (the key was deleted)

Return via AbortWithErrorMessage instead, matching the five other
validation failures in the same method. The wire text is unchanged: the
ReadOnlySpan<byte> overload emits '-' + payload + CRLF exactly as the
ReadOnlySpan<char> one did for this ASCII literal.

The neighbouring "Unsupported option" message reported the wrong token:
it formatted token 2 while validating token 3, so "EXPIRE keyA 100 NX ZZ"
answered "-ERR Unsupported option NX" and named the option that was
valid. Both call sites now pass the token to AbortWithErrorMessage rather
than pre-formatting it. That also fixes a session kill: a single string
argument binds to the params object[] overload, which runs string.Format
a second time over the already-substituted text, so a brace in the
rejected option threw FormatException out of the handler:

  EXPIRE keyA 100 {     -> b''    (no reply, connection closed)
  EXPIRE keyA 100 {     -> -ERR Unsupported option {     (after)

Pre-formatting only token 2 is why "EXPIRE keyA 100 NX {" escaped this
today; correcting the token index without also removing the second format
would have extended the crash to it.

One further behaviour change worth noting: AbortWithErrorMessage sets
commandErrorWritten, which the raw TryWriteError call it replaces did
not, so this rejection now counts toward INFO commandstats failed_calls
the way every other validation failure in the method already does.

Author: hexonal

libs/server/Resp/KeyAdminCommands.cs

@@ -387,14 +387,14 @@ private bool NetworkEXPIRE<TGarnetApi>(RespCommand command, ref TGarnetApi stora
             {
                 if (!parseState.TryGetExpireOption(2, out expireOption))
                 {
-                    return AbortWithErrorMessage(string.Format(CmdStrings.GenericErrUnsupportedOption, parseState.GetString(2)));
+                    return AbortWithErrorMessage(CmdStrings.GenericErrUnsupportedOption, parseState.GetString(2));
                 }
 
                 if (parseState.Count > 3)
                 {
                     if (!parseState.TryGetExpireOption(3, out var additionExpireOption))
                     {
-                        return AbortWithErrorMessage(string.Format(CmdStrings.GenericErrUnsupportedOption, parseState.GetString(2)));
+                        return AbortWithErrorMessage(CmdStrings.GenericErrUnsupportedOption, parseState.GetString(3));
                     }
 
                     if (expireOption == ExpireOption.XX && (additionExpireOption == ExpireOption.GT ||
@@ -412,10 +412,7 @@ private bool NetworkEXPIRE<TGarnetApi>(RespCommand command, ref TGarnetApi stora
                     }
                     else
                     {
-                        while (!RespWriteUtils.TryWriteError(
-                                   "ERR NX and XX, GT or LT options at the same time are not compatible", ref dcurr,
-                                   dend))
-                            SendAndReset();
+                        return AbortWithErrorMessage("ERR NX and XX, GT or LT options at the same time are not compatible"u8);
                     }
                 }
             }

test/standalone/Garnet.test/RespTests.cs

@@ -2740,6 +2740,93 @@ public void KeyExpireBadOptionTests(string command)
                 var exc = ClassicAssert.Throws<RedisServerException>(() => db.Execute(command, "foo", "100", "128"));
                 ClassicAssert.AreEqual("ERR Unsupported option 128", exc.Message);
             }
+
+            // A brace in the rejected option used to reach string.Format a second time, because
+            // AbortWithErrorMessage(string.Format(...)) bound to the params overload and re-formatted
+            // the already-substituted text. That threw FormatException out of the handler and dropped
+            // the connection instead of writing this error.
+            {
+                var exc = ClassicAssert.Throws<RedisServerException>(() => db.Execute(command, "foo", "100", "{"));
+                ClassicAssert.AreEqual("ERR Unsupported option {", exc.Message);
+            }
+
+            // The second option is reported when it is the one that is unsupported. Before this change
+            // the first option was echoed back regardless of which one failed to parse.
+            {
+                var exc = ClassicAssert.Throws<RedisServerException>(() => db.Execute(command, "foo", "100", "NX", "ZZ"));
+                ClassicAssert.AreEqual("ERR Unsupported option ZZ", exc.Message);
+            }
+
+            {
+                var exc = ClassicAssert.Throws<RedisServerException>(() => db.Execute(command, "foo", "100", "NX", "{"));
+                ClassicAssert.AreEqual("ERR Unsupported option {", exc.Message);
+            }
+        }
+
+        /// <summary>
+        /// An expiry option pair that Garnet rejects must be answered by the error and nothing else,
+        /// and must leave the key untouched. The trailing PING is what makes a second reply visible:
+        /// LightClientRequest reads raw RESP, while a reply-counting client would absorb the extra
+        /// reply and mis-associate every reply that follows it on the connection.
+        /// </summary>
+        [Test]
+        public void KeyExpireIncompatibleOptionsSingleReplyTest()
+        {
+            using var lightClientRequest = TestUtils.CreateRequest();
+
+            var incompatibleOptionsResponse = "-ERR NX and XX, GT or LT options at the same time are not compatible\r\n+PONG\r\n";
+            var unsupportedOptionResponse = "-ERR Unsupported option ZZ\r\n+PONG\r\n";
+
+            lightClientRequest.SendCommand("SET keyA valueA");
+
+            // All four commands share the same option parsing, so all four must reject the pair identically.
+            var response = lightClientRequest.SendCommands("EXPIRE keyA 100 NX XX", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(incompatibleOptionsResponse, response);
+
+            response = lightClientRequest.SendCommands("PEXPIRE keyA 100000 NX XX", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(incompatibleOptionsResponse, response);
+
+            response = lightClientRequest.SendCommands("EXPIREAT keyA 99999999999 NX XX", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(incompatibleOptionsResponse, response);
+
+            response = lightClientRequest.SendCommands("PEXPIREAT keyA 99999999999000 NX XX", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(incompatibleOptionsResponse, response);
+
+            response = lightClientRequest.SendCommands("EXPIRE keyA 100 GT LT", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(incompatibleOptionsResponse, response);
+
+            // None of the rejected expiries may have been applied.
+            response = lightClientRequest.SendCommand("TTL keyA");
+            TestUtils.AssertEqualUpToExpectedLength(":-1\r\n", response);
+
+            // A rejected expiry of 0 must not delete the key either.
+            lightClientRequest.SendCommand("SET keyB valueB");
+
+            response = lightClientRequest.SendCommands("EXPIRE keyB 0 NX XX", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(incompatibleOptionsResponse, response);
+
+            response = lightClientRequest.SendCommand("EXISTS keyB");
+            TestUtils.AssertEqualUpToExpectedLength(":1\r\n", response);
+
+            // The unsupported option error must name the option that failed to parse, not the one before it.
+            response = lightClientRequest.SendCommands("EXPIRE keyA 100 NX ZZ", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(unsupportedOptionResponse, response);
+
+            // The accepted pairs and the sibling error paths keep replying exactly once.
+            response = lightClientRequest.SendCommands("EXPIRE keyA 100 XX GT", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(":0\r\n+PONG\r\n", response);
+
+            response = lightClientRequest.SendCommands("EXPIRE keyA 100 LT XX", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(":0\r\n+PONG\r\n", response);
+
+            response = lightClientRequest.SendCommands("EXPIRE keyA 100 NX", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(":1\r\n+PONG\r\n", response);
+
+            response = lightClientRequest.SendCommands("EXPIRE keyA 100 ZZ", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength(unsupportedOptionResponse, response);
+
+            response = lightClientRequest.SendCommands("EXPIRE keyA -1 NX XX", "PING", 1, 1);
+            TestUtils.AssertEqualUpToExpectedLength("-ERR invalid expire time, must be >= 0\r\n+PONG\r\n", response);
         }
 
         #region ExpireAt