#36259 http: update m_keep_alive under m_send_mutex

ReviewabilityReviewsAgreementSize
#36259 azuchi http: update m_keep_alive under m_send_mutexReady0Strong12 + 144 tests
  • Prevent premature HTTP disconnects from dropping responses on pipelined requests
  • Reviewing now is worthwhile with nothing known that would invalidate it
  • Support for keeping state in sync to prevent premature disconnects (winterrdog)
  • Suggests delaying review because the bug is hard to hit in practice (winterrdog)
  • Concept approval noting low review priority (pinheadmz)
  • 12 lines added or modified outside tests
  • 144 lines added or modified in tests
  • 3 lines removed in total
  • 3 files, 1 commits
RPC / REST / ZMQ#29 of 55
P3 · bug fix
  • P3 because it fixes dropped responses during pipelined HTTP requests ending with close
  • The bug is a narrow race condition that is difficult to trigger in real workloads
  • Deferring the fix has minimal impact on overall RPC stability
  • Ranking pass: Moved up 13: dropped HTTP responses on pipelined requests is a genuine server-side correctness race, ahead of feature additions.
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

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.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • author: response to winterrdog's suggestion regarding pragmatism and priority 2026-09-15

Agreement

Strong

substantive support, no open objection (winterrdog)

Thread notes: Added pinheadmz's non-substantive Concept ACK.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
winterrdog (contributor)suggestion, usefulnessdelay review because the scenario is not easy to reach pragmaticallyopenansweredno2026-09-15: 'since this scenario is not easy to reach _pragmatically_, i'd rather delay review'

Support:

  • winterrdog (contributor) (Concept ACK, 2026-09-15) [read]: makes sense to update m_keep_alive in the same critical section as the send buffer, since both are read together under m_send_mutex, keeping state changes in sync and preventing the I/O thread from seeing an empty buffer with keep-alive cleared
  • pinheadmz (member) (Concept ACK, 2026-10-31): seems reasonable, but low priority for review [not substantive]

Participants:

  • winterrdog (contributor, support, 1 statements 2026-09-15 to 2026-09-15): Concept ACK, but suggested delaying review because the scenario is hard to reach
  • pinheadmz (member, support, 1 statements 2026-10-31 to 2026-10-31): Concept ACK, considered low priority for review

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

The short text the ranking pass compares across PRs.

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 ledger, generated 2026-11-01T01:16, confidence high, input hash fa2b2c3f4f9b8a11
thread read through 2026-10-31 (2 statements), head dc8fb12509
labels: RPC/REST/ZMQ

Categories

RPC / REST / ZMQ

P3 · bug fix (ranking pass: Moved up 13: dropped HTTP responses on pipelined requests is a genuine server-side correctness race, ahead of feature additions.)

Fixes an HTTP server race condition where pipelining a keep-alive request followed by a 'Connection: close' request can cause the I/O thread to disconnect prematurely and drop the second response. While a legitimate correctness fix for pipelined HTTP requests, it addresses a narrow edge case that is reasonably deferrable without broader impact on RPC stability.

Membership: Matches the RPC/REST/ZMQ label and directly modifies the HTTP server in src/httpserver.cpp and src/httpserver.h.

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