From 3c4357add1198c6e0f02b0b44467ba6639a5a568 Mon Sep 17 00:00:00 2001 From: Luke Policinski Date: Thu, 20 Aug 2026 12:06:26 -0400 Subject: [PATCH 1/3] feature: notify when a game plugin auto updates, and let it be turned off --- hasura/metadata/actions.graphql | 7 + hasura/metadata/actions.yaml | 8 + .../public_game_server_node_plugins.yaml | 3 + .../down.sql | 2 + .../up.sql | 6 + src/game-plugins/game-plugins.controller.ts | 19 +- src/game-plugins/game-plugins.module.ts | 2 + src/game-plugins/game-plugins.service.spec.ts | 167 ++++++++++++++++- src/game-plugins/game-plugins.service.ts | 175 ++++++++++++++++-- .../jobs/NotifyGamePluginUpdate.spec.ts | 108 +++++++++++ .../jobs/NotifyGamePluginUpdate.ts | 140 ++++++++++++++ test/game-plugin-install-state.spec.ts | 96 ++++++++++ test/game-plugin-registry-sync.spec.ts | 1 + 13 files changed, 719 insertions(+), 15 deletions(-) create mode 100644 hasura/migrations/default/1880000004000_game_plugin_update_tracking/down.sql create mode 100644 hasura/migrations/default/1880000004000_game_plugin_update_tracking/up.sql create mode 100644 src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts create mode 100644 src/game-plugins/jobs/NotifyGamePluginUpdate.ts diff --git a/hasura/metadata/actions.graphql b/hasura/metadata/actions.graphql index 1370a076..54efeba3 100644 --- a/hasura/metadata/actions.graphql +++ b/hasura/metadata/actions.graphql @@ -1864,6 +1864,13 @@ type Mutation { ): SuccessOutput } +type Mutation { + setGamePluginAutoUpdate( + slug: String! + enabled: Boolean! + ): SuccessOutput +} + type Mutation { reconcileNodePlugins( nodeId: String! diff --git a/hasura/metadata/actions.yaml b/hasura/metadata/actions.yaml index 1d8056e4..30cadeb1 100644 --- a/hasura/metadata/actions.yaml +++ b/hasura/metadata/actions.yaml @@ -1187,6 +1187,14 @@ actions: permissions: - role: administrator comment: Remove a game plugin from a node's plugin store + - name: setGamePluginAutoUpdate + definition: + kind: synchronous + handler: '{{HASURA_GRAPHQL_ACTIONS_HOOK}}' + forward_client_headers: true + permissions: + - role: administrator + comment: Track new releases of a game plugin, or pin it where it is - name: reconcileNodePlugins definition: kind: synchronous diff --git a/hasura/metadata/databases/default/tables/public_game_server_node_plugins.yaml b/hasura/metadata/databases/default/tables/public_game_server_node_plugins.yaml index cf9f95a6..df174174 100644 --- a/hasura/metadata/databases/default/tables/public_game_server_node_plugins.yaml +++ b/hasura/metadata/databases/default/tables/public_game_server_node_plugins.yaml @@ -24,6 +24,7 @@ select_permissions: - runtime - version - detected_version + - previous_version - channel - status - source @@ -46,6 +47,7 @@ insert_permissions: - runtime - version - detected_version + - previous_version - channel - status - source @@ -62,6 +64,7 @@ update_permissions: - runtime - version - detected_version + - previous_version - channel - status - source diff --git a/hasura/migrations/default/1880000004000_game_plugin_update_tracking/down.sql b/hasura/migrations/default/1880000004000_game_plugin_update_tracking/down.sql new file mode 100644 index 00000000..0385ed75 --- /dev/null +++ b/hasura/migrations/default/1880000004000_game_plugin_update_tracking/down.sql @@ -0,0 +1,2 @@ +ALTER TABLE "public"."game_server_node_plugins" + DROP COLUMN IF EXISTS "previous_version"; diff --git a/hasura/migrations/default/1880000004000_game_plugin_update_tracking/up.sql b/hasura/migrations/default/1880000004000_game_plugin_update_tracking/up.sql new file mode 100644 index 00000000..9c96f852 --- /dev/null +++ b/hasura/migrations/default/1880000004000_game_plugin_update_tracking/up.sql @@ -0,0 +1,6 @@ +-- An Auto channel install changes version with nobody asking it to, and the row +-- only ever held where it landed. Without the version it came from there is no +-- record that anything moved -- the API overwrites `version` the moment a node +-- reports Installing, so the old value is gone before the install finishes. +ALTER TABLE "public"."game_server_node_plugins" + ADD COLUMN IF NOT EXISTS "previous_version" text; diff --git a/src/game-plugins/game-plugins.controller.ts b/src/game-plugins/game-plugins.controller.ts index 955686cb..b8e8bbf7 100644 --- a/src/game-plugins/game-plugins.controller.ts +++ b/src/game-plugins/game-plugins.controller.ts @@ -46,8 +46,9 @@ export class GamePluginsController { @Body() body: { slug: string; - status: "Installing" | "Failed" | "Removing"; + status: "Installing" | "Installed" | "Failed" | "Removing"; version?: string | null; + previousVersion?: string | null; error?: string | null; }, ) { @@ -109,6 +110,22 @@ export class GamePluginsController { return { success: true }; } + // Auto is what installing gives you, so the toggle is the only way back to a + // pinned version -- and the only way to say "yes, I meant it" about a plugin + // that changes underneath the fleet on its own. + @HasuraAction() + public async setGamePluginAutoUpdate(data: { + user: User; + slug: string; + enabled: boolean; + }) { + this.assertAdministrator(data.user); + + await this.gamePlugins.setAutoUpdate(data.slug, data.enabled); + + return { success: true }; + } + @HasuraAction() public async uninstallGamePlugin(data: { user: User; diff --git a/src/game-plugins/game-plugins.module.ts b/src/game-plugins/game-plugins.module.ts index 3feb463a..c73e9318 100644 --- a/src/game-plugins/game-plugins.module.ts +++ b/src/game-plugins/game-plugins.module.ts @@ -16,6 +16,7 @@ import { GameModesService } from "./game-modes.service"; import { GamePluginsController } from "./game-plugins.controller"; import { SyncGamePluginRegistry } from "./jobs/SyncGamePluginRegistry"; import { CheckGamePluginUpdates } from "./jobs/CheckGamePluginUpdates"; +import { NotifyGamePluginUpdate } from "./jobs/NotifyGamePluginUpdate"; @Module({ providers: [ @@ -23,6 +24,7 @@ import { CheckGamePluginUpdates } from "./jobs/CheckGamePluginUpdates"; GameModesService, SyncGamePluginRegistry, CheckGamePluginUpdates, + NotifyGamePluginUpdate, ...getQueuesProcessors("GamePlugins"), loggerFactory(), Logger, diff --git a/src/game-plugins/game-plugins.service.spec.ts b/src/game-plugins/game-plugins.service.spec.ts index 77cb97c8..af23f98c 100644 --- a/src/game-plugins/game-plugins.service.spec.ts +++ b/src/game-plugins/game-plugins.service.spec.ts @@ -70,9 +70,9 @@ describe("GamePluginsService.slugFrom", () => { }); it("drops a v-prefixed version too", () => { - expect(GamePluginsService.slugFrom("InventorySimulator-v1.2.0.zip")).toEqual( - "inventorysimulator", - ); + expect( + GamePluginsService.slugFrom("InventorySimulator-v1.2.0.zip"), + ).toEqual("inventorysimulator"); }); // A digit that is part of the name, not a release number. @@ -86,3 +86,164 @@ describe("GamePluginsService.slugFrom", () => { expect(GamePluginsService.slugFrom("...")).toBeUndefined(); }); }); + +// An auto update is the one version change nobody asked for, so the decision +// to raise a notice is made here rather than left to whoever reads the panel. +describe("GamePluginsService update notices", () => { + let postgres: { query: jest.Mock }; + let queue: { add: jest.Mock }; + let pluginRuntime: { getPluginRuntime: jest.Mock }; + let service: GamePluginsService; + + const report = (progress: Record) => + service.recordNodeProgress("node-1", progress as any); + + const queued = () => queue.add.mock.calls[0]?.[1]; + const options = () => queue.add.mock.calls[0]?.[2]; + + beforeEach(() => { + postgres = { query: jest.fn(async (): Promise> => []) }; + queue = { add: jest.fn(async (): Promise => undefined) }; + pluginRuntime = { getPluginRuntime: jest.fn(async (): Promise => "swiftlys2") }; + + service = new GamePluginsService( + { warn: jest.fn(), log: jest.fn() } as any, + {} as any, + postgres as any, + pluginRuntime as any, + {} as any, + queue as any, + ); + }); + + it("raises a notice when a plugin replaced a different version", async () => { + await report({ + slug: "retakes", + status: "Installed", + version: "1.2.0", + previousVersion: "1.1.0", + }); + + expect(queued()).toEqual( + expect.objectContaining({ outcome: "updated", previousVersion: "1.1.0" }), + ); + }); + + it("says nothing about a first install", async () => { + await report({ + slug: "retakes", + status: "Installed", + version: "1.2.0", + previousVersion: null, + }); + + expect(queue.add).not.toHaveBeenCalled(); + }); + + it("says nothing when the version did not move", async () => { + await report({ + slug: "retakes", + status: "Installed", + version: "1.2.0", + previousVersion: "1.2.0", + }); + + expect(queue.add).not.toHaveBeenCalled(); + }); + + it("raises a notice when an install failed", async () => { + await report({ + slug: "retakes", + status: "Failed", + version: "1.2.0", + previousVersion: "1.1.0", + error: "digest mismatch", + }); + + expect(queued()).toEqual(expect.objectContaining({ outcome: "failed" })); + }); + + // The whole fleet reports the same release, and a failing install is retried + // every five minutes forever. Without one id per release that is a + // notification every five minutes per node. + it("keys the notice to the release rather than the node", async () => { + await report({ + slug: "retakes", + status: "Installed", + version: "1.2.0", + previousVersion: "1.1.0", + }); + + expect(options().jobId).toEqual("plugin-updated.retakes.1.2.0"); + }); + + it("records the progress even when the notice cannot be queued", async () => { + queue.add.mockRejectedValue(new Error("redis is down")); + + await expect( + report({ + slug: "retakes", + status: "Installed", + version: "1.2.0", + previousVersion: "1.1.0", + }), + ).resolves.toBeUndefined(); + + expect(postgres.query).toHaveBeenCalled(); + }); +}); + +// Turning auto updates off has to mean "stay where you are". Pinning to the +// newest published build would roll the fleet forward on the way to freezing +// it. +describe("GamePluginsService.setAutoUpdate", () => { + let postgres: { query: jest.Mock }; + let service: GamePluginsService; + + const build = () => { + postgres = { query: jest.fn(async (): Promise> => []) }; + + service = new GamePluginsService( + { warn: jest.fn(), log: jest.fn() } as any, + {} as any, + postgres as any, + { getPluginRuntime: jest.fn(async (): Promise => "swiftlys2") } as any, + {} as any, + { add: jest.fn() } as any, + ); + }; + + const update = () => + postgres.query.mock.calls.find(([sql]) => + sql.includes("UPDATE public.game_plugin_installs"), + ); + + it("pins to the version the nodes are actually running", async () => { + build(); + postgres.query + .mockResolvedValueOnce([{ channel: "Auto" }]) + .mockResolvedValueOnce([{ version: "1.1.0" }]) + .mockResolvedValue([]); + + await service.setAutoUpdate("retakes", false); + + expect(update()[1]).toEqual(["retakes", "1.1.0"]); + }); + + it("clears the pin when it is turned back on", async () => { + build(); + postgres.query.mockResolvedValueOnce([{ channel: "Pinned" }]); + + await service.setAutoUpdate("retakes", true); + + expect(update()[0]).toContain("'Auto'"); + }); + + it("refuses a plugin that is not installed", async () => { + build(); + + await expect(service.setAutoUpdate("retakes", false)).rejects.toThrow( + "not installed", + ); + }); +}); diff --git a/src/game-plugins/game-plugins.service.ts b/src/game-plugins/game-plugins.service.ts index 58d6e3b7..85866070 100644 --- a/src/game-plugins/game-plugins.service.ts +++ b/src/game-plugins/game-plugins.service.ts @@ -5,17 +5,20 @@ import { Logger, NotFoundException, } from "@nestjs/common"; +import { InjectQueue } from "@nestjs/bullmq"; +import { Queue } from "bullmq"; import { HasuraService } from "../hasura/hasura.service"; import { PostgresService } from "../postgres/postgres.service"; import { PluginRuntimeService } from "../plugin-runtime/plugin-runtime.service"; import { CacheService } from "../cache/cache.service"; import { SystemSettingName } from "../system/enums/SystemSettingName"; import { RegistryIndex, RegistryPlugin } from "./types/Registry"; +import { GamePluginQueues } from "./enums/GamePluginQueues"; +import { NotifyGamePluginUpdate } from "./jobs/NotifyGamePluginUpdate"; @Injectable() export class GamePluginsService { - private static readonly DEFAULT_REGISTRY_URL = - "https://registry.5stack.gg/"; + private static readonly DEFAULT_REGISTRY_URL = "https://registry.5stack.gg/"; // Big enough for anything a CS2 plugin ships and small enough that a wrong // URL -- a disk image, a game build -- fails instead of filling the pod. @@ -30,6 +33,8 @@ export class GamePluginsService { private readonly postgres: PostgresService, private readonly pluginRuntime: PluginRuntimeService, private readonly cache: CacheService, + @InjectQueue(GamePluginQueues.Registry) + private readonly registryQueue: Queue, ) {} public async getRegistryUrl(): Promise { @@ -741,9 +746,7 @@ export class GamePluginsService { const isMarkdown = /\.(md|markdown|mdown|mkd)$/i.test(body.name ?? ""); const readme = { - content: isMarkdown - ? this.absolutizeMarkdown(decoded, repo) - : decoded, + content: isMarkdown ? this.absolutizeMarkdown(decoded, repo) : decoded, format: (isMarkdown ? "markdown" : "text") as "markdown" | "text", repo, url: `https://github.com/${repo}`, @@ -760,7 +763,9 @@ export class GamePluginsService { ): Promise { const [plugin] = await this.postgres.query< Array<{ homepage: string | null; panel: { repo?: string } | null }> - >(`SELECT homepage, panel FROM public.game_plugins WHERE slug = $1`, [slug]); + >(`SELECT homepage, panel FROM public.game_plugins WHERE slug = $1`, [ + slug, + ]); if (!plugin) { throw new NotFoundException(`${slug} is not in the catalog`); @@ -1027,24 +1032,30 @@ export class GamePluginsService { nodeId: string, progress: { slug: string; - status: "Installing" | "Failed" | "Removing"; + status: "Installing" | "Installed" | "Failed" | "Removing"; version?: string | null; + previousVersion?: string | null; error?: string | null; }, ): Promise { await this.postgres.query( `INSERT INTO public.game_server_node_plugins - (game_server_node_id, plugin_slug, runtime, version, status, last_error, - source, detected, updated_at) + (game_server_node_id, plugin_slug, runtime, version, previous_version, + status, last_error, source, detected, installed_at, updated_at) SELECT n.id, $2, COALESCE(n.pin_plugin_runtime, active_plugin_runtime()), - $3, $4, $5, 'managed', false, now() + $3, $6, $4, $5, 'managed', false, + CASE WHEN $4 = 'Installed' THEN now() END, now() FROM public.game_server_nodes n WHERE n.id = $1 ON CONFLICT (game_server_node_id, plugin_slug) DO UPDATE SET status = EXCLUDED.status, version = COALESCE(EXCLUDED.version, game_server_node_plugins.version), + previous_version = COALESCE( + EXCLUDED.previous_version, game_server_node_plugins.previous_version), runtime = EXCLUDED.runtime, last_error = EXCLUDED.last_error, + installed_at = COALESCE( + EXCLUDED.installed_at, game_server_node_plugins.installed_at), updated_at = now()`, [ nodeId, @@ -1052,8 +1063,148 @@ export class GamePluginsService { progress.version ?? null, progress.status, progress.error ?? null, + progress.previousVersion ?? null, ], ); + + await this.queueUpdateNotice(nodeId, progress); + } + + // Both outcomes of a version change are silent otherwise: an Auto install + // moves with nobody asking it to, and a failed one leaves the node on the + // build it already had while the panel says Failed to whoever happens to + // open the page. + // + // The job id is the throttle, not a nicety. converge() retries a failing + // install every five minutes forever, and every node reports the same + // release separately -- keyed this way a whole fleet moving at once, or one + // plugin failing all week, is a single notification either way. + private async queueUpdateNotice( + nodeId: string, + progress: { + slug: string; + status: string; + version?: string | null; + previousVersion?: string | null; + }, + ): Promise { + if (!progress.version) { + return; + } + + const updated = + progress.status === "Installed" && + !!progress.previousVersion && + progress.previousVersion !== progress.version; + + if (!updated && progress.status !== "Failed") { + return; + } + + const outcome = updated ? "updated" : "failed"; + + try { + await this.registryQueue.add( + NotifyGamePluginUpdate.name, + { + slug: progress.slug, + version: progress.version, + previousVersion: progress.previousVersion ?? null, + outcome, + nodeId, + }, + { + jobId: `plugin-${outcome}.${progress.slug}.${progress.version}`, + // Long enough for the rest of the fleet to report the same release, + // so the notice can count nodes instead of naming whichever one was + // quickest. + delay: 30 * 1000, + removeOnComplete: { age: 7 * 24 * 60 * 60 }, + // A job that threw must not hold its id for the week: the id is what + // suppresses the retry, and suppressing a notice nobody ever got is + // the one outcome worse than a duplicate. + removeOnFail: { age: 60 * 60 }, + }, + ); + } catch (error) { + this.logger.warn( + `could not queue the ${progress.slug} update notice: ${error.message ?? error}`, + ); + } + } + + // Auto follows the newest release; Pinned stays where it is. Both columns + // move together or the channel/version check rejects the row. + // + // Turning it off freezes at what the nodes actually report running, not at + // the newest published build -- pinning to the latest would mean switching + // auto updates *off* could roll the fleet forward, which is backwards. + public async setAutoUpdate(slug: string, enabled: boolean): Promise { + const [install] = await this.postgres.query>( + `SELECT channel FROM public.game_plugin_installs WHERE plugin_slug = $1`, + [slug], + ); + + if (!install) { + throw new BadRequestException(`${slug} is not installed`); + } + + if (enabled) { + await this.postgres.query( + `UPDATE public.game_plugin_installs + SET channel = 'Auto', version = null, updated_at = now() + WHERE plugin_slug = $1`, + [slug], + ); + + this.nudgeNodes(); + + return; + } + + await this.postgres.query( + `UPDATE public.game_plugin_installs + SET channel = 'Pinned', version = $2, updated_at = now() + WHERE plugin_slug = $1`, + [slug, await this.versionToPin(slug)], + ); + } + + // What the fleet is on, preferring the version the most nodes report having + // installed. A version no build was ever published for cannot be pinned to: + // desiredForNode would resolve nothing and drop the plugin off the manifest + // entirely. + private async versionToPin(slug: string): Promise { + const runtime = await this.pluginRuntime.getPluginRuntime(); + + const [running] = await this.postgres.query>( + `SELECT p.version + FROM public.game_server_node_plugins p + WHERE p.plugin_slug = $1 + AND p.detected = true + AND p.status = 'Installed' + AND p.version IS NOT NULL + AND EXISTS ( + SELECT 1 FROM public.game_plugin_versions v + WHERE v.plugin_slug = p.plugin_slug + AND v.runtime = $2 + AND v.version = p.version) + GROUP BY p.version + ORDER BY count(*) DESC, max(p.updated_at) DESC + LIMIT 1`, + [slug, runtime], + ); + + if (running) { + return running.version; + } + + // Nothing has reported in yet -- a plugin requested minutes ago, or a + // fleet that is entirely offline. The version Auto would hand out right + // now is the honest answer to "where are we". + const resolved = await this.resolveVersion(slug, runtime); + + return resolved.version; } private async getNodeIP(nodeId: string): Promise { @@ -1115,7 +1266,9 @@ export class GamePluginsService { }); if (!response.ok) { - const body = await response.json().catch(() => ({}) as { message?: string }); + const body = await response + .json() + .catch(() => ({}) as { message?: string }); throw new BadRequestException( body.message || `node connector returned ${response.status}`, ); diff --git a/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts b/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts new file mode 100644 index 00000000..da6dce5d --- /dev/null +++ b/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts @@ -0,0 +1,108 @@ +import { NotifyGamePluginUpdate } from "./NotifyGamePluginUpdate"; + +describe("NotifyGamePluginUpdate", () => { + let postgres: { query: jest.Mock }; + let notifications: { send: jest.Mock }; + let job: NotifyGamePluginUpdate; + + const run = (data: Record) => job.process({ data } as any); + + const sent = () => notifications.send.mock.calls[0]?.[1]; + + const updated = { + slug: "retakes", + version: "1.2.0", + previousVersion: "1.1.0", + outcome: "updated", + nodeId: "node-1", + }; + + const failed = { ...updated, outcome: "failed" }; + + beforeEach(() => { + postgres = { query: jest.fn(async (): Promise> => []) }; + notifications = { send: jest.fn(async (): Promise => undefined) }; + + job = new NotifyGamePluginUpdate( + { warn: jest.fn() } as any, + postgres as any, + notifications as any, + ); + }); + + it("names both versions and how far the update reached", async () => { + postgres.query + .mockResolvedValueOnce([{ name: "Retakes", channel: "Auto" }]) + .mockResolvedValueOnce([{ count: "3" }]); + + await run(updated); + + expect(sent().message).toContain( + "Retakes auto-updated from 1.1.0 to 1.2.0", + ); + expect(sent().message).toContain("3 nodes"); + }); + + // Changing a pinned version is somebody typing it in. Reporting it back to + // them is noise, and it is the case the toggle exists to make explicit. + it("stays quiet about a pinned plugin moving", async () => { + postgres.query.mockResolvedValueOnce([ + { name: "Retakes", channel: "Pinned" }, + ]); + + await run(updated); + + expect(notifications.send).not.toHaveBeenCalled(); + }); + + it("says nothing about a plugin that was uninstalled since", async () => { + postgres.query.mockResolvedValue([]); + + await run(updated); + + expect(notifications.send).not.toHaveBeenCalled(); + }); + + // Which nodes failed, and what they are still running, are the two things an + // admin needs before deciding whether it can wait. + it("names the nodes that failed and the version they kept", async () => { + postgres.query + .mockResolvedValueOnce([{ name: "Retakes", channel: "Auto" }]) + .mockResolvedValueOnce([ + { game_server_node_id: "node-1", last_error: "digest mismatch" }, + { game_server_node_id: "node-2", last_error: null }, + ]); + + await run(failed); + + expect(sent().message).toContain("node-1, node-2"); + expect(sent().message).toContain("digest mismatch"); + expect(sent().message).toContain("still running 1.1.0"); + }); + + // A pinned install failing is just as silent as an auto one, so the channel + // filter deliberately does not apply here. + it("reports a pinned install failing too", async () => { + postgres.query + .mockResolvedValueOnce([{ name: "Retakes", channel: "Pinned" }]) + .mockResolvedValueOnce([ + { game_server_node_id: "node-1", last_error: "404" }, + ]); + + await run(failed); + + expect(notifications.send).toHaveBeenCalled(); + }); + + // converge() retries every five minutes, so by the time this runs the thing + // it is about to report may have already fixed itself. + it("drops a failure that recovered while the notice waited", async () => { + postgres.query + .mockResolvedValueOnce([{ name: "Retakes", channel: "Auto" }]) + .mockResolvedValueOnce([]); + + await run(failed); + + expect(notifications.send).not.toHaveBeenCalled(); + }); +}); diff --git a/src/game-plugins/jobs/NotifyGamePluginUpdate.ts b/src/game-plugins/jobs/NotifyGamePluginUpdate.ts new file mode 100644 index 00000000..5b8fdf54 --- /dev/null +++ b/src/game-plugins/jobs/NotifyGamePluginUpdate.ts @@ -0,0 +1,140 @@ +import { Job } from "bullmq"; +import { WorkerHost } from "@nestjs/bullmq"; +import { Logger } from "@nestjs/common"; +import { UseQueue } from "../../utilities/QueueProcessors"; +import { GamePluginQueues } from "../enums/GamePluginQueues"; +import { PostgresService } from "../../postgres/postgres.service"; +import { NotificationsService } from "../../notifications/notifications.service"; +import { DISCORD_COLORS } from "../../notifications/utilities/constants"; + +type UpdateNotice = { + slug: string; + version: string; + previousVersion: string | null; + outcome: "updated" | "failed"; + nodeId: string; +}; + +@UseQueue("GamePlugins", GamePluginQueues.Registry) +export class NotifyGamePluginUpdate extends WorkerHost { + constructor( + protected readonly logger: Logger, + protected readonly postgres: PostgresService, + protected readonly notifications: NotificationsService, + ) { + super(); + } + + async process(job: Job): Promise { + const notice = job.data; + + const [install] = await this.postgres.query< + Array<{ name: string; channel: string }> + >( + `SELECT p.name, i.channel + FROM public.game_plugin_installs i + INNER JOIN public.game_plugins p ON p.slug = i.plugin_slug + WHERE i.plugin_slug = $1 AND i.enabled = true`, + [notice.slug], + ); + + // Uninstalled between the report and this running, thirty seconds later. + if (!install) { + return; + } + + if (notice.outcome === "failed") { + await this.notifyFailed(notice, install.name); + return; + } + + // A pinned install only changes version because an admin changed it, and + // they do not need telling what they just did. The whole point of the + // notice is the version that moved on its own. + if (install.channel !== "Auto") { + return; + } + + await this.notifyUpdated(notice, install.name); + } + + private async notifyUpdated( + notice: UpdateNotice, + name: string, + ): Promise { + const [{ count }] = await this.postgres.query>( + `SELECT count(*) AS count + FROM public.game_server_node_plugins + WHERE plugin_slug = $1 AND version = $2 AND status = 'Installed'`, + [notice.slug, notice.version], + ); + + const nodes = Number(count); + + await this.notifications.send( + "GameNodeStatus", + { + title: "Game Plugin Auto-Updated", + message: + `${NotificationsService.escapeHtml(name)} auto-updated from ` + + `${NotificationsService.escapeHtml(notice.previousVersion)} to ` + + `${NotificationsService.escapeHtml(notice.version)} on ` + + `${nodes === 1 ? "1 node" : `${nodes} nodes`}. ` + + `View plugin`, + role: "administrator", + }, + undefined, + DISCORD_COLORS.ORANGE, + ); + } + + private async notifyFailed( + notice: UpdateNotice, + name: string, + ): Promise { + const failed = await this.postgres.query< + Array<{ game_server_node_id: string; last_error: string | null }> + >( + `SELECT game_server_node_id, last_error + FROM public.game_server_node_plugins + WHERE plugin_slug = $1 AND version = $2 AND status = 'Failed' + ORDER BY game_server_node_id`, + [notice.slug, notice.version], + ); + + // It recovered on a retry while this sat in its delay -- converge() runs + // every five minutes, so that is a real outcome rather than a race. + if (failed.length === 0) { + return; + } + + const error = failed.find((node) => node.last_error)?.last_error; + + const nodes = + failed.length > 3 + ? `${failed.length} nodes` + : failed + .map((node) => + NotificationsService.escapeHtml(node.game_server_node_id), + ) + .join(", "); + + await this.notifications.send( + "GameNodeStatus", + { + title: "Game Plugin Update Failed", + message: + `${NotificationsService.escapeHtml(name)} could not install ` + + `${NotificationsService.escapeHtml(notice.version)} on ${nodes}` + + `${error ? `: ${NotificationsService.escapeHtml(error)}` : ""}. ` + + (notice.previousVersion + ? `They are still running ${NotificationsService.escapeHtml(notice.previousVersion)}. ` + : "") + + `View plugin`, + role: "administrator", + }, + undefined, + DISCORD_COLORS.RED, + ); + } +} diff --git a/test/game-plugin-install-state.spec.ts b/test/game-plugin-install-state.spec.ts index 34dceffe..cd66329f 100644 --- a/test/game-plugin-install-state.spec.ts +++ b/test/game-plugin-install-state.spec.ts @@ -182,6 +182,101 @@ describe("game plugin install state (SQL-driven)", () => { expect(Number(row.target)).toEqual(2); }); + // An auto update overwrites the version the moment the node says it is + // downloading, so the version it came from has to be carried on the report + // or it is gone. + describe("recorded progress", () => { + const service = () => + new GamePluginsService( + { warn: jest.fn(), log: jest.fn() } as never, + {} as never, + postgres, + {} as never, + {} as never, + { add: jest.fn() } as never, + ); + + const row = async () => { + const [record] = await postgres.query>>( + `SELECT version, previous_version, status, installed_at + FROM game_server_node_plugins + WHERE game_server_node_id = 'node-1' AND plugin_slug = 'retakes'`, + ); + return record; + }; + + it("keeps the version an update replaced", async () => { + await addNode("node-1"); + await request(); + + await service().recordNodeProgress("node-1", { + slug: "retakes", + status: "Installed", + version: "1.2.0", + previousVersion: "1.1.0", + }); + + expect(await row()).toEqual( + expect.objectContaining({ + version: "1.2.0", + previous_version: "1.1.0", + }), + ); + }); + + it("dates the install only once it lands", async () => { + await addNode("node-1"); + await request(); + + await service().recordNodeProgress("node-1", { + slug: "retakes", + status: "Installing", + version: "1.2.0", + previousVersion: "1.1.0", + }); + + expect((await row()).installed_at).toBeNull(); + + await service().recordNodeProgress("node-1", { + slug: "retakes", + status: "Installed", + version: "1.2.0", + previousVersion: "1.1.0", + }); + + expect((await row()).installed_at).not.toBeNull(); + }); + + // Reported once at the start of the install and not again on the failure, + // which is the report that has to survive for the notice to say what the + // node is still running. + it("does not lose the previous version to a later report", async () => { + await addNode("node-1"); + await request(); + + await service().recordNodeProgress("node-1", { + slug: "retakes", + status: "Installing", + version: "1.2.0", + previousVersion: "1.1.0", + }); + + await service().recordNodeProgress("node-1", { + slug: "retakes", + status: "Failed", + version: "1.2.0", + error: "digest mismatch", + }); + + expect(await row()).toEqual( + expect.objectContaining({ + status: "Failed", + previous_version: "1.1.0", + }), + ); + }); + }); + // The catalog cannot say where a csgo-layout release lands, so the node // reports it and the panel opens that rather than a guessed configs folder. it("records where the node says the plugin lives", async () => { @@ -191,6 +286,7 @@ describe("game plugin install state (SQL-driven)", () => { postgres, {} as never, {} as never, + { add: jest.fn() } as never, ); await addNode("node-1"); await request(); diff --git a/test/game-plugin-registry-sync.spec.ts b/test/game-plugin-registry-sync.spec.ts index c771f528..224282f9 100644 --- a/test/game-plugin-registry-sync.spec.ts +++ b/test/game-plugin-registry-sync.spec.ts @@ -22,6 +22,7 @@ describe("game plugin registry sync (SQL-driven)", () => { postgres, {} as never, {} as never, + { add: jest.fn() } as never, ); global.fetch = (async () => ({ From 841863f2f880676d6960ad5d23b5e98ae76de545 Mon Sep 17 00:00:00 2001 From: Luke Policinski Date: Thu, 20 Aug 2026 12:39:07 -0400 Subject: [PATCH 2/3] bug: fix the review findings on the plugin auto update notice --- src/game-plugins/game-plugins.service.spec.ts | 221 ++++++++++++++-- src/game-plugins/game-plugins.service.ts | 243 ++++++++++++++---- .../jobs/NotifyGamePluginUpdate.spec.ts | 94 +++---- .../jobs/NotifyGamePluginUpdate.ts | 122 ++++----- test/game-plugin-install-state.spec.ts | 162 +++++++++++- test/game-plugin-registry-sync.spec.ts | 2 +- 6 files changed, 668 insertions(+), 176 deletions(-) diff --git a/src/game-plugins/game-plugins.service.spec.ts b/src/game-plugins/game-plugins.service.spec.ts index af23f98c..627a437d 100644 --- a/src/game-plugins/game-plugins.service.spec.ts +++ b/src/game-plugins/game-plugins.service.spec.ts @@ -91,8 +91,7 @@ describe("GamePluginsService.slugFrom", () => { // to raise a notice is made here rather than left to whoever reads the panel. describe("GamePluginsService update notices", () => { let postgres: { query: jest.Mock }; - let queue: { add: jest.Mock }; - let pluginRuntime: { getPluginRuntime: jest.Mock }; + let queue: { add: jest.Mock; getJob: jest.Mock }; let service: GamePluginsService; const report = (progress: Record) => @@ -101,19 +100,33 @@ describe("GamePluginsService update notices", () => { const queued = () => queue.add.mock.calls[0]?.[1]; const options = () => queue.add.mock.calls[0]?.[2]; + const installed = (channel = "Auto") => { + postgres.query.mockImplementation(async (sql: string) => + sql.includes("FROM public.game_plugin_installs i") + ? [{ name: "Retakes", channel }] + : [], + ); + }; + beforeEach(() => { postgres = { query: jest.fn(async (): Promise> => []) }; - queue = { add: jest.fn(async (): Promise => undefined) }; - pluginRuntime = { getPluginRuntime: jest.fn(async (): Promise => "swiftlys2") }; + queue = { + add: jest.fn(async (): Promise => undefined), + getJob: jest.fn(async (): Promise => null), + }; service = new GamePluginsService( { warn: jest.fn(), log: jest.fn() } as any, {} as any, postgres as any, - pluginRuntime as any, + { + getPluginRuntime: jest.fn(async (): Promise => "swiftlys2"), + } as any, {} as any, queue as any, ); + + installed(); }); it("raises a notice when a plugin replaced a different version", async () => { @@ -160,9 +173,54 @@ describe("GamePluginsService update notices", () => { error: "digest mismatch", }); + expect(queued()).toEqual( + expect.objectContaining({ outcome: "failed", error: "digest mismatch" }), + ); + }); + + // Deciding this inside the job would complete the job, and a completed job + // holds its id for the whole dedup window -- suppressing the notice for that + // release for a week rather than just this once. + it("decides against a pinned plugin before booking the id", async () => { + installed("Pinned"); + + await report({ + slug: "retakes", + status: "Installed", + version: "1.2.0", + previousVersion: "1.1.0", + }); + + expect(queue.add).not.toHaveBeenCalled(); + }); + + // A pinned install failing is exactly as silent as an auto one. + it("still reports a pinned install failing", async () => { + installed("Pinned"); + + await report({ + slug: "retakes", + status: "Failed", + version: "1.2.0", + error: "404", + }); + expect(queued()).toEqual(expect.objectContaining({ outcome: "failed" })); }); + it("books nothing for a plugin that is no longer installed", async () => { + postgres.query.mockResolvedValue([]); + + await report({ + slug: "retakes", + status: "Installed", + version: "1.2.0", + previousVersion: "1.1.0", + }); + + expect(queue.add).not.toHaveBeenCalled(); + }); + // The whole fleet reports the same release, and a failing install is retried // every five minutes forever. Without one id per release that is a // notification every five minutes per node. @@ -177,6 +235,45 @@ describe("GamePluginsService update notices", () => { expect(options().jobId).toEqual("plugin-updated.retakes.1.2.0"); }); + // Otherwise one notice names whichever node happened to report first and the + // rest of the fleet is silently dropped from it. + it("adds later nodes to the notice already waiting", async () => { + const booked = { + data: { nodes: ["node-1"] }, + updateData: jest.fn(async (): Promise => undefined), + }; + queue.getJob.mockResolvedValue(booked); + + await service.recordNodeProgress("node-2", { + slug: "retakes", + status: "Failed", + version: "1.2.0", + error: "digest mismatch", + } as any); + + expect(booked.updateData).toHaveBeenCalledWith( + expect.objectContaining({ nodes: ["node-1", "node-2"] }), + ); + expect(queue.add).not.toHaveBeenCalled(); + }); + + it("does not list the same node twice", async () => { + const booked = { + data: { nodes: ["node-1"] }, + updateData: jest.fn(async (): Promise => undefined), + }; + queue.getJob.mockResolvedValue(booked); + + await report({ + slug: "retakes", + status: "Failed", + version: "1.2.0", + error: "digest mismatch", + }); + + expect(booked.updateData).not.toHaveBeenCalled(); + }); + it("records the progress even when the notice cannot be queued", async () => { queue.add.mockRejectedValue(new Error("redis is down")); @@ -191,6 +288,27 @@ describe("GamePluginsService update notices", () => { expect(postgres.query).toHaveBeenCalled(); }); + + // A connector that predates the previousVersion field still says what it is + // installing, and the row still holds what it is replacing. + it("reads the replaced version off the row for an older connector", async () => { + postgres.query.mockImplementation(async (sql: string) => { + if (sql.includes("SELECT version, previous_version FROM")) { + return [{ version: "1.1.0" }]; + } + return sql.includes("FROM public.game_plugin_installs i") + ? [{ name: "Retakes", channel: "Auto" }] + : []; + }); + + await report({ slug: "retakes", status: "Installing", version: "1.2.0" }); + + const write = postgres.query.mock.calls.find(([sql]) => + sql.includes("INSERT INTO public.game_server_node_plugins"), + ); + + expect(write[1]).toContain("1.1.0"); + }); }); // Turning auto updates off has to mean "stay where you are". Pinning to the @@ -198,20 +316,53 @@ describe("GamePluginsService update notices", () => { // it. describe("GamePluginsService.setAutoUpdate", () => { let postgres: { query: jest.Mock }; + let responses: Record>; let service: GamePluginsService; - const build = () => { - postgres = { query: jest.fn(async (): Promise> => []) }; + // Order matters: the pin candidate query unnests its runtimes, so it also + // mentions "runtime" and would otherwise answer as the runtime lookup. + const match = (sql: string) => { + if (sql.includes("FROM public.game_server_node_plugins p")) { + return "running"; + } + if (sql.includes("DISTINCT COALESCE(pin_plugin_runtime")) { + return "runtimes"; + } + if (sql.includes("SELECT channel FROM public.game_plugin_installs")) { + return "install"; + } + if (sql.includes("FROM public.game_plugin_versions v")) { + return "publishable"; + } + return "other"; + }; + + beforeEach(() => { + responses = { + install: [{ channel: "Auto" }], + runtimes: [{ runtime: "swiftlys2" }], + running: [{ version: "1.1.0" }], + publishable: [], + other: [], + }; + + postgres = { + query: jest.fn(async (sql: string): Promise> => { + return responses[match(sql)] ?? []; + }), + }; service = new GamePluginsService( { warn: jest.fn(), log: jest.fn() } as any, {} as any, postgres as any, - { getPluginRuntime: jest.fn(async (): Promise => "swiftlys2") } as any, + { + getPluginRuntime: jest.fn(async (): Promise => "swiftlys2"), + } as any, {} as any, - { add: jest.fn() } as any, + { add: jest.fn(), getJob: jest.fn() } as any, ); - }; + }); const update = () => postgres.query.mock.calls.find(([sql]) => @@ -219,31 +370,61 @@ describe("GamePluginsService.setAutoUpdate", () => { ); it("pins to the version the nodes are actually running", async () => { - build(); - postgres.query - .mockResolvedValueOnce([{ channel: "Auto" }]) - .mockResolvedValueOnce([{ version: "1.1.0" }]) - .mockResolvedValue([]); - await service.setAutoUpdate("retakes", false); expect(update()[1]).toEqual(["retakes", "1.1.0"]); }); it("clears the pin when it is turned back on", async () => { - build(); - postgres.query.mockResolvedValueOnce([{ channel: "Pinned" }]); - await service.setAutoUpdate("retakes", true); expect(update()[0]).toContain("'Auto'"); }); it("refuses a plugin that is not installed", async () => { - build(); + responses.install = []; await expect(service.setAutoUpdate("retakes", false)).rejects.toThrow( "not installed", ); }); + + // Pinning to a version one runtime never published makes desiredForNode drop + // the plugin for those nodes, and converge() uninstalls whatever it is not + // sent -- so the wrong pin does not leave a node behind, it wipes the plugin + // off it. + it("refuses when no version covers every runtime in play", async () => { + responses.running = []; + responses.publishable = []; + responses.runtimes = [ + { runtime: "swiftlys2" }, + { runtime: "counterstrikesharp" }, + ]; + + await expect(service.setAutoUpdate("retakes", false)).rejects.toThrow( + "every runtime in use", + ); + }); + + it("falls back to a release every runtime can install", async () => { + responses.running = []; + responses.publishable = [{ version: "1.0.0" }]; + + await service.setAutoUpdate("retakes", false); + + expect(update()[1]).toEqual(["retakes", "1.0.0"]); + }); + + // Pinning down to an older build is as much a change to converge to as + // rolling forward, and without the nudge the toggle looks dead for the five + // minutes until the next poll. + it("nudges the fleet in both directions", async () => { + await service.setAutoUpdate("retakes", false); + + expect( + postgres.query.mock.calls.some(([sql]) => + sql.includes("FROM public.game_server_nodes\n WHERE enabled"), + ), + ).toBe(true); + }); }); diff --git a/src/game-plugins/game-plugins.service.ts b/src/game-plugins/game-plugins.service.ts index 85866070..a69df9ba 100644 --- a/src/game-plugins/game-plugins.service.ts +++ b/src/game-plugins/game-plugins.service.ts @@ -6,7 +6,7 @@ import { NotFoundException, } from "@nestjs/common"; import { InjectQueue } from "@nestjs/bullmq"; -import { Queue } from "bullmq"; +import { Job, Queue } from "bullmq"; import { HasuraService } from "../hasura/hasura.service"; import { PostgresService } from "../postgres/postgres.service"; import { PluginRuntimeService } from "../plugin-runtime/plugin-runtime.service"; @@ -1038,6 +1038,8 @@ export class GamePluginsService { error?: string | null; }, ): Promise { + const previousVersion = await this.previousVersionFor(nodeId, progress); + await this.postgres.query( `INSERT INTO public.game_server_node_plugins (game_server_node_id, plugin_slug, runtime, version, previous_version, @@ -1050,8 +1052,16 @@ export class GamePluginsService { ON CONFLICT (game_server_node_id, plugin_slug) DO UPDATE SET status = EXCLUDED.status, version = COALESCE(EXCLUDED.version, game_server_node_plugins.version), - previous_version = COALESCE( - EXCLUDED.previous_version, game_server_node_plugins.previous_version), + -- Installing opens an attempt and settles what it is replacing, so it + -- overwrites -- including back to null, which is what a reinstall + -- after an uninstall is. Every later report in the same attempt only + -- fills the gap, so a Failed that names no previous version cannot + -- erase the one the attempt started with. + previous_version = CASE + WHEN EXCLUDED.status = 'Installing' THEN EXCLUDED.previous_version + ELSE COALESCE( + EXCLUDED.previous_version, game_server_node_plugins.previous_version) + END, runtime = EXCLUDED.runtime, last_error = EXCLUDED.last_error, installed_at = COALESCE( @@ -1063,11 +1073,50 @@ export class GamePluginsService { progress.version ?? null, progress.status, progress.error ?? null, - progress.previousVersion ?? null, + previousVersion, ], ); - await this.queueUpdateNotice(nodeId, progress); + await this.queueUpdateNotice(nodeId, { ...progress, previousVersion }); + } + + // A connector old enough not to send it still reports the version it is + // moving to, and the row still holds the one it is moving from until this + // statement overwrites it -- so the answer is here to be read. Without this + // the whole feature is inert until every node in the fleet is upgraded. + private async previousVersionFor( + nodeId: string, + progress: { + slug: string; + status: string; + previousVersion?: string | null; + }, + ): Promise { + if (progress.previousVersion !== undefined) { + return progress.previousVersion; + } + + if (progress.status !== "Installing" && progress.status !== "Failed") { + return null; + } + + const [row] = await this.postgres.query< + Array<{ version: string | null; previous_version: string | null }> + >( + `SELECT version, previous_version FROM public.game_server_node_plugins + WHERE game_server_node_id = $1 AND plugin_slug = $2`, + [nodeId, progress.slug], + ); + + // Installing is the report that opens the attempt, so what the row still + // holds is what is being replaced. By the time the attempt fails that has + // already moved into previous_version and `version` is the build that did + // not land. + return ( + (progress.status === "Installing" + ? row?.version + : row?.previous_version) ?? null + ); } // Both outcomes of a version change are silent otherwise: an Auto install @@ -1075,10 +1124,11 @@ export class GamePluginsService { // build it already had while the panel says Failed to whoever happens to // open the page. // - // The job id is the throttle, not a nicety. converge() retries a failing - // install every five minutes forever, and every node reports the same - // release separately -- keyed this way a whole fleet moving at once, or one - // plugin failing all week, is a single notification either way. + // Everything that decides *whether* to notify happens here rather than in + // the job. A job that starts and then returns without sending still counts + // as completed, and a completed job holds its id for the whole dedup window + // -- so a decision made in there does not skip one notice, it suppresses + // every later one for that release too. private async queueUpdateNotice( nodeId: string, progress: { @@ -1086,6 +1136,7 @@ export class GamePluginsService { status: string; version?: string | null; previousVersion?: string | null; + error?: string | null; }, ): Promise { if (!progress.version) { @@ -1101,24 +1152,60 @@ export class GamePluginsService { return; } - const outcome = updated ? "updated" : "failed"; - try { + const [install] = await this.postgres.query< + Array<{ name: string; channel: string }> + >( + `SELECT p.name, i.channel + FROM public.game_plugin_installs i + INNER JOIN public.game_plugins p ON p.slug = i.plugin_slug + WHERE i.plugin_slug = $1 AND i.enabled = true`, + [progress.slug], + ); + + if (!install) { + return; + } + + // A pinned version only ever moves because an admin typed it, and they + // do not need telling what they just did. A pinned install *failing* is + // exactly as silent as an auto one, so that half is not filtered. + if (updated && install.channel !== "Auto") { + return; + } + + const outcome = updated ? "updated" : "failed"; + const jobId = `plugin-${outcome}.${progress.slug}.${progress.version}`; + + // Every node reports the same release separately. The first one to get + // here books the notice; the rest add themselves to it while it sits in + // its delay, which is what makes one notification cover a fleet. + const booked = await this.registryQueue.getJob(jobId); + + if (booked) { + await this.addNodeToNotice(booked, nodeId); + return; + } + await this.registryQueue.add( NotifyGamePluginUpdate.name, { slug: progress.slug, + name: install.name, version: progress.version, previousVersion: progress.previousVersion ?? null, + error: progress.error ?? null, outcome, - nodeId, + nodes: [nodeId], }, { - jobId: `plugin-${outcome}.${progress.slug}.${progress.version}`, - // Long enough for the rest of the fleet to report the same release, - // so the notice can count nodes instead of naming whichever one was - // quickest. + jobId, + // Long enough for the rest of the fleet to report the same release + // and be added to the notice above. delay: 30 * 1000, + // converge() retries a failing install every five minutes forever, + // so without a window this long a bad release is a notification + // every five minutes until somebody fixes it. removeOnComplete: { age: 7 * 24 * 60 * 60 }, // A job that threw must not hold its id for the week: the id is what // suppresses the retry, and suppressing a notice nobody ever got is @@ -1133,6 +1220,19 @@ export class GamePluginsService { } } + // Racy by nature -- two nodes can read the same job before either writes -- + // and deliberately left that way. Losing a node off the end of a list is a + // worse notification; taking a lock to prevent it is a worse system. + private async addNodeToNotice(job: Job, nodeId: string): Promise { + const nodes: Array = job.data?.nodes ?? []; + + if (nodes.includes(nodeId)) { + return; + } + + await job.updateData({ ...job.data, nodes: [...nodes, nodeId] }); + } + // Auto follows the newest release; Pinned stays where it is. Both columns // move together or the channel/version check rejects the row. // @@ -1156,43 +1256,59 @@ export class GamePluginsService { WHERE plugin_slug = $1`, [slug], ); - - this.nudgeNodes(); - - return; + } else { + await this.postgres.query( + `UPDATE public.game_plugin_installs + SET channel = 'Pinned', version = $2, updated_at = now() + WHERE plugin_slug = $1`, + [slug, await this.versionToPin(slug)], + ); } - await this.postgres.query( - `UPDATE public.game_plugin_installs - SET channel = 'Pinned', version = $2, updated_at = now() - WHERE plugin_slug = $1`, - [slug, await this.versionToPin(slug)], - ); + // Both directions, not just the one that rolls forward. Pinning down to an + // older build is just as much a change for a node to converge to, and + // without the nudge it sits there looking like the toggle did nothing + // until the five minute poll comes round. + this.nudgeNodes(); } // What the fleet is on, preferring the version the most nodes report having - // installed. A version no build was ever published for cannot be pinned to: - // desiredForNode would resolve nothing and drop the plugin off the manifest - // entirely. + // installed. + // + // The candidate has to have a build for every runtime in play, not just the + // deployment default. desiredForNode drops a plugin it cannot resolve for a + // node's runtime, and converge() uninstalls anything missing from what it is + // sent -- so pinning to a version one runtime never published does not leave + // those nodes behind, it wipes the plugin off them. private async versionToPin(slug: string): Promise { - const runtime = await this.pluginRuntime.getPluginRuntime(); + const runtimes = await this.runtimesInPlay(); const [running] = await this.postgres.query>( `SELECT p.version FROM public.game_server_node_plugins p WHERE p.plugin_slug = $1 - AND p.detected = true - AND p.status = 'Installed' + -- A hand-placed copy is not a version the panel can pin to: it is + -- whatever an admin dropped on that one node, and it would win a + -- vote it was never a candidate in. + AND p.source = 'managed' AND p.version IS NOT NULL - AND EXISTS ( - SELECT 1 FROM public.game_plugin_versions v - WHERE v.plugin_slug = p.plugin_slug - AND v.runtime = $2 - AND v.version = p.version) + -- needed(runtime), not a bare alias: an unqualified runtime inside + -- the inner query binds to the joined table's own column first, so + -- the check compared a row to itself and passed for everything. + AND NOT EXISTS ( + SELECT 1 FROM unnest($2::text[]) AS needed(runtime) + WHERE NOT EXISTS ( + SELECT 1 FROM public.game_plugin_versions v + WHERE v.plugin_slug = p.plugin_slug + AND v.runtime = needed.runtime + AND v.version = p.version)) GROUP BY p.version - ORDER BY count(*) DESC, max(p.updated_at) DESC + ORDER BY count(*) FILTER ( + WHERE p.detected AND p.status = 'Installed') DESC, + count(*) DESC, + max(p.updated_at) DESC LIMIT 1`, - [slug, runtime], + [slug, runtimes], ); if (running) { @@ -1200,11 +1316,52 @@ export class GamePluginsService { } // Nothing has reported in yet -- a plugin requested minutes ago, or a - // fleet that is entirely offline. The version Auto would hand out right - // now is the honest answer to "where are we". - const resolved = await this.resolveVersion(slug, runtime); + // fleet that is entirely offline. The newest release every runtime in play + // can actually install is the honest answer to "where are we". + const [publishable] = await this.postgres.query>( + `SELECT v.version + FROM public.game_plugin_versions v + WHERE v.plugin_slug = $1 + AND NOT EXISTS ( + SELECT 1 FROM unnest($2::text[]) AS needed(runtime) + WHERE NOT EXISTS ( + SELECT 1 FROM public.game_plugin_versions o + WHERE o.plugin_slug = v.plugin_slug + AND o.runtime = needed.runtime + AND o.version = v.version)) + GROUP BY v.version + ORDER BY bool_or(v.prerelease) ASC, max(v.published_at) DESC + LIMIT 1`, + [slug, runtimes], + ); + + if (!publishable) { + // Refusing is the correct answer rather than a failure to find one: + // there is no version that would survive on every node, so there is + // nothing to pin to that would not uninstall the plugin somewhere. + throw new BadRequestException( + `${slug} has no release published for every runtime in use (${runtimes.join(", ")}), so it cannot be pinned`, + ); + } + + return publishable.version; + } + + // Every runtime a node could ask for a build of, which is not the same as + // the deployment default: a node can pin its own. + private async runtimesInPlay(): Promise> { + const rows = await this.postgres.query>( + `SELECT DISTINCT COALESCE(pin_plugin_runtime, active_plugin_runtime()) + AS runtime + FROM public.game_server_nodes + WHERE enabled = true`, + ); + + if (rows.length === 0) { + return [await this.pluginRuntime.getPluginRuntime()]; + } - return resolved.version; + return rows.map((row) => row.runtime); } private async getNodeIP(nodeId: string): Promise { diff --git a/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts b/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts index da6dce5d..c848c1bf 100644 --- a/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts +++ b/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts @@ -11,13 +11,19 @@ describe("NotifyGamePluginUpdate", () => { const updated = { slug: "retakes", + name: "Retakes", version: "1.2.0", previousVersion: "1.1.0", + error: null as string | null, outcome: "updated", - nodeId: "node-1", + nodes: ["node-1"], }; - const failed = { ...updated, outcome: "failed" }; + const failed = { + ...updated, + outcome: "failed", + error: "digest mismatch", + }; beforeEach(() => { postgres = { query: jest.fn(async (): Promise> => []) }; @@ -31,9 +37,7 @@ describe("NotifyGamePluginUpdate", () => { }); it("names both versions and how far the update reached", async () => { - postgres.query - .mockResolvedValueOnce([{ name: "Retakes", channel: "Auto" }]) - .mockResolvedValueOnce([{ count: "3" }]); + postgres.query.mockResolvedValue([{ count: "3" }]); await run(updated); @@ -43,66 +47,64 @@ describe("NotifyGamePluginUpdate", () => { expect(sent().message).toContain("3 nodes"); }); - // Changing a pinned version is somebody typing it in. Reporting it back to - // them is noise, and it is the case the toggle exists to make explicit. - it("stays quiet about a pinned plugin moving", async () => { - postgres.query.mockResolvedValueOnce([ - { name: "Retakes", channel: "Pinned" }, - ]); - + // A node installing the plugin for the first time is also on the new + // version, and it did not update -- counting it says four nodes updated when + // three did. + it("counts only the nodes that replaced a version", async () => { await run(updated); - expect(notifications.send).not.toHaveBeenCalled(); + const [sql] = postgres.query.mock.calls[0]; + + expect(sql).toContain("previous_version IS NOT NULL"); + expect(sql).toContain("previous_version <> version"); }); - it("says nothing about a plugin that was uninstalled since", async () => { + // The failed row is gone by now: the inventory report at the end of the same + // pass writes it back to Installed at the version that is still on disk. If + // this asked the table anything it would find nothing and say nothing. + it("reports a failure the inventory report has already overwritten", async () => { postgres.query.mockResolvedValue([]); - await run(updated); + await run(failed); - expect(notifications.send).not.toHaveBeenCalled(); + expect(notifications.send).toHaveBeenCalled(); + expect(sent().message).toContain("could not install 1.2.0"); + expect(sent().message).toContain("digest mismatch"); + expect(sent().message).toContain("still running 1.1.0"); }); - // Which nodes failed, and what they are still running, are the two things an - // admin needs before deciding whether it can wait. - it("names the nodes that failed and the version they kept", async () => { - postgres.query - .mockResolvedValueOnce([{ name: "Retakes", channel: "Auto" }]) - .mockResolvedValueOnce([ - { game_server_node_id: "node-1", last_error: "digest mismatch" }, - { game_server_node_id: "node-2", last_error: null }, - ]); + it("names nodes by the label the panel shows", async () => { + postgres.query.mockResolvedValue([ + { label: "rack-a" }, + { label: "rack-b" }, + ]); - await run(failed); + await run({ ...failed, nodes: ["7f3a", "9c1b"] }); - expect(sent().message).toContain("node-1, node-2"); - expect(sent().message).toContain("digest mismatch"); - expect(sent().message).toContain("still running 1.1.0"); + expect(sent().message).toContain("rack-a, rack-b"); + expect(sent().message).not.toContain("7f3a"); }); - // A pinned install failing is just as silent as an auto one, so the channel - // filter deliberately does not apply here. - it("reports a pinned install failing too", async () => { - postgres.query - .mockResolvedValueOnce([{ name: "Retakes", channel: "Pinned" }]) - .mockResolvedValueOnce([ - { game_server_node_id: "node-1", last_error: "404" }, - ]); + it("falls back to the id for a node with no label", async () => { + postgres.query.mockResolvedValue([]); - await run(failed); + await run({ ...failed, nodes: ["node-1"] }); - expect(notifications.send).toHaveBeenCalled(); + expect(sent().message).toContain("node-1"); }); - // converge() retries every five minutes, so by the time this runs the thing - // it is about to report may have already fixed itself. - it("drops a failure that recovered while the notice waited", async () => { - postgres.query - .mockResolvedValueOnce([{ name: "Retakes", channel: "Auto" }]) - .mockResolvedValueOnce([]); + // Both notices are per release rather than per type, so the bell and the + // device thread one release's news together instead of collapsing it onto + // an unrelated node alert. + it("keys each notice to the release it is about", async () => { + await run(updated); + + expect(sent().entity_id).toEqual("game_plugin_updated:retakes:1.2.0"); + }); + it("keeps a failure in its own thread", async () => { await run(failed); - expect(notifications.send).not.toHaveBeenCalled(); + expect(sent().entity_id).toEqual("game_plugin_update_failed:retakes:1.2.0"); }); }); diff --git a/src/game-plugins/jobs/NotifyGamePluginUpdate.ts b/src/game-plugins/jobs/NotifyGamePluginUpdate.ts index 5b8fdf54..edadb51c 100644 --- a/src/game-plugins/jobs/NotifyGamePluginUpdate.ts +++ b/src/game-plugins/jobs/NotifyGamePluginUpdate.ts @@ -9,12 +9,18 @@ import { DISCORD_COLORS } from "../../notifications/utilities/constants"; type UpdateNotice = { slug: string; + name: string; version: string; previousVersion: string | null; + error: string | null; outcome: "updated" | "failed"; - nodeId: string; + nodes: Array; }; +// Whether to notify at all was decided before this was queued. What is left is +// wording it, so there is deliberately no path through here that returns +// without sending: a completed job holds its id for the dedup window, and a +// silent completion would take every later notice for the release with it. @UseQueue("GamePlugins", GamePluginQueues.Registry) export class NotifyGamePluginUpdate extends WorkerHost { constructor( @@ -28,55 +34,40 @@ export class NotifyGamePluginUpdate extends WorkerHost { async process(job: Job): Promise { const notice = job.data; - const [install] = await this.postgres.query< - Array<{ name: string; channel: string }> - >( - `SELECT p.name, i.channel - FROM public.game_plugin_installs i - INNER JOIN public.game_plugins p ON p.slug = i.plugin_slug - WHERE i.plugin_slug = $1 AND i.enabled = true`, - [notice.slug], - ); - - // Uninstalled between the report and this running, thirty seconds later. - if (!install) { - return; - } - if (notice.outcome === "failed") { - await this.notifyFailed(notice, install.name); + await this.notifyFailed(notice); return; } - // A pinned install only changes version because an admin changed it, and - // they do not need telling what they just did. The whole point of the - // notice is the version that moved on its own. - if (install.channel !== "Auto") { - return; - } - - await this.notifyUpdated(notice, install.name); + await this.notifyUpdated(notice); } - private async notifyUpdated( - notice: UpdateNotice, - name: string, - ): Promise { - const [{ count }] = await this.postgres.query>( + private async notifyUpdated(notice: UpdateNotice): Promise { + // Counted off previous_version rather than off who is on the new build: a + // node installing the plugin for the first time is also on it, and it did + // not update. previous_version is the one column the end of pass inventory + // report leaves alone, so it still says so by the time this runs. + const [counted] = await this.postgres.query>( `SELECT count(*) AS count FROM public.game_server_node_plugins - WHERE plugin_slug = $1 AND version = $2 AND status = 'Installed'`, + WHERE plugin_slug = $1 + AND version = $2 + AND previous_version IS NOT NULL + AND previous_version <> version`, [notice.slug, notice.version], ); - const nodes = Number(count); + // Never below what the payload already knows: the nodes that booked this + // notice reported the update themselves. + const nodes = Math.max(Number(counted?.count ?? 0), notice.nodes.length); await this.notifications.send( "GameNodeStatus", { + entity_id: `game_plugin_updated:${notice.slug}:${notice.version}`, title: "Game Plugin Auto-Updated", message: - `${NotificationsService.escapeHtml(name)} auto-updated from ` + + `${NotificationsService.escapeHtml(notice.name)} auto-updated from ` + `${NotificationsService.escapeHtml(notice.previousVersion)} to ` + `${NotificationsService.escapeHtml(notice.version)} on ` + `${nodes === 1 ? "1 node" : `${nodes} nodes`}. ` + @@ -88,45 +79,28 @@ export class NotifyGamePluginUpdate extends WorkerHost { ); } - private async notifyFailed( - notice: UpdateNotice, - name: string, - ): Promise { - const failed = await this.postgres.query< - Array<{ game_server_node_id: string; last_error: string | null }> - >( - `SELECT game_server_node_id, last_error - FROM public.game_server_node_plugins - WHERE plugin_slug = $1 AND version = $2 AND status = 'Failed' - ORDER BY game_server_node_id`, - [notice.slug, notice.version], - ); + // Read off the payload, not off the plugin's rows. A failed update leaves + // the previous version sitting on disk, so the inventory report at the end + // of the same pass writes the row back to Installed at that version with no + // error -- seconds before this runs. The row cannot be asked what failed; + // only the node that failed it can say, and it already did. + private async notifyFailed(notice: UpdateNotice): Promise { + const nodes = await this.labelled(notice.nodes); - // It recovered on a retry while this sat in its delay -- converge() runs - // every five minutes, so that is a real outcome rather than a race. - if (failed.length === 0) { - return; - } - - const error = failed.find((node) => node.last_error)?.last_error; - - const nodes = - failed.length > 3 - ? `${failed.length} nodes` - : failed - .map((node) => - NotificationsService.escapeHtml(node.game_server_node_id), - ) - .join(", "); + const named = + nodes.length > 3 + ? `${nodes.length} nodes` + : nodes.map((node) => NotificationsService.escapeHtml(node)).join(", "); await this.notifications.send( "GameNodeStatus", { + entity_id: `game_plugin_update_failed:${notice.slug}:${notice.version}`, title: "Game Plugin Update Failed", message: - `${NotificationsService.escapeHtml(name)} could not install ` + - `${NotificationsService.escapeHtml(notice.version)} on ${nodes}` + - `${error ? `: ${NotificationsService.escapeHtml(error)}` : ""}. ` + + `${NotificationsService.escapeHtml(notice.name)} could not install ` + + `${NotificationsService.escapeHtml(notice.version)} on ${named}` + + `${notice.error ? `: ${NotificationsService.escapeHtml(notice.error)}` : ""}. ` + (notice.previousVersion ? `They are still running ${NotificationsService.escapeHtml(notice.previousVersion)}. ` : "") + @@ -137,4 +111,22 @@ export class NotifyGamePluginUpdate extends WorkerHost { DISCORD_COLORS.RED, ); } + + // The id is a hostname nobody named; the label is what the panel puts on the + // node everywhere else, so it is what an admin can match to a machine. + private async labelled(nodeIds: Array): Promise> { + if (nodeIds.length === 0) { + return []; + } + + const rows = await this.postgres.query>( + `SELECT COALESCE(label, id) AS label + FROM public.game_server_nodes + WHERE id = ANY($1::text[]) + ORDER BY COALESCE(label, id)`, + [nodeIds], + ); + + return rows.length > 0 ? rows.map((row) => row.label) : nodeIds; + } } diff --git a/test/game-plugin-install-state.spec.ts b/test/game-plugin-install-state.spec.ts index cd66329f..d043523e 100644 --- a/test/game-plugin-install-state.spec.ts +++ b/test/game-plugin-install-state.spec.ts @@ -193,7 +193,7 @@ describe("game plugin install state (SQL-driven)", () => { postgres, {} as never, {} as never, - { add: jest.fn() } as never, + { add: jest.fn(), getJob: jest.fn() } as never, ); const row = async () => { @@ -247,6 +247,46 @@ describe("game plugin install state (SQL-driven)", () => { expect((await row()).installed_at).not.toBeNull(); }); + // A plugin uninstalled and then installed again replaced nothing, and a + // previous version left sticky from the last time round would have the + // notice claim an update that never happened. + it("clears the previous version when a new attempt replaces nothing", async () => { + await addNode("node-1"); + await request(); + + await service().recordNodeProgress("node-1", { + slug: "retakes", + status: "Installed", + version: "1.2.0", + previousVersion: "1.1.0", + }); + + await service().recordNodeProgress("node-1", { + slug: "retakes", + status: "Installing", + version: "1.3.0", + previousVersion: null, + }); + + expect((await row()).previous_version).toBeNull(); + }); + + // A connector too old to send it still says what it is installing, and the + // row still holds what it is replacing. + it("reads the replaced version off the row when it is not reported", async () => { + await addNode("node-1"); + await request(); + await observe("node-1"); + + await service().recordNodeProgress("node-1", { + slug: "retakes", + status: "Installing", + version: "1.2.0", + }); + + expect((await row()).previous_version).toEqual("1.0.0"); + }); + // Reported once at the start of the install and not again on the failure, // which is the report that has to survive for the notice to say what the // node is still running. @@ -277,6 +317,126 @@ describe("game plugin install state (SQL-driven)", () => { }); }); + // Pinning to a version a runtime never published is not a cosmetic mistake: + // desiredForNode cannot resolve it, so the plugin drops off that node's + // manifest and converge() uninstalls it. + describe("the version auto updates are turned off onto", () => { + const service = () => + new GamePluginsService( + { warn: jest.fn(), log: jest.fn() } as never, + {} as never, + postgres, + { + getPluginRuntime: async () => "swiftlys2", + } as never, + {} as never, + { add: jest.fn(), getJob: jest.fn() } as never, + ); + + const publish = async (version: string, runtime = "swiftlys2") => { + await postgres.query( + `INSERT INTO game_plugin_versions + (plugin_slug, runtime, version, url, sha256, layout, published_at) + VALUES ('retakes', $2, $1, 'https://e.test/a.zip', repeat('a', 64), 'csgo', now())`, + [version, runtime], + ); + }; + + // A node pinning its own runtime has to pin the framework build with it, + // and that build has to exist. + const pinRuntime = async (nodeId: string, runtime: string) => { + await postgres.query( + `INSERT INTO plugin_versions (version, runtime, published_at) + VALUES ('0.0.1', $1, now()) ON CONFLICT DO NOTHING`, + [runtime], + ); + await postgres.query( + `UPDATE game_server_nodes + SET pin_plugin_runtime = $2, pin_plugin_version = '0.0.1' + WHERE id = $1`, + [nodeId, runtime], + ); + }; + + const pinned = async () => { + const [row] = await postgres.query>( + `SELECT version FROM game_plugin_installs WHERE plugin_slug = 'retakes'`, + ); + return row.version; + }; + + beforeEach(async () => { + await postgres.query("DELETE FROM game_plugin_versions"); + }); + + it("freezes on what the fleet reports running", async () => { + await addNode("node-1"); + await request(); + await publish("1.0.0"); + await publish("2.0.0"); + await observe("node-1"); + + await service().setAutoUpdate("retakes", false); + + expect(await pinned()).toEqual("1.0.0"); + }); + + // It is whatever an admin dropped on one node by hand, and it was never a + // candidate the panel could hand out. + it("ignores a hand-placed copy", async () => { + await addNode("node-1"); + await addNode("node-2"); + await request(); + await publish("1.0.0"); + await observe("node-1"); + await postgres.query( + `UPDATE game_server_node_plugins SET source = 'manual', version = '9.9.9' + WHERE game_server_node_id = 'node-1'`, + ); + await observe("node-2"); + + await service().setAutoUpdate("retakes", false); + + expect(await pinned()).toEqual("1.0.0"); + }); + + it("refuses a version one runtime in play never published", async () => { + await addNode("node-1"); + await pinRuntime("node-1", "counterstrikesharp"); + await addNode("node-2"); + await request(); + await publish("1.0.0"); + await observe("node-2"); + + await expect(service().setAutoUpdate("retakes", false)).rejects.toThrow( + "every runtime in use", + ); + }); + + it("pins a version every runtime in play published", async () => { + await addNode("node-1"); + await pinRuntime("node-1", "counterstrikesharp"); + await addNode("node-2"); + await request(); + await publish("1.0.0"); + await publish("1.0.0", "counterstrikesharp"); + await observe("node-2"); + + await service().setAutoUpdate("retakes", false); + + expect(await pinned()).toEqual("1.0.0"); + }); + + it("turns back on by clearing the pin", async () => { + await addNode("node-1"); + await request("1.0.0"); + + await service().setAutoUpdate("retakes", true); + + expect(await pinned()).toBeNull(); + }); + }); + // The catalog cannot say where a csgo-layout release lands, so the node // reports it and the panel opens that rather than a guessed configs folder. it("records where the node says the plugin lives", async () => { diff --git a/test/game-plugin-registry-sync.spec.ts b/test/game-plugin-registry-sync.spec.ts index 224282f9..13b5cd69 100644 --- a/test/game-plugin-registry-sync.spec.ts +++ b/test/game-plugin-registry-sync.spec.ts @@ -22,7 +22,7 @@ describe("game plugin registry sync (SQL-driven)", () => { postgres, {} as never, {} as never, - { add: jest.fn() } as never, + { add: jest.fn(), getJob: jest.fn() } as never, ); global.fetch = (async () => ({ From c58b82f0a67df75dd51c1336ad1232feae922b1d Mon Sep 17 00:00:00 2001 From: Luke Policinski Date: Thu, 20 Aug 2026 13:01:09 -0400 Subject: [PATCH 3/3] bug: name every node a bad release breaks, and stop over-refusing a pin --- src/game-plugins/game-plugins.service.spec.ts | 68 +++++++++++++- src/game-plugins/game-plugins.service.ts | 91 +++++++++++++++---- .../jobs/NotifyGamePluginUpdate.spec.ts | 10 +- .../jobs/NotifyGamePluginUpdate.ts | 10 +- test/game-plugin-install-state.spec.ts | 27 +++++- 5 files changed, 174 insertions(+), 32 deletions(-) diff --git a/src/game-plugins/game-plugins.service.spec.ts b/src/game-plugins/game-plugins.service.spec.ts index 627a437d..833325d0 100644 --- a/src/game-plugins/game-plugins.service.spec.ts +++ b/src/game-plugins/game-plugins.service.spec.ts @@ -240,6 +240,7 @@ describe("GamePluginsService update notices", () => { it("adds later nodes to the notice already waiting", async () => { const booked = { data: { nodes: ["node-1"] }, + getState: jest.fn(async (): Promise => "delayed"), updateData: jest.fn(async (): Promise => undefined), }; queue.getJob.mockResolvedValue(booked); @@ -260,6 +261,7 @@ describe("GamePluginsService update notices", () => { it("does not list the same node twice", async () => { const booked = { data: { nodes: ["node-1"] }, + getState: jest.fn(async (): Promise => "delayed"), updateData: jest.fn(async (): Promise => undefined), }; queue.getJob.mockResolvedValue(booked); @@ -274,6 +276,63 @@ describe("GamePluginsService update notices", () => { expect(booked.updateData).not.toHaveBeenCalled(); }); + // updateData succeeds against a completed job, so appending to one edits a + // notice that was sent minutes ago and nothing fires. converge() retries + // forever, so this is the node that stays broken -- and it would never be + // named. + it("gives a node that failed after the notice went out its own", async () => { + const sent = { + data: { nodes: ["node-1"] }, + getState: jest.fn(async (): Promise => "completed"), + updateData: jest.fn(async (): Promise => undefined), + }; + queue.getJob.mockResolvedValue(sent); + + await service.recordNodeProgress("node-2", { + slug: "retakes", + status: "Failed", + version: "1.2.0", + error: "digest mismatch", + } as any); + + expect(sent.updateData).not.toHaveBeenCalled(); + expect(options().jobId).toEqual("plugin-failed.retakes.1.2.0.node-2"); + }); + + // The same node retrying every five minutes is exactly what the window is + // there to swallow. + it("stays quiet for a node already named in a notice that went out", async () => { + const sent = { + data: { nodes: ["node-1"] }, + getState: jest.fn(async (): Promise => "completed"), + updateData: jest.fn(async (): Promise => undefined), + }; + queue.getJob.mockResolvedValue(sent); + + await report({ + slug: "retakes", + status: "Failed", + version: "1.2.0", + error: "digest mismatch", + }); + + expect(queue.add).not.toHaveBeenCalled(); + expect(sent.updateData).not.toHaveBeenCalled(); + }); + + // Nodes poll on their own timers, so a bad release breaks a fleet over a five + // minute spread. Gathering for less names whichever node was quickest. + it("gathers for longer than a convergence interval", async () => { + await report({ + slug: "retakes", + status: "Failed", + version: "1.2.0", + error: "digest mismatch", + }); + + expect(options().delay).toBeGreaterThan(5 * 60 * 1000); + }); + it("records the progress even when the notice cannot be queued", async () => { queue.add.mockRejectedValue(new Error("redis is down")); @@ -322,10 +381,13 @@ describe("GamePluginsService.setAutoUpdate", () => { // Order matters: the pin candidate query unnests its runtimes, so it also // mentions "runtime" and would otherwise answer as the runtime lookup. const match = (sql: string) => { + if (sql.includes("DISTINCT COALESCE(n.pin_plugin_runtime")) { + return "runtimes"; + } if (sql.includes("FROM public.game_server_node_plugins p")) { return "running"; } - if (sql.includes("DISTINCT COALESCE(pin_plugin_runtime")) { + if (sql.includes("DISTINCT COALESCE(n.pin_plugin_runtime")) { return "runtimes"; } if (sql.includes("SELECT channel FROM public.game_plugin_installs")) { @@ -393,7 +455,7 @@ describe("GamePluginsService.setAutoUpdate", () => { // the plugin for those nodes, and converge() uninstalls whatever it is not // sent -- so the wrong pin does not leave a node behind, it wipes the plugin // off it. - it("refuses when no version covers every runtime in play", async () => { + it("refuses when no one release covers the runtimes running it", async () => { responses.running = []; responses.publishable = []; responses.runtimes = [ @@ -402,7 +464,7 @@ describe("GamePluginsService.setAutoUpdate", () => { ]; await expect(service.setAutoUpdate("retakes", false)).rejects.toThrow( - "every runtime in use", + "every runtime running it", ); }); diff --git a/src/game-plugins/game-plugins.service.ts b/src/game-plugins/game-plugins.service.ts index a69df9ba..e10a6e0f 100644 --- a/src/game-plugins/game-plugins.service.ts +++ b/src/game-plugins/game-plugins.service.ts @@ -24,6 +24,10 @@ export class GamePluginsService { // URL -- a disk image, a game build -- fails instead of filling the pod. private static readonly MAX_ARCHIVE_BYTES = 250 * 1024 * 1024; + // One convergence interval plus room for a node that was mid-download when + // the first report landed. PluginSyncService polls every five minutes. + private static readonly NOTICE_GATHER_MS = 6 * 60 * 1000; + private static readonly SLUG = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; private static readonly VERSION = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; @@ -1175,15 +1179,14 @@ export class GamePluginsService { } const outcome = updated ? "updated" : "failed"; - const jobId = `plugin-${outcome}.${progress.slug}.${progress.version}`; + const notice = `plugin-${outcome}.${progress.slug}.${progress.version}`; // Every node reports the same release separately. The first one to get // here books the notice; the rest add themselves to it while it sits in // its delay, which is what makes one notification cover a fleet. - const booked = await this.registryQueue.getJob(jobId); + const jobId = await this.claim(notice, nodeId); - if (booked) { - await this.addNodeToNotice(booked, nodeId); + if (!jobId) { return; } @@ -1200,9 +1203,11 @@ export class GamePluginsService { }, { jobId, - // Long enough for the rest of the fleet to report the same release - // and be added to the notice above. - delay: 30 * 1000, + // Longer than a node's convergence interval on purpose. Nodes poll on + // their own timers, so a release that breaks the fleet breaks it over + // a five minute spread -- gathering for less than that names whichever + // node was quickest and calls it the whole story. + delay: GamePluginsService.NOTICE_GATHER_MS, // converge() retries a failing install every five minutes forever, // so without a window this long a bad release is a notification // every five minutes until somebody fixes it. @@ -1220,6 +1225,40 @@ export class GamePluginsService { } } + // Which id this node should book the notice under, or null if it has nothing + // new to say. + // + // getJob finds a completed job as readily as a waiting one, and updateData + // succeeds against it -- so appending without checking edits a notice that + // was sent minutes ago and nothing fires. That is the case that matters: + // converge() retries forever, so a node that stays broken past the gathering + // window would never be named at all, and the notice that did go out would + // say one node while the fleet was down. + private async claim(notice: string, nodeId: string): Promise { + const booked = await this.registryQueue.getJob(notice); + + if (!booked) { + return notice; + } + + const gathering = ["delayed", "waiting", "waiting-children", "paused"]; + + if (gathering.includes(await booked.getState())) { + await this.addNodeToNotice(booked, nodeId); + return null; + } + + // Already sent. A node that was in it has said all it has to say -- this is + // the five minute retry, and swallowing it is the whole point of the + // window. A node that was not is news, and gets a notice of its own on the + // same throttle: add() is a no-op while that id is still held. + if ((booked.data?.nodes ?? []).includes(nodeId)) { + return null; + } + + return `${notice}.${nodeId}`; + } + // Racy by nature -- two nodes can read the same job before either writes -- // and deliberately left that way. Losing a node off the end of a list is a // worse notification; taking a lock to prevent it is a worse system. @@ -1281,7 +1320,7 @@ export class GamePluginsService { // sent -- so pinning to a version one runtime never published does not leave // those nodes behind, it wipes the plugin off them. private async versionToPin(slug: string): Promise { - const runtimes = await this.runtimesInPlay(); + const runtimes = await this.runtimesInPlay(slug); const [running] = await this.postgres.query>( `SELECT p.version @@ -1340,27 +1379,41 @@ export class GamePluginsService { // there is no version that would survive on every node, so there is // nothing to pin to that would not uninstall the plugin somewhere. throw new BadRequestException( - `${slug} has no release published for every runtime in use (${runtimes.join(", ")}), so it cannot be pinned`, + runtimes.length > 1 + ? `${slug} has no single release published for every runtime running it (${runtimes.join(", ")}), so it cannot be pinned` + : `${slug} has no release to pin to`, ); } return publishable.version; } - // Every runtime a node could ask for a build of, which is not the same as - // the deployment default: a node can pin its own. - private async runtimesInPlay(): Promise> { + // Every runtime a node could ask this plugin for a build of, which is not the + // same as the deployment default: a node can pin its own. + // + // Only the runtimes that already resolve the plugin count. One that has no + // build of it at all is not a node the pin could strand -- desiredForNode + // omits the plugin for it under Auto too, so those nodes have never had it + // and nothing changes by pinning. Counting them anyway meant a single-runtime + // plugin could not have auto updates turned off at all on a fleet where one + // node runs the other framework. + // + // An empty list means nothing to satisfy, and the coverage clauses below + // fall through rather than special-casing it. + private async runtimesInPlay(slug: string): Promise> { const rows = await this.postgres.query>( - `SELECT DISTINCT COALESCE(pin_plugin_runtime, active_plugin_runtime()) + `SELECT DISTINCT COALESCE(n.pin_plugin_runtime, active_plugin_runtime()) AS runtime - FROM public.game_server_nodes - WHERE enabled = true`, + FROM public.game_server_nodes n + WHERE n.enabled = true + AND EXISTS ( + SELECT 1 FROM public.game_plugin_versions v + WHERE v.plugin_slug = $1 + AND v.runtime = COALESCE( + n.pin_plugin_runtime, active_plugin_runtime()))`, + [slug], ); - if (rows.length === 0) { - return [await this.pluginRuntime.getPluginRuntime()]; - } - return rows.map((row) => row.runtime); } diff --git a/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts b/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts index c848c1bf..32e6449d 100644 --- a/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts +++ b/src/game-plugins/jobs/NotifyGamePluginUpdate.spec.ts @@ -50,13 +50,15 @@ describe("NotifyGamePluginUpdate", () => { // A node installing the plugin for the first time is also on the new // version, and it did not update -- counting it says four nodes updated when // three did. - it("counts only the nodes that replaced a version", async () => { + it("counts only the nodes that made the jump it names", async () => { await run(updated); - const [sql] = postgres.query.mock.calls[0]; + const [sql, bindings] = postgres.query.mock.calls[0]; - expect(sql).toContain("previous_version IS NOT NULL"); - expect(sql).toContain("previous_version <> version"); + // A fresh install is on the new version without having updated, and a node + // that came off a different build made a different jump. + expect(sql).toContain("previous_version = $3"); + expect(bindings).toEqual(["retakes", "1.2.0", "1.1.0"]); }); // The failed row is gone by now: the inventory report at the end of the same diff --git a/src/game-plugins/jobs/NotifyGamePluginUpdate.ts b/src/game-plugins/jobs/NotifyGamePluginUpdate.ts index edadb51c..e95f952f 100644 --- a/src/game-plugins/jobs/NotifyGamePluginUpdate.ts +++ b/src/game-plugins/jobs/NotifyGamePluginUpdate.ts @@ -47,14 +47,18 @@ export class NotifyGamePluginUpdate extends WorkerHost { // node installing the plugin for the first time is also on it, and it did // not update. previous_version is the one column the end of pass inventory // report leaves alone, so it still says so by the time this runs. + // + // Matched against the version this notice names, too. A fleet does not have + // to move in step -- some nodes can come off 1.0.0 while others come off + // 1.1.0 -- and counting both makes the notice claim ten nodes made a jump + // that four of them did not. const [counted] = await this.postgres.query>( `SELECT count(*) AS count FROM public.game_server_node_plugins WHERE plugin_slug = $1 AND version = $2 - AND previous_version IS NOT NULL - AND previous_version <> version`, - [notice.slug, notice.version], + AND previous_version = $3`, + [notice.slug, notice.version, notice.previousVersion], ); // Never below what the payload already knows: the nodes that booked this diff --git a/test/game-plugin-install-state.spec.ts b/test/game-plugin-install-state.spec.ts index d043523e..1a74ce3c 100644 --- a/test/game-plugin-install-state.spec.ts +++ b/test/game-plugin-install-state.spec.ts @@ -400,7 +400,11 @@ describe("game plugin install state (SQL-driven)", () => { expect(await pinned()).toEqual("1.0.0"); }); - it("refuses a version one runtime in play never published", async () => { + // That node has never had the plugin: desiredForNode cannot resolve it for + // a runtime with no build either, so nothing is being stranded. Counting it + // meant a single-runtime plugin could not be pinned at all once one node + // ran the other framework. + it("ignores a runtime that has no build of the plugin at all", async () => { await addNode("node-1"); await pinRuntime("node-1", "counterstrikesharp"); await addNode("node-2"); @@ -408,12 +412,29 @@ describe("game plugin install state (SQL-driven)", () => { await publish("1.0.0"); await observe("node-2"); + await service().setAutoUpdate("retakes", false); + + expect(await pinned()).toEqual("1.0.0"); + }); + + // Both runtimes run this plugin, so both have to be able to install what it + // is pinned to -- desiredForNode drops what it cannot resolve and converge() + // uninstalls whatever it is not sent. + it("refuses when no one release covers the runtimes running it", async () => { + await addNode("node-1"); + await pinRuntime("node-1", "counterstrikesharp"); + await addNode("node-2"); + await request(); + await publish("1.0.0"); + await publish("2.0.0", "counterstrikesharp"); + await observe("node-2"); + await expect(service().setAutoUpdate("retakes", false)).rejects.toThrow( - "every runtime in use", + "every runtime running it", ); }); - it("pins a version every runtime in play published", async () => { + it("pins a version every runtime running it published", async () => { await addNode("node-1"); await pinRuntime("node-1", "counterstrikesharp"); await addNode("node-2");