#36259 http: update m_keep_alive under m_send_mutex

full analysis

https://github.com/bitcoin/bitcoin/pull/36259 · azuchi · +156/-3 in 3 files, 1 commits · labels: RPC/REST/ZMQ

Goal

  • Prevent dropped responses when an HTTP client pipelines requests ending with a connection close header
  • Ensures the server does not disconnect clients before queuing all pending responses

Synchronizes `HTTPRemoteClient::m_keep_alive` updates with `m_send_buffer` by moving the assignment inside `m_send_mutex` and switching the field from `std::atomic_bool` to `bool GUARDED_BY(m_send_mutex)`. Also adds a regression unit test simulating the race condition.

Problem: When a client pipelines a keep-alive request followed by a 'Connection: close' request, the worker handling the second request clears `m_keep_alive` before taking `m_send_mutex`. If the I/O thread drains the first response in between, it observes an empty buffer with keep-alive disabled and disconnects the client before the second response can be queued, dropping it.

Category: RPC / REST / ZMQ (#4 of 52)

P3 · bug fix

  • P3 because it resolves dropped responses during pipelined HTTP requests
  • Low severity bug because the race window is small and bitcoin-cli does not pipeline requests

Fixes a dropped response bug during pipelined HTTP requests. As the author notes, 'the window is small and bitcoin-cli does not pipeline, so this is a low-severity fix', making it a worthwhile correctness improvement without high release urgency.

Membership: Modifies HTTP server concurrency and request lifecycle under src/httpserver.*, which powers the RPC and REST interfaces.

Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0

Reviewability: Ready

  • Ready to review
  • Self-contained fix accompanied by a deterministic regression test

The patch is small, self-contained, clean, and accompanied by a deterministic mock-socket regression test.

Author status: active

Agreement: Strong

  • Support for updating keep-alive state alongside the send buffer to prevent inconsistent reads (winterrdog)
  • No objections raised

Concept ACK with rationale from winterrdog; no objections raised.

winterrdog gave a Concept ACK noting that updating m_keep_alive in the same critical section as the send buffer prevents the I/O thread from viewing inconsistent state while responses are queued.

  • winterrdog: 'concept ACK... makes sense to update m_keep_alive in the same critical section as the send buffer, since both are read together under the same lock'

Review verdicts (DrahtBot): 0

Files

144 lines under test/bench/ci.

  • src/test/httpserver_tests.cpp +144/-0
  • src/httpserver.cpp +8/-2
  • src/httpserver.h +4/-1

Card

This PR fixes a race condition in the internal HTTP server where HTTPRemoteClient::m_keep_alive was updated outside m_send_mutex. For pipelined requests where a keep-alive request is followed by a connection-closing request, the I/O thread could observe an empty buffer and a disabled keep-alive flag between requests, disconnecting the client and dropping the second reply. The fix guards m_keep_alive with m_send_mutex and includes a deterministic mock-socket regression test. winterrdog provided a concept ACK supporting the lock synchronization, and the PR is ready for review.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T15:59, confidence high, input hash dcdddba990c0d5b3