Skip to content

drop signal messages while waiting for reconnect response - #1107

Open
lukasIO wants to merge 1 commit into
mainfrom
lukas/drop-msgs-before-reconnect-response
Open

drop signal messages while waiting for reconnect response#1107
lukasIO wants to merge 1 commit into
mainfrom
lukas/drop-msgs-before-reconnect-response

Conversation

@lukasIO

@lukasIO lukasIO commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

in an effort to align connection behaviour consistently across SDKs, this PR adopts what the rust SDK is currently doing, which is drop all signal messages during a reconnect until a ReconnectResponse is received.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

⚠️ This PR does not contain any files in the .changes directory.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@pblazej

pblazej commented Sep 3, 2026

Copy link
Copy Markdown
Contributor
SDK Before ReconnectResponse How staleness is actually scoped
Rust drops all but Reconnect/Leave (get_reconnect_response) restart() closes the old stream under a write lock; the gate reads the new stream's own channel
JS forwards any non-leave first message (validateFirstMessage) attemptId generation counter — a superseded attempt is discarded
Android forwards first message, buffers + replays the rest (handleSignalResponse) ws != currentWs in five places
Flutter no gate (_onSocketData)
Swift (this PR) drops by message type one Bool in state shared across connections

The "align across SDKs" claim holds for Rust only, and in every SDK that has a gate the thing doing the real work is connection scoping, not the message-type filter. Swift's Bool outlives the connection and cleanUp() clears it before await _responseQueue.clear(), so a straggler from the dead socket still passes the gate — a per-connection generation counter (JS's attemptId is exactly this) gives the real invariant and makes the deny-list unnecessary. Worth noting both prior fixes in this area narrowed a drop (rust #893, android #934), and refreshToken/trackSubscribed/trackUnpublished/roomMoved have no server replay path.

@lukasIO

lukasIO commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

gives the real invariant and makes the deny-list unnecessary.

how so? if the server sends updates out of order or rather other messages prior to a ReconnectResponse on a fresh socket connection, that would still be an issue?

@pblazej

pblazej commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

On a fresh socket they aren't applied early either: connect() starts with await cleanUp(), which clear()s _responseQueue to .suspended before the new socket is even opened, so everything but join/reconnect/leave is enqueued and replayed in wire order by resumeQueues() after configureTransports. So the deny-list isn't changing when those messages are applied — it's changing buffered → discarded.

That said, I think you're pointing at something real, just one layer down: the ordering isn't actually guaranteed today, because each message is dispatched with its own Task.detached, so enqueue order isn't tied to wire order — a message that arrived before the ReconnectResponse can have its task run after resumeQueues() and get processed live, out of order against the replay. The deny-list does paper over that during reconnect, but it's pre-existing and applies in steady state too, so I'd rather fix the dispatch than filter by type. (The three data-track types are the one genuinely live path — they bypass the queue by design.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants