generated from MetaMask/metamask-module-template
-
Notifications
You must be signed in to change notification settings - Fork 6
feat(remote-comms): handle reconnection to restarted peers with incarnation ID detection #807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
sirtimid
wants to merge
3
commits into
main
Choose a base branch
from
sirtimid/incarnation-id-reconnection
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
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
…nation ID detection - Add `clearRemoteSeqState` method to kernel store for clearing sequence state without removing the remote relationship - Add `handlePeerRestart` method to RemoteHandle that resets all state when a peer restarts with a new incarnation ID - Add `OnIncarnationChange` callback type and wire it through the transport layer, platform services, and RemoteManager - Clear permanent failure status on user-initiated sends to allow reconnection to previously-failed peers - Add RPC handler for browser runtime to notify kernel of incarnation changes - Update all test files to account for new callback parameters This enables proper handling when a peer restarts: the incarnation ID handshake detects the restart, and the callback resets RemoteHandle state (sequence numbers, pending messages) for a fresh start. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Coverage Report
File Coverage |
…hange
When a message triggers dial→handshake and incarnation change is detected,
the message was still being sent with stale content (old seq number, old
object/promise references). This could cause errors on the remote peer.
Changes:
- Update doOutboundHandshake to return { success, incarnationChanged }
- Throw error when incarnation changes during sendRemoteMessage to prevent
stale message delivery
- Update reconnection-lifecycle to handle new handshake return type
- Fix outdated comment in e2e test (onRemoteGiveUp → onIncarnationChange)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
The handleIncarnationChange callback was missing kernel promise rejection. When a remote peer restarts, any pending kernel promises that were waiting on that peer need to be rejected since the peer has lost its state and won't be able to resolve them. This mirrors the behavior in handleRemoteGiveUp and ensures consistent promise handling for all remote failure modes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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.
Closes #806
Summary
clearRemoteSeqStatemethod to kernel store for clearing sequence state without removing the remote relationshiphandlePeerRestartmethod to RemoteHandle that resets all state when a peer restarts with a new incarnation IDOnIncarnationChangecallback type and wire it through the transport layer, platform services, and RemoteManagerBehavior
Test plan
clearRemoteSeqStatein storehandlePeerRestartin RemoteHandle🤖 Generated with Claude Code
Note
Medium Risk
Touches core remote-transport/handshake and message sequencing/persistence; bugs here could cause dropped/rejected messages or reconnection regressions, though changes are gated behind explicit incarnation-change detection and are well-covered by tests.
Overview
Remote comms now distinguishes “peer restart” from “connection give-up”. A new
OnIncarnationChangecallback is plumbed end-to-end (kernelRemoteManager→initRemoteComms→PlatformServices→initTransport, plus browser RPCremoteIncarnationChange) so the kernel can reset a remote when the handshake detects a changed incarnation ID.Remote state is reset on restart instead of treated as a permanent failure.
RemoteHandle.handlePeerRestart()rejects pending work, clears timers, resets seq/ack counters, and clears persisted per-remote sequence/pending-message state via new store methodclearRemoteSeqState, allowing fresh messaging after a peer reboot.Transport reconnection/send behavior is adjusted for restarts. Outbound handshake now returns
{ success, incarnationChanged }, reconnection logic consumes this, user-initiated sends clear “permanently failed” status before dialing, and sends abort when an incarnation change is detected to avoid delivering stale queued messages.Tests are updated/added across kernel store, remote handle/manager, transport lifecycle, nodejs+browser platform services, and e2e coverage;
.playwright-mcp/is added to.gitignore.Written by Cursor Bugbot for commit 94e4569. This will update automatically on new commits. Configure here.