generated from MetaMask/metamask-module-template
-
Notifications
You must be signed in to change notification settings - Fork 6
Complete Ken protocol implementation #811
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
Merged
Merged
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
Identified bugs in handleRemoteMessage: no dedup check and wrong persistence order. Recommended fix: wrap in database transaction. Refs #808 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements issue #808: wrap handleRemoteMessage in a database transaction and add duplicate detection to ensure exactly-once delivery semantics. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…pty string to null Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
382e01f to
b74b31c
Compare
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- Move #highestReceivedSeq update after savepoint release to prevent ACKs for uncommitted message receipts - Add validation for seq field to prevent state corruption from malformed messages - Update tests to reflect that replies during transaction don't piggyback ACKs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When a reply is sent during message processing (e.g., redeemURLReply), #sendRemoteCommand clears the delayed ACK timer. Since the reply can't piggyback the ACK (transaction hasn't committed), we must restart the timer after commit to ensure ACK is eventually sent. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Split #handleRedeemURLReply into #prepareRedeemURLReply (validates and translates inside transaction) and #completeRedeemURLReply (modifies in-memory state after commit). This prevents promise resolution and pendingRedemptions deletion from escaping a rolled-back transaction. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The kernel-remote.ts file contained a stale duplicate of remoteDeliverSpec that didn't accept null results. Since index.ts imports from remoteDeliver.ts (which has the correct spec), kernel-remote.ts and its test were dead code. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Spread ...remoteDeliverSpec instead of duplicating method, params, and result fields. This ensures the handler stays in sync with the spec. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move #sendRemoteCommand call for redeemURL replies to after the savepoint commits. This prevents in-memory state (#nextSendSeq, #startSeq, timers) from diverging from the database if the transaction rolls back. Also refactor redeemURL and redeemURLReply handling into parallel prepare/complete phases for consistency. 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.
Post-commit operations (deferred completions, in-memory state updates) now execute outside the try-catch block. This prevents attempting to rollback an already-released savepoint if a completion operation fails. Also adds exemptFromCapacityLimit parameter to #sendRemoteCommand so redeemURLReply can bypass capacity checks - it's a kernel-initiated reply that must be sent to avoid leaving the remote hanging. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
4 tasks
sirtimid
approved these changes
Feb 6, 2026
Contributor
sirtimid
left a comment
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.
LGTM!
This was referenced Feb 6, 2026
sirtimid
added a commit
that referenced
this pull request
Feb 9, 2026
Move the #remoteGcRequested in-memory state change after the savepoint commit, consistent with the transactional message processing pattern from #811. Also consolidate test assertions to use toStrictEqual on parsed objects per project convention, and fix return value expectation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 9, 2026
…814) ## Summary Implements the DGC (Distributed Garbage Collection) protocol for remote kernel communication (Issue #779). - `deliverBringOutYourDead()` on `RemoteHandle` now sends a `['bringOutYourDead']` wire message to the remote kernel instead of being a no-op - The remote kernel schedules a local reap, runs GC, and sends back drops/retires - A ping-pong prevention flag (`#remoteGcRequested`) prevents infinite BOYD loops between kernels ## Changes **GC store widening (`gc.ts`, `garbage-collection.ts`):** - Widen `scheduleReap` to accept `EndpointId` (union of `VatId | RemoteId`) instead of just `VatId` - Update `addGCActions` and `processGCActionSet` to use `insistEndpointId` instead of `insistVatId` - Rename internal variables from `vatId`/`actionsByVat` to `endpointId`/`actionsByEndpoint` for clarity **BOYD protocol (`RemoteHandle.ts`):** - Add `BringOutYourDeadDelivery` to the `DeliveryParams` union type - Implement `deliverBringOutYourDead()` to send BOYD over the wire via `#sendRemoteCommand` - Add `'bringOutYourDead'` case to `#handleRemoteDeliver` that calls `scheduleReap` - Add `#remoteGcRequested` flag: set after the savepoint commit (consistent with the transactional message processing pattern from #811), when BOYD was triggered by an incoming remote request the subsequent local BOYD is suppressed **Tests:** - Unit tests for GC with remote endpoints and reap scheduling - Unit tests for BOYD send/receive, ping-pong prevention, seq/ack tracking, and persistence - E2E tests for distributed GC across two kernels via libp2p ## Test plan - [x] All ocap-kernel unit tests pass (1800+) - [x] All nodejs e2e tests pass (35) - [x] All extension e2e tests pass (19) - [x] CI passes 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adds a new cross-kernel `bringOutYourDead` control message and widens GC scheduling from vats to all endpoints, which can affect remote message flow and garbage-collection behavior if mis-triggered, but changes are mostly additive and well-covered by new unit/e2e tests. > > **Overview** > Implements **distributed garbage collection** across kernels by turning `RemoteHandle.deliverBringOutYourDead()` into a real wire-level `deliver` message (`['bringOutYourDead']`) that causes the receiving kernel to `scheduleReap` its remote endpoint and run GC, with a `#remoteGcRequested` flag to prevent infinite BOYD ping-pong and to reset on peer restart. > > Widens GC plumbing from vat-only to **endpoint-wide** operation by treating GC actions and reap scheduling as `EndpointId` (vat or remote) rather than `VatId`, and adds `Kernel.reapRemotes()` / `RemoteManager.reapRemotes()` debugging/test hooks to trigger remote reaping. > > Adds extensive coverage: new RemoteHandle unit tests for BOYD send/receive, seq/ack/persistence, ping-pong prevention, RemoteManager tests for reap scheduling, GC store tests for remote endpoint IDs, and nodejs e2e scenarios validating BOYD exchange and continued bidirectional messaging after DGC. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit ae4b9cc. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.6 <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.
Summary
handleRemoteMessage"no response" convention from empty string tonullWith issue #786 (crank buffering) already merged, this PR completes our Ken protocol implementation. All Ken protocol properties are now achieved:
Changes
Receive-side deduplication and transactional processing
RemoteHandle.handleRemoteMessage()now:seq <= highestReceivedSeqto skip duplicate messageshighestReceivedSeqat the end, inside the transactionReturn type convention change
Changed the "no response needed" return value from empty string (
'') tonullacross thehandleRemoteMessagepathway (10 files).Documentation
Updated
docs/ken-protocol-assessment.mdto reflect that all Ken protocol properties are now implemented.Test plan
🤖 Generated with Claude Code
Note
Medium Risk
Touches core remote messaging delivery/ACK sequencing and RPC return-type conventions; regressions could cause dropped messages, duplicate processing, or broken RPC interop despite added tests.
Overview
Completes receive-side Ken semantics by adding duplicate detection (
seq <= highestReceivedSeq) and wrapping inboundRemoteHandle.handleRemoteMessage()processing in a database savepoint, persistinghighestReceivedSeqonly on commit and deferringredeemURL/redeemURLReplyside effects until after commit.Changes the remote-message RPC contract to return
string | null(rather than empty string for “no response”) across browser runtime, Node platform services,RemoteManager, and RPCremoteDelivervalidation, and updates/extends tests accordingly. Documentation indocs/ken-protocol-assessment.mdis revised to mark all Ken protocol properties as implemented.Written by Cursor Bugbot for commit 210ff11. This will update automatically on new commits. Configure here.