http: disconnect clients that never finish a request #36204

janb84 commented at 11:55 AM on September 9, 2026: contributor

The -rpcservertimeout timer only measures inactivity. Every read from a client resets it, so a client that keeps sending bytes without ever completing a request can hold the connection for ever.

Connection slots are capped by default 16 or by -rpcmaxconnections. Once those slots are taken, the server stops accepting new connections. Because no request ever completes the authentication functions are never hit. The impact is low because RPC port binds to localhost by default, so this is a robustness fix rather than a remotely exploitable security issue.

<details> <summary> How to reproduce </summary>

To reproduce: start a node with -rpcmaxconnections=2, open two sockets, and write "GET / HTTP/1.1\r\n" one byte at a time with ten seconds between bytes. bitcoin-cli getblockcount then hangs until one of the (trickling) sockets is closed.

</details>

The fix adds a second deadline next to the idle timer. It is armed on the first byte of a request and expires after -rpcservertimeout (the same value as the idle timer). This timer also does not resets or is extended by later reads. Completing request parsing clears the deadline, so a long-running RPC keeps its connection as before. The deadline is deferred while a worker holds the request, for the same reason the idle timer is. It is restarted while response bytes are written, because the client cannot start a new request during that window. -rpcservertimeout=0 disables both timers. No new option is added.

Disclaimer this bug is found by utilizing ASTRA. Verified using KIMI 3 and Claude.

DrahtBot added the label RPC/REST/ZMQ on Sep 9, 2026

DrahtBot commented at 11:55 AM on September 9, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36204.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	hodlinator

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36159 (http: Improve HTTPRemoteClient::MaybeDisconnect() by hodlinator)
#36124 (http: Make HTTPRequest update state internally by hodlinator)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

[test/functional/interface_http.py] assert duration >= RPCSERVERTIMEOUT - 1, f"Slot reclaimed too fast: {duration}" -> consider assert_greater_than_or_equal(duration, RPCSERVERTIMEOUT - 1)

janb84 force-pushed on Sep 9, 2026

DrahtBot added the label CI failed on Sep 9, 2026

DrahtBot removed the label CI failed on Sep 9, 2026

in src/httpserver.cpp:1166 in 537944cb78 outdated

1162 | +    // defers to a busy server, because disconnecting a client while a worker
1163 | +    // thread still owes it a response is unsafe.
1164 | +    const bool is_stalled{rpcservertimeout.count() > 0 &&
1165 | +                          m_request_since &&
1166 | +                          now - m_request_since.value() > rpcservertimeout &&
1167 | +                          !m_req_busy};

hodlinator commented at 12:39 PM on September 11, 2026:

nanonit: Feels like !m_req_busy would be the cheapest check and be the first one (same for is_idle).

janb84 commented at 6:03 PM on September 14, 2026:

Have to pushback on this one, m_req_busy is mostly false so the negate makes that true. It would cause an extra check that is mostly true and therefor does not short the evaluation. I think the rpcservertimeout.count() is the cheapest one, given it's already in a register.

hodlinator commented at 10:18 AM on September 17, 2026:

Would expect m_req_busy to be true most of the time, a worker thread is busy processing the request. (The exception being very bad connections with larger requests which take time to receive). But maybe there's something with keep-alive connections that make them tend to linger after the response was sent.

I think the rpcservertimeout.count() is the cheapest one, given it's already in a register.

Agree on this.

in src/httpserver.h:655 in 537944cb78

 647 | @@ -631,6 +648,11 @@ class HTTPRemoteClient
 648 |      //! Due to optimistic sends it may be updated in either a worker thread or in the
 649 |      //! I/O thread. It is checked in the I/O thread to disconnect idle clients.
 650 |      std::atomic<SteadySeconds> m_idle_since;
 651 | +
 652 | +    //! Timestamp of the first byte of the request currently being parsed, or
 653 | +    //! nullopt when no request is in progress. Together with -rpcservertimeout
 654 | +    //! this is the deadline for delivering one complete request.
 655 | +    std::optional<SteadySeconds> m_request_since;

