Fix silent truncation of large command responses in BedrockServer::_reply()#2661
Open
roryabraham wants to merge 10 commits into
Open
Fix silent truncation of large command responses in BedrockServer::_reply()#2661roryabraham wants to merge 10 commits into
roryabraham wants to merge 10 commits into
Conversation
…ncation BedrockServer::_reply() sends a command response with a single blocking socket->send() call, then immediately shuts the socket down, with no loop to confirm the whole response was actually queued to the kernel. If that one send() queues fewer bytes than the full response (e.g. because a signal interrupts the blocking wait), the remainder is silently dropped instead of retried. Add SocketSendDrainTest, which forces this exact condition deterministically: it wraps a real loopback socket in the actual STCPManager::Socket class, interrupts a large (20MB) blocking send() mid-transfer with a targeted SIGALRM (via pthread_kill, not a mock), and asserts the client receives the complete response. This currently fails (truncated at ~100KB), reproducing the bug. Co-authored-by: Cursor <cursoragent@cursor.com>
…eply() A single socket->send() call can queue fewer bytes than the whole serialized response (e.g. if a signal interrupts the blocking wait), and _reply() shut the socket down immediately afterward regardless, silently discarding whatever was left in the in-process sendBuffer. Loop send() until sendBufferEmpty() (or a hard error) before shutting down, mirroring the pattern already used in SQLiteClusterMessenger and STCPManager::postPoll. Updates SocketSendDrainTest to exercise this drain-loop pattern instead of the old single-send-then-shutdown one; it now passes reliably (verified 5/5 runs) under the same forced-interruption fault injection that previously reproduced the truncation. Co-authored-by: Cursor <cursoragent@cursor.com>
Adds LargeCommandResponseTest, which drives a real BedrockServer over the real command port with a query that returns a 5MB response, and verifies the client receives it complete and byte-for-byte correct. This complements SocketSendDrainTest: it exercises the full real pipeline (plugin -> BedrockCommand -> serialization -> _reply() -> command port) end-to-end, whereas SocketSendDrainTest exercises STCPManager::Socket directly under a forced fault (an interrupted send()) to reliably reproduce/guard against the truncation bug itself. Co-authored-by: Cursor <cursoragent@cursor.com>
Empty function bodies need their brace on its own line per .uncrustify.cfg. Co-authored-by: Cursor <cursoragent@cursor.com>
It only confirms a large response is delivered correctly over a healthy loopback connection, which was never in question -- a single blocking send() call typically blocks internally until fully queued under those conditions. It doesn't force the actual triggering condition for the bug this branch fixes (a signal interrupting the blocking send()), so it wouldn't catch a regression back to the old single-send-then-shutdown pattern in _reply(). SocketSendDrainTest is the real regression guard; this test was redundant coverage without a concrete bug it protects against. Co-authored-by: Cursor <cursoragent@cursor.com>
No behavior change. Co-authored-by: Cursor <cursoragent@cursor.com>
Moves the drain loop out of BedrockServer::_reply() and into a new STCPManager::Socket::sendAll(), which loops send() until sendBuffer is empty (or a hard error occurs). Keeps the fix in one place instead of duplicated between production code and any test that wants to exercise it. Co-authored-by: Cursor <cursoragent@cursor.com>
The drain loop it exercised is now factored out into STCPManager::Socket::sendAll() and already covered indirectly by every existing test that sends a response over the command port (CommandPort, Query, Read, Write, the Jobs tests, etc.), all of which go through BedrockServer::_reply() -> sendAll(). Co-authored-by: Cursor <cursoragent@cursor.com>
buildCommandFromRequest() sends the "202 Successfully queued" ack with a single send() call and then immediately shuts the socket down. Same truncation risk as the bug fixed in _reply(): a partial send() here would leave bytes stranded in sendBuffer, which shutdown(CLOSED) then discards without ever reaching the wire. Co-authored-by: Cursor <cursoragent@cursor.com>
sendAll() loops send() until the buffer drains, which relies on the kernel blocking send() when its buffer is full. On a non-blocking socket, send() would instead return immediately on a full buffer, turning the loop into a CPU-burning busy-spin. Assert against that misuse instead of silently spinning. Co-authored-by: Cursor <cursoragent@cursor.com>
roryabraham
marked this pull request as ready for review
July 2, 2026 21:24
tylerkaraszewski
requested changes
Jul 2, 2026
tylerkaraszewski
left a comment
tylerkaraszewski
left a comment
Contributor
There was a problem hiding this comment.
This is the wrong fix for this. STCPManager::Socket::Send
| socket.send(response.serialize()); | ||
| // sendAll() loops until the whole response is queued to the kernel (or a hard error occurs); we're about to | ||
| // shut down the socket, so we won't get another chance to send anything left behind by a partial send(). | ||
| socket.sendAll(response.serialize()); |
Contributor
There was a problem hiding this comment.
This fix is "correct" because we actually shut down the socket here but is sort of pointless because there is no message being sent. We are literally sending 202 Successfully queued and close.
| if (!command->socket->send(command->response.serialize())) { | ||
| // Otherwise we send the standard response. sendAll() loops until the whole response is queued to | ||
| // the kernel (or a hard error occurs), rather than assuming a single send() call queued everything. | ||
| if (!command->socket->sendAll(command->response.serialize())) { |
Contributor
There was a problem hiding this comment.
So this really isn't correct. If there's more data than we can send here we don't want to block, we want this to get sent on the next loop.
Contributor
There was a problem hiding this comment.
I think a better way to handle this (maybe it is already handled, I'm not sure) is to check if the sendbuffer is empty when we attempt to close a socket, and then refuse to until it's emptied.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
BedrockServer::_reply()sent a command's response with a single blockingsocket->send()call and then immediately shut the socket down, without checking whether the entire serialized response was actually queued to the kernel first. If that onesend()call queued fewer bytes than the full response (e.g. because a signal interrupts the blocking wait), the remainder sat in the socket's in-processsendBufferand was silently discarded — the client would receive a truncated response with no error indication.This PR adds
STCPManager::Socket::sendAll(), which loopssend()until the socket'ssendBufferis fully drained (or a hard error occurs), and switches_reply()to use it instead of a singlesend()call — mirroring the drain pattern already used inSQLiteClusterMessengerandSTCPManager::postPoll.The other
Socket::send()call sites (plugins/MySQL.cpp,SQLitePeer::sendMessage,SHTTPSManager's outbound request) are all driven by the ongoingprePoll/postPollloop rather than a one-shot blocking thread, so a singlesend()there is correct as-is — the poll loop keeps drainingsendBufferon subsequent iterations, and switching those tosendAll()would risk busy-spinning the shared poll thread on a non-blocking socket instead. Left those alone.sendAll()only avoids busy-spinning because command-port sockets are accepted as blocking (S_accept(..., true)), so the kernel itself blockssend()until it can accept more data instead of returningEAGAIN. To make sure this doesn't quietly regress ifsendAll()is ever called on a non-blocking socket in the future, it now asserts the fd is blocking before looping.Fixed Issues
N/A — this is a proactively-identified correctness bug, not filed as a separate GitHub issue.
Tests
Covered by existing unit and integration tests that exercise the command port response path (e.g.
CommandPortTest).