Make the server reject misbehaving connections #212
Merged
Conversation
This file contains 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
lhchavez
requested review from
masad-frost and
jackyzha0
and removed request for
a team and
masad-frost
lhchavez
force-pushed
the
lh-make-the-server-reject-misbehaving-connections
branch
from
aed012a to
1c24ddb
Compare
masad-frost
approved these changes
Jun 18, 2024
We currently have a setup where the server just trusts whatever the client requests, and stomps over whatever was already there. That's okay in some cases, but not all! For example, if the client's timers are all wonky (like they are when the tab is hidden), we might get in a situation where the client tries to reconnect after its server-side grace period has been exceeded, and they don't agree on what the state should be. Also, if pid2 restarts for whatever reason, both parties should quickly agree that they're not agreeing upfront. This change now adds a new (optional) field to the handshake, `expectedState`. The client should fill this up and explicitly tell the server whether this is a reconnect attempt (currently this is done by checking if the `advertisedSessionId` is set, but that can be done by explicitly tracking whether the session has seen the server at least once when we move away from this model), and tell the server what its expected next sequence number is (so that the server can also detect drifts in this). If the clients don't fill this information up, the server will behave in a backwards-compatible fashion for safety.
lhchavez
force-pushed
the
lh-make-the-server-reject-misbehaving-connections
branch
from
1c24ddb to
9ef652b
Compare
lhchavez
added a commit
to replit/river-python
that referenced
this pull request
Jun 18, 2024
Why === We have a few cases where the server recreates a connection that was meant to be a reconnect. What changed ============ This change brings the Python implementation in line with the TypeScript one so that the server can proactively tell clients to recreate their sessions. See replit/river#212 for reference Test plan ========= Logs like https://app.datadoghq.com/logs?query=%40replid%3Ae2698176-9a19-4058-8619-7f5793b02cde&agg_m=count&agg_m_source=base&agg_t=count&cols=host%2Cservice%2C%40river.sessionId&event=AgAAAZAssKXztskj6QAAAAAAAAAYAAAAAEFaQXNzS2NBQUFBR1hKdG9Bd1NLLVFBbwAAACQAAAAAMDE5MDJjYjItYzE2Ny00YmZiLThjMjctNzdkNjI5Y2NkY2Vj&fromUser=true&messageDisplay=inline&refresh_mode=paused&storage=hot&stream_sort=desc&viz=stream&from_ts=1718734624299&to_ts=1718737623927&live=false should completely go away
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.
Why
We currently have a setup where the server just trusts whatever the client requests, and stomps over whatever was already there. That's okay in some cases, but not all! For example, if the client's timers are all wonky (like they are when the tab is hidden), we might get in a situation where the client tries to reconnect after its server-side grace period has been exceeded, and they don't agree on what the state should be. Also, if pid2 restarts for whatever reason, both parties should quickly agree that they're not agreeing upfront.
What changed
This change now adds a new (optional) field to the handshake,
expectedState. The client should fill this up and explicitly tell the server whether this is a reconnect attempt (currently this is done by checking if theadvertisedSessionIdis set, but that can be done by explicitly tracking whether the session has seen the server at least once when we move away from this model), and tell the server what its expected next sequence number is (so that the server can also detect drifts in this). If the clients don't fill this information up, the server will behave in a backwards-compatible fashion for safety.Versioning