[leonard-amsler]

This PR involves:

• The creation of a unit test suite for the federated controller
• Some fixes applied to the controller to match its correct fonctionality
• The modularization of the fake websocket for both unit decentralized and federated testing
• Discovery of bugs that need attention and not corrected
• Minor: Relocalization of the existing unit/end2end tests for decentralized and federated controllers into folders

Unit tests:

implemented at: server/tests/unit/federated_controller.spec.ts
18 tests in three groups: join handshake (3), aggregation (8, one commented out - see know issue), leaving and reset (7)

Controller discovered bugs:

• Contributions for future rounds were accepted. isWithinRoundCutoff was only discarding previous rounds.
• Contributions from a socket that never joined (ClientConnected was never sent by the client) were aggregated. We now ensure the connections store has an entry for the client before adding its update to the aggregator.
• A stale client was never catching up the current round. A simple offset of 1 was removed to the round id given to stale clients.
• The minimum number client needed wasn't reset when all peers disconnected from the protocol and that the controller was reset. The minimum is now set in the #makeAggregator() method, so set by all aggregators.

Aggregator bugs:

• The contributions of a node that left the protocol were still stored, possibly validating the minimum of clients required (as the client left, the number of required updates decreases and the update still stored). In other words, the contributions were no longer a subset of the active nodes. We now discard the contributions of unactive clients with disposeContributionsOf in the aggregator.
• We weren't checking if the update thresholds were met after the departure of a node. When the last client that was still missing to contribute for a round was leaving, we were in a stale mode forever. Now, we check if the aggregator is full also when nodes quit.

Known issue but not fixed:

• A socket that never sends ClientConnected increases the absolute thresholds forever. It blocks the current round even if it sends updates. We could either:
  • Notify the client that the server did not receive its ClientConnected message so that it can resend it to be registered in the connections set.
  • Skip the need for the ClientConnected message and do its content when receiving an update.
• The aggregator is fixed and hardcoded to the mean aggregator with no stale cutoff and a 100% relative aggregation strategy. The usage of the Byzantine robust aggregator is impossible. If we choose, we could use the abstract class aggregator in the controller and instantiate the appropriate one in the constructor (similar to as the getAggregator() for the client). We could add the aggregation threshold in the task description. This would need both server and UI changes. See Hardcoded MeanAggregator for the federated server #1238

Fixes #1220, fixes #1221, fixes #1222

[JulienVig]

Regarding known issue not fixed #1: how would you skip the ClientConnected message? In FL for example, it's currently used to let the server know how many contributions it should wait for and send the clients the base model

[leonard-amsler]

New progress on the PR:

• Synchronization of the connection list with the aggregator's registered peers. A connected client that does not send the ClientConnected message does not block the training anymore.
• The client now sends the ClientConnected message with retries (default 5 with 5 seconds timeout) until it receives the answer from the server. Resolve the problem of a missed ClientConnected message by the server.
• If it happens that the server receives model update from a non-registered client, the servers sends a notification to the client and make it crash. This is a robustness mechanism as it should never happen in a correct execution.

[JulienVig]

Thanks for the great work! I left mostly superficial comments and a bit of refactoring but there are two points I would like to discuss:

• What should the server answer to a client that sent a future contribution?
• Sending the model weights every time we receive a Client Connected sounds a bit risky regarding DDoS attacks, I think we should implement a counter

[leonard-amsler]

New state

• CrashClient: The federated server can now stop a client with a reason. The client aborts any waiting state (waiting for a server message) immediately, or stops before sending its update if it arrives during local training. The caller of Disco.train gets a ClientCrashError, and the webapp shows a dedicated toast.
• Browser WebSocket fix: the new @epfml/isomorphic-ws package uses ws (keeping the 1 GiB message limit) on Node and the native WebSocket in browsers. Same structure as @epfml/isomorphic-wrtc. Since GPT-2 training with Goldfish loss #1111, { maxPayload } made browser connections fail. Replaces main's runtime check from Test federated webapp training #1230.
• ClientConnected limit: Implementation of a local counter to detect abusive clients sending too many ClientConnected messages, flooding the network with weights. If more than 5 are sent per websocket, the server sends CrashClient instead of resending the global weights.
• Smaller fixes and refactors: reconnecting clients no longer stuck waiting for participants, federated specs moved next to their code, fewer and longer join retries.

Tests

• Cypress training/server-crash.cy.ts: Webapp test with a scripted WebSocket server (cy.task("scriptServer")).

Notes

• The e2e server URL is now http://server:8090. It was previously either http://server:1351 (.env.test) or http://server (test:e2e scripts) with a default port 80, not accessible. To attach a websocket we need a proper accessible hostname and port. The "server" gets translated to 127.0.0.1. Chose port 8090 as it is available, above 1024 (accessible), not 1351 (vite) and not 8080 (real disco server). The scripted server only starts for the server host, so the other tests keep 8080 for their real server.
• A crashed client stays registered and the round waits for its contribution until it closes its socket, which is up to the caller of disco.train (the webapp does it).

[leonard-amsler]

Description of the mechanism to make a webapp test with a scripted server

Cypress can intercept HTTP requests by calling cy.intercept and send a predefined answer. However, there is no mechanism to intercept websocket request natively in cypress.

In order to make a test that relies on websocket connections of the server (such as cypress/e2e/training/scripted/server-crash.cy.ts), we run a small websocket server scripted in cypress.config.ts, separately from the other e2e tests.

Run	VITE_SERVER_URL	HTTP to the server	Websocket
Regular e2e	http://server	intercepted by setupServerWith, other requests fail (server doesn't exist in DNS)	none
Scripted (DISCO_SCRIPTED_SERVER_E2E=1)	http://server:8080	intercepted by setupServerWith, as above	scripted server on 127.0.0.1:8080
Federated training	http://localhost:8080	real server	real server

How the scripted run works

• Only with DISCO_SCRIPTED_SERVER_E2E=1: the scripted/ tests are included, Cypress maps server to 127.0.0.1 (hosts), and the scripted server starts on the URL's port.
• Tests script its answers by message type with cy.task("scriptServer", { [type]: [messages] }).
• The config fails right away if the URL isn't http://server:<port>.
• CI job: test-e2e-scripted. Locally: pnpm -F webapp test:e2e:scripted. Make sure you stop a local DISCO server on 8080 first.
• There's no conflict with the federated jobs' real server on port 8080, as they never run in the same job.
• The URL needs an explicit port, as the scripted server listens on it. .env.test used to say http://server:1351, but 1351 is vite's port for the webapp, which the scripted server can't share. So it now uses 8080, the real server's default port.

The regular e2e tests are unchanged. They keep http://server without a port and without the mapping, so nothing listens there and a forgotten request still fails.

[JulienVig]

Thanks for the great work, only nitpicking comments about names and documentation. I will create a follow up PR to yours to address a couple things that I realized when reviewing this PR. I'll let you know exactly what it's adressing so that we don't fix the same things in your next branch