fix(companion): stop remote desyncing and hanging on reconnect#1530
Open
8bitgentleman wants to merge 1 commit into
Open
fix(companion): stop remote desyncing and hanging on reconnect#15308bitgentleman wants to merge 1 commit into
8bitgentleman wants to merge 1 commit into
Conversation
Active remote use jammed the encrypted channel because decrypts weren't serialized: Stream.listen doesn't backpressure async callbacks, so bursty commands interleaved the read-modify-write of the recv counter and desynced it permanently. Mirror the existing send-chain with a recv-chain so decrypts stay strictly ordered. Also re-discover the host on reconnect instead of dialing a stale cached address, bound the connect so a dead endpoint fails fast instead of spinning, and stop wiping the discovery list on a failed connect.
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.
Problem
Companion Remote (phone controlling playback on a desktop/TV host over the LAN) had three failure modes I kept hitting:
Root causes
The desync was the interesting one. Each frame is encrypted with a monotonically increasing counter as the SecretBox nonce, so the receiver has to decrypt frames strictly in order. The receive path read
_recvCounter,awaited the decrypt, then incremented it. The problem is thatStream.listendoesn't backpressure anasynccallback: it hands you the next frame without waiting for the previous callback's Future to complete. So two frames arriving close together both read the same counter across theawait, and the channel desyncs. It never recovers either, because a failed decrypt doesn't increment. This is why an idle session (keepalive pings 5s apart, never racing) stayed fine while actually using the remote jammed it. The send path was already serialized through_sendChainfor exactly this reason; the receive path just never got the same treatment.The reconnect spinner had two causes. Reconnect trusted an in-memory cached
ip:portand never re-discovered, so if the host's advertised address drifted while the phone slept, every attempt dialed a dead endpoint. AndIOWebSocketChannel.connect()had noconnectTimeout, soawait _channel!.readyon a dead endpoint hung until the OS TCP timeout (~2 min) per attempt, on top of exponential backoff over the same dead address.The discovery list emptied because the view called
stopDiscovery()before awaiting the connect action.Changes
Added
_recvChainto mirror the existing_sendChain, so_decryptIncomingchains each decrypt onto the previous one and the read/increment of_recvCountercan't interleave across anawait. The chain gets reset at the same session boundaries as_sendChain(join and cleanup). This is purely about ordering local decrypt work; the wire format is unchanged.On reconnect, re-discover the host instead of trusting the cache. New
_rediscoverHost(clientId, {timeout})listens on the mDNS/UDP discovery stream, matches the beacon byclientId, and returns the fresh host, ornullon timeout (it never throws)._attemptReconnectre-discovers first, races to the fresh address, and updates the cache; if discovery times out it falls back to the cached address. Manualip:portsessions have no clientId, so they skip discovery. There's a cancellation guard so it bails if you tapped Cancel while discovery was running.Gave
IOWebSocketChannel.connect()aconnectTimeout, so a dead endpoint fails in a few seconds and the existing catch path tears down and reports it instead of hanging.Removed the premature
stopDiscovery()so the host list survives a failed connect.The branch also carries some earlier hardening in the same subsystem:
_teardownChannelnulls the channel before a bounded close,disconnect()bounds both the socket and channel closes, a clean-state reset guards a fast-fail path that used to leave stale handshake state around, and the mid-join cancel path now cancels subscriptions before disconnecting the peer.Tests
companion_remote_peer_service_connect_timeout_test.dart: a dead endpoint that accepts TCP but never speaks WebSocket fails well under 15s (proving the connect bound), and connection-refused fails promptly.companion_remote_peer_service_disconnect_test.dart:disconnect()on a dead channel completes quickly, and a subsequent connect opens a new socket (guards the teardown/close bounding).Verification
I verified this on a real phone against a macOS host, using the host-side decrypt counter as ground truth rather than trusting the UI.
The desync: before the fix, a burst of rapid taps made the host decrypt counter 6 twice and then jam at 7, followed by 21 straight MAC failures with every later command dropped. After the fix, the same 21-command burst decrypted with a clean monotonic counter (0 through 28) and zero failures, and the client got all its replies back.
Reconnect after sleep: I killed the host and forced its advertised port to drift. The phone reconnected on its own to the new address, and the reconnected session decrypted a full command burst (0 through 23) with zero failures. No spinner.
The discovery list survives a failed connect.
Worth flagging honestly: the two tests above guard the timeout and teardown paths, not the crypto fix. The evidence for the desync fix is the on-device counter traces, not a unit test.