generated from MetaMask/metamask-module-template
-
Notifications
You must be signed in to change notification settings - Fork 6
Add message sequencing and acknowledgment to remote messaging #744
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
FUDCo
wants to merge
6
commits into
main
Choose a base branch
from
chip/message-ack-retransmission
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
8 tasks
Implements message acknowledgment and retransmission: - Update RemoteCommand type to include seq (sequence number) and ack (piggybacked ACK) fields - Add per-peer sequence tracking that persists across reconnections - Implement data structures with cumulative ACK support - Implement transmission timeout with limited retries - Reject pending promises when giving up on reconnection
- Fix deadlock in receiveMessage by using fire-and-forget for reply sends - Fix PlatformServices to return reply instead of sending it - Add handleAck and updateReceivedSeq to PlatformServices interface - Implement delayed ACK mechanism (50ms timer) for standalone ACKs - Add timeout to libp2p.stop() to prevent cleanup hangs - Close channel streams explicitly on stop to unblock pending reads - Fix e2e test cleanup with parallel stops and increased hook timeout - Skip flaky "handles connection failure and recovery" test Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
74790e7 to
7697239
Compare
Restored validateMessageSize, checkConnectionLimit, and cleanupStalePeers functions that were accidentally removed during message sequencing changes. Also restored lastConnectionTime tracking and cleanup interval setup/teardown. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…scarding The browser runtime's #handleRemoteMessage was always returning an empty string, discarding the reply from the remoteDeliver RPC call. This broke reply-based protocols like ocap URL redemption. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Previously getNextSeq() was called before attempting to add a message to the queue, so rejected messages would still consume sequence numbers. This caused gaps in sequence numbering, which led to incorrect sequence numbers during retransmission since they were inferred from position. Changed addPendingMessage to assign and return the sequence number internally, only incrementing after successful enqueue. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
In the browser runtime, when the kernel worker calls sendRemoteMessage, the offscreen document awaits sendWithAck which waits for an ACK. When the ACK arrives via remoteDeliver, the kernel worker calls handleAck back to the offscreen. If handleAck awaited, this creates a deadlock because the offscreen's RPC message handler is blocked on the original sendRemoteMessage request. Making handleAck fire-and-forget breaks the deadlock while still ensuring ACKs are processed correctly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
12d0fcf to
2a0f438
Compare
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.
Implements message acknowledgment and retransmission.
Closes #656
Note
Introduces reliable remote messaging with sequencing and acknowledgments, plus API and infrastructure updates.
sendRemoteMessageto acceptRemoteMessageBase(object) instead of string across kernel, browser, and node runtimes; updates RPC payloads accordinglyhandleAckandupdateReceivedSeqmethods and wires them through PlatformServices (browser/node) as fire-and-forget callsRemoteHandleto parseseq/ackon inbound messages, call ACK handlers, and send base messages without local stringifyPeerConnectionStatefor per-peer state (seq tracking, pending queue) and revampsMessageQueueto storePendingMessagewith capacity-based rejectionlibp2p.stop()Kernel/RemoteManagerto forwardRemoteMessageBase; removes auto-reply send on handler returnWritten by Cursor Bugbot for commit 2a0f438. This will update automatically on new commits. Configure here.