Fix bugs from code audit (server respawn, leaks, hardening)

Verified findings from a code audit and fixed the legitimate, well-scoped
ones. Notable: crashed cluster workers were never respawned because
worker.process.env is undefined in the exit handler, so WORKER_ID was always
undefined (confirmed empirically).

Fixes:
- Master: respawn crashed workers via a worker.id -> WORKER_ID registry
  instead of the always-undefined worker.process.env.WORKER_ID.
- GameServer: report Finished when _hasEnded so abandoned private lobbies are
  cleaned up instead of lingering until max duration; remove sockets from the
  websockets Set on close/rejoin; reset hasReachedMaxPlayerCount when a player
  leaves before start (avoids premature under-capacity starts, e.g. ranked 1v1).
- Worker: reap WebSocket connections that upgrade but never authenticate
  (30s join timeout) to prevent Slowloris-style FD exhaustion.
- ServerEnv: add a 1h TTL to the cached JWKS public key so IdP key rotation
  doesn't break all logins until a restart.
- SpawnExecution: ignore a client-supplied tile in random-spawn mode so a
  forged spawn intent can't pick its own coordinates (+ test).
- GameRenderer / MultiTabDetector: remove window listeners on teardown
  (bind once so add/removeEventListener references match).
- LocalPersistantStats: guard localStorage access with try/catch so sandboxed
  iframes / disabled-storage contexts don't crash game start.
- StructurePass: reuse per-frame uniform arrays instead of allocating each draw.

Also documents a known, intentionally-unfixed auth gap on
/api/archive_singleplayer_game (pending a product decision on logged-out users).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

src/client/ClientGameRunner.ts

