# HG changeset patch
# User Patric Stout <truebrain@openttd.org>
# Date 2021-02-27 10:01:57
# Node ID 089fed249fe78ba4c863c5b18bf22b7c02342aff
# Parent  987ffc0c740575a6e4cab1ebe9a9776dfc0a35a9

Fix: [Network] don't desync if client leaves before you finish downloading map

When you are downloading a map, all the commands are queued up
for you. Clients joining/leaving is done by the network protocol,
and as such are processed immediately. This means that by the
time you are processing the commands, a client that triggered
it, might already have left.

So, all commands that do something with ClientID, shouldn't
error on an invalid ClientID when DC_EXEC is set, but
gracefully handle the command anyway, to make sure the
game-state is kept in sync with all the clients that did
execute the DoCommand while the now-gone client was still
there.

Additionally, in the small chance a client disconnects between
the server validating a DoCommand and the command being
executed, also just process the command as if the client was
still there. Otherwise, lag or latency can cause clients that
did not receive the disconnect yet to desync.

diff --git a/src/company_cmd.cpp b/src/company_cmd.cpp
--- a/src/company_cmd.cpp
+++ b/src/company_cmd.cpp
@@ -822,13 +822,6 @@ CommandCost CmdCompanyCtrl(TileIndex til
 
 			ClientID client_id = (ClientID)p2;
 			NetworkClientInfo *ci = NetworkClientInfo::GetByClientID(client_id);
-#ifndef DEBUG_DUMP_COMMANDS
-			/* When replaying the client ID is not a valid client; there
-			 * are actually no clients at all. However, the company has to
-			 * be created, otherwise we cannot rerun the game properly.
-			 * So only allow a nullptr client info in that case. */
-			if (ci == nullptr) return CommandCost();
-#endif /* NOT DEBUG_DUMP_COMMANDS */
 
 			/* Delete multiplayer progress bar */
 			DeleteWindowById(WC_NETWORK_STATUS_WINDOW, WN_NETWORK_STATUS_WINDOW_JOIN);
@@ -837,7 +830,9 @@ CommandCost CmdCompanyCtrl(TileIndex til
 
 			/* A new company could not be created, revert to being a spectator */
 			if (c == nullptr) {
-				if (_network_server) {
+				/* We check for "ci != nullptr" as a client could have left by
+				 * the time we execute this command. */
+				if (_network_server && ci != nullptr) {
 					ci->client_playas = COMPANY_SPECTATOR;
 					NetworkUpdateClientInfo(ci->client_id);
 				}
diff --git a/src/goal.cpp b/src/goal.cpp
--- a/src/goal.cpp
+++ b/src/goal.cpp
@@ -256,7 +256,10 @@ CommandCost CmdGoalQuestion(TileIndex ti
 	if (_current_company != OWNER_DEITY) return CMD_ERROR;
 	if (StrEmpty(text)) return CMD_ERROR;
 	if (is_client) {
-		if (NetworkClientInfo::GetByClientID(client) == nullptr) return CMD_ERROR;
+		/* Only check during pre-flight; the client might have left between
+		 * testing and executing. In that case it is fine to just ignore the
+		 * fact the client is no longer here. */
+		if (!(flags & DC_EXEC) && _network_server && NetworkClientInfo::GetByClientID(client) == nullptr) return CMD_ERROR;
 	} else {
 		if (company != INVALID_COMPANY && !Company::IsValidID(company)) return CMD_ERROR;
 	}