fix(cable): propagate post-handshake errors from protocol::connection#217
Merged
Conversation
msirringhaus
requested changes
May 18, 2026
Collaborator
msirringhaus
left a comment
msirringhaus
left a comment
There was a problem hiding this comment.
If I'm not mistaken, some things could be simplified a bit
| Err(e) => { | ||
| error!(?e, "Failed to decode CABLE tunnel message"); | ||
| return Err(Error::Transport(TransportError::ConnectionFailed)); | ||
| return Err(TransportError::InvalidFraming); |
Collaborator
There was a problem hiding this comment.
This is a bit convoluted. CableTunnelMessage::from_slice() only returns Error::Transport(TransportError::InvalidFraming) on error, which we throw away here and hardcode it to TransportError::InvalidFraming again. from_slice() could just return a TransportError which we log here and send along.
| let known_device = CableKnownDeviceInfo::new(tunnel_domain, linking_info)?; | ||
| ) -> Result<CableKnownDeviceInfo, TransportError> { | ||
| let known_device = CableKnownDeviceInfo::new(tunnel_domain, linking_info) | ||
| .map_err(|_| TransportError::InvalidFraming)?; |
Collaborator
There was a problem hiding this comment.
Same here. CableKnownDeviceInfo::new() returns InvalidFraming anyways, and we throw away the error and hardcode it again.
AlfioEmanueleFresta
force-pushed
the
feat/pxp-ble
branch
from
78214b6 to
1bca6b7
Compare
Distinct from NegotiationFailed (handshake-time) and from generic ConnectionFailed so post-handshake Noise crypto failures can be surfaced explicitly. Used by the next commit in cable/protocol.rs.
protocol::connection previously returned () and swallowed every post-handshake fault via 'let _ = ...': Noise decrypt/encrypt failures, malformed framing inside the encrypted channel, dropped CBOR-response receivers and transport-level I/O errors. The connection silently degraded (subsequent frames keep failing the same way) and the channel eventually moved to Terminated without the caller ever learning what went wrong. Return Result<(), TransportError> instead, and replace each 'let _' in the select loop with explicit handling that exits on real faults and continues on benign ones: - Noise transport-mode encrypt/decrypt fail -> EncryptionFailed. - Outer cable-frame parse / oversize CBOR -> InvalidFraming. - Dropped CBOR-response receiver / I/O error -> propagate. - Malformed update message or bad linking signature -> log and continue (the Noise channel is intact; only that one update is unusable, matching Chromium's behaviour). - Peer-sent CableTunnelMessageType::Shutdown -> return Ok(()) via a new RecvOutcome::PeerShutdown. Helpers below the loop now return TransportError directly so the errors flow through without Error<->TransportError gymnastics at the top.
Both the QR-code and known-device channel tasks called protocol::connection(tunnel_input).await; ux_sender.set_connection_state(Terminated).await and discarded the result. With the previous commit propagating post-handshake faults, route Err(e) through send_error so the broadcast subscribers see a CableUpdate::Error before the channel state transitions to Terminated. send_error already performs the Terminated transition itself.
… to TransportError
AlfioEmanueleFresta
force-pushed
the
feat/pxp-ble-error-propagation
branch
from
7888d0f to
8cb03ac
Compare
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.
Summary
Stacked on
feat/pxp-ble(#216).protocol::connectionpreviously returned()and swallowed every post-handshake fault vialet _ = ...: Noise decrypt/encrypt failures, malformed framing inside the encrypted channel, dropped CBOR-response receivers and transport-level I/O errors. The connection silently degraded (subsequent frames keep failing the same way) and the channel eventually moved toTerminatedwithout ever emittingCableUxUpdate::CableUpdate(CableUpdate::Error(_)). Subscribers had no way to distinguish a clean close from a fault.Behaviour changes
protocol::connectionreturnsResult<(), TransportError>. Each arm of the select loop is explicit about whether a condition is fatal (exit) or benign (continue + log).TransportError::EncryptionFailedvariant is emitted for Noise transport-mode encrypt/decrypt failures, distinct from handshake-timeNegotiationFailedand genericConnectionFailed.CableTunnelMessageType::Shutdownis treated as a clean close (returnsOk(())) instead of an "unexpected" error.qr_code_deviceandknown_devicesrouteErr(_)from the tunnel loop throughUxUpdateSender::send_error, which already transitions toTerminated.Test plan
cargo check --workspace --all-targetscargo test --workspace --all-targetscargo clippy --workspace --all-targets -- -D warningscargo fmt --all -- --check