bug: map rotation review fixes

hasura/metadata/databases/default/tables/public_server_map_rotation.yaml

@@ -9,17 +9,12 @@ object_relationships:
     using:
       foreign_key_constraint_on: server_id
 select_permissions:
-  - role: guest
+  - role: administrator
     permission:
       columns:
         - map_id
         - position
         - server_id
-      filter:
-        server:
-          type:
-            _nin:
-              - Ranked
-              - Practice
+      filter: {}
       allow_aggregations: true
     comment: ""

src/dedicated-servers/dedicated-server-config.service.ts

@@ -15,6 +15,7 @@ type WorkshopItem = {
   banned?: number | boolean;
   title?: string;
   preview_url?: string;
+  tags?: Array<{ tag: string }>;
 };
 
 export type ImportedWorkshopMap = {
@@ -49,20 +50,25 @@ export class DedicatedServerConfigService {
     const ids = [...new Set(mapIds)];
 
     await this.postgres.transaction(async (client) => {
-      const { rows } = await client.query<{ count: number }>(
-        `SELECT count(*)::int AS count
+      const { rows } = await client.query<{ id: string; deleted: boolean }>(
+        `SELECT id, deleted_at IS NOT NULL AS deleted
            FROM maps
-          WHERE id = ANY($1::uuid[])
-            AND deleted_at IS NULL`,
+          WHERE id = ANY($1::uuid[])`,
         [ids],
       );
 
-      if (rows[0].count !== ids.length) {
+      if (rows.length !== ids.length) {
         throw new BadRequestException(
-          "The rotation names a map that does not exist or was deleted",
+          "The rotation names a map that does not exist",
         );
       }
 
+      // A map deleted since the page loaded is skipped at boot anyway, so it
+      // is dropped here rather than blocking every save that still lists it.
+      const deleted = new Set(
+        rows.filter((row) => row.deleted).map((row) => row.id),
+      );
+
       await client.query(
         `DELETE FROM server_map_rotation WHERE server_id = $1`,
         [serverId],
@@ -72,7 +78,7 @@ export class DedicatedServerConfigService {
         `INSERT INTO server_map_rotation (server_id, map_id, position)
          SELECT $1, rotation.map_id, rotation.position - 1
            FROM unnest($2::uuid[]) WITH ORDINALITY AS rotation(map_id, position)`,
-        [serverId, ids],
+        [serverId, ids.filter((id) => !deleted.has(id))],
       );
 
       await client.query(
@@ -133,13 +139,7 @@ export class DedicatedServerConfigService {
     for (const itemId of itemIds) {
       const item = details.get(itemId);
 
-      if (
-        !item ||
-        item.result !== 1 ||
-        item.consumer_app_id !== 730 ||
-        item.banned ||
-        !item.title
-      ) {
+      if (!DedicatedServerConfigService.isPlayableMap(item)) {
         skipped++;
         continue;
       }
@@ -158,6 +158,19 @@ export class DedicatedServerConfigService {
     return { maps, skipped };
   }
 
+  // CS2 skins, stickers and collections are workshop items for app 730 too;
+  // only a map carries the Map tag.
+  public static isPlayableMap(item: WorkshopItem | undefined): boolean {
+    return (
+      !!item &&
+      item.result === 1 &&
+      item.consumer_app_id === 730 &&
+      !item.banned &&
+      !!item.title &&
+      (item.tags ?? []).some(({ tag }) => tag.toLowerCase() === "map")
+    );
+  }
+
   public static workshopId(input: string): string | null {
     const trimmed = (input ?? "").trim();
 
@@ -212,9 +225,7 @@ export class DedicatedServerConfigService {
       return;
     }
 
-    await this.dedicatedServers.removeDedicatedServer(server.id);
-
-    if (!(await this.dedicatedServers.setupDedicatedServer(server.id))) {
+    if (!(await this.dedicatedServers.rebuildDedicatedServer(server.id))) {
       throw new BadRequestException(
         "Saved, but the server failed to start again; check the API logs",
       );
@@ -296,7 +307,9 @@ export class DedicatedServerConfigService {
 
   // A workshop map is catalogued once, under Competitive and disabled, the same
   // way the map form adds one: that keeps it out of every match map pool until
-  // an admin opts it in. An existing row is reused, and restored if deleted.
+  // an admin opts it in. An existing row is reused. A deleted one is restored
+  // disabled and out of the active pool, because restoring an active map puts
+  // it straight back into the seed pools that matchmaking plays.
   private async upsertWorkshopMap(
     workshopId: string,
     title: string,
@@ -317,7 +330,10 @@ export class DedicatedServerConfigService {
     if (existing) {
       if (existing.deleted) {
         await this.postgres.query(
-          `UPDATE maps SET deleted_at = NULL WHERE workshop_map_id = $1`,
+          `UPDATE maps
+              SET deleted_at = NULL, enabled = false, active_pool = false
+            WHERE workshop_map_id = $1
+              AND deleted_at IS NOT NULL`,
           [workshopId],
         );
       }

src/dedicated-servers/dedicated-servers.controller.ts

@@ -31,17 +31,12 @@ export class DedicatedServersController {
       return;
     }
 
-    await this.dedicatedServersService.removeDedicatedServer(serverId);
-
-    if (
-      data.op === "DELETE" ||
-      !data.new.game_server_node_id ||
-      data.new.enabled === false
-    ) {
-      return;
-    }
-
-    await this.dedicatedServersService.setupDedicatedServer(serverId);
+    await this.dedicatedServersService.rebuildDedicatedServer(
+      serverId,
+      data.op !== "DELETE" &&
+        !!data.new.game_server_node_id &&
+        data.new.enabled !== false,
+    );
   }
 
   @HasuraEvent()
@@ -71,8 +66,7 @@ export class DedicatedServersController {
     });
 
     for (const server of servers) {
-      await this.dedicatedServersService.removeDedicatedServer(server.id);
-      await this.dedicatedServersService.setupDedicatedServer(server.id);
+      await this.dedicatedServersService.rebuildDedicatedServer(server.id);
     }
   }
 

src/dedicated-servers/dedicated-servers.service.spec.ts

@@ -0,0 +1,86 @@
+import { DedicatedServersService } from "./dedicated-servers.service";
+
+// Remove-then-create is not atomic. Two overlapping rebuilds of one server used
+// to interleave: both removed, one created, and the other's AlreadyExists
+// handler deleted the deployment the first had just created.
+describe("DedicatedServersService.rebuildDedicatedServer", () => {
+  const service = new DedicatedServersService(
+    { log: jest.fn(), error: jest.fn(), verbose: jest.fn() } as never,
+    { get: () => ({ namespace: "5stack" }) } as never,
+    null as never,
+    null as never,
+    null as never,
+    { getConnection: () => ({}) } as never,
+    null as never,
+    null as never,
+    null as never,
+    null as never,
+  );
+
+  const steps: Array<string> = [];
+  const tick = () => new Promise((resolve) => setTimeout(resolve, 5));
+
+  beforeEach(() => {
+    steps.length = 0;
+
+    jest
+      .spyOn(service, "removeDedicatedServer")
+      .mockImplementation(async (serverId: string) => {
+        steps.push(`remove ${serverId}`);
+        await tick();
+      });
+    jest
+      .spyOn(service, "setupDedicatedServer")
+      .mockImplementation(async (serverId: string) => {
+        steps.push(`setup ${serverId}`);
+        await tick();
+        steps.push(`created ${serverId}`);
+        return true;
+      });
+  });
+
+  it("runs overlapping rebuilds of one server one after the other", async () => {
+    await Promise.all([
+      service.rebuildDedicatedServer("a"),
+      service.rebuildDedicatedServer("a"),
+    ]);
+
+    expect(steps).toEqual([
+      "remove a",
+      "setup a",
+      "created a",
+      "remove a",
+      "setup a",
+      "created a",
+    ]);
+  });
+
+  it("does not hold up a different server", async () => {
+    await Promise.all([
+      service.rebuildDedicatedServer("a"),
+      service.rebuildDedicatedServer("b"),
+    ]);
+
+    expect(steps.slice(0, 2)).toEqual(["remove a", "remove b"]);
+  });
+
+  it("only removes when the server should not start", async () => {
+    await service.rebuildDedicatedServer("a", false);
+
+    expect(steps).toEqual(["remove a"]);
+  });
+
+  it("keeps going after a rebuild that failed", async () => {
+    jest
+      .spyOn(service, "setupDedicatedServer")
+      .mockRejectedValueOnce(new Error("boom"));
+
+    const [first, second] = await Promise.allSettled([
+      service.rebuildDedicatedServer("a"),
+      service.rebuildDedicatedServer("a"),
+    ]);
+
+    expect(first.status).toBe("rejected");
+    expect(second).toEqual({ status: "fulfilled", value: true });
+  });
+});

src/dedicated-servers/dedicated-servers.service.ts

@@ -16,6 +16,8 @@ import { MapRotationService } from "../game-plugins/map-rotation.service";
 
 @Injectable()
 export class DedicatedServersService {
+  private static readonly rebuilds = new Map<string, Promise<boolean>>();
+
   private appConfig: AppConfig;
   private gameServerConfig: GameServersConfig;
   private readonly namespace: string;
@@ -280,18 +282,17 @@ export class DedicatedServersService {
                         name: "EXTRA_GAME_PARAMS",
                         value: [
                           `-maxplayers ${server.type === "Ranked" ? 16 : server.max_players}`,
-                          startMap?.workshop_map_id
-                            ? null
-                            : `+map ${startMap?.name ?? "de_dust2"}`,
+                          `+map ${startMap && !startMap.workshop_map_id ? startMap.name : "de_dust2"}`,
                           `+game_type ${this.getGameType(server.type)}`,
                           `+game_mode ${this.getGameMode(server.type)}`,
                           `+sv_skirmish_id ${this.getWarGameType(server.type)}`,
                           server.connect_password
                             ? `+sv_password ${server.connect_password}`
                             : null,
                           gameMode?.extraGameParams,
-                          // Runs in command-line order, unlike +map, so it
-                          // goes after game_type/game_mode have been set.
+                          // CS2 only logs on to Steam once a level is loaded, so
+                          // on its own this leaves the server idle with no map;
+                          // after the stock +map it downloads and replaces it.
                           startMap?.workshop_map_id
                             ? `+host_workshop_map ${startMap.workshop_map_id}`
                             : null,
@@ -424,7 +425,11 @@ export class DedicatedServersService {
 
       return true;
     } catch (error) {
-      await this.removeDedicatedServer(serverId);
+      // AlreadyExists means another rebuild created it; deleting it here would
+      // take down the server that rebuild just started.
+      if (error?.code?.toString() !== "409") {
+        await this.removeDedicatedServer(serverId);
+      }
 
       this.logger.error(
         `[${serverId}] unable to create dedicated server`,
@@ -435,6 +440,36 @@ export class DedicatedServersService {
     }
   }
 
+  // Remove-then-create is not atomic, and a servers event, a region relay
+  // change and the dedicated server settings all trigger it. Overlapping runs
+  // can both remove, one creates, and the other fails on AlreadyExists, so
+  // they run one at a time per server.
+  public async rebuildDedicatedServer(
+    serverId: string,
+    start = true,
+  ): Promise<boolean> {
+    const previous =
+      DedicatedServersService.rebuilds.get(serverId) ?? Promise.resolve(true);
+
+    const rebuild = previous
+      .catch(() => false)
+      .then(async () => {
+        await this.removeDedicatedServer(serverId);
+
+        return start ? await this.setupDedicatedServer(serverId) : true;
+      });
+
+    DedicatedServersService.rebuilds.set(serverId, rebuild);
+
+    try {
+      return await rebuild;
+    } finally {
+      if (DedicatedServersService.rebuilds.get(serverId) === rebuild) {
+        DedicatedServersService.rebuilds.delete(serverId);
+      }
+    }
+  }
+
   public async removeDedicatedServer(serverId: string): Promise<void> {
     this.logger.log(`[${serverId}] removing dedicated server`);