diff --git a/hasura/functions/match/map-veto/get_map_veto_pattern.sql b/hasura/functions/match/map-veto/get_map_veto_pattern.sql index 026fc346..3f24e134 100644 --- a/hasura/functions/match/map-veto/get_map_veto_pattern.sql +++ b/hasura/functions/match/map-veto/get_map_veto_pattern.sql @@ -9,6 +9,7 @@ DECLARE i INT; pool_size INT; _type TEXT; + pattern_matched BOOLEAN := false; BEGIN SELECT mo.best_of INTO best_of FROM matches m @@ -33,36 +34,45 @@ BEGIN FOR i IN 1..(pool_size - 1) LOOP base_pattern := array_append(base_pattern, 'Ban'); END LOOP; - base_pattern := array_append(base_pattern, 'Decider'); + pattern_matched := true; ELSIF pool_size = best_of THEN FOR i IN 1..(pool_size - 1) LOOP base_pattern := array_append(base_pattern, 'Pick'); END LOOP; - base_pattern := array_append(base_pattern, 'Decider'); + pattern_matched := true; ELSIF best_of = 3 THEN IF pool_size = 4 THEN - base_pattern := ARRAY['Ban', 'Pick', 'Pick', 'Decider']; + base_pattern := ARRAY['Ban', 'Pick', 'Pick']; ELSIF pool_size = 5 THEN - base_pattern := ARRAY['Ban', 'Pick', 'Pick', 'Ban', 'Decider']; + base_pattern := ARRAY['Ban', 'Pick', 'Pick', 'Ban']; ELSIF pool_size = 6 THEN - base_pattern := ARRAY['Ban', 'Ban', 'Pick', 'Pick', 'Ban', 'Decider']; + base_pattern := ARRAY['Ban', 'Ban', 'Pick', 'Pick', 'Ban']; ELSE - base_pattern := ARRAY['Ban', 'Ban', 'Pick', 'Pick', 'Ban', 'Ban', 'Decider']; + base_pattern := ARRAY['Ban', 'Ban', 'Pick', 'Pick', 'Ban', 'Ban']; END IF; + pattern_matched := true; ELSIF best_of = 5 THEN if pool_size = 6 THEN - base_pattern := ARRAY['Ban', 'Pick', 'Pick', 'Pick', 'Pick', 'Decider']; + base_pattern := ARRAY['Ban', 'Pick', 'Pick', 'Pick', 'Pick']; ELSE - base_pattern := ARRAY['Ban', 'Ban', 'Pick', 'Pick', 'Pick', 'Pick', 'Decider']; + base_pattern := ARRAY['Ban', 'Ban', 'Pick', 'Pick', 'Pick', 'Pick']; END IF; + pattern_matched := true; END IF; - IF pool_size > array_length(base_pattern, 1) THEN - FOR i IN 1..(pool_size - array_length(base_pattern, 1)) LOOP + -- Maps the pattern above doesn't account for become extra Bans, and they + -- MUST land before the Decider. get_map_veto_type reads this array + -- positionally, and the Decider is only ever auto-inserted by + -- create_match_map_from_veto once exactly one map is left, so any step + -- sitting after it can never be satisfied and the veto deadlocks. + IF pattern_matched THEN + FOR i IN 1..(pool_size - 1 - coalesce(array_length(base_pattern, 1), 0)) LOOP base_pattern := array_append(base_pattern, 'Ban'); END LOOP; + + base_pattern := array_append(base_pattern, 'Decider'); END IF; - + FOR i IN 1..(pool_size) LOOP _type := base_pattern[i]; diff --git a/hasura/functions/match/options/cleanup_orphaned_match_options.sql b/hasura/functions/match/options/cleanup_orphaned_match_options.sql new file mode 100644 index 00000000..2acd86b8 --- /dev/null +++ b/hasura/functions/match/options/cleanup_orphaned_match_options.sql @@ -0,0 +1,18 @@ +CREATE OR REPLACE FUNCTION public.cleanup_orphaned_match_options(target_id uuid) +RETURNS void +LANGUAGE plpgsql +AS $$ +BEGIN + IF target_id IS NULL THEN + RETURN; + END IF; + + IF NOT EXISTS (SELECT 1 FROM public.matches WHERE match_options_id = target_id) + AND NOT EXISTS (SELECT 1 FROM public.tournaments WHERE match_options_id = target_id) + AND NOT EXISTS (SELECT 1 FROM public.tournament_stages WHERE match_options_id = target_id) + AND NOT EXISTS (SELECT 1 FROM public.tournament_brackets WHERE match_options_id = target_id) + THEN + DELETE FROM public.match_options WHERE id = target_id; + END IF; +END; +$$; diff --git a/hasura/triggers/draft_games.sql b/hasura/triggers/draft_games.sql index 37cc15ae..1252f69b 100644 --- a/hasura/triggers/draft_games.sql +++ b/hasura/triggers/draft_games.sql @@ -81,9 +81,10 @@ CREATE OR REPLACE FUNCTION public.tad_draft_games() RETURNS TRIGGER LANGUAGE plpgsql AS $$ BEGIN - IF OLD.match_options_id IS NOT NULL THEN - DELETE FROM public.match_options WHERE id = OLD.match_options_id; - END IF; + -- Deleting unconditionally fails: tbd_matches removes the draft_games row + -- before the owning matches row is gone, and matches.match_options_id still + -- points at it (ON DELETE RESTRICT). tad_matches cleans up afterwards. + PERFORM cleanup_orphaned_match_options(OLD.match_options_id); RETURN OLD; END; $$; diff --git a/hasura/triggers/match_options.sql b/hasura/triggers/match_options.sql index 15a0533b..54aa8aae 100644 --- a/hasura/triggers/match_options.sql +++ b/hasura/triggers/match_options.sql @@ -163,23 +163,4 @@ END; $$; DROP TRIGGER IF EXISTS tad_match_options ON public.match_options; -CREATE TRIGGER tad_match_options AFTER DELETE ON public.match_options FOR EACH ROW EXECUTE FUNCTION public.tad_match_options(); - -CREATE OR REPLACE FUNCTION public.cleanup_orphaned_match_options(target_id uuid) -RETURNS void -LANGUAGE plpgsql -AS $$ -BEGIN - IF target_id IS NULL THEN - RETURN; - END IF; - - IF NOT EXISTS (SELECT 1 FROM public.matches WHERE match_options_id = target_id) - AND NOT EXISTS (SELECT 1 FROM public.tournaments WHERE match_options_id = target_id) - AND NOT EXISTS (SELECT 1 FROM public.tournament_stages WHERE match_options_id = target_id) - AND NOT EXISTS (SELECT 1 FROM public.tournament_brackets WHERE match_options_id = target_id) - THEN - DELETE FROM public.match_options WHERE id = target_id; - END IF; -END; -$$; \ No newline at end of file +CREATE TRIGGER tad_match_options AFTER DELETE ON public.match_options FOR EACH ROW EXECUTE FUNCTION public.tad_match_options(); \ No newline at end of file diff --git a/src/awards/awards.module.ts b/src/awards/awards.module.ts index 4f96c7ad..edafbe6a 100644 --- a/src/awards/awards.module.ts +++ b/src/awards/awards.module.ts @@ -1,7 +1,6 @@ import { Module } from "@nestjs/common"; import { AwardsService } from "./awards.service"; import { AwardsController } from "./awards.controller"; -import { TrophiesLegacyController } from "./trophies-legacy.controller"; import { S3Module } from "../s3/s3.module"; import { PostgresModule } from "../postgres/postgres.module"; import { SystemModule } from "../system/system.module"; @@ -10,7 +9,7 @@ import { loggerFactory } from "../utilities/LoggerFactory"; @Module({ imports: [S3Module, PostgresModule, SystemModule], providers: [AwardsService, loggerFactory()], - controllers: [AwardsController, TrophiesLegacyController], + controllers: [AwardsController], exports: [AwardsService], }) export class AwardsModule {} diff --git a/src/awards/awards.service.ts b/src/awards/awards.service.ts index a492df7c..1bfa341e 100644 --- a/src/awards/awards.service.ts +++ b/src/awards/awards.service.ts @@ -430,8 +430,9 @@ export class AwardsService { public async getStream( filename: string, ): Promise<{ stream: Readable; contentType: string; etag?: string } | null> { - // Images uploaded before the awards rename still live under `trophies/` - // and their keys are stored verbatim in image_url. + // Images uploaded before the awards rename still live under `trophies/`: + // the migration renamed tables in place, so image_url keeps those keys + // verbatim. Callers pass a bare filename, so try both prefixes. let key = `awards/${filename}`; if (!(await this.s3.has(key))) { diff --git a/src/awards/trophies-legacy.controller.ts b/src/awards/trophies-legacy.controller.ts deleted file mode 100644 index 18e3512a..00000000 --- a/src/awards/trophies-legacy.controller.ts +++ /dev/null @@ -1,33 +0,0 @@ -import { Controller, Get, Param, Res, NotFoundException } from "@nestjs/common"; -import { Response } from "express"; -import { AwardsService } from "./awards.service"; - -// Award artwork uploaded before the awards rename is served immutable, so -// links to /trophies/ stay in caches and embeds indefinitely. -@Controller("trophies") -export class TrophiesLegacyController { - constructor(private readonly awards: AwardsService) {} - - @Get(":filename") - public async serve( - @Param("filename") filename: string, - @Res() res: Response, - ) { - if (!/^[A-Za-z0-9._-]+$/.test(filename)) { - throw new NotFoundException("Award image not found"); - } - - const result = await this.awards.getStream(filename); - if (!result) { - throw new NotFoundException("Award image not found"); - } - - res.setHeader("Content-Type", result.contentType); - res.setHeader("Cache-Control", "public, max-age=31536000, immutable"); - if (result.etag) { - res.setHeader("ETag", result.etag); - } - - result.stream.pipe(res); - } -} diff --git a/test/map-veto.spec.ts b/test/map-veto.spec.ts index 8c284e72..f49b198e 100644 --- a/test/map-veto.spec.ts +++ b/test/map-veto.spec.ts @@ -68,6 +68,15 @@ describe("map veto (SQL-driven)", () => { [matchId, type, lineupId, mapId, side], ); + const patternFor = async (bestOf: number, poolSize: number) => { + const match = await createVetoMatch(bestOf, poolSize); + const [{ pattern }] = await postgres.query>( + "SELECT get_map_veto_pattern(m) AS pattern FROM matches m WHERE id = $1", + [match.id], + ); + return pattern; + }; + it("computes the CS rulebook patterns", async () => { const bo1 = await createVetoMatch(1, 3); const [{ pattern: p1 }] = await postgres.query< @@ -266,6 +275,168 @@ describe("map veto (SQL-driven)", () => { expect(maps.length).toBe(0); }); + // Pools larger than the hardcoded rulebook patterns get their surplus maps + // banned. Those bans used to be appended AFTER the Decider, so once the + // rulebook steps ran out get_map_veto_type reported 'Decider' with several + // maps still unaccounted for: nothing could satisfy that step (the Decider is + // only ever auto-inserted once one map is left) and every BO3/BO5 veto on a + // pool larger than 7 hung there permanently. + describe("pools larger than the rulebook pattern", () => { + it.each([ + [1, 8], + [1, 12], + [3, 8], + [3, 9], + [3, 12], + [5, 8], + [5, 9], + [5, 12], + ])( + "BO%i pool %i: the pattern covers the whole pool and ends on the Decider", + async (bestOf, poolSize) => { + const pattern = await patternFor(bestOf, poolSize); + + const bans = pattern.filter((type) => type === "Ban").length; + const picks = pattern.filter((type) => type === "Pick").length; + const sides = pattern.filter((type) => type === "Side").length; + const deciders = pattern.filter((type) => type === "Decider").length; + + // Every map in the pool is consumed exactly once, and the maps that + // survive to be played are the picks plus the decider. + expect(bans + picks + deciders).toBe(poolSize); + expect(picks + deciders).toBe(bestOf); + expect(sides).toBe(picks); + expect(deciders).toBe(1); + expect(pattern[pattern.length - 1]).toBe("Decider"); + }, + ); + + it.each([ + [3, 5, ["Ban", "Pick", "Side", "Pick", "Side", "Ban", "Decider"]], + [3, 6, ["Ban", "Ban", "Pick", "Side", "Pick", "Side", "Ban", "Decider"]], + [ + 3, + 7, + [ + "Ban", + "Ban", + "Pick", + "Side", + "Pick", + "Side", + "Ban", + "Ban", + "Decider", + ], + ], + [ + 5, + 6, + [ + "Ban", + "Pick", + "Side", + "Pick", + "Side", + "Pick", + "Side", + "Pick", + "Side", + "Decider", + ], + ], + [ + 5, + 7, + [ + "Ban", + "Ban", + "Pick", + "Side", + "Pick", + "Side", + "Pick", + "Side", + "Pick", + "Side", + "Decider", + ], + ], + ])( + "BO%i pool %i is unchanged by the surplus-ban fix", + async (bestOf, poolSize, expected) => { + expect(await patternFor(bestOf as number, poolSize as number)).toEqual( + expected, + ); + }, + ); + + // Drives the veto by always submitting whatever the SQL reports as next, + // which is the actual proof a large pool completes: asserting on the + // pattern array alone would not have caught the original hang. + const runVetoToCompletion = async (bestOf: number, poolSize: number) => { + const match = await createVetoMatch(bestOf, poolSize); + const used = new Set(); + let lastPicked: string | null = null; + + for (let step = 0; step <= poolSize * 2; step++) { + const state = await vetoState(match.id); + if (state.status !== "Veto") { + return match; + } + + const remaining = match.mapIds.filter((id) => !used.has(id)); + if (state.veto_type === "Decider") { + throw new Error( + `Decider requested with ${remaining.length} maps left — the veto cannot progress`, + ); + } + + if (state.veto_type === "Side") { + await insertPick(match.id, "Side", state.picking!, lastPicked!, "CT"); + continue; + } + + const mapId = remaining[0]; + used.add(mapId); + if (state.veto_type === "Pick") { + lastPicked = mapId; + } + await insertPick(match.id, state.veto_type!, state.picking!, mapId); + } + + throw new Error("veto never completed"); + }; + + it.each([ + [3, 12], + [5, 12], + ])( + "BO%i pool %i runs to completion and goes Live", + async (bestOf, poolSize) => { + const match = await runVetoToCompletion(bestOf, poolSize); + + expect((await vetoState(match.id)).status).toBe("Live"); + + const maps = await postgres.query>( + "SELECT id FROM match_maps WHERE match_id = $1", + [match.id], + ); + expect(maps.length).toBe(bestOf); + + const picks = await postgres.query>( + "SELECT type FROM match_map_veto_picks WHERE match_id = $1", + [match.id], + ); + expect(picks.filter((p) => p.type === "Ban").length).toBe( + poolSize - bestOf, + ); + expect(picks.filter((p) => p.type === "Pick").length).toBe(bestOf - 1); + expect(picks.filter((p) => p.type === "Decider").length).toBe(1); + }, + ); + }); + it("cancelling a match mid-veto wipes its veto picks", async () => { const match = await createVetoMatch(1, 3); await insertPick(match.id, "Ban", match.lineup_1_id, match.mapIds[0]); diff --git a/test/match-options.spec.ts b/test/match-options.spec.ts index 0a65cb88..1e6f6bdf 100644 --- a/test/match-options.spec.ts +++ b/test/match-options.spec.ts @@ -29,9 +29,11 @@ describe("match options locks (SQL-driven)", () => { }); beforeEach(async () => { + await postgres.query("DELETE FROM draft_games"); await postgres.query("DELETE FROM matches"); await postgres.query("DELETE FROM match_options"); await postgres.query("DELETE FROM map_pools WHERE type = 'Custom'"); + await postgres.query("DELETE FROM players"); }); const createPool = async (offset = 0) => { @@ -45,6 +47,21 @@ describe("match options locks (SQL-driven)", () => { return { matchId: match.id, optionsId: match.options_id, poolId, mapId }; }; + // Mirrors DraftMatchService.createMatch: the draft game and the match it + // creates point at the same match_options row. + const createDraftGame = async (optionsId: string, matchId?: string) => { + const host = await fx.player(); + const [draft] = await postgres.query>( + `INSERT INTO draft_games (host_steam_id, type, match_options_id, match_id) + VALUES ($1, 'Wingman', $2, $3) RETURNING id`, + [host, optionsId, matchId ?? null], + ); + return draft.id; + }; + + const linkDraftGame = (matchId: string, optionsId: string) => + createDraftGame(optionsId, matchId); + const setMatchStatus = (matchId: string, status: string) => postgres.query("UPDATE matches SET status = $1 WHERE id = $2", [ status, @@ -162,4 +179,56 @@ describe("match options locks (SQL-driven)", () => { ); expect(pools.length).toBe(0); }); + + // A draft game and the match it creates share one match_options row, and + // tbd_matches drops the draft game while the match is still present. An + // unconditional delete in tad_draft_games trips matches_match_options_id_fkey + // (ON DELETE RESTRICT) here. + it("deleting a match with a linked draft game still garbage-collects its options", async () => { + const { matchId, optionsId } = await createMatch(); + await linkDraftGame(matchId, optionsId); + + await postgres.query("DELETE FROM matches WHERE id = $1", [matchId]); + + const options = await postgres.query>( + "SELECT 1 FROM match_options WHERE id = $1", + [optionsId], + ); + expect(options.length).toBe(0); + }); + + it("deleting the draft game keeps options alive until the match goes", async () => { + const { matchId, optionsId } = await createMatch(); + const draftId = await linkDraftGame(matchId, optionsId); + + await postgres.query("DELETE FROM draft_games WHERE id = $1", [draftId]); + + const stillThere = await postgres.query>( + "SELECT 1 FROM match_options WHERE id = $1", + [optionsId], + ); + expect(stillThere.length).toBe(1); + + await postgres.query("DELETE FROM matches WHERE id = $1", [matchId]); + + const cleaned = await postgres.query>( + "SELECT 1 FROM match_options WHERE id = $1", + [optionsId], + ); + expect(cleaned.length).toBe(0); + }); + + it("deleting a draft game with no match garbage-collects its options", async () => { + const { poolId } = await createPool(0); + const optionsId = await fx.matchOptions({ mapPoolId: poolId }); + const draftId = await createDraftGame(optionsId); + + await postgres.query("DELETE FROM draft_games WHERE id = $1", [draftId]); + + const remaining = await postgres.query>( + "SELECT 1 FROM match_options WHERE id = $1", + [optionsId], + ); + expect(remaining.length).toBe(0); + }); });