Skip to content

Fix BLMPOP writing a second reply when force-unblocked via CLIENT UNBLOCK - #2030

Merged
kevin-montrose merged 3 commits into
microsoft:mainfrom
hexonal:fix-blmpop-double-reply-on-unblock
Aug 7, 2026
Merged

Fix BLMPOP writing a second reply when force-unblocked via CLIENT UNBLOCK#2030
kevin-montrose merged 3 commits into
microsoft:mainfrom
hexonal:fix-blmpop-double-reply-on-unblock

Conversation

@hexonal

Copy link
Copy Markdown
Contributor

Symptom

A connection blocked in BLMPOP and released with CLIENT UNBLOCK <id> ERROR receives two top-level replies for that one command. Every reply on that connection afterwards is shifted by one, permanently.

# terminal 1
$ redis-cli -p 6379
127.0.0.1:6379> CLIENT ID
(integer) 3
127.0.0.1:6379> BLMPOP 30 1 mylist LEFT          # blocks

# terminal 2
$ redis-cli -p 6379 CLIENT UNBLOCK 3 ERROR
(integer) 1

# terminal 1 unblocks, then:
(error) UNBLOCKED client unblocked via CLIENT UNBLOCK
127.0.0.1:6379> PING
(nil)                                            # expected: PONG
127.0.0.1:6379> ECHO hi
PONG                                             # expected: "hi"

On the wire, the single BLMPOP produces:

current:   -UNBLOCKED client unblocked via CLIENT UNBLOCK\r\n$-1\r\n
expected:  -UNBLOCKED client unblocked via CLIENT UNBLOCK\r\n

Under RESP3 the stray second reply is _\r\n instead of $-1\r\n.

BLPOP, BRPOP, BLMOVE, BRPOPLPUSH, BZPOPMIN, BZPOPMAX and BZMPOP are not affected.

Root cause

libs/server/Resp/Objects/ListCommands.cs:915-919 — the force-unblock branch of ListBlockingPopMultiple writes the error but does not return:

if (result.IsForceUnblocked)
{
    while (!RespWriteUtils.TryWriteError(CmdStrings.RESP_UNBLOCKED_CLIENT_VIA_CLIENT_UNBLOCK, ref dcurr, dend))
        SendAndReset();
}

CollectionItemResult.ForceUnblocked (libs/server/Objects/ItemBroker/CollectionItemResult.cs:91) leaves Key null and IsTypeMismatch false, so Found => Key != default is false. Execution falls through the IsTypeMismatch check into if (!result.Found) { WriteNull(); return true; } at line 928 and emits the second reply.

Fix

Add the missing return true;. All four sibling blocking handlers already return from this branch:

  • ListCommands.cs:286ListBlockingPop (BLPOP/BRPOP)
  • ListCommands.cs:378ListBlockingMove (BLMOVE/BRPOPLPUSH)
  • SortedSetCommands.cs:1563SortedSetBlockingPop (BZPOPMIN/BZPOPMAX)
  • SortedSetCommands.cs:1671SortedSetBlockingMPop (BZMPOP)

ListBlockingPopMultiple was the only one of the five missing it. The skipped code is write-only, so nothing else is affected.

Test

ClientUnblockWithErrorWritesSingleReplyTest in test/standalone/Garnet.test/RespTests.cs blocks a raw-RESP client (LightClientRequest), unblocks it with CLIENT UNBLOCK <id> ERROR, asserts the error is the first reply, then sends PING on that same connection and asserts +PONG. The PING check has to be raw RESP — a StackExchange.Redis multiplexer cannot observe an extra reply.

Which assertions are actually red on main:

  • BLMPOP 10 1 keyA LEFT — the final PING assertion fails; the client reads the stray $-1 where +PONG belongs. This is the only new assertion that fails without the production change.
  • BLPOP keyA 10, BLMOVE keyA keyB LEFT LEFT 10, BZMPOP 10 1 keyA MIN — green on main already. They are regression guards so the missing return cannot reappear in the siblings.

The -UNBLOCKED assertion inside the BLMPOP case also passes on main; the error is still written first, and the desync only becomes visible on the next command.

Copilot AI lite review requested due to automatic review settings August 6, 2026 09:21

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 a RESP protocol correctness bug in the BLMPOP blocking path where a force-unblock via CLIENT UNBLOCK <id> ERROR could cause BLMPOP to emit two top-level replies (error + null), permanently desynchronizing subsequent replies on that connection.

Changes:

  • Add the missing early return true; in ListBlockingPopMultiple when IsForceUnblocked is set, preventing the fallthrough null reply.
  • Add a regression test that reproduces the reply-shift on main (via a raw RESP LightClientRequest) and verifies PING still returns +PONG after unblock.
  • Add coverage for sibling blocking commands to ensure they continue to emit only the unblock error reply.

Reviewed changes

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

File Description
test/standalone/Garnet.test/RespTests.cs Adds regression test ensuring only a single reply is written after CLIENT UNBLOCK ... ERROR, and verifies connection remains in-sync via PING.
libs/server/Resp/Objects/ListCommands.cs Fixes BLMPOP force-unblock handling by returning immediately after writing the unblock error, preventing a stray null reply.

@kevin-montrose kevin-montrose self-assigned this Aug 6, 2026
ListBlockingPopMultiple wrote the -UNBLOCKED error and then fell through to
the `!result.Found` branch, emitting a second top-level reply ($-1 on RESP2,
_ on RESP3) and desynchronising the connection for every subsequent command.

CollectionItemResult.ForceUnblocked leaves Key null, so Found (=> Key !=
default) is false and execution reached the null-reply path. Return right
after writing the error, matching the four sibling handlers: BLPOP/BRPOP and
BLMOVE/BRPOPLPUSH in ListCommands.cs, BZPOPMIN/BZPOPMAX and BZMPOP in
SortedSetCommands.cs.

Adds a regression test that unblocks each of the four blocking commands with
CLIENT UNBLOCK <id> ERROR and asserts a following PING is answered with
+PONG, which fails on the extra reply.
@hexonal
hexonal (hexonal) force-pushed the fix-blmpop-double-reply-on-unblock branch from 284dfdf to 7e052af Compare August 7, 2026 05:25
@kevin-montrose
kevin-montrose merged commit 4d652a8 into microsoft:main Aug 7, 2026
315 of 317 checks passed
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