Fix ZRANGESTORE aborting the RESP session on invalid range parameters

[hexonal (hexonal)]

Symptom

ZRANGESTORE aborts the whole RESP session with a protocol error when the range parameters are invalid, instead of returning the syntax error that the equivalent ZRANGE returns. Every pipelined command after it on that connection is silently dropped, and a pre-existing destination key is destroyed.

Debug build of GarnetServer, raw RESP with a trailing PING so a missing/dropped reply is visible:

C: ZADD z 1 a 2 b 3 c
C: ZRANGESTORE dst z 0 -1 LIMIT 0 2
C: PING
S: :3
S: (connection closed after "-ERR Protocol Error: Unexpected character '-'.")

server log:

crit: Session[0] Aborting open session due to RESP parsing error
Garnet.common.Parsing.RespParsingException: Unexpected character '-'.
  at Garnet.common.RespReadUtils.TryReadUnsignedArrayLength(...)
  at Garnet.server.StorageSession.SortedSetRangeStore[TObjectContext](...)

Two inputs reach the fault, both unauthenticated and with no special state:

ZRANGESTORE dst z 0 -1 LIMIT 0 2      # index mode with LIMIT
ZRANGESTORE dst z notafloat 5 BYSCORE # non-float min/max

The equivalent ZRANGE handles both cleanly and keeps the session alive:

ZRANGE z 0 -1 LIMIT 0 2      -> -ERR syntax error, LIMIT is only supported in combination with either BYSCORE or BYLEX
ZRANGE z notafloat 5 BYSCORE -> -ERR min or max is not a float

Because the faulty path runs Delete(dstKey) before it crashes, a rejected ZRANGESTORE against an existing destination also destroys that key.

Root cause

libs/server/Storage/Session/ObjectStore/SortedSetOps.cs, SortedSetRangeStore. It runs the range over the source via SortedSetRange, then treats the range output buffer as an array of members/scores to ZADD into the destination. But SortedSetRange reports an invalid-parameter error by writing a RESP error string into that same output buffer.

The status returned is not WRONGTYPE/NOTFOUND, so SortedSetRangeStore falls through to Delete(dstKey) and TryReadUnsignedArrayLength, which reads the '-' of -ERR and throws RespParsingException, aborting the session.

ZRANGE does not hit this because its handler writes the range output straight back to the client, error or not.

Fix

Per kevin-montrose's review, the store path no longer inspects the RESP payload to decide whether the range succeeded, and no longer carries an extra out-parameter for the error.

• SortedSetRange sets ObjectOutput.result1 to SortedSetObject.RangeError (-1) on each of the five paths where it writes a RESP error instead of a range: LIMIT with fewer than two following tokens, non-integer LIMIT arguments, non-float min/max, LIMIT in index mode, and invalid min/max in BYLEX. A range reply never produces a negative result1, so the flag is unambiguous.
• SortedSetRangeStore checks that flag instead of sniffing for a - prefix. On it, the range operation's own error payload is written straight through to the client and the destination key is left alone.
• The reply now travels through a ref SpanByteAndMemory + RespMemoryWriter, exactly like the sibling GeoSearchStore in SortedSetGeoOps.cs. That removes the out PinnedSpanByte error parameter and the scratch-allocator copy that went with it — the payload is written into the caller's buffer while the range output is still alive, so nothing has to outlive it. The ZRANGESTORE RESP handler is now the same shape as the GEOSEARCHSTORE one.

One extra fix the flag exposed

BYSCORE and BYLEX are independent booleans rather than mutually exclusive options, so the BYLEX error is the one error path reachable after a reply has already been written — the BYSCORE block runs first and leaves an array in the buffer. Without handling that, ZRANGESTORE dst z 1 3 BYSCORE BYLEX would answer one command with two RESP payloads:

*6\r\n$1\r\na\r\n$1\r\n1\r\n$1\r\nb\r\n$1\r\n2\r\n$1\r\nc\r\n$1\r\n3\r\n-ERR min or max not valid string range item\r\n

which desynchronises the stream for every command after it — the exact class of bug this PR exists to fix. SortedSetRange now rewinds the writer before that error so RangeError means what its doc comment says: the output holds the error and nothing else. This also fixes plain ZRANGE key 1 3 BYSCORE BYLEX, which emits the same two payloads on unpatched main today.

Known, deliberate scope limits

• IGarnetApi.SortedSetRangeStore changes shape (out int result → ref SpanByteAndMemory output) rather than staying untouched. Zeroing the API delta is not reachable while preserving the five distinct error messages: the alternatives are a code→message table duplicated across the storage and RESP layers, or one generic error, which would make ZRANGESTORE reply differently from ZRANGE for the same input. GeoSearchStore already carries ref SpanByteAndMemory output on this same interface, so this follows the sibling rather than inventing a shape. Happy to switch if you'd prefer one of the alternatives.
• When the source key does not exist, the range operation never runs, so the parameters are not validated: ZRANGESTORE dst nosuchkey 0 -1 LIMIT 0 2 still replies :0 and still expires the destination. That ordering is byte-identical to main and is not changed here; Redis validates arguments before the key lookup. Hoisting the parse into the RESP handler would change ZRANGE too, so it is left for a separate change.
• ZRANGESTORE dst src <a> <b> BYSCORE BYLEX where both blocks succeed still writes two arrays into the range buffer, of which the store path consumes the first. Also identical to main; the real fix is making the two options mutually exclusive (last-wins), which belongs in its own change. Say the word if you'd rather have it here.

Tests

RespSortedSetTests.ZRangeStoreInvalidParamsReturnErrorAndKeepSessionAlive (raw RESP via LightClientRequest, each command followed by PING so a missing reply is visible):

