Fix ZRANGESTORE aborting the RESP session on invalid range parameters

ZRANGESTORE with LIMIT in index mode, or with a non-float min/max, made
SortedSetRangeStore parse the range operation's RESP error output as an
array length. That threw a RESP parsing exception which aborted the
connection (dropping every pipelined command after it), and because
Delete(dstKey) ran before the throwing parse, a rejected ZRANGESTORE also
destroyed a pre-existing destination key.

Detect the error output before consuming it - as GEOSEARCHSTORE already
does - and return it to the client unchanged, leaving the session alive
and the destination untouched. The error payload is copied via the
scratch allocator so it can coexist with other commands' scratch
allocations in the same network batch.

Author: hexonal

libs/server/API/GarnetApiObjectCommands.cs

@@ -31,8 +31,8 @@ public GarnetStatus SortedSetAdd(PinnedSpanByte key, ref ObjectInput input, ref
             => storageSession.SortedSetAdd(key, ref input, ref output, ref objectContext);
 
         /// <inheritdoc />
-        public GarnetStatus SortedSetRangeStore(PinnedSpanByte dstKey, PinnedSpanByte srcKey, ref ObjectInput input, out int result)
-            => storageSession.SortedSetRangeStore(dstKey, srcKey, ref input, out result, ref objectContext);
+        public GarnetStatus SortedSetRangeStore(PinnedSpanByte dstKey, PinnedSpanByte srcKey, ref ObjectInput input, out int result, out PinnedSpanByte error)
+            => storageSession.SortedSetRangeStore(dstKey, srcKey, ref input, out result, out error, ref objectContext);
 
         /// <inheritdoc />
         public GarnetStatus SortedSetRemove(PinnedSpanByte key, PinnedSpanByte member, out int zremCount)

libs/server/API/IGarnetApi.cs

@@ -398,8 +398,9 @@ public interface IGarnetApi : IGarnetReadApi, IGarnetAdvancedApi
         /// <param name="srcKey">The sub-key for the sorted set.</param>
         /// <param name="input">The input object containing the elements to store.</param>
         /// <param name="result">The result of the store operation.</param>
+        /// <param name="error">When the range parameters are invalid, receives the RESP error payload (without the leading '-' or trailing CRLF); otherwise empty.</param>
         /// <returns>A <see cref="GarnetStatus"/> indicating the status of the operation.</returns>
-        GarnetStatus SortedSetRangeStore(PinnedSpanByte dstKey, PinnedSpanByte srcKey, ref ObjectInput input, out int result);
+        GarnetStatus SortedSetRangeStore(PinnedSpanByte dstKey, PinnedSpanByte srcKey, ref ObjectInput input, out int result, out PinnedSpanByte error);
 
         /// <summary>
         /// Removes the specified member from the sorted set stored at key.

libs/server/Resp/Objects/SortedSetCommands.cs

@@ -220,7 +220,14 @@ private unsafe bool SortedSetRangeStore<TGarnetApi>(ref TGarnetApi storageApi)
             var header = new RespInputHeader(GarnetObjectType.SortedSet) { SortedSetOp = SortedSetOperation.ZRANGE };
             var input = new ObjectInput(header, ref parseState, startIdx: 2, arg1: respProtocolVersion, arg2: (int)SortedSetRangeOpts.Store);
 
-            var status = storageApi.SortedSetRangeStore(dstKey, srcKey, ref input, out int result);
+            var status = storageApi.SortedSetRangeStore(dstKey, srcKey, ref input, out int result, out var error);
+
+            if (error.Length > 0)
+            {
+                while (!RespWriteUtils.TryWriteError(error.ReadOnlySpan, ref dcurr, dend))
+                    SendAndReset();
+                return true;
+            }
 
             switch (status)
             {

libs/server/Storage/Session/ObjectStore/SortedSetOps.cs

@@ -710,13 +710,14 @@ public GarnetStatus SortedSetAdd<TObjectContext>(PinnedSpanByte key, ref ObjectI
         /// <param name="result">The result of the operation, indicating the number of elements stored.</param>
         /// <param name="objectContext">The context of the object store.</param>
         /// <returns>Returns a GarnetStatus indicating the success or failure of the operation.</returns>
-        public unsafe GarnetStatus SortedSetRangeStore<TObjectContext>(PinnedSpanByte dstKey, PinnedSpanByte srcKey, ref ObjectInput input, out int result, ref TObjectContext objectContext)
+        public unsafe GarnetStatus SortedSetRangeStore<TObjectContext>(PinnedSpanByte dstKey, PinnedSpanByte srcKey, ref ObjectInput input, out int result, out PinnedSpanByte error, ref TObjectContext objectContext)
             where TObjectContext : ITsavoriteContext<FixedSpanByteKey, ObjectInput, ObjectOutput, long, ObjectSessionFunctions, StoreFunctions, StoreAllocator>
         {
             if (txnManager.ObjectTransactionalContext.Session is null)
                 ThrowObjectStoreUninitializedException();
 
             result = 0;
+            error = default;
 
             if (dstKey.Length == 0 || srcKey.Length == 0)
                 return GarnetStatus.OK;
@@ -763,6 +764,23 @@ public unsafe GarnetStatus SortedSetRangeStore<TObjectContext>(PinnedSpanByte ds
                     ref var currOutPtr = ref rangeOutPtr;
                     var endOutPtr = rangeOutPtr + rangeOutputMem.Length;
 
+                    // SortedSetRange signals a parameter error (LIMIT in index mode, or a
+                    // non-float min/max) by writing a RESP error into this output buffer
+                    // instead of an array. Surface it to the caller — as GEOSEARCHSTORE
+                    // already does in SortedSetGeoOps — rather than parsing the '-' error
+                    // prefix as an array length, which aborts the RESP session with a
+                    // protocol error, or clobbering the destination key.
+                    if (RespReadUtils.TryReadErrorAsSpan(out var rangeError, ref currOutPtr, endOutPtr))
+                    {
+                        // Copy the payload so it outlives the disposed range output. Use the
+                        // allocator, not scratchBufferBuilder: this slice is intentionally never
+                        // rewound (it must survive until the RESP handler writes it), and the
+                        // builder's single-outstanding-slice invariant would otherwise trip once
+                        // another command in the same network batch allocates from it.
+                        error = scratchBufferAllocator.CreateArgSlice(rangeError);
+                        return GarnetStatus.OK;
+                    }
+
                     var destinationKey = dstKey.ReadOnlySpan;
                     ssUnifiedTransactionalContext.Delete((FixedSpanByteKey)destinationKey);
 

test/standalone/Garnet.test.collections/RespSortedSetTests.cs

@@ -828,6 +828,50 @@ public void CanDoZMScoreLC()
             TestUtils.AssertEqualUpToExpectedLength(expectedResponse, response);
         }
 
+        [Test]
+        public void ZRangeStoreInvalidParamsReturnErrorAndKeepSessionAlive()
+        {
+            using var lightClientRequest = TestUtils.CreateRequest();
+
+            lightClientRequest.SendCommands("ZADD zrs 1 a 2 b 3 c", "PING");
+            // A pre-existing destination must survive a rejected ZRANGESTORE.
+            lightClientRequest.SendCommands("ZADD zrsdst 9 keep", "PING");
+
+            // Index mode with LIMIT. On unpatched main SortedSetRange writes the
+            // "-ERR syntax error, LIMIT ..." into the range output buffer, which the
+            // store path then parses as an array length, aborting the RESP session
+            // with a protocol error (a missing reply here would hang this request).
+            var response = lightClientRequest.SendCommands("ZRANGESTORE zrsdst zrs 0 -1 LIMIT 0 2", "PING");
+            var expectedResponse = "-ERR syntax error, LIMIT is only supported in combination with either BYSCORE or BYLEX\r\n+PONG\r\n";
+            TestUtils.AssertEqualUpToExpectedLength(expectedResponse, response);
+
+            // Non-float min/max in BYSCORE mode reaches the same faulty path on main.
+            response = lightClientRequest.SendCommands("ZRANGESTORE zrsdst zrs notafloat 5 BYSCORE", "PING");
+            expectedResponse = "-ERR min or max is not a float\r\n+PONG\r\n";
+            TestUtils.AssertEqualUpToExpectedLength(expectedResponse, response);
+
+            // The destination is left untouched by the rejected commands.
+            response = lightClientRequest.SendCommands("ZRANGE zrsdst 0 -1", "PING", 2, 1);
+            expectedResponse = "*1\r\n$4\r\nkeep\r\n+PONG\r\n";
+            TestUtils.AssertEqualUpToExpectedLength(expectedResponse, response);
+
+            // A well-formed ZRANGESTORE still overwrites the destination.
+            response = lightClientRequest.SendCommands("ZRANGESTORE zrsdst zrs 0 -1", "PING");
+            expectedResponse = ":3\r\n+PONG\r\n";
+            TestUtils.AssertEqualUpToExpectedLength(expectedResponse, response);
+
+            response = lightClientRequest.SendCommands("ZRANGE zrsdst 0 -1", "PING", 4, 1);
+            expectedResponse = "*3\r\n$1\r\na\r\n$1\r\nb\r\n$1\r\nc\r\n+PONG\r\n";
+            TestUtils.AssertEqualUpToExpectedLength(expectedResponse, response);
+
+            // A rejected ZRANGESTORE pipelined with another scratch-allocating command
+            // in the same network batch must not corrupt the shared scratch buffer: the
+            // error payload has to coexist with the following ZRANGE's own allocation.
+            response = lightClientRequest.SendCommands("ZRANGESTORE zrsdst zrs 0 -1 LIMIT 0 2", "ZRANGE zrs (1 3 BYSCORE LIMIT 0 2", 1, 3);
+            expectedResponse = "-ERR syntax error, LIMIT is only supported in combination with either BYSCORE or BYLEX\r\n*2\r\n$1\r\nb\r\n$1\r\nc\r\n";
+            TestUtils.AssertEqualUpToExpectedLength(expectedResponse, response);
+        }
+
         [Test]
         public void CandDoZIncrby()
         {