From 9f9fa9ca8b9bfcbe71a6939889f57902c1bb83d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9odore=20L=C3=A9on?= Date: Fri, 6 Jun 2025 22:30:23 +0200 Subject: [PATCH] Monitoring client connections (#941) ## Description: Disconnected client detection : If a client haven't send a ping to the server since more than 30 seconds They will then be marked disconnected with a dedicated icon. No action are yet taken, this allows for extensive in-game test before adding the *consequences* of the player leaving the game. I also added extensive unit tests, lessening the risk of regression for the future. ![image](https://github.com/user-attachments/assets/884e5e99-15e8-4544-bd52-7524542cc82a) ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced - [x] I understand that submitting code with bugs that could have been caught through manual testing blocks releases and new features for all contributors ## Please put your Discord username so you can be contacted if a bug or regression is found: theodoreleon.aetarax --------- Co-authored-by: evanpelle --- resources/images/DisconnectedIcon.svg | 1 + src/client/graphics/layers/NameLayer.ts | 22 +++ src/core/Schemas.ts | 13 +- src/core/execution/ExecutionManager.ts | 3 + .../execution/MarkDisconnectedExecution.ts | 24 +++ src/core/game/Game.ts | 3 + src/core/game/GameUpdates.ts | 1 + src/core/game/GameView.ts | 3 + src/core/game/PlayerImpl.ts | 10 ++ src/server/Client.ts | 3 +- src/server/GameServer.ts | 43 +++++ tests/Disconnected.test.ts | 163 ++++++++++++++++++ 12 files changed, 287 insertions(+), 2 deletions(-) create mode 100644 resources/images/DisconnectedIcon.svg create mode 100644 src/core/execution/MarkDisconnectedExecution.ts create mode 100644 tests/Disconnected.test.ts diff --git a/resources/images/DisconnectedIcon.svg b/resources/images/DisconnectedIcon.svg new file mode 100644 index 000000000..fd07fe512 --- /dev/null +++ b/resources/images/DisconnectedIcon.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/src/client/graphics/layers/NameLayer.ts b/src/client/graphics/layers/NameLayer.ts index db7993f90..23f596c01 100644 --- a/src/client/graphics/layers/NameLayer.ts +++ b/src/client/graphics/layers/NameLayer.ts @@ -2,6 +2,7 @@ import allianceIcon from "../../../../resources/images/AllianceIcon.svg"; import allianceRequestBlackIcon from "../../../../resources/images/AllianceRequestBlackIcon.svg"; import allianceRequestWhiteIcon from "../../../../resources/images/AllianceRequestWhiteIcon.svg"; import crownIcon from "../../../../resources/images/CrownIcon.svg"; +import disconnectedIcon from "../../../../resources/images/DisconnectedIcon.svg"; import embargoBlackIcon from "../../../../resources/images/EmbargoBlackIcon.svg"; import embargoWhiteIcon from "../../../../resources/images/EmbargoWhiteIcon.svg"; import nukeRedIcon from "../../../../resources/images/NukeIconRed.svg"; @@ -40,6 +41,7 @@ export class NameLayer implements Layer { private renders: RenderInfo[] = []; private seenPlayers: Set = new Set(); private traitorIconImage: HTMLImageElement; + private disconnectedIconImage: HTMLImageElement; private allianceRequestBlackIconImage: HTMLImageElement; private allianceRequestWhiteIconImage: HTMLImageElement; private allianceIconImage: HTMLImageElement; @@ -61,6 +63,8 @@ export class NameLayer implements Layer { ) { this.traitorIconImage = new Image(); this.traitorIconImage.src = traitorIcon; + this.disconnectedIconImage = new Image(); + this.disconnectedIconImage.src = disconnectedIcon; this.allianceIconImage = new Image(); this.allianceIconImage.src = allianceIcon; this.allianceRequestBlackIconImage = new Image(); @@ -370,6 +374,24 @@ export class NameLayer implements Layer { existingTraitor.remove(); } + // Disconnected icon + const existingDisconnected = iconsDiv.querySelector( + '[data-icon="disconnected"]', + ); + if (render.player.isDisconnected()) { + if (!existingDisconnected) { + iconsDiv.appendChild( + this.createIconElement( + this.disconnectedIconImage.src, + iconSize, + "disconnected", + ), + ); + } + } else if (existingDisconnected) { + existingDisconnected.remove(); + } + // Alliance icon const existingAlliance = iconsDiv.querySelector('[data-icon="alliance"]'); if (myPlayer !== null && myPlayer.isAlliedWith(render.player)) { diff --git a/src/core/Schemas.ts b/src/core/Schemas.ts index 74aa99b2a..e4fff95a6 100644 --- a/src/core/Schemas.ts +++ b/src/core/Schemas.ts @@ -33,7 +33,8 @@ export type Intent = | BuildUnitIntent | EmbargoIntent | QuickChatIntent - | MoveWarshipIntent; + | MoveWarshipIntent + | MarkDisconnectedIntent; export type AttackIntent = z.infer; export type CancelAttackIntent = z.infer; @@ -56,6 +57,9 @@ export type TargetTroopRatioIntent = z.infer< export type BuildUnitIntent = z.infer; export type MoveWarshipIntent = z.infer; export type QuickChatIntent = z.infer; +export type MarkDisconnectedIntent = z.infer< + typeof MarkDisconnectedIntentSchema +>; export type Turn = z.infer; export type GameConfig = z.infer; @@ -166,6 +170,7 @@ const BaseIntentSchema = z.object({ "attack", "cancel_attack", "spawn", + "mark_disconnected", "boat", "cancel_boat", "name", @@ -290,10 +295,16 @@ export const QuickChatIntentSchema = BaseIntentSchema.extend({ variables: z.record(SafeString).optional(), }); +export const MarkDisconnectedIntentSchema = BaseIntentSchema.extend({ + type: z.literal("mark_disconnected"), + isDisconnected: z.boolean(), +}); + const IntentSchema = z.union([ AttackIntentSchema, CancelAttackIntentSchema, SpawnIntentSchema, + MarkDisconnectedIntentSchema, BoatAttackIntentSchema, CancelBoatIntentSchema, AllianceRequestIntentSchema, diff --git a/src/core/execution/ExecutionManager.ts b/src/core/execution/ExecutionManager.ts index 66d2fd0e8..a333d95ec 100644 --- a/src/core/execution/ExecutionManager.ts +++ b/src/core/execution/ExecutionManager.ts @@ -15,6 +15,7 @@ import { DonateTroopsExecution } from "./DonateTroopExecution"; import { EmbargoExecution } from "./EmbargoExecution"; import { EmojiExecution } from "./EmojiExecution"; import { FakeHumanExecution } from "./FakeHumanExecution"; +import { MarkDisconnectedExecution } from "./MarkDisconnectedExecution"; import { MoveWarshipExecution } from "./MoveWarshipExecution"; import { NoOpExecution } from "./NoOpExecution"; import { QuickChatExecution } from "./QuickChatExecution"; @@ -120,6 +121,8 @@ export class Executor { intent.quickChatKey, intent.variables ?? {}, ); + case "mark_disconnected": + return new MarkDisconnectedExecution(player, intent.isDisconnected); default: throw new Error(`intent type ${intent} not found`); } diff --git a/src/core/execution/MarkDisconnectedExecution.ts b/src/core/execution/MarkDisconnectedExecution.ts new file mode 100644 index 000000000..95d530a4a --- /dev/null +++ b/src/core/execution/MarkDisconnectedExecution.ts @@ -0,0 +1,24 @@ +import { Execution, Game, Player } from "../game/Game"; + +export class MarkDisconnectedExecution implements Execution { + constructor( + private player: Player, + private isDisconnected: boolean, + ) {} + + init(mg: Game, ticks: number): void { + this.player.markDisconnected(this.isDisconnected); + } + + tick(ticks: number): void { + return; + } + + isActive(): boolean { + return false; + } + + activeDuringSpawnPhase(): boolean { + return false; + } +} diff --git a/src/core/game/Game.ts b/src/core/game/Game.ts index ec2851747..43433f403 100644 --- a/src/core/game/Game.ts +++ b/src/core/game/Game.ts @@ -433,6 +433,9 @@ export interface Player { largestClusterBoundingBox: { min: Cell; max: Cell } | null; lastTileChange(): Tick; + isDisconnected(): boolean; + markDisconnected(isDisconnected: boolean): void; + hasSpawned(): boolean; setHasSpawned(hasSpawned: boolean): void; diff --git a/src/core/game/GameUpdates.ts b/src/core/game/GameUpdates.ts index b238365d0..7078d6b9f 100644 --- a/src/core/game/GameUpdates.ts +++ b/src/core/game/GameUpdates.ts @@ -103,6 +103,7 @@ export interface PlayerUpdate { smallID: number; playerType: PlayerType; isAlive: boolean; + isDisconnected: boolean; tilesOwned: number; gold: Gold; population: number; diff --git a/src/core/game/GameView.ts b/src/core/game/GameView.ts index 5358bc568..aaaaa591c 100644 --- a/src/core/game/GameView.ts +++ b/src/core/game/GameView.ts @@ -289,6 +289,9 @@ export class PlayerView { hasSpawned(): boolean { return this.data.hasSpawned; } + isDisconnected(): boolean { + return this.data.isDisconnected; + } } export class GameView implements GameMap { diff --git a/src/core/game/PlayerImpl.ts b/src/core/game/PlayerImpl.ts index a055799d5..2b4146b10 100644 --- a/src/core/game/PlayerImpl.ts +++ b/src/core/game/PlayerImpl.ts @@ -98,6 +98,7 @@ export class PlayerImpl implements Player { public _outgoingLandAttacks: Attack[] = []; private _hasSpawned = false; + private _isDisconnected = false; constructor( private mg: GameImpl, @@ -135,6 +136,7 @@ export class PlayerImpl implements Player { smallID: this.smallID(), playerType: this.type(), isAlive: this.isAlive(), + isDisconnected: this.isDisconnected(), tilesOwned: this.numTilesOwned(), gold: this._gold, population: this.population(), @@ -919,6 +921,14 @@ export class PlayerImpl implements Player { return this._lastTileChange; } + isDisconnected(): boolean { + return this._isDisconnected; + } + + markDisconnected(isDisconnected: boolean): void { + this._isDisconnected = isDisconnected; + } + hash(): number { return ( simpleHash(this.id()) * (this.population() + this.numTilesOwned()) + diff --git a/src/server/Client.ts b/src/server/Client.ts index 295ca499e..c367c0e04 100644 --- a/src/server/Client.ts +++ b/src/server/Client.ts @@ -4,7 +4,8 @@ import { Tick } from "../core/game/Game"; import { ClientID } from "../core/Schemas"; export class Client { - public lastPing: number; + public lastPing: number = Date.now(); + public isDisconnected: boolean = false; public hashes: Map = new Map(); diff --git a/src/server/GameServer.ts b/src/server/GameServer.ts index 5754ad218..c74b0d8e4 100644 --- a/src/server/GameServer.ts +++ b/src/server/GameServer.ts @@ -35,6 +35,8 @@ export class GameServer { private maxGameDuration = 3 * 60 * 60 * 1000; // 3 hours + private disconnectedTimeout = 1 * 30 * 1000; // 30 seconds + private turns: Turn[] = []; private intents: Intent[] = []; public activeClients: Client[] = []; @@ -164,6 +166,10 @@ export class GameServer { }); return; } + + client.isDisconnected = existing.isDisconnected; + client.lastPing = existing.lastPing; + existing.ws.removeAllListeners("message"); this.activeClients = this.activeClients.filter((c) => c !== existing); } @@ -195,6 +201,12 @@ export class GameServer { ); return; } + if (clientMsg.intent.type === "mark_disconnected") { + this.log.warn( + `Should not receive mark_disconnected intent from client`, + ); + return; + } this.addIntent(clientMsg.intent); } if (clientMsg.type === "ping") { @@ -357,6 +369,7 @@ export class GameServer { this.intents = []; this.handleSynchronization(); + this.checkDisconnectedStatus(); let msg = ""; try { @@ -537,6 +550,36 @@ export class GameServer { } } + private checkDisconnectedStatus() { + if (this.turns.length % 5 !== 0) { + return; + } + + const now = Date.now(); + for (const [clientID, client] of this.allClients) { + if ( + client.isDisconnected === false && + now - client.lastPing > this.disconnectedTimeout + ) { + this.markClientDisconnected(client, true); + } else if ( + client.isDisconnected && + now - client.lastPing < this.disconnectedTimeout + ) { + this.markClientDisconnected(client, false); + } + } + } + + private markClientDisconnected(client: Client, isDisconnected: boolean) { + client.isDisconnected = isDisconnected; + this.addIntent({ + type: "mark_disconnected", + clientID: client.clientID, + isDisconnected: isDisconnected, + }); + } + private archiveGame() { this.log.info("archiving game", { gameID: this.id, diff --git a/tests/Disconnected.test.ts b/tests/Disconnected.test.ts new file mode 100644 index 000000000..1932500be --- /dev/null +++ b/tests/Disconnected.test.ts @@ -0,0 +1,163 @@ +import { MarkDisconnectedExecution } from "../src/core/execution/MarkDisconnectedExecution"; +import { SpawnExecution } from "../src/core/execution/SpawnExecution"; +import { Game, Player, PlayerInfo, PlayerType } from "../src/core/game/Game"; +import { setup } from "./util/Setup"; +import { executeTicks } from "./util/utils"; + +let game: Game; +let player1: Player; +let player2: Player; + +describe("Disconnected", () => { + beforeEach(async () => { + game = await setup("Plains", { + infiniteGold: true, + instantBuild: true, + }); + + const player1Info = new PlayerInfo( + "us", + "Active Player", + PlayerType.Human, + null, + "player1_id", + ); + + const player2Info = new PlayerInfo( + "fr", + "Disconnected Player", + PlayerType.Human, + null, + "player2_id", + ); + + player1 = game.addPlayer(player1Info); + player2 = game.addPlayer(player2Info); + + game.addExecution( + new SpawnExecution(player1Info, game.ref(1, 1)), + new SpawnExecution(player2Info, game.ref(7, 7)), + ); + + while (game.inSpawnPhase()) { + game.executeNextTick(); + } + }); + + describe("Player disconnected state", () => { + test("should initialize players as not disconnected", () => { + expect(player1.isDisconnected()).toBe(false); + expect(player2.isDisconnected()).toBe(false); + }); + + test("should mark player as disconnected and not disconnected", () => { + player1.markDisconnected(true); + expect(player1.isDisconnected()).toBe(true); + + player1.markDisconnected(false); + expect(player1.isDisconnected()).toBe(false); + }); + + test("should include disconnected state in player update", () => { + player1.markDisconnected(true); + const update = player1.toUpdate(); + expect(update.isDisconnected).toBe(true); + }); + }); + + describe("Player view", () => { + test("should reflect disconnected state in player view", () => { + // Mark player2 as disconnected + player2.markDisconnected(true); + + // Get player1's view of player2 + const player2View = game.player(player2.id()); + expect(player2View.isDisconnected()).toBe(true); + + // Mark player2 as connected again + player2.markDisconnected(false); + + // Verify the view is updated + const updatedPlayer2View = game.player(player2.id()); + expect(updatedPlayer2View.isDisconnected()).toBe(false); + }); + + test("should maintain disconnected state in view across game ticks", () => { + player2.markDisconnected(true); + executeTicks(game, 3); + + const player2View = game.player(player2.id()); + expect(player2View.isDisconnected()).toBe(true); + }); + }); + + describe("MarkDisconnectedExecution", () => { + test("should mark player as disconnected when executed", () => { + const execution = new MarkDisconnectedExecution(player1, true); + game.addExecution(execution); + executeTicks(game, 1); + expect(player1.isDisconnected()).toBe(true); + expect(execution.isActive()).toBe(false); + }); + + test("should handle multiple players with different disconnected states", () => { + const execution1 = new MarkDisconnectedExecution(player1, true); + const execution2 = new MarkDisconnectedExecution(player2, false); + game.addExecution(execution1, execution2); + executeTicks(game, 1); + expect(player1.isDisconnected()).toBe(true); + expect(player2.isDisconnected()).toBe(false); + }); + + test("should not be active during spawn phase", () => { + const execution = new MarkDisconnectedExecution(player1, true); + expect(execution.activeDuringSpawnPhase()).toBe(false); + }); + + test("should handle multiple executions for same player in same tick", () => { + const execution1 = new MarkDisconnectedExecution(player1, true); + const execution2 = new MarkDisconnectedExecution(player1, false); + game.addExecution(execution1, execution2); + executeTicks(game, 1); + // Last execution should win + expect(player1.isDisconnected()).toBe(false); + }); + }); + + describe("Disconnected state persistence", () => { + test("should maintain disconnected state across game ticks", () => { + player1.markDisconnected(true); + executeTicks(game, 5); + expect(player1.isDisconnected()).toBe(true); + }); + + test("should maintain disconnected state in player updates across ticks", () => { + player1.markDisconnected(true); + executeTicks(game, 3); + const update = player1.toUpdate(); + expect(update.isDisconnected).toBe(true); + }); + }); + + describe("Edge cases", () => { + test("should handle marking same disconnected state multiple times", () => { + player1.markDisconnected(true); + player1.markDisconnected(true); + player1.markDisconnected(true); + expect(player1.isDisconnected()).toBe(true); + + player1.markDisconnected(false); + player1.markDisconnected(false); + player1.markDisconnected(false); + expect(player1.isDisconnected()).toBe(false); + }); + + test("should handle execution with same disconnected state", () => { + player1.markDisconnected(true); + const execution = new MarkDisconnectedExecution(player1, true); + game.addExecution(execution); + executeTicks(game, 1); + expect(player1.isDisconnected()).toBe(true); + }); + }); +});