http: linger-close after parse errors so clients can read the reply #35780

b-l-u-e commented at 9:52 PM on July 22, 2026: contributor

Based on this issue #35632 raised i believe the issue is not with whitespace header validation since the logic works fine coz the server detects the malformed request and queues a 400 reply as expected.

The problem is how the server closes the connection afterwards. It queues the reply and closes the socket right away sometimes before it has finished reading everything the client had sent. So when socket is closed while there's still unread data sitting in the kernel's receive buffer, Windows responds with a TCP RST. That reset just throws away the reply that was already on its way out, so the client never gets to read the 400 and instead sees the connection aborted.

So my LLM Buddie and I realized that we should change how the server closes a connection after a parse/size error so the client always gets a chance to read the response first by queuing the 400/413 response with Connection: close. Then, waits until the response has actually left the send buffer. Then, Half-close only the write side (shutdown(SHUT_WR/SD_SEND)) so the client can still finish reading while the server stops sending. Afterwards, Keep draining whatever the client is still sending until it hits EOF or a socket error and only force the connection closed after a 1s fallback timeout, for a client that never finishes.

DrahtBot added the label RPC/REST/ZMQ on Jul 22, 2026

DrahtBot commented at 9:52 PM on July 22, 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/35780.

Reviews

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

Type	Reviewers
Concept ACK	pinheadmz, frankomosh

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:

#35735 (Add state to HTTPRequest by pinheadmz)
#34794 (rest: add Cache-Control headers to REST responses by w0xlt)

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.

b-l-u-e marked this as a draft on Jul 23, 2026

b-l-u-e renamed this:
~~http: linger-close after parse errors so clients can read the reply~~
[WIP] http: linger-close after parse errors so clients can read the reply
on Jul 23, 2026

b-l-u-e renamed this:
~~[WIP] http: linger-close after parse errors so clients can read the reply~~
http: linger-close after parse errors so clients can read the reply
on Jul 28, 2026

b-l-u-e force-pushed on Jul 28, 2026

DrahtBot added the label CI failed on Jul 28, 2026

util: add Sock::ShutdownSend() to half-close the send side

Wrap shutdown(2) (SHUT_WR, or SD_SEND on Windows) behind the Sock interface so it can be overridden by the mock sockets used in the unit and fuzz tests.

ac4e3636af

http: linger-close after parse errors so clients can read the reply

When the HTTP server rejects a request during parsing a malformed header or an oversized body, it replied and then immediately closed the connection. On Windows, closing a socket that still has unread inbound data can trigger a TCP RST, which discards the queued reply before the client reads it, so the client sees a connection reset instead of the 400/413 response (see #35632).
Instead of closing abruptly, flush the error reply, half-close the send side, and drain any remaining inbound data until the peer sends EOF. A 1s LINGERING_CLOSE_TIMEOUT bounds clients that never close their side.

2f14e212e8

b-l-u-e force-pushed on Jul 28, 2026

DrahtBot removed the label CI failed on Jul 28, 2026

b-l-u-e marked this as ready for review on Jul 29, 2026

pinheadmz commented at 5:26 PM on July 29, 2026: member

Is there any way to cover the new behavior with functional tests? For example, maybe in interface_http can assertions be added to the try/except blocks? Like, does this PR eliminate those race conditions?

in src/httpserver.cpp:543 in 2f14e212e8

 539 | @@ -512,7 +540,7 @@ bool HTTPRequest::LoadBody(LineReader& reader)
 540 |      }
 541 |  }
 542 |  
 543 | -void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> reply_body)
 544 | +void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> reply_body, bool force_close)

pinheadmz commented at 5:29 PM on July 29, 2026:

I'm not sure why we need a flag force_close in addition to HTTPRemoteClient.m_disconnect?

b-l-u-e commented at 5:05 PM on July 30, 2026:

the force_close is about HTTP response, send connection:close and dont keep-alive then for m_disconnect is for tear down this socket now .. so we need force_close first then m_disconnect later in order to tell client we are done flush the error reply drain a little bit then close so if we set m_disconnect immediately will close too early that will hit RST and hide the reply

