Skip to content

Fix non-positive count on LMPOP and LPOP/RPOP - #2010

Open
hexonal wants to merge 1 commit into
microsoft:mainfrom
hexonal:fix-lmpop-count-zero
Open

Fix non-positive count on LMPOP and LPOP/RPOP#2010
hexonal wants to merge 1 commit into
microsoft:mainfrom
hexonal:fix-lmpop-count-zero

Conversation

@hexonal

@hexonal hexonal commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Two symptoms of the same gap — a count of 0 reaching the list pop path unhandled.

LMPOP with a non-positive COUNT kills the session

RPUSH L a b c
LMPOP 1 L LEFT COUNT 0    -> connection closed, no reply
PING                      -> Connection reset by peer

NullReferenceException out of ProcessMessages. The count is read at ListCommands.cs with no lower bound and flows into the storage layer.

BLMPOP already rejects the same input correctly — I checked against a live server:

BLMPOP 0.01 1 L LEFT COUNT 0  -> -ERR count should be greater than 0

So LMPOP is the odd one out. The fix adds the same bound to the existing condition; the error path and message a few lines below were already there and already match what Redis returns for this case, so nothing new is introduced.

LPOP/RPOP with an explicit count of 0 send no reply

In ListPop, for a non-empty list with count == 0, neither the empty-list branch nor the count > 1 branch fires, so the command writes zero bytes.

Interactively the client just hangs. In a pipeline it is worse — every following reply shifts by one and is silently attributed to the wrong command:

LPOP P 0 / ECHO marker1 / ECHO marker2

before:  $7 marker1 | $7 marker2          <- two replies for three commands
after:   *0 | $7 marker1 | $7 marker2

Redis replies with an empty array here (t_list.c, the hascount && !count fast-exit path), so write one.

Scope

Single-element pops still reply with a bare bulk string, and the count > 1 path is untouched — spot-checked against a live server:

LPOP P 1  -> $1 a          (unchanged)
LPOP P 5  -> $1 b          (clamped to list length, unchanged)
LMPOP 1 L LEFT COUNT 2 -> *2 L *2 a b   (unchanged)

Testing

  • CanDoRejectBadLMPOPCommand extended with COUNT 0 and COUNT -1, asserting the error, that the connection survives, and that the list is untouched.
  • New LPOPAndRPOPWithZeroCountReturnEmptyArray, which also asserts a following ECHO is not mis-paired.
  • Both fail without the change — the LMPOP one immediately, the LPOP one after a 30 s timeout — and pass with it.
  • RespListTests: 104/104. dotnet format --verify-no-changes clean on all three files.
  • macOS arm64, net10.0 Release. net8.0 builds clean but its tests were not run here — no net8.0 runtime in this environment.

Copilot AI review requested due to automatic review settings August 3, 2026 05:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes two RESP list-pop edge cases where a COUNT of 0 (or non-positive for LMPOP) previously reached unhandled paths, causing either a session drop (LMPOP COUNT 0) or a missing reply (LPOP/RPOP key 0), and adds regression tests to lock in Redis-compatible behavior.

Changes:

  • Reject LMPOP ... COUNT < 1 at the RESP parsing layer with the existing Redis-compatible error message.
  • Ensure LPOP/RPOP with an explicit count == 0 returns an empty array reply instead of producing no reply.
  • Extend/add tests covering both scenarios, including verifying the connection remains usable and subsequent pipelined replies are not mis-paired.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
test/standalone/Garnet.test.collections/RespListTests.cs Adds/extends regression tests for LMPOP COUNT 0/-1 and LPOP/RPOP count 0 reply semantics and connection/pipeline safety.
libs/server/Resp/Objects/ListCommands.cs Validates LMPOP COUNT is >= 1 and returns the established “count should be greater than 0” error.
libs/server/Objects/List/ListObjectImpl.cs Writes an explicit empty-array RESP reply when LPOP/RPOP is invoked with count <= 0, preventing zero-byte responses.

Two symptoms of the same gap: a count of 0 reaches the list pop path
without being handled.

LMPOP with COUNT 0 or a negative COUNT throws NullReferenceException out
of ProcessMessages and terminates the RESP session:

    RPUSH L a b c
    LMPOP 1 L LEFT COUNT 0    -> connection closed, no reply

The count is parsed at ListCommands.cs without a lower bound and flows
into the storage layer. BLMPOP already rejects the same input with
"ERR count should be greater than 0", so add the same check to LMPOP.
It reuses the error path and message already present a few lines below,
which matches what Redis returns for this case.

LPOP/RPOP with an explicit count of 0 write no reply at all. In ListPop
neither the empty-list branch nor the count > 1 branch fires, so the
command emits zero bytes. Interactively the client just waits; in a
pipeline every following reply shifts by one and is silently attributed
to the wrong command:

    LPOP P 0 / ECHO marker1 / ECHO marker2
    before: $7 marker1 | $7 marker2        (two replies for three commands)
    after:  *0 | $7 marker1 | $7 marker2

Redis replies with an empty array here (t_list.c, "fast exit path"), so
write one. Single-element pops still reply with a bare bulk string and
the count > 1 path is untouched.

Verified on macOS arm64, net10.0 Release: both new assertions fail
without the change -- LMPOP with RedisServerException missing, LPOP
after a 30s timeout -- and pass with it. RespListTests: 104/104.
dotnet format clean. net8.0 builds clean; its tests were not run here,
no net8.0 runtime in this environment.
@hexonal
hexonal force-pushed the fix-lmpop-count-zero branch from 93780ad to b58e03c Compare August 4, 2026 07:56
@hexonal

hexonal commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main — the branch was 2 commits behind and GitHub had it as BEHIND. The patch is unchanged (diff of the pre- and post-rebase commit is byte-identical), and neither of the two new upstream commits touches ListObjectImpl.cs or ListCommands.cs.

Two checks came back red on the new run. I looked into both rather than assuming they were noise, since one of them is Garnet.test.collections — where this PR's own tests live — so it deserved a real check.

ubuntu-latest, net8.0, Debug, Garnet.test.collections — 65 failures, and every one of them is connection-level: RedisConnectionException: SocketClosed / SocketFailure, GarnetException: Disconnected, ConnectionResetException. There is not a single assertion failure among them. They span RespSetTest (26), RespListTests (24) and RespHashTests (15), and they all land inside a ~1.5 s window; the cascade begins at RespHashTests.CanDoHLenWithExpire, a hash test, not a list one. That reads as the test server going away and taking every in-flight connection with it.

Supporting evidence that it isn't this diff:

  • The other 7 of 8 Garnet.test.collections configurations passed on this same commit.
  • Running the full Garnet.test.collections project locally in Debug: 747 passed, 0 failed.
  • Direction of the change argues against it too — the count <= 0 branch adds a reply that was previously missing. The old behaviour wrote no reply at all, which desynchronises everything after it in a pipeline; that is the failure mode that produces connection-level errors, and this PR removes it.

ubuntu-latest, net10.0, Debug, Garnet.test.vectorset — 2 failures, both VectorSetOverwriteTests.SETAsync, both RedisTimeoutException ... 30139ms elapsed, timeout is 30000ms, command=SAVE. A SAVE overrunning a 30 s timeout is runner I/O; this PR doesn't touch vector sets or checkpointing. 15 of 16 vectorset configurations passed here, and the same suite is currently red on #2015 and #1906 as well.

Let me know if you'd like a re-trigger, or I can rebase again to get a fresh run.

@kevin-montrose kevin-montrose self-assigned this Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants