Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Heads-up for this branch's next rebase — #1103 gained a commit that collides with this one. Bugbot flagged that This PR deletes
Two textual conflicts to expect as well, both in prose only: #1103 rewrote the stale paragraph in Worth saying plainly, since it argues for this PR: the approach here is the better fix. Holding the incarnation change behind the arrivals that peer already sent carries the old incarnation's messages out instead of discarding them, and running it as a crank closes a race the discard cannot reach — an arrival already shifted off the list by |
2e949a4 to
15a742a
Compare
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
15a742a to
e8b6cb0
Compare
e8b6cb0 to
0b446fa
Compare
0b446fa to
9b38dfc
Compare
33674dc to
fcbeaa2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fcbeaa2. Configure here.
| // The in-memory counters only, which no rollback could put back and | ||
| // which must not be visible before the writes above are durable. | ||
| afterCommit: async () => remote.finalizePeerRestart(), | ||
| }; |
There was a problem hiding this comment.
Restart cleanup races the transport
High Severity
#handleIncarnationChange answers the handshake and releases the transport before any restart cleanup. persistPeerRestart then runs in the crank, but finalizePeerRestart waits for afterCommit — after await deliver has already yielded. The send continuation's give-up path can run in that gap and write old sequence counters back over clearRemoteSeqState, while new-connection ACKs and delayed-ACK timers still operate on pre-restart in-memory state.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit fcbeaa2. Configure here.
The change took a `peerIncarnation_*` savepoint of its own, which nested inside whichever crank was open — the same defect the inbound message path just shed. It becomes a run queue item, carried out in a crank of its own. The handshake still gets its answer immediately: whether this is a restart is a read of what the store already says, and the transport needs it to decide whether to reset the connection. Only the writes are queued. Queued behind anything that peer has already sent, which is what keeps the two incarnations apart: the messages ahead of the change belong to the one that is ending and are recorded against it, the ones behind it to the one that is starting. So the eager discard the previous branch needed goes away — it would now throw away the new incarnation's messages rather than the old one's. Rejecting the promises the restarted remote was deciding, and resetting its in-memory state, move to `afterCommit`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up, and the same mistake the branch below it made: `afterCommit` must not write the kernel store, and `resolvePromises` writes — promise state, reference counts, and a notify row per subscriber, all in autocommit once `endCrank` has committed. It also raced the transport's own give-up handling, which rejects the same promises from a send continuation; whichever arrived second would `Fail` out of a post-commit hook and kill the kernel. The rejections move into the crank, buffered with `immediate: false` so the notifies they produce still wait for the commit, the way a vat's syscalls do. `afterCommit` keeps only the in-memory counter reset. An incarnation change that cannot be recorded no longer kills the run loop — the same containment the inbound message path already has — and the router's exhaustiveness check comes back by excluding the item type the kernel handles itself, rather than by deleting the directive that proved it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An arrival is the one kind of work that does not go through `#enqueueRun`, so its wake is its own. Deleting either call left every test green; a parked loop with work waiting is a permanent wedge. `does not reject promises when there are none` also asserted only a negative, and passed whether or not the restart it describes had happened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fcbeaa2 to
ff8bb85
Compare
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
This branch has not been deployed
1 participant
A peer's incarnation change took a
peerIncarnation_*savepoint of its own, which nested inside whichever crank happened to be open — the same defect #1103 just removed from the inbound message path. It becomes a run queue item, carried out by the run loop in a crank of its own.The handshake still gets its answer immediately. Whether this is a restart is a read of what the store already says, and the transport awaits that answer to decide whether to reset the connection; it cannot wait for a crank. Only the writes are queued.
Changes
RemoteManager.#handleIncarnationChangeanswers fromgetPeerIncarnationand callsKernelQueue.acceptPeerIncarnation.applyIncarnationChangedoes the writes in a crank.applyIncarnationChangere-reads the stored incarnation and returns early if it already matches, so a peer that re-dials while its first change is still waiting does not get its c-list torn down twice.Carries the incarnation half of #1079.
What review changed
afterCommitwas doing the promise rejections, andresolvePromiseswrites the kernel store — promise state, reference counts, and a notify row per subscriber, all in autocommit onceendCrankhas committed. That is exactly what the contract added in #1101 forbids, and I had made the same mistake there. Two reviewers found it independently, and one traced a second consequence: the transport's give-up handling rejects the same promises from a send continuation, so whichever arrived second wouldFailout of a post-commit hook and take the kernel with it.The rejections now happen inside the crank, buffered with
immediate: falseso the notifies they produce still wait for the commit — the mechanism a vat's own syscalls already use.afterCommitkeeps onlyfinalizePeerRestart, which is in-memory counters and nothing else.Restoring the router's exhaustiveness check also came out of review: this PR had deleted the
@ts-expect-erroronKernelRouter.deliver's default case, becausepeerIncarnationmade theneverreachable.delivernow takesExclude<RunQueueItem, RunQueueItemPeerIncarnation>, which says in the type what the kernel does at runtime and brings the check back.Testing
The handshake is answered without its writes having happened; the rejections are buffered and
finalizePeerRestartwaits for the commit; a peer's messages and its incarnation change come out in arrival order; a change queued twice is recorded once; the kernel carries out the item itself rather than routing it, and survives one it cannot record; andacceptPeerIncarnationis refused once the run loop has died. Every fix was mutation-checked.Six existing incarnation tests now go through a
handshakeAndRunCrankhelper that does what the transport and the run loop do between them.@metamask/ocap-kernelis green, as is@ocap/kernel-test'sremote-commssuite against a rebuiltdist. That suite intermittently crashes the Node worker withAssertion failed: (env) != nullptr; it reproduces onorigin/mainand is not from this change — two consecutive clean runs here.🤖 Generated with Claude Code
Note
High Risk
Changes when and how peer restarts are persisted, promises rejected, and remote state reset relative to crank commit—core remote comms consistency that can affect duplicate detection and double-rejection races with transport give-up handling.
Overview
Peer incarnation changes now run on the kernel queue in their own crank, instead of taking a nested
peerIncarnation_*savepoint during whatever crank was already open (same class of bug #1103 fixed for inbound messages).The transport still answers the handshake immediately from
getPeerIncarnation; only the durable work is queued viaKernelQueue.acceptPeerIncarnation, behind any inbound messages that peer already sent, so old- and new-incarnation traffic stay separated by arrival order rather than discarding queued arrivals.RemoteManager.#handleIncarnationChangeonly queues;applyIncarnationChangeperforms persist/finalize logic in the crank. Promise rejections for the restarted remote’s decider promises run inside the crank with buffered notifies (immediate: false);finalizePeerRestart(in-memory reset) moves toafterCommit. Duplicate queued changes no-op if the store already matches. Failures to record are logged and return{ abort: true }so the run loop survives. The run loop callback handlespeerIncarnationdirectly (notKernelRouter);discardRemoteInboundis removed.Reviewed by Cursor Bugbot for commit fcbeaa2. Bugbot is set up for automated code reviews on this repo. Configure here.