hodlinator commented at 12:46 PM on September 11, 2026:

Slightly more accurate?

    //! Timestamp for when the first byte of the current request was read,
    //! or when we last attempted to send more data to the client.
    //! nullopt when no request is in progress. Together with -rpcservertimeout
    //! this is the deadline for delivering one complete request.
    std::optional<SteadySeconds> m_request_since;

An alternative would be to always add rpcservertimeout to it when updating the value and call it m_request_deadline or m_req_read_deadline, which is easier to wrap one's head around?

janb84 commented at 5:52 PM on September 14, 2026:

Taken thanks

in src/test/httpserver_tests.cpp:805 in 537944cb78 outdated

 800 | +    constexpr auto timeout{30s};
 801 | +
 802 | +    // These tests can't move the steady clock, so they either hand
 803 | +    // MaybeDisconnect() a "now" that is well past the deadline, or plant a start
 804 | +    // time far enough in the past for the deadline to have expired.
 805 | +    const auto start{Now<SteadySeconds>()};

hodlinator commented at 12:50 PM on September 11, 2026:

Not sure time drift is an issue with these tests, but #36159's 9afe15897e8dbee9429722d0db7859d895919bf4 switches to the mockable steady clock and freezes time in 784ac4fb9b4b3b9a5614e4ea54c108e1246ce50a using a FakeSteadyClock. Worth adopting here?

janb84 commented at 5:59 PM on September 14, 2026:

Not sure what to do with this, yes you are correct. But to do this correctly that PR has to land first right?

hodlinator commented at 10:19 AM on September 17, 2026:

Or you could incorporate parts of it into this one? I'm happy however the changes get in.

in src/httpserver.cpp:920 in 537944cb78 outdated

 915 | +            // readability, so a request it has half-delivered cannot make
 916 | +            // progress while the response goes out. Restart its completion
 917 | +            // deadline, because the client is waiting on the server.
 918 | +            // Nothing is restarted while the socket stays unwriteable, so a
 919 | +            // client that never reads still times out.
 920 | +            client->RestartRequestDeadline();

hodlinator commented at 1:04 PM on September 11, 2026:

remark: Was wondering whether we were missing another call to RestartRequestDeadline() when HTTPRemoteClient::Send() calls MaybeSendBytesFromBuffer() and always sets m_req_busy = false. If it succeeds in emptying the send buffer ReadyToSend() will return false and we will not set Sock::SendEvent, so the request deadline will not be reset. But m_request_since will be nullopt when the request completes parsing and is handed to the worker, so no need to bump it along in that case.

janb84 commented at 6:12 PM on September 14, 2026:

After some checking, changed the comment. Do not think extra action is needed in send(), the extra call to RestartRequestDeadline() would not do a thing because of the empty m_reqeust_since. Imho, happy to be wrong.

in test/functional/interface_http.py:447 in 537944cb78

 442 | @@ -442,6 +443,102 @@ def check_idle_timeout(self):
 443 |          assert not conn.sock_closed()
 444 |  
 445 |  
 446 | +    def trickle_request(self, conn, bad_http_request, stop_trickling):
 447 | +        """Feed a bad http request to the server one byte at a time.

hodlinator commented at 1:06 PM on September 11, 2026:

nitlinator:

        """Feed a bad HTTP request to the server one byte at a time.

(Actually the first time in this file where the casing was messed up).

janb84 commented at 5:57 PM on September 14, 2026:

If you do not count Line 326 it's the first messed up casing indeed, stupid mistake, rectified!

in test/functional/interface_http.py:501 in 537944cb78

 496 | +            # Each client now starts a second request it never finishes. The
 497 | +            # deadlines all start with this first byte, which is what the
 498 | +            # timing below measures from.
 499 | +            start = time.time()
 500 | +            for conn in hogs:
 501 | +                conn.send_raw(bad_http_request[:1])

