rpcclient: avoid double-resolving POST requests on shutdown
What the developer wrote
Authored by Boris Nagaev
rpcclient: avoid double-resolving POST requests on shutdown
When shutdown races with sendPostRequest, a request could be marked
as ErrClientShutdown and still be enqueued. The sendPostHandler cleanup
loop would then try to send a second terminal response and could block
forever on a full response channel.
Fix this by prioritizing the shutdown path. First check shutdown with a
non-blocking select and return immediately when it is already closed.
Then use a second select to choose between enqueue and shutdown for the
remaining race window.
A regression test verifies a shutdown request is failed immediately and
never enqueued.
What changed, and why it matters
This commit fixes a race condition in btcd's RPC client that could cause the client to hang forever during shutdown. When the client was shutting down at the same moment a request was being sent, the old code could both mark the request as failed due to shutdown and still place it in the outgoing queue. A cleanup loop would later try to send a second response on a channel that was already full, blocking indefinitely. The fix prioritizes the shutdown path so requests are failed immediately and never queued once shutdown has started. A regression test was added to confirm the behavior.
Treat this as a reliability and availability fix rather than an active exploit. Upgrade btcd nodes that rely on the RPC client to a version containing this commit to prevent shutdown hangs. If running an affected version, avoid issuing RPC calls immediately before or during client shutdown, and monitor for stuck goroutines in rpcclient.
Security signals we found
Race condition between shutdown and request enqueueing
Potential indefinite goroutine block on full response channel
Double-response / double-resolution of a single RPC request
Denial-of-service-like client hang during shutdown
Regression test added for shutdown prioritization
Evidence from the diff
In rpcclient/infrastructure.go, sendPostRequest previously used a single select that could non-deterministically choose between c.sendPostChan and c.shutdown when both were ready. If shutdown won, it returned without sending a terminal response; if sendPostChan won, the request was enqueued. A separate early shutdown check could also send ErrClientShutdown and then fall through to the second select, allowing the request to be enqueued after already being marked failed. The cleanup loop in sendPostHandler then attempted to respond again on jReq.responseChan, which could block forever if the caller had already received the first response and was no longer reading. The patch restructures sendPostRequest into two selects: first, a non-blocking check that immediately fails and returns if shutdown is already closed; second, a choice between enqueue and shutdown where shutdown now sends the terminal response. A new test verifies that, after shutdown is closed, 200 consecutive calls always fail immediately and never enqueue.
Changed components
rpcclient/infrastructure.go: sendPostRequestrpcclient/infrastructure.go: sendPostHandler cleanup looprpcclient/infrastructure_test.goInspect captured patch +55 / −3
diff --git a/rpcclient/infrastructure.go b/rpcclient/infrastructure.go
index 694e992..09856c6 100644
--- a/rpcclient/infrastructure.go
+++ b/rpcclient/infrastructure.go
@@ -956,19 +956,32 @@ cleanup:
// HTTP client associated with the client. It is backed by a buffered channel,
// so it will not block until the send channel is full.
func (c *Client) sendPostRequest(jReq *jsonRequest) {
- // Don't send the message if shutting down.
+ // Prefer shutdown when it is already closed so this path is
+ // deterministic. This mirrors addRequest and avoids post-shutdown
+ // enqueueing.
select {
case <-c.shutdown:
- jReq.responseChan <- &Response{result: nil, err: ErrClientShutdown}
+ jReq.responseChan <- &Response{
+ result: nil,
+ err: ErrClientShutdown,
+ }
+
+ return
+
default:
}
+ // Normal path: either enqueue, or fail if shutdown closes in the race
+ // window after the guard above.
select {
case c.sendPostChan <- jReq:
log.Tracef("Sent command [%s] with id %d", jReq.method, jReq.id)
case <-c.shutdown:
- return
+ jReq.responseChan <- &Response{
+ result: nil,
+ err: ErrClientShutdown,
+ }
}
}
diff --git a/rpcclient/infrastructure_test.go b/rpcclient/infrastructure_test.go
index 90795b5..bf09c65 100644
--- a/rpcclient/infrastructure_test.go
+++ b/rpcclient/infrastructure_test.go
@@ -478,3 +478,42 @@ func TestSendPostRequestAndRespondShutdown(t *testing.T) {
})
}
}
+
+// TestSendPostRequestShutdownPrioritizesFailure ensures shutdown always wins
+// when it is already closed before sendPostRequest is called.
+func TestSendPostRequestShutdownPrioritizesFailure(t *testing.T) {
+ client := &Client{
+ sendPostChan: make(chan *jsonRequest, 1),
+ shutdown: make(chan struct{}),
+ }
+
+ close(client.shutdown)
+
+ const attempts = 200
+ // The old single-select implementation chose randomly when both channels
+ // were ready, so repeat enough times to make an accidental enqueue show up.
+ for i := 0; i < attempts; i++ {
+ jReq := &jsonRequest{
+ id: uint64(i),
+ method: "getblockcount",
+ responseChan: make(chan *Response, 1),
+ }
+ client.sendPostRequest(jReq)
+
+ select {
+ case resp := <-jReq.responseChan:
+ require.ErrorIs(t, resp.err, ErrClientShutdown)
+ default:
+ t.Fatalf("request id=%d was not failed immediately",
+ jReq.id)
+ }
+
+ select {
+ case <-client.sendPostChan:
+ t.Fatalf("request id=%d was enqueued after shutdown",
+ jReq.id)
+
+ case <-time.After(10 * time.Millisecond):
+ }
+ }
+}
Why this scored 42/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.