@@ -617,6 +617,7 @@ async function createClientGame(
       if (rendererDisposed) return;
       rendererDisposed = true;
       stopFrameLoop();
+      gameRenderer.destroy();
       view.dispose();
       glCanvas.remove();
       inputOverlay.remove();

src/client/LocalPersistantStats.ts

@@ -12,25 +12,30 @@ export interface LocalStatsData {
 let _startTime: number;
 
 function getStats(): LocalStatsData {
-  const statsStr = localStorage.getItem("game-records");
-  return statsStr ? JSON.parse(statsStr) : {};
+  try {
+    const statsStr = localStorage.getItem("game-records");
+    return statsStr ? JSON.parse(statsStr) : {};
+  } catch {
+    // Accessing localStorage throws in sandboxed iframes (e.g. gaming portals)
+    // or when storage is disabled; treat as empty rather than crashing.
+    return {};
+  }
 }
 
 function save(stats: LocalStatsData) {
   // To execute asynchronously
-  setTimeout(
-    () => localStorage.setItem("game-records", JSON.stringify(stats, replacer)),
-    0,
-  );
+  setTimeout(() => {
+    try {
+      localStorage.setItem("game-records", JSON.stringify(stats, replacer));
+    } catch {
+      // Storage unavailable (sandboxed iframe / disabled) — skip persistence.
+    }
+  }, 0);
 }
 
 // The user can quit the game anytime so better save the lobby as soon as the
 // game starts.
 export function startGame(id: GameID, lobby: Partial<GameConfig>) {
-  if (localStorage === undefined) {
-    return;
-  }
-
   _startTime = Date.now();
   const stats = getStats();
   stats[id] = { lobby };
@@ -42,10 +47,6 @@ export function startTime() {
 }
 
 export function endGame(gameRecord: PartialGameRecord) {
-  if (localStorage === undefined) {
-    return;
-  }
-
   const stats = getStats();
   const gameStat = stats[gameRecord.info.gameID];
 

src/client/MultiTabDetector.ts

@@ -9,9 +9,15 @@ export class MultiTabDetector {
   private punishmentCount = 0;
   private startPenaltyCallback: (duration: number) => void = () => {};
 
+  // Bind once so addEventListener and removeEventListener share the same
+  // reference; otherwise stopMonitoring() can never remove the listeners.
+  private readonly boundStorageEvent = (e: StorageEvent) =>
+    this.onStorageEvent(e);
+  private readonly boundBeforeUnload = () => this.onBeforeUnload();
+
   constructor() {
-    window.addEventListener("storage", this.onStorageEvent.bind(this));
-    window.addEventListener("beforeunload", this.onBeforeUnload.bind(this));
+    window.addEventListener("storage", this.boundStorageEvent);
+    window.addEventListener("beforeunload", this.boundBeforeUnload);
   }
 
   public startMonitoring(startPenalty: (duration: number) => void): void {
@@ -33,8 +39,8 @@ export class MultiTabDetector {
     if (lock?.owner === this.tabId) {
       localStorage.removeItem(this.lockKey);
     }
-    window.removeEventListener("storage", this.onStorageEvent.bind(this));
-    window.removeEventListener("beforeunload", this.onBeforeUnload.bind(this));
+    window.removeEventListener("storage", this.boundStorageEvent);
+    window.removeEventListener("beforeunload", this.boundBeforeUnload);
   }
 
   private heartbeat(): void {

src/client/hud/GameRenderer.ts

@@ -345,6 +345,12 @@ export function createRenderer(
 export class GameRenderer {
   private layerTickState = new Map<Controller, { lastTickAtMs: number }>();
 
+  // Bound once so it can be removed on destroy(); a new GameRenderer is created
+  // per game (join without page reload), so an un-removed listener would pin
+  // this renderer and its transformHandler forever.
+  private readonly onResize = () =>
+    this.transformHandler.updateCanvasBoundingRect();
+
   constructor(
     public transformHandler: TransformHandler,
     public uiState: UIState,
@@ -359,14 +365,16 @@ export class GameRenderer {
 
     this.layers.forEach((l) => l.init?.());
 
-    window.addEventListener("resize", () =>
-      this.transformHandler.updateCanvasBoundingRect(),
-    );
+    window.addEventListener("resize", this.onResize);
 
     //show whole map on startup
     this.transformHandler.centerAll(0.9);
   }
 
+  destroy() {
+    window.removeEventListener("resize", this.onResize);
+  }
+
   tick() {
     const nowMs = performance.now();
     const shouldProfileTick = FrameProfiler.isEnabled();

src/client/render/gl/passes/StructurePass.ts

@@ -115,6 +115,9 @@ export class StructurePass {
   private ghost: GhostPreviewData | null = null;
   /** Scratch buffer for the single ghost instance (avoids allocation). */
   private ghostBuf = new Float32Array(FLOATS_PER_INSTANCE);
+  /** Scratch per-structure uniform arrays, rebuilt each frame (avoids per-frame allocation). */
+  private readonly shapeScales = new Float32Array(ATLAS_COLS);
+  private readonly iconFills = new Float32Array(ATLAS_COLS);
 
   constructor(
     gl: WebGL2RenderingContext,
@@ -362,8 +365,8 @@ export class StructurePass {
     gl.uniform1f(this.uIconGrowZoom, ss.iconGrowZoom);
 
     // Build per-structure uniform arrays from settings, ordered by atlas column
-    const scales = new Float32Array(ATLAS_COLS);
-    const fills = new Float32Array(ATLAS_COLS);
+    const scales = this.shapeScales;
+    const fills = this.iconFills;
     for (let i = 0; i < STRUCTURE_ORDER.length; i++) {
       const cfg = ss.shapes[STRUCTURE_ORDER[i]];
       scales[i] = cfg?.scale ?? 1.0;

src/core/execution/SpawnExecution.ts

@@ -53,7 +53,12 @@ export class SpawnExecution implements Execution {
     }
 
     player.tiles().forEach((t) => player.relinquish(t));
-    const spawn = this.getSpawn(this.tile);
+    // In random-spawn mode the location must be chosen by the simulation, never
+    // by a client-supplied tile. Ignore this.tile so a forged spawn intent
+    // can't pick its own coordinates.
+    const spawn = this.getSpawn(
+      this.mg.config().isRandomSpawn() ? undefined : this.tile,
+    );
 
     if (!spawn) {
       console.warn(`SpawnExecution: cannot spawn ${this.playerInfo.name}`);

src/server/GameServer.ts

@@ -541,6 +541,7 @@ export class GameServer {
 
     // Close old WebSocket to prevent resource leaks
     if (client.ws !== ws) {
+      this.websockets.delete(client.ws);
       client.ws.removeAllListeners();
       client.ws.close();
     }
@@ -676,6 +677,7 @@ export class GameServer {
         clientID: client.clientID,
         persistentID: client.persistentID,
       });
+      this.websockets.delete(client.ws);
       this.activeClients = this.activeClients.filter(
         (c) => c.clientID !== client.clientID,
       );
@@ -683,6 +685,14 @@ export class GameServer {
       if (!this._hasStarted) {
         // Remove persistentId if the game has not started to prevent going over max players
         this.persistentIdToClientId.delete(client.persistentID);
+        // A player left before start: if we dropped back under capacity, clear
+        // the max-players latch so the lobby doesn't commit to a premature
+        // under-capacity start (notably small public lobbies / ranked 1v1).
+        if (
+          this.activeClients.length < (this.gameConfig.maxPlayers ?? Infinity)
+        ) {
+          this.hasReachedMaxPlayerCount = false;
+        }
         // Close lobby when host leaves before game starts
         if (
           !this.isPublic() &&
@@ -716,6 +726,11 @@ export class GameServer {
       // Remove persistentId if the game has not started to prevent going over max players
       if (!this._hasStarted) {
         this.persistentIdToClientId.delete(client.persistentID);
+        if (
+          this.activeClients.length < (this.gameConfig.maxPlayers ?? Infinity)
+        ) {
+          this.hasReachedMaxPlayerCount = false;
+        }
       }
     }
   }
@@ -1016,6 +1031,12 @@ export class GameServer {
   }
 
   phase(): GamePhase {
+    // Once the game has been torn down (e.g. private-lobby host left, or end()
+    // was called) it must be reported Finished so GameManager stops scheduling
+    // and removes it, instead of lingering as a Lobby until max duration.
+    if (this._hasEnded) {
+      return GamePhase.Finished;
+    }
     const now = Date.now();
     const alive: Client[] = [];
     for (const client of this.activeClients) {

src/server/Master.ts

@@ -90,6 +90,11 @@ export async function startMaster() {
 
   log.info(`Instance ID: ${INSTANCE_ID}`);
 
+  // Track which WORKER_ID each cluster worker owns. `worker.process` is a
+  // ChildProcess and does NOT expose the forked env, so the exit handler can't
+  // recover WORKER_ID from worker.process.env — keep an explicit registry.
+  const workerIdByClusterId = new Map<number, number>();
+
   // Fork workers
   for (let i = 0; i < ServerEnv.numWorkers(); i++) {
     const worker = cluster.fork({
@@ -97,20 +102,21 @@ export async function startMaster() {
       INSTANCE_ID,
     });
 
+    workerIdByClusterId.set(worker.id, i);
     lobbyService.registerWorker(i, worker);
     log.info(`Started worker ${i} (PID: ${worker.process.pid})`);
   }
 
   // Handle worker crashes
   cluster.on("exit", (worker, code, signal) => {
-    const workerId = (worker as any).process?.env?.WORKER_ID;
+    const workerId = workerIdByClusterId.get(worker.id);
     if (workerId === undefined) {
-      log.error(`worker crashed could not find id`);
+      log.error(`worker crashed could not find id for cluster id ${worker.id}`);
       return;
     }
+    workerIdByClusterId.delete(worker.id);
 
-    const workerIdNum = parseInt(workerId);
-    lobbyService.removeWorker(workerIdNum);
+    lobbyService.removeWorker(workerId);
 
     log.warn(
       `Worker ${workerId} (PID: ${worker.process.pid}) died with code: ${code} and signal: ${signal}`,
@@ -123,7 +129,8 @@ export async function startMaster() {
       INSTANCE_ID,
     });
 
-    lobbyService.registerWorker(workerIdNum, newWorker);
+    workerIdByClusterId.set(newWorker.id, workerId);
+    lobbyService.registerWorker(workerId, newWorker);
     log.info(
       `Restarted worker ${workerId} (New PID: ${newWorker.process.pid})`,
     );

src/server/ServerEnv.ts

@@ -19,6 +19,10 @@ const JwksSchema = z.object({
 export class ServerEnv {
   private static readonly gameEnv: GameEnv = parseGameEnv(process.env.GAME_ENV);
   private static publicKey: JWK | null = null;
+  private static publicKeyFetchedAt = 0;
+  // Refresh the cached JWKS key periodically so an IdP signing-key rotation
+  // doesn't break all token verification until the process is restarted.
+  private static readonly PUBLIC_KEY_TTL_MS = 60 * 60 * 1000; // 1 hour
 
   // Values that also flow to the client via index.html, but on the server
   // are read from process.env directly. Server code never reaches into
@@ -90,7 +94,12 @@ export class ServerEnv {
       : `https://api.${audience}`;
   }
   static async jwkPublicKey(): Promise<JWK> {
-    if (ServerEnv.publicKey) return ServerEnv.publicKey;
+    if (
+      ServerEnv.publicKey &&
+      Date.now() - ServerEnv.publicKeyFetchedAt < ServerEnv.PUBLIC_KEY_TTL_MS
+    ) {
+      return ServerEnv.publicKey;
+    }
     const jwksUrl = ServerEnv.jwtIssuer() + "/.well-known/jwks.json";
     console.log(`Fetching JWKS from ${jwksUrl}`);
     const response = await fetch(jwksUrl);
@@ -105,6 +114,7 @@ export class ServerEnv {
       throw new Error("Invalid JWKS");
     }
     ServerEnv.publicKey = result.data.keys[0];
+    ServerEnv.publicKeyFetchedAt = Date.now();
     return ServerEnv.publicKey;
   }
   static turnIntervalMs(): number {

src/server/Worker.ts

@@ -221,6 +221,16 @@ export async function startWorker() {
 
   registerAdminBotRoutes({ app, gm, workerId, log });
 
+  // SECURITY (known gap, intentionally unfixed for now): this endpoint is
+  // unauthenticated — it validates the schema then forwards the record to the
+  // central API with the server's privileged x-api-key, trusting the
+  // client-supplied players[].persistentID. A caller can submit forged/spam
+  // single-player records, and anyone who learns another account's persistentID
+  // could attribute a forged game to it. Recommended fix: require a Bearer JWT
+  // (client sends getAuthHeader()), verifyClientToken it, and stamp
+  // players[0].persistentID from auth.persistentId instead of trusting the body
+  // (so a leaked persistentID can't forge — only a real JWT can). Open question
+  // is how to treat logged-out singleplayer users (no JWT in prod).
   app.post("/api/archive_singleplayer_game", async (req, res) => {
     try {
       const record = req.body;
@@ -269,6 +279,18 @@ export async function startWorker() {
 
   // WebSocket handling
   wss.on("connection", (ws: WebSocket, req) => {
+    // Reap sockets that upgrade but never send a valid join/rejoin. Until a
+    // client authenticates, no GameServer tracks the socket, so nothing else
+    // would ever close it — without this, idle connections accumulate
+    // (Slowloris-style FD exhaustion).
+    let authenticated = false;
+    const authTimeout = setTimeout(() => {
+      if (!authenticated) {
+        ws.close(1008, "join timeout");
+      }
+    }, 30_000);
+    ws.on("close", () => clearTimeout(authTimeout));
+
     ws.on("message", async (message: string) => {
       const ip = getClientIp(req);
 
@@ -321,6 +343,10 @@ export async function startWorker() {
         }
         const { persistentId, claims } = result;
 
+        // Token verified — the connection is authenticated; stop the reaper.
+        authenticated = true;
+        clearTimeout(authTimeout);
+
         if (claims?.role === "banned") {
           ws.close(1002, "Account Banned");
           return;

tests/RandomSpawnNoOverride.test.ts

@@ -0,0 +1,45 @@
+import { SpawnExecution } from "../src/core/execution/SpawnExecution";
+import { Game, PlayerInfo, PlayerType } from "../src/core/game/Game";
+import { TileRef } from "../src/core/game/GameMap";
+import { GameID } from "../src/core/Schemas";
+import { setup } from "./util/Setup";
+
+const GAME_ID: GameID = "game_id";
+const PLAYER_ID = "p_id";
+
+async function spawnWith(
+  randomSpawn: boolean,
+  x: number,
+  y: number,
+): Promise<{ game: Game; injected: TileRef; spawnTile: TileRef | undefined }> {
+  const game = await setup("plains", { randomSpawn });
+  game.addPlayer(new PlayerInfo("p", PlayerType.Human, null, PLAYER_ID));
+  const injected = game.map().ref(x, y);
+  game.addExecution(
+    new SpawnExecution(GAME_ID, game.player(PLAYER_ID).info(), injected),
+  );
+  game.executeNextTick(); // init the execution
+  game.executeNextTick(); // run the execution
+  return { game, injected, spawnTile: game.player(PLAYER_ID).spawnTile() };
+}
+
+describe("Random spawn cannot be overridden by a client-supplied tile", () => {
+  test("non-random mode honors the requested tile", async () => {
+    const { injected, spawnTile } = await spawnWith(false, 50, 50);
+    expect(spawnTile).toBe(injected);
+  });
+
+  test("random mode ignores the injected tile", async () => {
+    const a = await spawnWith(true, 50, 50);
+    const b = await spawnWith(true, 60, 60);
+
+    // The player still spawns on a valid land tile.
+    expect(a.spawnTile).toBeDefined();
+    expect(a.game.isLand(a.spawnTile!)).toBe(true);
+
+    // If the injected tile were honored, the two runs would spawn at the two
+    // distinct injected tiles. Because random spawn ignores it and uses the
+    // deterministic per-player seed instead, both runs land on the same tile.
+    expect(a.spawnTile).toBe(b.spawnTile);
+  });
+});