hodlinator commented at 1:09 PM on September 11, 2026:

Seems to work fine without send the first byte here and instead sending it in the thread?

janb84 commented at 5:56 PM on September 14, 2026:

Yes and I reworked the test, thanks

in test/functional/interface_http.py:532 in 537944cb78

 527 | +
 528 | +            # The deadline runs from the first byte of a request, not from the
 529 | +            # last one, so the trickling does not push it back.
 530 | +            assert duration <= RPCSERVERTIMEOUT + 2, f"Slot reclaimed too slow: {duration}"
 531 | +            assert duration >= RPCSERVERTIMEOUT - 1, f"Slot reclaimed too fast: {duration}"
 532 | +        finally:

hodlinator commented at 1:19 PM on September 11, 2026:

If the test fails for some reason we don't need to clean up the tricklers right? See 659671ac3db7e5157178efe4d9f8bce7d92ea237

janb84 commented at 5:56 PM on September 14, 2026:

correct, thanks, see above

hodlinator commented at 1:29 PM on September 11, 2026: contributor

Concept ACK 537944cb7869039912139793b58bc6296b64eebe

janb84 force-pushed on Sep 14, 2026

janb84 commented at 5:55 PM on September 14, 2026: contributor

Rebased to latests master Made change for the changes made by pr #36174 Incoporated suggestions made by @hodlinator Reworked functional tests

DrahtBot added the label Needs rebase on Sep 16, 2026

http: disconnect clients that never finish a request

Every read resets the idle timer, so a client that keeps sending bytes without ever completing a request holds on to its connection slot
indefinitely. A handful of them max-connections and no other
client gets served.

Added a second deadline timer that starts at the first byte of a request and is not extended by subsequent reads. It is cleared once the request has been parsed, so a slow RPC keeps its connection. The deadline is restarted while response data goes out.

fa53e0a958

test: cover HTTP connection slot exhaustion by unfinished requests

Limit the server to one connection and occupy it with a client that
trickles in a request one byte at a time and never finishes it. The
bytes arrive a quarter of -rpcservertimeout apart, so the idle timeout
never fires and only the completion deadline can free the slot.

A second client queued behind it must get a response about one
timeout after the first byte. A late response would mean each
byte pushed the deadline back. An early response would mean the server
freed the slot before the deadline. The debug log check confirms that
the server dropped the first client for missing its completion deadline.

99e880356b

janb84 force-pushed on Sep 17, 2026

janb84 commented at 8:21 AM on September 17, 2026: contributor

Rebased and adjusted the function naming.

DrahtBot removed the label Needs rebase on Sep 17, 2026

in test/functional/interface_http.py:505 in 99e880356b

 500 | +            response = waiting_conn.recv_raw()
 501 | +            duration = time.time() - start
 502 | +            assert response.startswith(b"HTTP/1.1 200 OK")
 503 | +
 504 | +        # The slot isn't freed before the deadline
 505 | +        assert duration >= RPCSERVERTIMEOUT - 1, f"Slot reclaimed too fast: {duration}"

hodlinator commented at 12:43 PM on September 18, 2026:

Why would we allow less than RPCSERVERTIMEOUT (which is only 2)?

        assert duration >= RPCSERVERTIMEOUT, f"Slot reclaimed too fast: {duration}"

in test/functional/interface_http.py:1 in 99e880356b

hodlinator commented at 1:10 PM on September 18, 2026:

nit: Maybe could switch commits from {fix+test} to {characterization-test+fix}? That way the ancestor commits CI job can also verify the pre-fix behavior (edit: and humans can clearly see the behavior-change). See https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/36204_suggestions

hodlinator commented at 1:13 PM on September 18, 2026: contributor

Reviewed 99e880356bac7ab6b0ed9f3fb1ff5cfdab4a4266

Nice how you cleaned up the test, makes it easy to grasp.