in src/httpserver.cpp:931 in 2f14e212e8

 931 | +                if (!lingering_close) {
 932 | +                    // Copy data from socket buffer to client receive buffer
 933 | +                    client->m_recv_buffer.insert(
 934 | +                        client->m_recv_buffer.end(),
 935 | +                        buf,
 936 | +                        buf + nrecv);

pinheadmz commented at 5:31 PM on July 29, 2026:

If we're just draining the socket and ignoring the data anyway, do we still need to copy it into memory at all? (And then clear it immediately?)

b-l-u-e commented at 5:07 PM on July 30, 2026:

i agree i will clean it up

in src/httpserver.cpp:1081 in 2f14e212e8

1077 | @@ -1045,6 +1078,7 @@ void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr<HTTPRemot
1078 |  void HTTPServer::DisconnectClients()
1079 |  {
1080 |      const auto now{Now<SteadySeconds>()};
1081 | +    const auto now_ms{Now<SteadyMilliseconds>()};

pinheadmz commented at 5:33 PM on July 29, 2026:

I think if we need this level of accuracy here, we can just replace the seconds counter with ms and only use one. See #35182 (review)

b-l-u-e commented at 5:07 PM on July 30, 2026:

agreed will update

in src/httpserver.cpp:283 in 2f14e212e8

 278 | + * Covers requests rejected while parsing: malformed syntax as well as an
 279 | + * oversized body. Runs only on the HTTP I/O thread. The send path starts the
 280 | + * timeout and half-closes after flushing the reply; the receive path then
 281 | + * drains to EOF.
 282 | + */
 283 | +static void SendErrorReplyAndLingerClose(HTTPRequest& req, HTTPRemoteClient& client, HTTPStatusCode status)

pinheadmz commented at 5:34 PM on July 29, 2026:

I don't totally understand why we need a specific function for this, like a linger close is a special type of close? Why don't we just make it so every client close (for any reason) drains the incoming socket for up to 1 second first?

b-l-u-e commented at 5:07 PM on July 30, 2026:

well linger isnt needed for every close its useful when we reject a request while client is still sending huge body or bad request so if we close with unread data still sitting there then windows can RST and client never sees 400 or 413

in src/httpserver.cpp:939 in 2f14e212e8

 940 |          // Process as much received data as we can.
 941 |          // This executes for every client whether or not reading or writing
 942 |          // took place because it also (might) parse a request we have already
 943 |          // received and pass it to a worker thread.
 944 | -        MaybeDispatchRequestsFromClient(client);
 945 | +        if (!lingering_close) {

pinheadmz commented at 5:36 PM on July 29, 2026:

I don't think we need to guard this, since a lingering close happens in the middle of a request being processed already. This will be even better handled after #35735 when we won't even read from the socket if one request is being handled.

b-l-u-e commented at 5:09 PM on July 30, 2026:

i thinkk we still want the guard because once we are lingering that connection should only drain and go away not keep parsing or dispatching more work in #35735 helps with one request at time less queue weirdnes but doesnt remove the need for if we are shutting this connection via linger doesnt treat it as a normal live client

pinheadmz commented at 5:38 PM on July 29, 2026: member

Concept ACK on lingering close to be more polite to clients and improve test determinism. However I am not convinced about the approach. I also think this is a feature that would be easier to implement after #35735

b-l-u-e commented at 5:09 PM on July 30, 2026: contributor

Concept ACK on lingering close to be more polite to clients and improve test determinism. However I am not convinced about the approach. I also think this is a feature that would be easier to implement after #35735

i will wait for your PR to be merged first

fanquake marked this as a draft on Aug 10, 2026

fanquake commented at 9:13 AM on August 10, 2026: member

I've moved this to draft for now. In the mean time, @b-l-u-e if you want to rebase this on top of #35735, that could be worthwhile.

DrahtBot added the label Needs rebase on Aug 10, 2026

DrahtBot commented at 10:11 AM on August 10, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

in src/httpserver.cpp:1097 in 2f14e212e8

1093 | +                                            client->m_lingering_half_closed &&
1094 | +                                            now_ms >= client->m_lingering_close_deadline.load()};
1095 |  
1096 | -                                        // Disconnect this client due to error, end of communication, or idle timeout.
1097 | +                                        // Disconnect on error, EOF, idle timeout, or a stalled lingering close.
1098 |                                          // May drop unsent data if we are closing due to error.

pinheadmz commented at 5:39 PM on August 11, 2026:

I think the motivation behind this PR is to be able to honestly remove this comment? Even in case of an error, we shouldn't drop the response for the client.

frankomosh commented at 7:18 PM on August 12, 2026: contributor

Concept ACK. Agree the goal here is worth solving

151henry151 commented at 1:27 AM on August 13, 2026: contributor

Is there any way to cover the new behavior with functional tests? For example, maybe in interface_http can assertions be added to the try/except blocks? Like, does this PR eliminate those race conditions?

The try/excepts in check_excessive_request_size and check_chunked_transfer are the 413 cases, where the client is still uploading when we reject. Drain-until-EOF is meant for that, and on loopback it probably does make those reliable. I still wouldn't turn them into hard asserts — if the upload hasn't finished by the fallback timeout, that's a flake that only shows up on slow runners.

The #35632 flake is the other way around: we reject at the headers and close with the body still unread, so Windows RST's and the 400 never arrives. check_whitespace_in_headers already asserts 400.