Mirrord connection reconnect #3000
Open
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
|
…roxy-randomly-closes-during-connection
DmitryDodzin
changed the title
gememma
reviewed
Jan 27, 2025
…roxy-randomly-closes-during-connection
…roxy-randomly-closes-during-connection
…roxy-randomly-closes-during-connection
Razz4780
requested changes
Jan 29, 2025
There was a problem hiding this comment.
Beyond what's in the comments:
- I think we need to check if operator supports reconnecting (new operator feature). Otherwise things will get wild.
FilesProxyis never informed about the reconnect, and it is very stateful (queues expected responses, caches dir entries and file contents). This scares me 👀 Same applies to the ping pong task.- Not that I want to force my idea, but I think we can do sth simpler:
- Let the
AgentConntask fail in peace. - When
IntProxypicks it up, it enters a resetting state. It notifies all relevant tasks with a new message variant (e.gReset). Then, it makes a new agent connection and starts a newAgentConntask. For each task that was sent aReset,IntProxymemorizes that it expects one moreResetAckfrom it. - When a task gets the
Resetcommand, it immediately responds withResetAck. Then it does all necessary housekeeping, like cleaning state, sending error responses to the layer (for requests in progress), resending port subscriptions to the agent, dropping local connections, resetting ping pong state. - Intproxy ignores all messages from a resettable task until it gets expected count of
ResetAcks. - Due to layer storing remote file descriptors, we'd have to create an independent mapping in the
FilesProxyand make sure not to let through any requests for unknown remote fds. - If it doesn't do that yet,
BackgroundTasksshould remove the task's receiver from the streammap when this task exits (in order to drop all previous messages)
- Let the
Let me know what you think 'bout this
Comment on lines
545
to
551
| for subscription in self.subscriptions.iter_mut() { | ||
| tracing::debug!(?subscription, "resubscribing"); | ||
|
|
||
| for message in subscription.resubscribe() { | ||
| message_bus.send(ProxyMessage::ToAgent(message)).await | ||
| } | ||
| } |
There was a problem hiding this comment.
As far as IncomingProxy is concerned, these subscriptions were already confirmed. This looks kind of sus, I'd check out handling of DaemonTcp::PortSubscribe
There was a problem hiding this comment.
Well this is assuming that all agent side connections are dead thus we need someone to ask the agent to perform the subscription again so user's app isn't required to re-listen on incoming socket (like intproxy will just recreate the incoming tasks with new listens results)
…roxy-randomly-closes-during-connection
Co-authored-by: Michał Smolarek <[email protected]>
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.
Add reconnect option for operator to fix #2901