Fix EXPIRE family emitting two replies for incompatible options (#2037)

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.

Co-authored-by: kevin-montrose <kmontrose@microsoft.com>

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