fix: discard a connection whose reply was never read - #545
Merged
Conversation
If a command is sent and its reply is not fully read — a read timeout, or any error
raised while reading — that reply stays queued on the socket. Reusing the connection
hands it to the NEXT command, and every command afterwards is one reply behind, so a
lookup can return a completely different record's value with no error anywhere.
Observed in a long-running deployment: a shared-key lookup returned the previous
command's reply, which is not an RSA ciphertext, and reported
Failed to decrypt shared_key.<recipient>@<sender> - Ciphertext length must be equal
to key size.
The stored record was perfectly good; restarting the process cleared it, because the
fault was in the connection rather than the data. Reproduced directly: after an
abandoned read, 'llookup:publickey@bravo' returned the earlier shared_key reply.
execute_command now disconnects when a reply cannot be read, so the next use starts
from a clean stream. The successful path is untouched, and commands sent with
read_the_response=False (monitor, noop) are unaffected.
Adds network-free tests: an abandoned read leaves the connection unusable, a successful
command keeps it, and a command that reads no reply is unaffected.
JeremyTubongbanua
approved these changes
Jul 30, 2026
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
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.
The problem
If a command is sent and its reply is not fully read — a read timeout, or any error raised while reading — that reply stays queued on the socket. Reusing the connection hands it to the next command, and every command after that is one reply behind. A lookup can then return a completely different record's value, with no error raised anywhere.
How it showed up
In a long-running deployment, publishing to one recipient began failing every cycle with:
The stored
shared_keyrecord was perfectly good. The lookup was returning the previous command's reply, which is not an RSA ciphertext. Restarting the process cleared it — the fault was in the connection, not the data.Reproduced directly against a live atServer: after an abandoned read,
llookup:publickey@bravoreturned the earliershared_keyreply.The fix
execute_commanddisconnects when a reply cannot be read, so the next use starts from a clean stream (disconnect()clears_connected, soconnect()rebuilds — #523).Deliberately minimal:
read_the_response=False(monitor,noop) are unaffected;retry_on_exceptionbranch, which callsconnect()and will now get a fresh socket.This matters more as callers adopt read timeouts (which are otherwise the right way to stop a silently dead peer blocking forever): without this, a timeout trades a hang for a connection that quietly returns wrong answers.
Tests
test/abandoned_read_test.py, network-free:read_the_response=FalseRelated
Supersedes #544, which I closed: I had misattributed this incident to a damaged
shared_keyrecord. Since the desynchronised case reports the identical error, replacing the key there would have rotated a perfectly good shared key for both parties — fixing the cause is both safer and sufficient.