From 5cd86cd01c02ec1f4c2d2c42f8a8e3f0bb36602d Mon Sep 17 00:00:00 2001 From: Matthew Duong Date: Sun, 5 Jul 2026 19:19:14 -0700 Subject: [PATCH] Fix engine connection lifecycle: leaks, crashes, unhandled rejections Hardens the WebSocket/networking and lifecycle layer of the engine, which previously had no test coverage and several process-crash paths. ConnectionTracker: remove connections by identity instead of a stored index. The index map went stale the moment connections.sort() ran on every add, so a disconnect removed the wrong socket (leaking the dead one, which kept receiving broadcasts). Add ConnectionTracker.test.ts covering multi-connection removal, role ordering, and interleaved churn. Socket handlers: register an "error" listener on every socket so an abnormal reset logs instead of throwing an unhandled EventEmitter error that crashes the process. Guard Send with a readyState check + try/catch so one dead socket can't abort a broadcast to everyone else. Close rejected connections (GameWebSocket catch + attachSocketHandler now throws on unknown roles) instead of leaking open sockets. Lifecycle: install process-level unhandledRejection/uncaughtException handlers so a stray async failure (webhook, telemetry, admin state eval) no longer tears down a live match, and attach .catch to the previously un-awaited async calls. Also add the missing return after the game-complete branch that broadcast an extra Tick after EndGameState. Verified: tsc --noEmit clean; jest 15 suites / 46 tests passing. --- .../bomberland-engine/src/Game/GameRunner.ts | 13 ++- engine/bomberland-engine/src/Program.ts | 14 +++- .../src/Services/ConnectionTracker.test.ts | 82 +++++++++++++++++++ .../src/Services/ConnectionTracker.ts | 9 +- .../src/Services/GameWebSocket.ts | 1 + .../SocketHandler/AdminSocketHandler.ts | 13 ++- .../SocketHandler/AgentSocketHandler.ts | 13 ++- .../SocketHandler/SpectatorSocketHandler.ts | 13 ++- .../SocketHandler/attachSocketHandler.ts | 3 +- 9 files changed, 148 insertions(+), 13 deletions(-) create mode 100644 engine/bomberland-engine/src/Services/ConnectionTracker.test.ts diff --git a/engine/bomberland-engine/src/Game/GameRunner.ts b/engine/bomberland-engine/src/Game/GameRunner.ts index af2b4017..057e0651 100644 --- a/engine/bomberland-engine/src/Game/GameRunner.ts +++ b/engine/bomberland-engine/src/Game/GameRunner.ts @@ -80,10 +80,13 @@ export class GameRunner { }; this.sockets.BroadCast(endGamePacket); this.writeEndGamePacket(endGamePacket); - this.api.SendReplayToWebhook(endGamePacket); + this.api.SendReplayToWebhook(endGamePacket).catch((error) => { + this.telemetry.Error(`Failed to send replay to webhook: ${error}`); + }); await this.telemetry.Engine.LogEvent(EngineTelemetryEvent.GameEnded, { tick: this.tickCount - 1 }); this.shutdown(); + return; } const tickResult = await this.game.GetTickResult(); const tickPacket: GameTickPacket = { @@ -140,7 +143,9 @@ export class GameRunner { switch (adminPacket.type) { case PacketType.RequestTick: if (this.shouldWaitForPlayers === false) { - this.tick(); + this.tick().catch((error) => { + this.telemetry.Error(`Failed to process requested tick: ${error}`); + }); } break; @@ -231,7 +236,9 @@ export class GameRunner { private resetGame = async (worldSeed?: number, prngSeed?: number) => { this.telemetry.Info(`Resetting game with world_seed: ${worldSeed ?? "current"}, prng_seed: ${prngSeed ?? "current"}`); - this.api.LogEvent(EngineTelemetryEvent.GameReset, null); + this.api.LogEvent(EngineTelemetryEvent.GameReset, null).catch((error) => { + this.telemetry.Error(`Failed to log game reset event: ${error}`); + }); await this.Stop(); this.tickCount = 1; this.game = createGameFromSeed(this.telemetry, this.config, worldSeed ?? this.config.WorldSeed, prngSeed ?? this.config.PrngSeed); diff --git a/engine/bomberland-engine/src/Program.ts b/engine/bomberland-engine/src/Program.ts index a1c525ec..65b8fd05 100644 --- a/engine/bomberland-engine/src/Program.ts +++ b/engine/bomberland-engine/src/Program.ts @@ -55,7 +55,9 @@ class Program { config.IsTrainingModeEnabled === true ); - gameRunner.Start(); + gameRunner.Start().catch((error) => { + this.telemetry.Error(`GameRunner failed to start: ${error}`); + }); }; private instantiateUI = () => { @@ -85,6 +87,16 @@ class Program { process.on("SIGINT", handle); process.on("SIGTERM", handle); + + // Keep the match alive when a stray async rejection or exception escapes. + // Without these, an unhandled rejection (telemetry/webhook/admin state eval) or + // an uncaught throw would tear down the process for every connected player. + process.on("unhandledRejection", (reason) => { + this.telemetry.Error(`Unhandled promise rejection: ${reason instanceof Error ? reason.stack : reason}`); + }); + process.on("uncaughtException", (error) => { + this.telemetry.Error(`Uncaught exception: ${error.stack ?? error.message}`); + }); }; public Listen = () => { this.httpServer.listen(config.Port); diff --git a/engine/bomberland-engine/src/Services/ConnectionTracker.test.ts b/engine/bomberland-engine/src/Services/ConnectionTracker.test.ts new file mode 100644 index 00000000..1ea65728 --- /dev/null +++ b/engine/bomberland-engine/src/Services/ConnectionTracker.test.ts @@ -0,0 +1,82 @@ +import { AbstractSocketHandler } from "./SocketHandler/AbstractSocketHandler"; +import { AdminSocketHandler } from "./SocketHandler/AdminSocketHandler"; +import { AgentSocketHandler } from "./SocketHandler/AgentSocketHandler"; +import { ConnectionTracker } from "./ConnectionTracker"; +import { GameRole } from "@coderone/bomberland-library"; +import { SpectatorSocketHandler } from "./SocketHandler/SpectatorSocketHandler"; + +// Minimal fakes: ConnectionTracker only reads ConnectionId / AgentId / Role and stores +// the handlers, so we avoid instantiating real socket handlers (which open ws listeners). +const makeAgent = (connectionId: number, agentId: string): AgentSocketHandler => + ({ ConnectionId: connectionId, AgentId: agentId, Role: GameRole.Agent } as unknown as AgentSocketHandler); + +const makeSpectator = (connectionId: number): SpectatorSocketHandler => + ({ ConnectionId: connectionId, AgentId: null, Role: GameRole.Spectator } as unknown as SpectatorSocketHandler); + +const makeAdmin = (connectionId: number): AdminSocketHandler => + ({ ConnectionId: connectionId, AgentId: null, Role: GameRole.Admin } as unknown as AdminSocketHandler); + +const connectionIds = (tracker: ConnectionTracker): Array => tracker.Connections.map((c) => c.ConnectionId); + +describe("ConnectionTracker", () => { + test("tracks total agents as agents connect and disconnect", () => { + const tracker = new ConnectionTracker(); + tracker.AddAgent(makeAgent(1, "a")); + tracker.AddAgent(makeAgent(2, "b")); + expect(tracker.TotalAgents).toStrictEqual(2); + expect(tracker.IsAgentConnected("a")).toStrictEqual(true); + + tracker.RemoveAgent("a"); + expect(tracker.TotalAgents).toStrictEqual(1); + expect(tracker.IsAgentConnected("a")).toStrictEqual(false); + expect(tracker.IsAgentConnected("b")).toStrictEqual(true); + }); + + test("removes the correct connection when multiple are connected", () => { + const tracker = new ConnectionTracker(); + tracker.AddSpectator(makeSpectator(1)); + tracker.AddSpectator(makeSpectator(2)); + tracker.AddSpectator(makeSpectator(3)); + + tracker.RemoveSpectator(2); + + // Only spectator 2 should be gone; 1 and 3 must remain. + const remaining = connectionIds(tracker).sort((a, b) => a - b); + expect(remaining).toStrictEqual([1, 3]); + }); + + test("keeps the connection list consistent across interleaved add/remove", () => { + const tracker = new ConnectionTracker(); + tracker.AddAgent(makeAgent(1, "a")); + tracker.AddSpectator(makeSpectator(2)); + tracker.AddAdmin(makeAdmin(3)); + tracker.AddSpectator(makeSpectator(4)); + + // Sorting on add (agent > admin > spectator) must not corrupt later removals. + tracker.RemoveSpectator(2); + tracker.RemoveAgent("a"); + + const remaining = connectionIds(tracker).sort((a, b) => a - b); + expect(remaining).toStrictEqual([3, 4]); + expect(tracker.TotalAgents).toStrictEqual(0); + }); + + test("orders connections by role weighting (agent, then admin, then spectator)", () => { + const tracker = new ConnectionTracker(); + tracker.AddSpectator(makeSpectator(1)); + tracker.AddAdmin(makeAdmin(2)); + tracker.AddAgent(makeAgent(3, "a")); + + const roles = tracker.Connections.map((c: AbstractSocketHandler) => c.Role); + expect(roles).toStrictEqual([GameRole.Agent, GameRole.Admin, GameRole.Spectator]); + }); + + test("ignores removal of an unknown connection id", () => { + const tracker = new ConnectionTracker(); + tracker.AddSpectator(makeSpectator(1)); + + tracker.RemoveSpectator(999); + + expect(connectionIds(tracker)).toStrictEqual([1]); + }); +}); diff --git a/engine/bomberland-engine/src/Services/ConnectionTracker.ts b/engine/bomberland-engine/src/Services/ConnectionTracker.ts index a3d2851f..ea3c47ab 100644 --- a/engine/bomberland-engine/src/Services/ConnectionTracker.ts +++ b/engine/bomberland-engine/src/Services/ConnectionTracker.ts @@ -22,7 +22,6 @@ export class ConnectionTracker { private readonly agentSockets = new Map(); private readonly spectators = new Map(); private readonly connections: Array = []; - private readonly connectionNumberIndexMap = new Map(); public get Connections(): Array { return this.connections; @@ -70,16 +69,16 @@ export class ConnectionTracker { }; private addConnection = (connection: AbstractSocketHandler) => { - this.connectionNumberIndexMap.set(connection.ConnectionId, this.connections.length); this.connections.push(connection); this.connections.sort(connectionSortComparatorFn); }; private removeConnection = (connectionId: number) => { - const index = this.connectionNumberIndexMap.get(connectionId); - if (index !== undefined) { + // Remove by identity. The connections array is re-sorted on every add, so a + // stored index would be stale; look the connection up by its unique id instead. + const index = this.connections.findIndex((connection) => connection.ConnectionId === connectionId); + if (index !== -1) { this.connections.splice(index, 1); - this.connectionNumberIndexMap.delete(connectionId); } }; } diff --git a/engine/bomberland-engine/src/Services/GameWebSocket.ts b/engine/bomberland-engine/src/Services/GameWebSocket.ts index 28dfadaa..ca5381b2 100644 --- a/engine/bomberland-engine/src/Services/GameWebSocket.ts +++ b/engine/bomberland-engine/src/Services/GameWebSocket.ts @@ -62,6 +62,7 @@ export class GameWebsocket { ); } catch (e) { this.telemetry.Error(`Unable to connect socket with error: ${e}`); + connection.close(); } } else { this.telemetry.Error(`Client tried to connect with invalid query params: ${request.url}`); diff --git a/engine/bomberland-engine/src/Services/SocketHandler/AdminSocketHandler.ts b/engine/bomberland-engine/src/Services/SocketHandler/AdminSocketHandler.ts index bfc18802..285a645f 100644 --- a/engine/bomberland-engine/src/Services/SocketHandler/AdminSocketHandler.ts +++ b/engine/bomberland-engine/src/Services/SocketHandler/AdminSocketHandler.ts @@ -26,6 +26,9 @@ export class AdminSocketHandler extends AbstractSocketHandler { this.onMessage(message); }); this.connection.on("close", this.onClose()); + this.connection.on("error", (error: Error) => { + this.telemetry.Error(`Admin socket error [${this.ConnectionId}]: ${error.message}`); + }); this.onConnection?.(this); this.telemetry.Info(`Admin [${this.ConnectionId}] connected to the server`); }; @@ -44,7 +47,15 @@ export class AdminSocketHandler extends AbstractSocketHandler { }; public Send = (message: string) => { - (this.connection as ws.Server & { send: (message: string) => void }).send(message); + const socket = this.connection as ws.Server & { send: (message: string) => void; readyState: number }; + if (socket.readyState !== ws.OPEN) { + return; + } + try { + socket.send(message); + } catch (error) { + this.telemetry.Error(`Failed to send to admin ${this.ConnectionId}: ${error}`); + } }; private onClose = () => { diff --git a/engine/bomberland-engine/src/Services/SocketHandler/AgentSocketHandler.ts b/engine/bomberland-engine/src/Services/SocketHandler/AgentSocketHandler.ts index 28b522a4..8a9e3976 100644 --- a/engine/bomberland-engine/src/Services/SocketHandler/AgentSocketHandler.ts +++ b/engine/bomberland-engine/src/Services/SocketHandler/AgentSocketHandler.ts @@ -28,6 +28,9 @@ export class AgentSocketHandler extends AbstractSocketHandler { this.onMessage(message); }); this.connection.on("close", this.onClose()); + this.connection.on("error", (error: Error) => { + this.telemetry.Error(`Agent socket error [${this.name}](${this.AgentId}): ${error.message}`); + }); this.onConnection?.(this); this.telemetry.Info(`Agent [${this.name}](${this.agentId}) connected to the server`); }; @@ -45,7 +48,15 @@ export class AgentSocketHandler extends AbstractSocketHandler { }; public Send = (message: string) => { - (this.connection as ws.Server & { send: (message: string) => void }).send(message); + const socket = this.connection as ws.Server & { send: (message: string) => void; readyState: number }; + if (socket.readyState !== ws.OPEN) { + return; + } + try { + socket.send(message); + } catch (error) { + this.telemetry.Error(`Failed to send to agent ${this.AgentId}: ${error}`); + } }; private onClose = () => { diff --git a/engine/bomberland-engine/src/Services/SocketHandler/SpectatorSocketHandler.ts b/engine/bomberland-engine/src/Services/SocketHandler/SpectatorSocketHandler.ts index 3955271c..f04b073b 100644 --- a/engine/bomberland-engine/src/Services/SocketHandler/SpectatorSocketHandler.ts +++ b/engine/bomberland-engine/src/Services/SocketHandler/SpectatorSocketHandler.ts @@ -21,12 +21,23 @@ export class SpectatorSocketHandler extends AbstractSocketHandler { protected instantiateSocketHandler = () => { this.connection.on("close", this.onClose()); + this.connection.on("error", (error: Error) => { + this.telemetry.Error(`Spectator socket error (${this.ConnectionId}): ${error.message}`); + }); this.onConnection?.(this); this.telemetry.Info(`Spectator (${this.ConnectionId}) connected to the server`); }; public Send = (message: string) => { - (this.connection as ws.Server & { send: (message: string) => void }).send(message); + const socket = this.connection as ws.Server & { send: (message: string) => void; readyState: number }; + if (socket.readyState !== ws.OPEN) { + return; + } + try { + socket.send(message); + } catch (error) { + this.telemetry.Error(`Failed to send to spectator ${this.ConnectionId}: ${error}`); + } }; private onClose = () => { diff --git a/engine/bomberland-engine/src/Services/SocketHandler/attachSocketHandler.ts b/engine/bomberland-engine/src/Services/SocketHandler/attachSocketHandler.ts index 8459c0f7..f621796a 100644 --- a/engine/bomberland-engine/src/Services/SocketHandler/attachSocketHandler.ts +++ b/engine/bomberland-engine/src/Services/SocketHandler/attachSocketHandler.ts @@ -67,6 +67,7 @@ export const attachSocketHandler = ( connectionTracker.AddSpectator(spectator); break; default: - telemetry.Error(`Unknown role ${role}`); + // Throw so the caller closes the socket instead of leaving it open with no handlers. + throw new Error(`Unknown role ${role}`); } };