clientmanager: fix index mangling when removing balancer clients on balancer disconnect (#1214)

* clientmanager: fix index mangling when removing balancer clients on balancer disconnect

* add a unit test

Author: dyc3

server/clientmanager.ts

@@ -229,12 +229,17 @@ function onBalancerConnect(conn: BalancerConnection) {
 
 function onBalancerDisconnect(conn: BalancerConnection) {
 	log.info(`Disconnected from balancer ${conn.id}`);
+	// We can't immediately remove the balancer clients from the connections array because that would mess up the index of the other clients, so we have to do it later
+	const leavingClients: BalancerClient[] = [];
 	for (const client of connections) {
 		if (client instanceof BalancerClient && client.conn.id === conn.id) {
 			log.debug(`Kicking balancer client ${client.id}`);
-			client.leave();
+			leavingClients.push(client);
 		}
 	}
+	for (const client of leavingClients) {
+		client.leave();
+	}
 }
 
 async function onBalancerMessage(conn: BalancerConnection, message: MsgB2M) {

server/tests/unit/clientmanager.spec.ts

@@ -117,6 +117,37 @@ describe("ClientManager", () => {
 		const joins2 = clientmanager.getClientsInRoom("foo");
 		expect(joins2).toHaveLength(0);
 	});
+
+	it("should disconnect only clients that were from the balancer when a balancer disconnects", async () => {
+		const mockBalancerCon = new BalancerConnectionMock();
+		const mockBalancerCon2 = new BalancerConnectionMock();
+		balancerManager.addBalancerConnection(mockBalancerCon);
+		balancerManager.addBalancerConnection(mockBalancerCon2);
+		const client1 = new BalancerClient("foo", "foo1", mockBalancerCon);
+		const client2 = new BalancerClient("foo", "foo2", mockBalancerCon);
+		const client3 = new TestClient("foo");
+		const client4 = new BalancerClient("foo", "bar", mockBalancerCon2);
+		const client5 = new BalancerClient("foo", "foo2", mockBalancerCon);
+		const clients = [client1, client2, client3, client4, client5];
+		for (let [i, client] of clients.entries()) {
+			clientmanager.addClient(client);
+			client.emit("auth", client, "token" + i, { isLoggedIn: false, username: "foo" + i });
+		}
+		await new Promise(resolve => setTimeout(resolve, 100));
+		const joins = clientmanager.getClientsInRoom("foo");
+		expect(joins).toHaveLength(5);
+
+		mockBalancerCon.emit("disconnect", 1000, "reason");
+
+		expect(clientmanager.getClient(client1.id)).toBeUndefined();
+		expect(clientmanager.getClient(client2.id)).toBeUndefined();
+		expect(clientmanager.getClient(client3.id)).toBeDefined();
+		expect(clientmanager.getClient(client4.id)).toBeDefined();
+		expect(clientmanager.getClient(client5.id)).toBeUndefined();
+
+		const joins2 = clientmanager.getClientsInRoom("foo");
+		expect(joins2).toHaveLength(2);
+	});
 });
 
 describe("BalancerManager", () => {