fix(pipe): don't let Close hang when the command ring is full by FZambia · Pull Request #1013 · redis/rueidis

FZambia · 2026-07-26T10:07:08Z

Summary

pipe.Close() can block forever when the command ring is full and the peer has stopped replying without erroring. This bounds it to the existing one-second escape.

Impact

A client.Close() that never returns during a silent connection stall (network partition, frozen server) — a hung shutdown.
Worse in the sentinel client: it calls Close() on a replaced connection while holding its mutex, so a stuck Close() wedges all further topology handling on that client.

Root cause

Close() enqueues a final PING and waits for the reply with a one-second escape — but the escape covered only the reply. The enqueue itself (queue.PutOne) blocks while every ring slot is occupied, and at Close() time nothing frees a slot: the keepalive ping is disabled (blcksig is set and errClosing is stored), and the network connection is closed only after the enqueue. So the enqueue blocks before Close() ever reaches the code that drains the ring.

Fix

Run the enqueue-and-wait in a goroutine so the existing one-second escape bounds both. If the enqueue is stuck, Close() proceeds and closes the connection, which errors the background loops and drains the ring — unparking that goroutine, so nothing leaks.

Tests

TestCloseReturnsOnFullRing — fails against the previous Close() (deadlocks past 3s), passes with the fix.
TestCloseAfterConnErrorOnFullRing — pins the already-safe path where the connection errors first.

Note

Medium Risk
Touches connection teardown and queue draining under failure, which affects client shutdown and sentinel topology handling, but the change is narrowly scoped with targeted regression tests.

Overview
pipe.Close() could hang indefinitely when the command ring was full and Redis stopped replying without tearing down TCP (partition / frozen peer). The old one-second timeout only applied while waiting for the shutdown PING reply, not while queue.PutOne blocked for a free ring slot—keepalive was disabled during close, and the connection was not closed until after that enqueue finished.

The close path now arms a one-second time.AfterFunc that Close()s the connection before enqueueing the final PING. A forced close errors the read/write loops, drains the ring, frees a slot, and completes the PING with an error so Close() can finish and then close the connection again as usual.

Regression coverage in pipe_close_test.go: full ring + silent peer (with background worker), full ring after the connection already errored, and full ring in sync mode before any background worker (escape via syncDo starting drain).

^{Reviewed by Cursor Bugbot for commit b7bf06a. Bugbot is set up for automated code reviews on this repo. Configure here.}

rueian · 2026-07-26T20:58:53Z

