Found in a second reading of #1021 (sirtimid/crank-rollback-integrity). Two read-modify-write sequences that used to be synchronous now have await withStoreOutOfCrank(...) in the middle, and that await can span a whole crank.
A. RemoteManager's incarnation change
getPeerIncarnation and the #remotesByPeer.get both run before the new await; setPeerIncarnation runs after it.
onIncarnationChange has two unserialised call sites — doOutboundHandshake and doInboundHandshake in remotes/platform/transport.ts — so a simultaneous dial/accept hits both.
Stored = A, peer now at B. Both invocations read stored = A, both set isRestart = true, both capture the same remote. First turn tears the c-list down and sets B; second turn runs persistPeerRestart() again on an already-torn-down c-list and sets B again, and both run the post-commit fan-out, so finalizePeerRestart() fires twice.
Sharper variant: two handshakes carrying B then C, whose turns run in the opposite order, leave the persisted incarnation at B while the peer is at C. The next handshake then reads stored = B ≠ C, declares a restart, and tears down the c-list of the live connection.
The diff already moved getPromisesByDecider inside the turn for exactly this reason — the comment there says so — but left stored / isRestart / remote outside.
B. RemoteHandle's duplicate-seq guard
The dedup check reads #highestReceivedSeq; the persisted write and the in-memory update both happen after the new await. Pre-PR, deliver had no await anywhere between them. (redeemURL already awaited inside the savepoint, and the PR genuinely improves that case by hoisting the decrypt out.)
Needs two concurrent readers for one peer, which reconnect produces: handleConnectionLoss/registerChannel starts a new readChannel while the old may still be draining, both routing to the same RemoteHandle, and the protocol retransmits unacked messages after reconnect. With #highestReceivedSeq = 6, seq 9 (old channel) and seq 7 (retransmit, new channel) both pass the check; whichever turn runs second writes 7, regressing the high-water mark below a message already processed. Seq 8 and 9 are then re-delivered — the exactly-once property the savepoint exists to provide.
Savepoint atomicity itself is not at risk: work() is synchronous, so two turns cannot interleave their savepoints. Only the check-to-write window widened.
Suggested fix
Move the checks inside work(), where they are synchronous with the write: re-read getPeerIncarnation(peerId) at the top of RemoteManager's callback, and if (seq <= this.#highestReceivedSeq) return undefined; at the top of RemoteHandle's.
Found in a second reading of #1021 (
sirtimid/crank-rollback-integrity). Two read-modify-write sequences that used to be synchronous now haveawait withStoreOutOfCrank(...)in the middle, and that await can span a whole crank.A.
RemoteManager's incarnation changegetPeerIncarnationand the#remotesByPeer.getboth run before the new await;setPeerIncarnationruns after it.onIncarnationChangehas two unserialised call sites —doOutboundHandshakeanddoInboundHandshakeinremotes/platform/transport.ts— so a simultaneous dial/accept hits both.Stored = A, peer now at B. Both invocations read stored = A, both set
isRestart = true, both capture the same remote. First turn tears the c-list down and sets B; second turn runspersistPeerRestart()again on an already-torn-down c-list and sets B again, and both run the post-commit fan-out, sofinalizePeerRestart()fires twice.Sharper variant: two handshakes carrying B then C, whose turns run in the opposite order, leave the persisted incarnation at B while the peer is at C. The next handshake then reads stored = B ≠ C, declares a restart, and tears down the c-list of the live connection.
The diff already moved
getPromisesByDeciderinside the turn for exactly this reason — the comment there says so — but leftstored/isRestart/remoteoutside.B.
RemoteHandle's duplicate-seq guardThe dedup check reads
#highestReceivedSeq; the persisted write and the in-memory update both happen after the new await. Pre-PR,deliverhad no await anywhere between them. (redeemURLalready awaited inside the savepoint, and the PR genuinely improves that case by hoisting the decrypt out.)Needs two concurrent readers for one peer, which reconnect produces:
handleConnectionLoss/registerChannelstarts a newreadChannelwhile the old may still be draining, both routing to the sameRemoteHandle, and the protocol retransmits unacked messages after reconnect. With#highestReceivedSeq = 6, seq 9 (old channel) and seq 7 (retransmit, new channel) both pass the check; whichever turn runs second writes 7, regressing the high-water mark below a message already processed. Seq 8 and 9 are then re-delivered — the exactly-once property the savepoint exists to provide.Savepoint atomicity itself is not at risk:
work()is synchronous, so two turns cannot interleave their savepoints. Only the check-to-write window widened.Suggested fix
Move the checks inside
work(), where they are synchronous with the write: re-readgetPeerIncarnation(peerId)at the top ofRemoteManager's callback, andif (seq <= this.#highestReceivedSeq) return undefined;at the top ofRemoteHandle's.