• ZRANGESTORE dst z 0 -1 LIMIT 0 2 → -ERR syntax error, LIMIT ... then +PONG. On unpatched main this returns the protocol error and the session dies.
• ZRANGESTORE dst z notafloat 5 BYSCORE → -ERR min or max is not a float then +PONG.
• ZRANGESTORE dst z 1 3 BYSCORE BYLEX → the error alone. Without the rewind this asserts red with the array-then-error payload quoted above, so the case is covered rather than assumed.
• A pre-existing destination (ZADD dst 9 keep) still contains keep after the rejected commands, and still holds a b c after the BYSCORE BYLEX rejection.
• A well-formed ZRANGESTORE dst z 0 -1 still returns :3 and overwrites the destination.
• A rejected ZRANGESTORE pipelined with a second scratch-allocating command in the same network batch still produces both replies intact.

Every assertion was verified to fail on the unpatched build before being added. Full Garnet.test.collections (750 tests, 324 of them sorted-set) green locally.

[Copilot]

Pull request overview

Fixes a RESP-session abort in ZRANGESTORE when invalid range parameters cause SortedSetRange to emit a RESP error payload into the output buffer (previously mis-parsed as an array length), ensuring the client receives the same error as ZRANGE and the destination key is not modified on rejection.

Changes:

• Detect and surface RESP error payloads produced by SortedSetRange before deleting the destination key or parsing the response as an array.
• Extend the SortedSetRangeStore API surface/handler path to return and write the RESP error payload when present.
• Add a regression test to verify errors are returned, the session stays alive (pipelining), and the destination key remains intact on rejected requests.

Reviewed changes

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

Show a summary per file

File	Description
test/standalone/Garnet.test.collections/RespSortedSetTests.cs	Adds a LightClient-based regression test covering invalid ZRANGESTORE params, session liveness, and destination preservation.
libs/server/Storage/Session/ObjectStore/SortedSetOps.cs	Adds TryReadErrorAsSpan check to short-circuit on parameter errors and avoid deleting/parsing destination updates.
libs/server/Resp/Objects/SortedSetCommands.cs	Writes the surfaced RESP error when present, otherwise retains existing integer/WRONGTYPE responses.
libs/server/API/IGarnetApi.cs	Updates SortedSetRangeStore signature to include an out PinnedSpanByte error payload.
libs/server/API/GarnetApiObjectCommands.cs	Plumbs the new out PinnedSpanByte error parameter through the API wrapper to StorageSession.

[hexonal (hexonal)]

Reworked along both lines, and rebased onto current main.

No more RESP parsing to detect the error. SortedSetRange now sets output.result1 to SortedSetObject.RangeError (-1) on each of the five paths where it writes a RESP error instead of a range (LIMIT with fewer than two tokens, non-integer LIMIT args, non-float min/max, LIMIT in index mode, and invalid min/max in BYLEX). SortedSetRangeStore checks that flag instead of sniffing for a - prefix. A range reply never produces a negative result1, so the signal is unambiguous.

The error is written out rather than plumbed through a new parameter. SortedSetRangeStore now returns its reply through a ref SpanByteAndMemory output and a RespMemoryWriter, exactly like GeoSearchStore next door: the element count on success, and the range operation's own error payload passed through verbatim on the flagged path. That drops the out PinnedSpanByte error parameter you flagged, and with it the scratch-allocator copy — the payload is now written straight into the caller's buffer while it is still alive, so nothing has to outlive the range output. The ZRANGESTORE RESP handler is now the same shape as the GEOSEARCHSTORE one.

The IGarnetApi change is therefore out int result → ref SpanByteAndMemory output rather than an added parameter. I couldn't get it to zero: the five errors above are five distinct messages, so with only result1 to go on, the RESP layer would need either a code→message table duplicated across the two layers, or a single generic error, which would regress ZRANGESTORE to a different error than ZRANGE returns for the same input. Matching GeoSearchStore's signature seemed the better of the three. Say the word if you'd rather have one of the others.

All 750 tests in Garnet.test.collections pass locally (324 of them sorted-set).

[hexonal (hexonal)]

Pushed a follow-up and rewrote the description, which still described the abandoned TryReadErrorAsSpan + scratch-buffer approach.

A defect in my own rework, found before you looked at it. BYSCORE and BYLEX are independent booleans, not mutually exclusive, so the BYLEX error site is the one RangeError path reachable after a reply has already been written into the buffer. ZRANGESTORE dst z 1 3 BYSCORE BYLEX therefore forwarded both payloads:

*6\r\n$1\r\na\r\n$1\r\n1\r\n$1\r\nb\r\n$1\r\n2\r\n$1\r\nc\r\n$1\r\n3\r\n-ERR min or max not valid string range item\r\n

One command, two RESP replies — i.e. the rework reintroduced the desync class this PR exists to fix, on a different input. main replies a single :3 there. SortedSetRange now rewinds the writer before that error, so RangeError means what its doc comment claims: the output is the error and nothing else. As a side effect plain ZRANGE key 1 3 BYSCORE BYLEX stops emitting two payloads too.

The case is now in the regression test, and I confirmed it asserts red without the rewind rather than passing by luck.

Three limits are stated explicitly in the description rather than left for you to find: the IGarnetApi delta could not be taken to zero without either duplicating a code→message table across layers or collapsing five distinct errors into one generic message (which would make ZRANGESTORE disagree with ZRANGE); a missing source key still short-circuits parameter validation exactly as on main; and BYSCORE BYLEX with both blocks succeeding still writes two arrays, also as on main. The last two are pre-existing and I have deliberately not folded them in — say the word if you'd rather they were.

Garnet.test.collections is green locally, 750 tests.