+			// the connection, which makes the background loops drain the ring
+			// and unpark this goroutine — so it does not leak.
+			answered := make(chan struct{})
+			go func() {


Hi @FZambia, thanks for the PR, but I am wondering if adding a goroutine in Cose() could be too costly.

We also have another goroutine sending PING in the _background function

rueidis/pipe.go

Line 451 in fdf45cd

ch, _ := p.queue.PutOne(context.Background(), cmds.PingCmd) // avoid _backgroundWrite hanging at p.queue.WaitForWrite()

Although they serve different purposes, we probably already have too many goroutines on the termination path.

For Close(), I am even wondering if this PingCmd, which is for a 1-second graceful shutdown, is worth having. Or can we have a TryPutOne to the ring to do this graceful shutdown in a best-effort way?

Pushed a new commit - without goroutine in normal case, and additional regression test to make sure the behavior will be preserved. I tried to investigate TryPutOne (Opus 5) – but it seems problematic:

On TryPutOne — two problems. PutOne claims its slot with an unconditional atomic.AddUint32(&r.write, 1) before checking mark, so a variant that claims sequence N and then bails leaves a phantom entry: read1 reaches N&mask needing mark == 1, finds the old occupant's 2-or-0, and WaitForWrite parks there forever while later writers mark slots past it. Doing it correctly means conditional reservation (lock the node, check mark, CAS write, retry on loss) — surgery on the hottest structure in the library for a shutdown path. It also degrades in the wrong direction: a full ring isn't only the stalled-server case, it's also a momentarily-wrapped ring with a healthy server, which is exactly where the final PING earns its keep.

rueian · 2026-07-28T03:27:55Z

 			case <-ch:
 				p.decrWaits()
-			case <-time.After(time.Second):
+			case <-time.After(time.Until(deadline)): // the remaining budget, so the whole shutdown stays within one second


With the escape, does the <-time.After(time.Until(deadline)) entire case become unnecessary anymore?

Thx! It seems so, ch should be closed reliably, pushed new commit with it

Close enqueues a final PING through the ring and waits for its reply with a one-second escape. The escape covered only the reply: the enqueue itself (queue.PutOne) blocks while every ring slot is occupied, and at Close time nothing can free a slot when the peer has stopped replying but the TCP connection is still open: - the keepalive ping would normally detect the stall and error the connection, which drains the ring — but Close increments blcksig, which makes backgroundPing skip its check, and stores errClosing, which stops the ping timer from re-arming; - Close closes the network connection only after the enqueue, so the background loops keep waiting and the drain never starts. So Close blocked forever. In practice that is a client.Close() that never returns during a silent connection stall (network partition, frozen server) — a hung shutdown — or the sentinel client closing a replaced connection while holding its mutex, wedging all of its further topology handling. Run the enqueue-and-wait in a goroutine so the existing one-second escape bounds both. If the enqueue is stuck, Close proceeds and closes the network connection, which errors the background loops and drains the ring — unparking that goroutine, so nothing leaks. The regression test fails against the previous Close. A companion test pins the already-safe path: when the connection errors first, the ring is drained and Close returns promptly.

The previous commit moved the enqueue-and-wait into a goroutine so the existing one-second escape could cover both. That costs a goroutine on every graceful close, on a termination path that already has enough of them. Bound the enqueue directly instead: arm a timer that cuts the connection after one second, and stop it as soon as queue.PutOne returns. Cutting the connection is what unparks a stuck enqueue anyway — the background loops error out and drain the queue — and time.AfterFunc costs a goroutine only when it fires, which happens only when the enqueue is stuck. A normal close now allocates a timer and stops it, nothing more. The escape does nothing Close does not already do a few lines further down, and syncDo closes the same connection on its own error path. A deadline on the connection would be cheaper still, but it is shared state that _background and the syncDo family overwrite, with no way to read it back, so it cannot be relied on. Cutting the connection also breaks a sync read parked on it, which the goroutine variant did not do. That covers the remaining case: a pipe still in sync mode, with callers queued behind the sync command filling the ring. Close does not start the background worker there, because waits != 1 tells it a sync read may be in flight, so the drain waits on the sync read failing first. TestCloseReturnsOnFullRingBeforeBackground pins it; like TestCloseReturnsOnFullRing it deadlocks without the fix.

The timer was stopped as soon as queue.PutOne returned, so it covered only the enqueue and the reply still needed its own one-second escape. Stopping it after the wait covers both: a silent peer leaves the timer to close the connection, the background loops fail and drain the queue, and the drain answers the PING with an error, which ends the wait. That removes the select and the goroutine it spawned on timeout, so a stuck shutdown now costs the timer callback alone, and a healthy one costs nothing. TestCloseWithGracefulPeriodExceeded covers the case this rests on, a peer that never answers a PING that did get a slot. It still returns after a second.

rueian reviewed Jul 26, 2026

View reviewed changes

rueian reviewed Jul 28, 2026

View reviewed changes

FZambia added 3 commits July 31, 2026 09:40

FZambia force-pushed the fix/pipe-close-full-ring branch from 50930ea to b7bf06a Compare July 31, 2026 06:41

rueian approved these changes Aug 2, 2026

View reviewed changes

rueian merged commit 98f5bc3 into redis:main Aug 2, 2026
29 checks passed

Navigation Menu

Uh oh!

fix(pipe): don't let Close hang when the command ring is full - #1013

fix(pipe): don't let Close hang when the command ring is full#1013
rueian merged 3 commits into
redis:mainfrom
FZambia:fix/pipe-close-full-ring

FZambia commented Jul 26, 2026 •

edited by cursor Bot

Loading

Uh oh!

rueian Jul 26, 2026

Uh oh!

FZambia Jul 27, 2026 •

edited

Loading

Uh oh!

rueian Jul 28, 2026

Uh oh!

FZambia Jul 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

FZambia commented Jul 26, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Impact

Root cause

Fix

Tests

Uh oh!

rueian Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

FZambia Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rueian Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

FZambia Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

FZambia commented Jul 26, 2026 •

edited by cursor Bot

Loading

FZambia Jul 27, 2026 •

edited

Loading