don't panic when sending error response to disconnected client#332
Open
yoyo930021 wants to merge 1 commit intodoy:mainfrom
Open
don't panic when sending error response to disconnected client#332yoyo930021 wants to merge 1 commit intodoy:mainfrom
yoyo930021 wants to merge 1 commit intodoy:mainfrom
Conversation
If handle_request fails after the client has disconnected (for example a client timeout while the agent was still waiting on pinentry), the unwrap in the error-reporting path panics the tokio task and drops the socket. The client then reads EOF from the socket and reports an unhelpful "failed to parse message from agent: EOF while parsing a value at line 1 column 0", hiding the original error. Log the send failure as a warning instead.
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.
Summary
If
handle_requestfails and the client has already disconnected (forexample, the client was killed, timed out, or aborted while the agent
was still blocked on pinentry),
sock.send(...).await.unwrap()onsrc/bin/rbw-agent/agent.rs:82panics the spawned tokio task onBroken pipe (os error 32). Because it's a detachedtokio::spawntask, the panic is effectively silent from the client's perspective —
the socket is just dropped, and the client then reads EOF and reports:
This hides the original error (pinentry failure, network error, TLS
error, vault error, etc.) and makes agent-side issues very hard to
diagnose, because the real error message is thrown away and the panic
output in
agent.errcan be easy to miss (or missed entirely if theuser is looking in the wrong location for the log file).
Replace the
.unwrap()with alog::warn!on send failure so thespawned task exits cleanly, the agent stays alive, and at least the
send failure shows up in the agent log.
This does not fix the root cause of whatever made
handle_requestreturn
Err— the caller will still see EOF in the disconnect race— but it removes the silent panic, which means any error path that
would have been reported to a still-connected client now is, and
agent logs become meaningful again.
Reproduction
On macOS with a daemonized agent, configuring
pinentryto point ata curses pinentry that can't acquire a usable TTY makes
handle_requestreturnErrfromactions::login. If the clientalso experiences a delay (e.g.
ttyname_rcan take several secondson macOS — see #315) and the two races line up so the client has
closed the socket by the time the agent tries to write the error back,
the agent hits this
unwrap()and the user sees the unhelpful"EOF while parsing" message with no obvious cause.
The same silent-panic pattern triggers for any handler error when
the client has disconnected — network, TLS, 2FA, etc. — so the bug
isn't macOS- or pinentry-specific, just particularly easy to hit
there.
Test plan
--no-daemonizesocket (simulating client timeout/Ctrl+C mid-pinentry)
thread 'tokio-runtime-worker' panicked at src/bin/rbw-agent/agent.rs:82:30: called Result::unwrap() on an Err value: failed to write message to socket ... Broken pipeWARN rbw_agent::agent: failed to send error response to client: failed to write message to socket: Broken pipe (os error 32), agent stays alive, subsequent requests still served