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

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