Decentralized controller consistency by leonard-amsler · Pull Request #1273 · epfml/disco

leonard-amsler · 2026-10-27T11:32:26Z

Context

#1227 reworked the federated setting. This PR moves the shared parts to the abstract classes and uses them also in the decentralized setting.

Server

Moved send, crashClient, disconnect, the client ids, the ClientConnected counter and the connect/disconnect cleanup from the federated controller to TrainingController, so both controllers share them.
The decentralized controller now crashes a peer that:
- sends a message before ClientConnected
- sends more than 5 ClientConnected
- sends an invalid message, or a message the server fails to handle
A repeated ClientConnected gets the same answer again, without counting the peer twice or notifying the other peers.
Every message goes through the send helper, so a failed send disconnects the peer.
A peer leaving is now also removed from #syncingNodes.
Documented the sets of the controller and moved to # methods.
Fixed the connection timer: after a retry, the old timer kept running in the next rounds and could send RetryPeerConnections, and then ConnectionFail, to peers that were fine. There is now one timer per PeersForRound, and the retry count restarts at each round.

Client

disconnect() is now shared in Client. It closes the server connection if there is one (local training) and stops waiting for participants, so a client no longer keeps waiting after disconnecting. It also works when called multiple times.
The decentralized client retries its join with sendAndWaitWithRetry.
It stops with a ClientCrashError when the server crashes it, at the start of a round and while waiting for the server.
CrashClient is now part of the decentralized messages.

Tests

Decentralized controller: join retry, crashing or disconnecting a peer
Decentralized client: crashed and disconnected by the server after too many joins

Fixes #1250

…zed-controller-consistency

…ler-consistency

…eouts

JulienVig

All good! Only minor comments. Fyi, I will follow up this PR with another one for these points:

create an abort signal when the participant disconnects manually to stop any promise it was waiting on
disconnect server-side any peer that timeouts on PeerIsReady
check that ConnectionsReady only come from peers that sent PeerIsReady first (an attacker could send a ConnectionsReady during training and currently it would increment the round)

JulienVig · 2026-10-29T09:25:35Z

      this.#connectionRetry = 0;
-      // Restart the timeout after sending retry messages
-      this.startTimeout();
+


Let's make sure we add a timeout back here. If we are sending a RetryPeerConnections it means one peer failed to connect in time so it is likely that it will fail again. We need a timeout to ensure the server doesn't get stuck waiting and can exclude the peer.

JulienVig · 2026-10-29T09:50:19Z

      const providerInfo = await waitMessageWithTimeout(
        this.server,
        MType.ModelProviderInfo,
        this.task.trainingInformation.maxModelSyncTime,
        "Timeout while waiting for the latest model provider",
      );


I think we should also check for an abort signal here. There is still the timeout as fallback but it would be cleaner to fail with the proper cause I think

JulienVig · 2026-10-29T10:01:43Z

    if (this.#timeout !== undefined) {
      clearTimeout(this.#timeout);
      this.#timeout = undefined;
    }


you can use this.#clearTimeout() now

JulienVig · 2026-10-29T10:03:10Z

+   * Complete list of client ids that have connected their websocket
+   * Make sure two clients don't share the same ID.
+   */
+  protected clientIds = new Set<NodeID>();


Can you import Set from immutable rather than using the native JS Set? To be consistent with Map

JulienVig · 2026-10-29T10:04:56Z

+  protected clientIds = new Set<NodeID>();
+  /**
+   * Number of ClientConnected messages each client's socket sent.
+   * We answer them with the global weights, so a client resending it


Suggested change

* We answer them with the global weights, so a client resending it

* In FL we answer with the global weights, so a client resending it

JulienVig · 2026-10-29T10:07:34Z

+ * - `connections` (inherited): map between the ids of clients
+ *   that completed the connection handshake and their socket.
+ * - `#roundPeers`: the nodes that want to join the current round and their readiness status.
+ * - `#connectFinishedNodes`: the nodes that have completed the connection handshake and are ready for the current round.


Suggested change

* - `#connectFinishedNodes`: the nodes that have completed the connection handshake and are ready for the current round.

* - `#connectFinishedNodes`: the nodes that have completed their peer-to-peer webRTC connections and are ready for the current round.

To make sure it's not confused with the connection handshake with the server

JulienVig · 2026-10-29T10:14:12Z

    );
+  }
+
+  protected override connectClient(nodeId: string, ws: WebSocket): void {


Suggested change

protected override connectClient(nodeId: string, ws: WebSocket): void {

protected override connectClient(nodeId: NodeID, ws: WebSocket): void {

JulienVig · 2026-10-29T10:14:21Z

+    }
+  }
+
+  protected override disconnectClient(nodeId: string): void {


Suggested change

protected override disconnectClient(nodeId: string): void {

protected override disconnectClient(nodeId: NodeID): void {

leonard-amsler added 7 commits October 14, 2026 12:39

apply and abstract federated controller fixes to the decentralized one

649a984

apply federated client fixes to the decentralized one

f7bf9b5

Merge branch 'main' of https://github.com/epfml/disco into decentrali…

c29c148

…zed-controller-consistency

decentralized controller unit tests

bf5a4d3

Decentralized client test for disconnection on crash

3cd5c9a

Merge remote-tracking branch 'origin/main' into decentralized-control…

fd27aba

…ler-consistency

Decentralized controller: reset connection retry count and manage tim…

6166610

…eouts

leonard-amsler requested a review from JulienVig October 28, 2026 13:17

JulienVig approved these changes Oct 29, 2026

View reviewed changes

Navigation Menu

Repository navigation

Decentralized controller consistency - #1273

Decentralized controller consistency#1273
leonard-amsler wants to merge 7 commits into
mainfrom
decentralized-controller-consistency

leonard-amsler commented Oct 27, 2026

Uh oh!

JulienVig left a comment

Uh oh!

JulienVig Oct 29, 2026

Uh oh!

JulienVig Oct 29, 2026

Uh oh!

JulienVig Oct 29, 2026

Uh oh!

JulienVig Oct 29, 2026

Uh oh!

JulienVig Oct 29, 2026

Uh oh!

JulienVig Oct 29, 2026

Uh oh!

JulienVig Oct 29, 2026

Uh oh!

JulienVig Oct 29, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	* We answer them with the global weights, so a client resending it
	* In FL we answer with the global weights, so a client resending it

	* - `#connectFinishedNodes`: the nodes that have completed the connection handshake and are ready for the current round.
	* - `#connectFinishedNodes`: the nodes that have completed their peer-to-peer webRTC connections and are ready for the current round.

	protected override connectClient(nodeId: string, ws: WebSocket): void {
	protected override connectClient(nodeId: NodeID, ws: WebSocket): void {

	protected override disconnectClient(nodeId: string): void {
	protected override disconnectClient(nodeId: NodeID): void {

Conversation

leonard-amsler commented Oct 27, 2026

Context

Server

Client

Tests

Uh oh!

JulienVig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants