[vheathen]

Fixes #92.

When a node handles an ack_sync from a node that is not in its nodes_map yet and the merge resolves a conflict in favour of the remote process, the peer gets a snapshot that no longer shows the conflict, and the resolution broadcast does not reach it. A third node that held the losing process is left without an entry for the name, and nothing repairs it. It takes a partial netsplit, a resolver that keeps the earlier registration, and the connecting node handling the peer's discover before its own nodeup.

Before: the unknown-node branch of the ack_sync handler merged the remote data, then monitored the node, sent the local snapshot and added the node to nodes_map. After: it monitors the node, adds it to nodes_map and sends the snapshot first, and merges after. The snapshot still holds the entries the resolution is about to remove, so the peer sees the conflict and resolves it on its side too, and the broadcasts of the merge reach the new node after the snapshot, through the same sender process. A peer that joins through discover already gets its snapshot before any broadcast; the ack_sync path now has the same order.

No message type is added and the message format is unchanged. The one difference on the wire is that the broadcasts of the merge have one more recipient, the new node.

Mixed clusters. What an unpatched node receives from a patched one, a snapshot followed by the merge's broadcasts, is what it receives today when the join goes through discover first. The bug stays possible while the node that takes the unknown-node branch is unpatched. There is no new failure mode for unpatched nodes.

Of the four new cases in syn_registry_SUITE, three_nodes_ack_sync_from_unknown_node_keeps_remote is the order of the issue and fails on 3.4.2: the lookup for the name returns undefined where the kept process is expected. The other three pass before and after the change. Each half of the change has a case that fails without it: keeps_remote still fails when the merge runs first even with the updated nodes_map, and three_nodes_ack_sync_from_unknown_node_resolution_reaches_peer fails when the snapshot goes first but the merge still broadcasts with the old nodes_map.

syn_registry_SUITE (19 cases) passes on OTP 25.3.2.8, 27.3.4.11 and 29.0.6, syn_pg_SUITE (13 cases) too; dialyzer is clean on 27.3.4.11 and 29.0.6. The natural-order loops from the issue, 300 iterations with the resolver that keeps the earlier registration and 100 with one that keeps the later one, stay consistent with the change.

[ostinelli]

Hey @vheathen, thanks a lot for this, and for the detailed write-up in #92. I ran your new tests on top of current master and the fix does what you describe: without it the name gets lost on the third node, with it everything ends up in the right place.

While going through it I found two things that the new order changes, and I'd like to think about them before merging:

1. If the custom resolver keeps neither process (returns something that isn't one of the two pids, or crashes), a node can be left with a dead entry. Say node B gets node A's ack_sync before it knows A. With your change B first sends A its own data, then runs the resolver: it stops its own process, removes the name, and tells every node except A to drop it, because it expects A to run the resolver too and clean up on its side. But if A's process died just before A handles B's data, A doesn't see any conflict and just stores B's process, which B has already stopped. Nothing ever tells A to remove it, so it stays there.

2. In this path the resolver now runs on both nodes instead of only one. That's how every other conflict already works and it's what the docs say, so I think it's the right behavior, but people with side effects in their resolver would see it run twice where today it runs once. Also, if one of the processes dies at the wrong moment, the other node can briefly see the other process registered and then unregistered, so a couple of extra callbacks fire (the end result is correct).

I want to think a bit about how to handle these, I'll get back to you here.

[ostinelli]

Thanks again for this. I've looked into the two points from my earlier comment.

1. The dead entry when the resolver keeps neither process already happens on master, in a normal reconnect, not only with your change. I fixed it on master in 3af7df8: when the resolver keeps neither, the other owner now gets told to drop the name too. A correction to what I wrote before: in that case syn doesn't stop either process, it only drops the name.

2. The resolver running on both nodes is what the docs say, so that's fine. The extra callbacks when a process dies at the wrong moment are real changes in the table, and master does the same thing in the normal path, so I'm OK with them.

I merged master into your branch and added one commit on the tests:

• keeps_remote now checks the callbacks too. Without your change it fails because the third node never gets the "registered" callback for the kept process.
• I removed keeps_local and default_resolution, since they pass with or without the change.
• join_with_ack_sync_first now queues the real snapshot from the slave's scope instead of building it by hand.

There's one more thing I found while doing this, not caused by your PR. If one node unregisters a process and another node registers that same process right after, the old unregister can arrive late and remove the new entry on the second node. It already happens on master. I noted it in test/tla/README.md and will look at it separately.