From d7254ceb1937db5116a595e896cc42668b520dfc Mon Sep 17 00:00:00 2001 From: Josh Harris Date: Sat, 12 Sep 2026 10:39:18 +0100 Subject: [PATCH 01/10] Show backend reachability in the UI, fed by the server-list heartbeat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server-list poll (OPE-430) already knows whether the API answers: backendReachable() is null until the first attempt settles, true when the API answered at all (a 404 included), false on a timeout or network error, and every change is announced on the document as "backend-reachability". Nothing consumed it. This is the half a player can see. - ServerList.retryServerList(): one attempt right now, at the player's request, ignoring the retry interval that exists to stop timer-driven callers hammering a down API. Still deduped through fetchOnce(), so a repeat-clicker costs one request. - DesktopStatusBar: an "Offline" state with that Retry, ranked between the session and the update. Above the update because an update failure while the backend is unreachable is a symptom of it -- "Couldn't download the update -- Retry" points at a button that provably cannot work -- and below the session, which names a more specific remedy. Nothing is shown while reachability is unknown: this bar has no neutral state to hang a "Checking…" on, and adding one would put a permanent strip across the bottom of a healthy game. - The multiplayer entry points (GameModeSelector, DetailedGameViewModal) and the join funnel in Main gate on it, on the WEB as well as on desktop -- which is what separates this from the existing update/session gates. The funnel matters because matchmaking, deep links and the host/join modals dispatch join-lobby without passing a dimmed button. On desktop a refusal wiggles the bar that is already naming the reason; on the web, where there is no bar, it shows a transient message instead. Two rules the tests pin: - null never gates. Every page is in that state for its first few hundred milliseconds, and blocking there would lock every player out of multiplayer on every load over a suspicion we have not even tested. - Single-player is never gated, whatever reachability says. Bot games run entirely in-client, and refusing one would break the desktop build's core offline promise. Transport and the in-game flows are untouched: this only affects starting and joining. Consumers seed from the accessor before subscribing to the event, because the event is one-shot and a component that mounts after the first attempt settles would otherwise gate on null forever -- OPE-396's bug, on a new signal. Covered by a test that mounts only after the attempt has failed. No circuit breaker here: OPE-403 can consume this same signal, but this change only exposes the state and the retry. OPE-439 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PTKyUrxqfwKvf2QxAovR6Z --- resources/lang/en.json | 5 + src/client/GameModeSelector.ts | 144 ++++++++-- src/client/Main.ts | 63 ++-- src/client/ServerList.ts | 46 ++- src/client/components/DesktopStatusBar.ts | 126 ++++++-- .../components/DetailedGameViewModal.ts | 41 ++- tests/DesktopStatusBar.test.ts | 171 ++++++++++- tests/GameModeSelectorGating.test.ts | 129 +++++++-- tests/ReachabilityGating.test.ts | 271 ++++++++++++++++++ tests/client/MainInitialize.test.ts | 116 +++++++- tests/client/ServerList.test.ts | 83 ++++++ 11 files changed, 1088 insertions(+), 107 deletions(-) create mode 100644 tests/ReachabilityGating.test.ts diff --git a/resources/lang/en.json b/resources/lang/en.json index 750c7a8cb2..1decbdd819 100644 --- a/resources/lang/en.json +++ b/resources/lang/en.json @@ -581,6 +581,10 @@ "steam_unavailable": "Steam isn't running. Start Steam to play online.", "steam_wedged": "Steam couldn't verify your session. Restarting Steam usually fixes this." }, + "desktop_status": { + "offline": "Offline: can't reach the OpenFront servers", + "retry": "Retry" + }, "desktop_update": { "blocked": "A Steam update is required for the latest version", "downloading": "Downloading update… {percent}%", @@ -675,6 +679,7 @@ } }, "error_modal": { + "backend_unreachable": "Can't reach the OpenFront servers. Check your connection and try again.", "connection_error": "Connection error!", "connection_lost": "Lost connection to the game server and could not reconnect. Refresh the page to try rejoining.", "connection_refused": "Connection refused: {reason}", diff --git a/src/client/GameModeSelector.ts b/src/client/GameModeSelector.ts index 172966dd6f..314f98999e 100644 --- a/src/client/GameModeSelector.ts +++ b/src/client/GameModeSelector.ts @@ -36,7 +36,11 @@ import { showInGameAlert } from "./InGameModal"; import { JoinLobbyModal } from "./JoinLobbyModal"; import { PublicLobbySocket } from "./LobbySocket"; import { JoinLobbyEvent } from "./Main"; -import { isPinnedToAVersion } from "./ServerList"; +import { + backendReachable, + isPinnedToAVersion, + type BackendReachabilityDetail, +} from "./ServerList"; import { SinglePlayerModal } from "./SinglePlayerModal"; import { UsernameInput } from "./UsernameInput"; import { @@ -45,6 +49,7 @@ import { getSecondsUntilServerTimestamp, reloadForUpdate, renderDuration, + showToast, translateText, } from "./Utils"; import { isReplayShellHost } from "./VersionedReplay"; @@ -61,28 +66,86 @@ const TUTORIAL_ACTION = /** The Tutorial card shows beside Solo until the player has played this many games. */ const TUTORIAL_CARD_MAX_GAMES = 5; +/** + * Whether multiplayer should be available given what we know about the + * backend (OPE-439). + * + * ONLY a settled `false` gates. `null` means the server-list heartbeat's + * first attempt has not landed yet -- a page is in that state for its first + * few hundred milliseconds, and blocking there would mean every player is + * briefly locked out of multiplayer on every load, on a suspicion we have not + * even tested. Unknown is not unreachable. + * + * `true` is "the API answered at all", not "the API served a usable list": a + * site with no list is a reachable backend and must not gate. See + * ServerList.backendReachable(). + */ +export function multiplayerAllowedForBackend( + reachable: boolean | null, +): boolean { + return reachable !== false; +} + /** * Whether a multiplayer entry point should refuse to act. Exported for tests * and kept free of component state so the rule is checkable in isolation. - * A null state means that bridge is absent (the web build), so it gates - * nothing; either state alone is enough to block. + * A null update/session means that bridge is absent (the web build), so it + * gates nothing; any one of the three alone is enough to block. + * + * `reachable` is the only one of the three that also applies on the web, + * which is why it is a required parameter rather than an optional one: an + * entry point that forgets to pass it would silently stay ungated, and a + * compile error is the cheapest way to notice. */ export function shouldBlockMultiplayerAction( update: DesktopUpdateState | null, session: DesktopSessionState | null, + reachable: boolean | null, ): boolean { if (update !== null && !multiplayerAllowed(update)) return true; if (session !== null && !multiplayerAllowedForSession(session)) return true; + if (!multiplayerAllowedForBackend(reachable)) return true; return false; } /** - * Whether the desktop gate applies to a given join at all. Single-player runs - * entirely in-client and a replay simulates from an archived record, so - * neither needs a session or an up-to-date build. getTurnstileToken in - * Main.ts exempts the same pair (alongside two conditions irrelevant here), - * and calls this so the two cannot drift. Exported for tests and kept free of - * component state, like shouldBlockMultiplayerAction above. + * Tells the player why a multiplayer action was refused. + * + * On desktop the status bar is already showing the reason and its remedy, so + * the click lands there as a wiggle rather than as a message that would say + * the same thing twice. The web has no status bar, so an unreachable backend + * would refuse in complete silence -- which reads as a broken button -- and + * gets a transient message instead. + * + * Only reachability needs the web half: every other reason to refuse here is + * desktop-only, and on desktop the bar always carries it. + */ +export function reportMultiplayerRefusal(reachable: boolean | null): void { + // Optional-call the method rather than dispatching an event: the bar is a + // sibling custom element that may not have upgraded yet, and `?.wiggle?.()` + // degrades to a silent no-op in that case instead of firing an event with + // no listener. + ( + document.querySelector("desktop-status-bar") as + | (HTMLElement & { wiggle?: () => void }) + | null + )?.wiggle?.(); + // Keyed on the shell, not on the element: is in + // index.html on every build and simply renders nothing on the web, so its + // presence proves nothing about whether the player can see a reason. + if (!isDesktopShell() && reachable === false) { + showToast(translateText("error_modal.backend_unreachable"), "red"); + } +} + +/** + * Whether the multiplayer gate applies to a given join at all. Single-player + * runs entirely in-client and a replay simulates from an archived record, so + * neither needs a session, an up-to-date build, or a backend that is up. + * getTurnstileToken in Main.ts exempts the same pair (alongside two + * conditions irrelevant here), and calls this so the two cannot drift. + * Exported for tests and kept free of component state, like + * shouldBlockMultiplayerAction above. */ export function joinIsGateable(lobby: JoinLobbyEvent): boolean { return ( @@ -93,16 +156,22 @@ export function joinIsGateable(lobby: JoinLobbyEvent): boolean { /** * The whole gate decision for one join, as a pure function so it is testable - * without mounting Main's client. Main adds only the shell check and the - * status-bar wiggle around it. + * without mounting Main's client. Main adds only the shell check (which + * decides whether the two desktop states are even read) and the refusal + * feedback around it. + * + * Named for the join rather than for the desktop since OPE-439: the update + * and session halves are still desktop-only, but an unreachable backend + * refuses a join on the web too. */ -export function shouldBlockDesktopJoin( +export function shouldBlockJoin( lobby: JoinLobbyEvent, update: DesktopUpdateState | null, session: DesktopSessionState | null, + reachable: boolean | null, ): boolean { if (!joinIsGateable(lobby)) return false; - return shouldBlockMultiplayerAction(update, session); + return shouldBlockMultiplayerAction(update, session, reachable); } @customElement("game-mode-selector") @@ -114,6 +183,9 @@ export class GameModeSelector extends LitElement { @state() private viewerSignedIn: boolean = false; @state() private showTrustRequired: boolean = false; @state() private desktopSessionState: DesktopSessionState | null = null; + // Null until the server-list heartbeat's first attempt settles; see + // multiplayerAllowedForBackend for why null never gates. + @state() private backendReachableState: boolean | null = null; private serverTimeOffset: number = 0; private defaultLobbyTime: number = 0; @@ -204,6 +276,15 @@ export class GameModeSelector extends LitElement { "desktop-session-state", this.onDesktopSessionState, ); + // Seeded unconditionally, unlike the two above: the backend is just as + // unreachable on the web, and the heartbeat's first attempt often settles + // before this element exists (it is started in Main's initialize, we are + // rendered by later), so the event alone would miss it. + this.backendReachableState = backendReachable(); + document.addEventListener( + "backend-reachability", + this.onBackendReachability, + ); document.addEventListener("join-lobby", this.onJoinLobby); document.addEventListener("leave-lobby", this.onLeaveLobby); // Pick up the current value in case username-input validated before us. @@ -230,6 +311,10 @@ export class GameModeSelector extends LitElement { "desktop-session-state", this.onDesktopSessionState, ); + document.removeEventListener( + "backend-reachability", + this.onBackendReachability, + ); document.removeEventListener("join-lobby", this.onJoinLobby); document.removeEventListener("leave-lobby", this.onLeaveLobby); super.disconnectedCallback(); @@ -272,6 +357,12 @@ export class GameModeSelector extends LitElement { this.desktopSessionState = (e as CustomEvent).detail; }; + private onBackendReachability = (e: Event) => { + this.backendReachableState = ( + e as CustomEvent + ).detail.reachable; + }; + public stop() { this.lobbySocket.stop(); } @@ -424,8 +515,8 @@ export class GameModeSelector extends LitElement { } /** - * Refuses the action and draws attention to the update bar. Returns true when - * the caller should stop. + * Refuses the action and tells the player why. Returns true when the caller + * should stop. * * Deliberately NOT implemented with the `disabled` attribute the way * renderSmallActionCard handles invalid input: a disabled control (and @@ -433,28 +524,21 @@ export class GameModeSelector extends LitElement { * trigger the wiggle. The button stays clickable and merely stops being * actionable. */ - private blockedByUpdate(): boolean { + private blockedFromMultiplayer(): boolean { if ( !shouldBlockMultiplayerAction( this.desktopUpdateState, this.desktopSessionState, + this.backendReachableState, ) ) return false; - // Optional-call the method rather than dispatching an event: the bar is a - // sibling custom element that may not have upgraded yet, and `?.wiggle?.()` - // degrades to a silent no-op in that case instead of firing an event with - // no listener. - ( - document.querySelector("desktop-status-bar") as - | (HTMLElement & { wiggle?: () => void }) - | null - )?.wiggle?.(); + reportMultiplayerRefusal(this.backendReachableState); return true; } private openRankedMenu = () => { - if (this.blockedByUpdate()) return; + if (this.blockedFromMultiplayer()) return; if (!this.validateUsername()) return; window.showPage?.("page-ranked"); }; @@ -478,13 +562,13 @@ export class GameModeSelector extends LitElement { }; private openHostLobby = () => { - if (this.blockedByUpdate()) return; + if (this.blockedFromMultiplayer()) return; if (!this.validateUsername()) return; (document.querySelector("host-lobby-modal") as HostLobbyModal)?.open(); }; private openJoinLobby = () => { - if (this.blockedByUpdate()) return; + if (this.blockedFromMultiplayer()) return; if (!this.validateUsername()) return; (document.querySelector("join-lobby-modal") as JoinLobbyModal)?.open(); }; @@ -567,6 +651,7 @@ export class GameModeSelector extends LitElement { shouldBlockMultiplayerAction( this.desktopUpdateState, this.desktopSessionState, + this.backendReachableState, ); return html` `; + } + private label(s: DesktopUpdateState) { switch (s.status) { case "downloading": diff --git a/src/client/components/DetailedGameViewModal.ts b/src/client/components/DetailedGameViewModal.ts index c356177dab..6bde053932 100644 --- a/src/client/components/DetailedGameViewModal.ts +++ b/src/client/components/DetailedGameViewModal.ts @@ -12,10 +12,17 @@ import { type DesktopSessionState, type DesktopUpdateState, } from "../DesktopShell"; -import { shouldBlockMultiplayerAction } from "../GameModeSelector"; +import { + reportMultiplayerRefusal, + shouldBlockMultiplayerAction, +} from "../GameModeSelector"; import { JoinLobbyModal } from "../JoinLobbyModal"; import { PublicLobbySocket } from "../LobbySocket"; import { JoinLobbyEvent } from "../Main"; +import { + backendReachable, + type BackendReachabilityDetail, +} from "../ServerList"; import { UsernameInput } from "../UsernameInput"; import { calculateServerTimeOffset, @@ -119,6 +126,9 @@ export class DetailedGameViewModal extends BaseModal { @state() private viewerSignedIn: boolean = false; @state() private showTrustRequired: boolean = false; @state() private desktopSessionState: DesktopSessionState | null = null; + // Null until the server-list heartbeat's first attempt settles; see + // multiplayerAllowedForBackend for why null never gates. + @state() private backendReachableState: boolean | null = null; private serverTimeOffset = 0; private countdownTimer: number | null = null; @@ -190,6 +200,13 @@ export class DetailedGameViewModal extends BaseModal { "desktop-session-state", this.onDesktopSessionState, ); + // Seeded unconditionally, unlike the two above: an unreachable backend + // refuses a join on the web as well as on desktop (OPE-439). + this.backendReachableState = backendReachable(); + document.addEventListener( + "backend-reachability", + this.onBackendReachability, + ); } disconnectedCallback() { @@ -202,6 +219,10 @@ export class DetailedGameViewModal extends BaseModal { "desktop-session-state", this.onDesktopSessionState, ); + document.removeEventListener( + "backend-reachability", + this.onBackendReachability, + ); this.onClose(); super.disconnectedCallback(); } @@ -227,6 +248,12 @@ export class DetailedGameViewModal extends BaseModal { this.desktopSessionState = (e as CustomEvent).detail; }; + private onBackendReachability = (e: Event) => { + this.backendReachableState = ( + e as CustomEvent + ).detail.reachable; + }; + // ---- Slot animation ---- // // When the lobby at the top of a pane starts, the one queued behind it takes @@ -456,6 +483,7 @@ export class DetailedGameViewModal extends BaseModal { blocked: shouldBlockMultiplayerAction( this.desktopUpdateState, this.desktopSessionState, + this.backendReachableState, ), viewerTrusted: this.viewerTrusted, onClick: () => this.join(lobby), @@ -778,19 +806,16 @@ export class DetailedGameViewModal extends BaseModal { * nudges the bar instead of relying on `disabled`, which would swallow the * click. */ - private blockedByUpdate(): boolean { + private blockedFromMultiplayer(): boolean { if ( !shouldBlockMultiplayerAction( this.desktopUpdateState, this.desktopSessionState, + this.backendReachableState, ) ) return false; - ( - document.querySelector("desktop-status-bar") as - | (HTMLElement & { wiggle?: () => void }) - | null - )?.wiggle?.(); + reportMultiplayerRefusal(this.backendReachableState); return true; } @@ -799,7 +824,7 @@ export class DetailedGameViewModal extends BaseModal { // Checked -- and the bar nudged -- before close(): a blocked attempt must // leave the modal open and tell the player why, not vanish silently. This // sits above the hosted/public branch below so both paths are covered. - if (this.blockedByUpdate()) return; + if (this.blockedFromMultiplayer()) return; // Also before close(): the popup explains how to become trusted, so it // must stay on screen with the browser rather than vanish with it. if (!canJoinTrustedLobby(lobby, this.viewerTrusted)) { diff --git a/tests/DesktopStatusBar.test.ts b/tests/DesktopStatusBar.test.ts index d59395e844..3660735a50 100644 --- a/tests/DesktopStatusBar.test.ts +++ b/tests/DesktopStatusBar.test.ts @@ -1,5 +1,12 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ClientEnv } from "../src/client/ClientEnv"; +import "../src/client/components/DesktopStatusBar"; import { barSource } from "../src/client/components/DesktopStatusBar"; +import { + backendReachable, + ensureServerList, + resetServerList, +} from "../src/client/ServerList"; describe("barSource", () => { it("shows nothing when both states are healthy", () => { @@ -7,6 +14,7 @@ describe("barSource", () => { barSource( { status: "current", bytes: 0, total: 0 }, { status: "signed-in" }, + true, ), ).toBe("none"); }); @@ -16,6 +24,7 @@ describe("barSource", () => { barSource( { status: "downloading", bytes: 1, total: 2 }, { status: "signed-in" }, + true, ), ).toBe("update"); }); @@ -38,6 +47,7 @@ describe("barSource", () => { status: "signed-out", reason: "steam-wedged", }, + true, ), ).toBe("session"); }); @@ -58,11 +68,168 @@ describe("barSource", () => { error: { kind: "quota-exceeded", message: "from a newer shell" }, }, { status: "signed-in" }, + true, ), ).toBe("update"); }); it("shows nothing on the web, where neither bridge exists", () => { - expect(barSource(null, null)).toBe("none"); + expect(barSource(null, null, null)).toBe("none"); + }); + + // OPE-439. Reachability sits between the two: below the session, because a + // session failure names a more specific remedy, and above the update, + // because an update failure while the backend is unreachable is a SYMPTOM + // of it -- "Couldn't download the update -- Retry" points at a button that + // provably cannot work until the network is back. + it("shows the offline state over any update state", () => { + expect( + barSource({ status: "current", bytes: 0, total: 0 }, null, false), + ).toBe("reachability"); + expect( + barSource( + { + status: "failed", + bytes: 0, + total: 0, + error: { kind: "network", message: "offline" }, + }, + { status: "signed-in" }, + false, + ), + ).toBe("reachability"); + }); + + it("still shows the session over the offline state", () => { + expect( + barSource(null, { status: "signed-out", reason: "network" }, false), + ).toBe("session"); + }); + + // No neutral state exists in this bar to hang a "Checking…" on, and + // inventing one would put a permanent strip across the bottom of a healthy + // game for the sake of its first few hundred milliseconds. + it("shows nothing while the first attempt has not settled", () => { + expect(barSource(null, { status: "signed-in" }, null)).toBe("none"); + expect( + barSource({ status: "current", bytes: 0, total: 0 }, null, null), + ).toBe("none"); + }); +}); + +/** + * The rendered offline state and its Retry, driven through the REAL + * ServerList module rather than a mock of it: the seed (backendReachable()) + * and the announcement ("backend-reachability") are the two halves this + * feature actually depends on, and a mocked accessor would prove neither. + */ +describe("the rendered offline state", () => { + let fetchMock: ReturnType; + + function mountBar(): HTMLElement & { updateComplete: Promise } { + const bar = document.createElement("desktop-status-bar") as HTMLElement & { + updateComplete: Promise; + }; + document.body.appendChild(bar); + return bar; + } + + function retryButton(bar: HTMLElement): HTMLButtonElement | undefined { + return Array.from(bar.querySelectorAll("button")).find((b) => + b.textContent?.includes("desktop_status.retry"), + ); + } + + beforeEach(() => { + // The bar renders nothing on the web, so every assertion here needs a + // shell. No `update` bridge on it: a shell too old to expose one must + // still show this. + (window as { openfrontDesktop?: unknown }).openfrontDesktop = {}; + window.BOOTSTRAP_CONFIG = { + gameEnv: "dev", + numWorkers: 1, + turnstileSiteKey: "", + jwtAudience: "test", + instanceId: "test", + gitCommit: "test", + serverHost: "openfront.io", + } as unknown as typeof window.BOOTSTRAP_CONFIG; + ClientEnv.reset(); + resetServerList(); + fetchMock = vi.fn(async () => { + throw new TypeError("network down"); + }); + vi.stubGlobal("fetch", fetchMock); + vi.spyOn(console, "warn").mockImplementation(() => {}); + vi.spyOn(console, "info").mockImplementation(() => {}); + }); + + afterEach(() => { + document.body.innerHTML = ""; + (window as { openfrontDesktop?: unknown }).openfrontDesktop = undefined; + window.BOOTSTRAP_CONFIG = undefined; + ClientEnv.reset(); + resetServerList(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + it("shows nothing while the backend is fine", async () => { + // The control: the bar must not become a permanent fixture just because + // this feature exists. + fetchMock.mockImplementation( + async () => new Response("{}", { status: 404 }), + ); + await ensureServerList(); + expect(backendReachable()).toBe(true); + + const bar = mountBar(); + await bar.updateComplete; + expect(bar.textContent?.trim()).toBe(""); + }); + + it("seeds the offline state from an attempt that failed before it mounted", async () => { + await ensureServerList(); + expect(backendReachable()).toBe(false); + + // Mounted AFTER the announcement it would have needed. The accessor is + // the only path left, exactly as in OPE-396. + const bar = mountBar(); + await bar.updateComplete; + + expect(bar.textContent).toContain("desktop_status.offline"); + expect(retryButton(bar)).toBeDefined(); + }); + + it("picks the offline state up from the announcement when it mounts first", async () => { + const bar = mountBar(); + await bar.updateComplete; + expect(bar.textContent?.trim()).toBe(""); + + await ensureServerList(); + await bar.updateComplete; + + expect(bar.textContent).toContain("desktop_status.offline"); + }); + + it("Retry attempts again immediately, and the bar clears when the API answers", async () => { + await ensureServerList(); + const bar = mountBar(); + await bar.updateComplete; + expect(fetchMock).toHaveBeenCalledTimes(1); + + // A 404 is an answer: this site has no list, but the backend is up. That + // is the boundary the bar keys on, so it is the one worth clearing on. + fetchMock.mockImplementation( + async () => new Response("{}", { status: 404 }), + ); + retryButton(bar)!.click(); + + // Immediately, without waiting out the heartbeat's retry interval -- the + // whole point of the button. + expect(fetchMock).toHaveBeenCalledTimes(2); + await vi.waitFor(() => expect(backendReachable()).toBe(true)); + await bar.updateComplete; + expect(bar.textContent?.trim()).toBe(""); }); }); diff --git a/tests/GameModeSelectorGating.test.ts b/tests/GameModeSelectorGating.test.ts index 2792886319..568d76a9da 100644 --- a/tests/GameModeSelectorGating.test.ts +++ b/tests/GameModeSelectorGating.test.ts @@ -1,14 +1,15 @@ import { describe, expect, it } from "vitest"; import { joinIsGateable, - shouldBlockDesktopJoin, + multiplayerAllowedForBackend, + shouldBlockJoin, shouldBlockMultiplayerAction, } from "../src/client/GameModeSelector"; import { GameType } from "../src/core/game/Game"; describe("shouldBlockMultiplayerAction", () => { it("allows everything when no desktop update state has arrived", () => { - expect(shouldBlockMultiplayerAction(null, null)).toBe(false); + expect(shouldBlockMultiplayerAction(null, null, null)).toBe(false); }); it("allows multiplayer when the client is current", () => { @@ -16,6 +17,7 @@ describe("shouldBlockMultiplayerAction", () => { shouldBlockMultiplayerAction( { status: "current", bytes: 0, total: 0 }, null, + null, ), ).toBe(false); }); @@ -29,12 +31,14 @@ describe("shouldBlockMultiplayerAction", () => { total: 2, }, null, + null, ), ).toBe(true); expect( shouldBlockMultiplayerAction( { status: "staged", bytes: 2, total: 2 }, null, + null, ), ).toBe(true); }); @@ -44,6 +48,7 @@ describe("shouldBlockMultiplayerAction", () => { shouldBlockMultiplayerAction( { status: "blocked", bytes: 0, total: 0 }, null, + null, ), ).toBe(false); }); @@ -58,13 +63,21 @@ describe("shouldBlockMultiplayerAction", () => { }); it("blocks a failed check when Retry is a real remedy", () => { - expect(shouldBlockMultiplayerAction(failed("network"), null)).toBe(true); - expect(shouldBlockMultiplayerAction(failed("verify"), null)).toBe(true); + expect(shouldBlockMultiplayerAction(failed("network"), null, null)).toBe( + true, + ); + expect(shouldBlockMultiplayerAction(failed("verify"), null, null)).toBe( + true, + ); }); it("does not block failures no player-side action can change", () => { - expect(shouldBlockMultiplayerAction(failed("refused"), null)).toBe(false); - expect(shouldBlockMultiplayerAction(failed("parse"), null)).toBe(false); + expect(shouldBlockMultiplayerAction(failed("refused"), null, null)).toBe( + false, + ); + expect(shouldBlockMultiplayerAction(failed("parse"), null, null)).toBe( + false, + ); }); }); @@ -73,16 +86,24 @@ describe("shouldBlockMultiplayerAction with a session", () => { it("does not block when both are healthy", () => { expect( - shouldBlockMultiplayerAction(healthyUpdate, { status: "signed-in" }), + shouldBlockMultiplayerAction( + healthyUpdate, + { status: "signed-in" }, + null, + ), ).toBe(false); }); it("blocks on a signed-out session even when the update is current", () => { expect( - shouldBlockMultiplayerAction(healthyUpdate, { - status: "signed-out", - reason: "steam-wedged", - }), + shouldBlockMultiplayerAction( + healthyUpdate, + { + status: "signed-out", + reason: "steam-wedged", + }, + null, + ), ).toBe(true); }); @@ -93,12 +114,53 @@ describe("shouldBlockMultiplayerAction with a session", () => { { status: "signed-in", }, + null, ), ).toBe(true); }); it("does not block on the web, where neither state exists", () => { - expect(shouldBlockMultiplayerAction(null, null)).toBe(false); + expect(shouldBlockMultiplayerAction(null, null, null)).toBe(false); + }); +}); + +describe("multiplayerAllowedForBackend", () => { + it("allows multiplayer before the first attempt has settled", () => { + // OPE-439's central rule: unknown is not unreachable. Every page is in + // this state for its first few hundred milliseconds, and gating there + // would lock every player out of multiplayer on every load. + expect(multiplayerAllowedForBackend(null)).toBe(true); + }); + + it("allows multiplayer when the API answered", () => { + // "Answered" and not "served a usable list": a site with no list at all + // still proves the backend is up. + expect(multiplayerAllowedForBackend(true)).toBe(true); + }); + + it("blocks multiplayer once an attempt has failed outright", () => { + expect(multiplayerAllowedForBackend(false)).toBe(false); + }); +}); + +describe("shouldBlockMultiplayerAction with backend reachability", () => { + it("blocks on the web, where both desktop states are absent", () => { + expect(shouldBlockMultiplayerAction(null, null, false)).toBe(true); + }); + + it("does not block on an unknown or reachable backend", () => { + expect(shouldBlockMultiplayerAction(null, null, null)).toBe(false); + expect(shouldBlockMultiplayerAction(null, null, true)).toBe(false); + }); + + it("still blocks on a desktop reason while the backend is fine", () => { + expect( + shouldBlockMultiplayerAction( + { status: "staged", bytes: 0, total: 0 }, + { status: "signed-in" }, + true, + ), + ).toBe(true); }); }); @@ -141,7 +203,7 @@ describe("joinIsGateable", () => { }); }); -describe("shouldBlockDesktopJoin", () => { +describe("shouldBlockJoin", () => { const mp = { gameID: "g", source: "matchmaking" } as any; const solo = { gameID: "g", @@ -151,42 +213,65 @@ describe("shouldBlockDesktopJoin", () => { const healthy = { status: "current", bytes: 0, total: 0 } as const; it("allows a multiplayer join when both states are healthy", () => { - expect(shouldBlockDesktopJoin(mp, healthy, { status: "signed-in" })).toBe( + expect(shouldBlockJoin(mp, healthy, { status: "signed-in" }, true)).toBe( false, ); }); it("blocks a multiplayer join when signed out", () => { expect( - shouldBlockDesktopJoin(mp, healthy, { - status: "signed-out", - reason: "steam-wedged", - }), + shouldBlockJoin( + mp, + healthy, + { + status: "signed-out", + reason: "steam-wedged", + }, + true, + ), ).toBe(true); }); // The claim that update-gating inherits the funnel fix, actually pinned. it("blocks a multiplayer join on a pending update even when signed in", () => { expect( - shouldBlockDesktopJoin( + shouldBlockJoin( mp, { status: "staged", bytes: 0, total: 0 }, { status: "signed-in" }, + true, ), ).toBe(true); }); it("never blocks single-player, whatever the states say", () => { expect( - shouldBlockDesktopJoin( + shouldBlockJoin( solo, { status: "staged", bytes: 0, total: 0 }, { status: "signed-out", reason: "steam-wedged" }, + false, ), ).toBe(false); }); - it("does not block on the web, where neither state exists", () => { - expect(shouldBlockDesktopJoin(mp, null, null)).toBe(false); + it("does not block on the web, where neither desktop state exists", () => { + expect(shouldBlockJoin(mp, null, null, true)).toBe(false); + // Nor before the heartbeat's first attempt has settled. + expect(shouldBlockJoin(mp, null, null, null)).toBe(false); + }); + + // OPE-439. The one input that gates on the web as well as on desktop. + it("blocks a multiplayer join while the backend is unreachable", () => { + expect(shouldBlockJoin(mp, null, null, false)).toBe(true); + expect(shouldBlockJoin(mp, healthy, { status: "signed-in" }, false)).toBe( + true, + ); + }); + + it("never blocks single-player on an unreachable backend", () => { + // The desktop build's core offline promise: bot games run entirely + // in-client, so an unreachable backend is no reason to refuse one. + expect(shouldBlockJoin(solo, null, null, false)).toBe(false); }); }); diff --git a/tests/ReachabilityGating.test.ts b/tests/ReachabilityGating.test.ts new file mode 100644 index 0000000000..20ebaa4b8f --- /dev/null +++ b/tests/ReachabilityGating.test.ts @@ -0,0 +1,271 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ClientEnv } from "../src/client/ClientEnv"; +import { + backendReachable, + ensureServerList, + resetServerList, +} from "../src/client/ServerList"; +import { GameMapType, GameMode } from "../src/core/game/Game"; +import type { + GameConfig, + PublicGameInfo, + PublicGames, +} from "../src/core/Schemas"; + +// The component opens a public-lobby WebSocket the moment it connects. jsdom +// has no WebSocket worth talking to and this file is about the gate, not the +// lobby list, so the socket is a no-op -- except for retaining the update +// callback the real socket would drive off the wire, which is the only way a +// public-lobby card (one of the gated entry points) ever renders. +const { lobbiesCallbackRef } = vi.hoisted(() => ({ + lobbiesCallbackRef: { current: null as ((g: PublicGames) => void) | null }, +})); + +vi.mock("../src/client/LobbySocket", () => ({ + PublicLobbySocket: class { + constructor(onUpdate: (g: PublicGames) => void) { + lobbiesCallbackRef.current = onUpdate; + } + start(): void {} + stop(): void {} + }, +})); + +// Registers as a side effect. +import "../src/client/GameModeSelector"; + +/** + * OPE-439. The server-list heartbeat already knows whether the API answers; + * this is the half that turns that into something the player can see, on the + * WEB as well as on desktop -- which is what separates it from the desktop + * update/session gates the sibling files cover. + * + * Everything here runs against the real ServerList module, driven by a + * stubbed fetch. A mocked backendReachable() would prove the call sites + * consult *something*, but not that the signal the heartbeat actually + * produces is the one they consult, nor that a component mounting after the + * first attempt settles can still find it. + */ +let selector: HTMLElement & { updateComplete: Promise }; +let joinOpen: ReturnType; +let hostOpen: ReturnType; +let wiggle: ReturnType; +let joinLobby: ReturnType; +let messages: string[]; +let fetchMock: ReturnType; + +function stub(tag: string, methods: Record): void { + const el = document.createElement(tag); + Object.assign(el, methods); + document.body.appendChild(el); +} + +function publicLobby(gameID: string): PublicGameInfo { + return { + gameID, + numClients: 3, + publicGameType: "ffa", + gameConfig: { + gameMap: GameMapType.World, + gameMode: GameMode.FFA, + maxPlayers: 8, + } as unknown as GameConfig, + }; +} + +/** Mounts with one rendered public-lobby card. */ +async function mountSelector(): Promise< + HTMLElement & { updateComplete: Promise } +> { + const el = document.createElement("game-mode-selector") as HTMLElement & { + updateComplete: Promise; + }; + document.body.appendChild(el); + await el.updateComplete; + lobbiesCallbackRef.current?.({ + serverTime: Date.now(), + games: { ffa: [publicLobby("public-1")] }, + }); + await el.updateComplete; + return el; +} + +/** Clicks every button the selector renders. Returns how many it clicked. */ +function clickEveryButton(): number { + const buttons = Array.from(selector.querySelectorAll("button")); + for (const button of buttons) button.click(); + return buttons.length; +} + +/** Announces a reachability change the way the heartbeat does. */ +async function announce(reachable: boolean): Promise { + document.dispatchEvent( + new CustomEvent("backend-reachability", { detail: { reachable } }), + ); + await selector.updateComplete; +} + +beforeEach(() => { + // connectedCallback reads ClientEnv.gameCreationRate(), which throws + // without the config the server injects into index.html. No serverHost and + // no openfrontDesktop: this is the web build, where the update and session + // gates do not exist and reachability is the only one that can fire. + window.BOOTSTRAP_CONFIG = { + gameEnv: "dev", + numWorkers: 1, + turnstileSiteKey: "", + jwtAudience: "test", + instanceId: "test", + gitCommit: "test", + }; + ClientEnv.reset(); + resetServerList(); + + fetchMock = vi.fn(async () => new Response("{}", { status: 404 })); + vi.stubGlobal("fetch", fetchMock); + vi.spyOn(console, "warn").mockImplementation(() => {}); + vi.spyOn(console, "info").mockImplementation(() => {}); + + joinOpen = vi.fn(); + hostOpen = vi.fn(); + wiggle = vi.fn(); + joinLobby = vi.fn(); + stub("join-lobby-modal", { open: joinOpen }); + stub("host-lobby-modal", { open: hostOpen }); + stub("single-player-modal", { open: vi.fn() }); + // Present on the web too -- index.html mounts it on every build and it + // simply renders nothing there -- which is exactly why the web message + // cannot key on whether this element exists. + stub("desktop-status-bar", { wiggle }); + (window as { showPage?: (id: string) => void }).showPage = vi.fn(); + document.addEventListener("join-lobby", joinLobby as EventListener); + + messages = []; + window.addEventListener("show-message", (e) => { + messages.push((e as CustomEvent).detail?.message); + }); + + lobbiesCallbackRef.current = null; +}); + +afterEach(() => { + document.removeEventListener("join-lobby", joinLobby as EventListener); + document.body.innerHTML = ""; + window.BOOTSTRAP_CONFIG = undefined; + ClientEnv.reset(); + resetServerList(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe("the multiplayer entry points while the backend is unreachable", () => { + it("lets everything through before the first attempt has settled", async () => { + // The rule that matters most: a player must never be locked out of + // multiplayer on a suspicion we have not even tested yet. Every page is + // in this state for its first few hundred milliseconds. + selector = await mountSelector(); + expect(backendReachable()).toBe(null); + + expect(clickEveryButton()).toBeGreaterThan(0); + + expect(joinOpen).toHaveBeenCalled(); + expect(hostOpen).toHaveBeenCalled(); + expect( + selector.querySelectorAll('button[aria-disabled="true"]').length, + ).toBe(0); + expect(messages).toEqual([]); + }); + + it("dims and refuses every entry point once an attempt fails", async () => { + selector = await mountSelector(); + await announce(false); + + expect( + selector.querySelectorAll('button[aria-disabled="true"]').length, + ).toBeGreaterThan(0); + clickEveryButton(); + + expect(joinOpen).not.toHaveBeenCalled(); + expect(hostOpen).not.toHaveBeenCalled(); + expect(joinLobby).not.toHaveBeenCalled(); + }); + + it("says why, on the web, where there is no status bar to read", async () => { + selector = await mountSelector(); + await announce(false); + + clickEveryButton(); + + // Refusing silently would look like a broken button, and unlike the + // desktop gates there is nothing else on screen naming the reason. + expect(messages).toContain("error_modal.backend_unreachable"); + }); + + it("re-enables everything when the backend comes back", async () => { + selector = await mountSelector(); + await announce(false); + clickEveryButton(); + expect(joinOpen).not.toHaveBeenCalled(); + + await announce(true); + + expect( + selector.querySelectorAll('button[aria-disabled="true"]').length, + ).toBe(0); + expect(clickEveryButton()).toBeGreaterThan(0); + expect(joinOpen).toHaveBeenCalled(); + expect(hostOpen).toHaveBeenCalled(); + }); + + it("leaves single-player alone", async () => { + selector = await mountSelector(); + const soloOpen = vi.fn(); + ( + document.querySelector("single-player-modal") as unknown as { + open: () => void; + } + ).open = soloOpen; + await announce(false); + + clickEveryButton(); + + // Bot games run entirely in-client: an unreachable backend is no reason + // to refuse one, and refusing would break the desktop build's core + // offline promise. + expect(soloOpen).toHaveBeenCalled(); + }); + + it("gates a selector that mounted after the attempt had already failed", async () => { + // The seed half. No "backend-reachability" event is dispatched anywhere + // below: the only one this document will ever see fired while nothing + // was listening, so the accessor is the sole path by which the selector + // can know. This is OPE-396's bug, on a new signal. + fetchMock.mockImplementation(async () => { + throw new TypeError("network down"); + }); + await ensureServerList(); + expect(backendReachable()).toBe(false); + + selector = await mountSelector(); + + expect( + selector.querySelectorAll('button[aria-disabled="true"]').length, + ).toBeGreaterThan(0); + clickEveryButton(); + expect(joinOpen).not.toHaveBeenCalled(); + expect(hostOpen).not.toHaveBeenCalled(); + }); + + it("does not gate a selector that mounted after an attempt SUCCEEDED", async () => { + // The control for the seed: a 404 is an answer, so a site with no list + // at all is still a reachable backend. + await ensureServerList(); + expect(backendReachable()).toBe(true); + + selector = await mountSelector(); + + expect(clickEveryButton()).toBeGreaterThan(0); + expect(joinOpen).toHaveBeenCalled(); + expect(hostOpen).toHaveBeenCalled(); + }); +}); diff --git a/tests/client/MainInitialize.test.ts b/tests/client/MainInitialize.test.ts index 37dc8e6067..d57fe3932f 100644 --- a/tests/client/MainInitialize.test.ts +++ b/tests/client/MainInitialize.test.ts @@ -8,9 +8,10 @@ */ import fs from "node:fs"; import path from "node:path"; -import { beforeAll, describe, expect, it, vi } from "vitest"; +import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; import { capturePagePin } from "../../src/client/PagePin"; import { SendKickPlayerIntentEvent } from "../../src/client/Transport"; +import { translateText } from "../../src/client/Utils"; import { EventBus } from "../../src/core/EventBus"; const mocks = vi.hoisted(() => ({ @@ -387,4 +388,117 @@ describe("Client.initialize() booted from Main.ts module scope", () => { ), ); }); + + /** + * OPE-439. The entry-point components dim their own buttons, but every join + * -- matchmaking's, a deep link, the host/join modals -- funnels through + * handleJoinLobby without passing one, so the funnel gate is the only thing + * that covers them. This is a WEB boot (no openfrontDesktop), which is + * precisely what the pre-existing desktop gate could not cover. + * + * Driven through the real ServerList module: the reachability the funnel + * reads has to be the one the heartbeat actually produces. + */ + describe("the join funnel while the backend is unreachable", () => { + let ServerList: typeof import("../../src/client/ServerList"); + let messages: string[]; + let onMessage: EventListener; + + /** Replaces the fetch stub and re-settles the heartbeat's first attempt. */ + async function settleReachability(reachable: boolean): Promise { + vi.stubGlobal( + "fetch", + vi.fn(async () => { + if (!reachable) throw new TypeError("network down"); + return { + ok: false, + status: 404, + statusText: "Not Found", + headers: new Map(), + json: async () => ({}), + text: async () => "", + arrayBuffer: async () => new ArrayBuffer(0), + }; + }), + ); + ServerList.resetServerList(); + await ServerList.ensureServerList(); + expect(ServerList.backendReachable()).toBe(reachable); + } + + beforeAll(async () => { + ServerList = await import("../../src/client/ServerList"); + // Test 3 above left the username gate closed; every join below has to + // get past it to reach the gate under test. + const input = document.querySelector("username-input") as unknown as { + canPlay: () => boolean; + }; + input.canPlay = () => true; + messages = []; + onMessage = (e: Event) => { + messages.push((e as CustomEvent).detail?.message); + }; + window.addEventListener("show-message", onMessage); + }); + + afterAll(() => { + window.removeEventListener("show-message", onMessage); + ServerList.resetServerList(); + }); + + it("refuses a join and says why", async () => { + await settleReachability(false); + logSpy.mockClear(); + messages.length = 0; + + document.dispatchEvent( + new CustomEvent("join-lobby", { + detail: { gameID: "AbCd1234", source: "matchmaking" }, + bubbles: true, + }), + ); + + await vi.waitFor(() => + expect(messages).toContain( + translateText("error_modal.backend_unreachable"), + ), + ); + // Refused before anything was joined -- not merely reported after. + expect(logSpy).not.toHaveBeenCalledWith( + expect.stringContaining("joining lobby"), + ); + expect(mocks.joinLobby).not.toHaveBeenCalled(); + }); + + it("lets the same join through once the backend answers", async () => { + // The control. Without it a join refused for any unrelated reason -- + // the username gate, a listener that never ran -- would pass above. + await settleReachability(true); + logSpy.mockClear(); + messages.length = 0; + mocks.joinLobby.mockReturnValue({ + // Neither settles: the assertion is that the join was ATTEMPTED, and + // the in-game path beyond it is not what this file boots. + prestart: new Promise(() => {}), + join: new Promise(() => {}), + stop: vi.fn(), + }); + + document.dispatchEvent( + new CustomEvent("join-lobby", { + detail: { gameID: "AbCd1234", source: "matchmaking" }, + bubbles: true, + }), + ); + + await vi.waitFor(() => + expect(logSpy).toHaveBeenCalledWith( + expect.stringContaining("joining lobby"), + ), + ); + expect(messages).not.toContain( + translateText("error_modal.backend_unreachable"), + ); + }); + }); }); diff --git a/tests/client/ServerList.test.ts b/tests/client/ServerList.test.ts index 67a0cdea9e..a5f2df2641 100644 --- a/tests/client/ServerList.test.ts +++ b/tests/client/ServerList.test.ts @@ -7,6 +7,7 @@ import { redirectToGameVersion, reloadWouldRescue, resetServerList, + retryServerList, serverListSite, serverListUrl, startServerListPolling, @@ -456,6 +457,88 @@ describe("backend reachability", () => { }); }); +describe("retryServerList", () => { + // The Retry on the desktop status bar's offline state (OPE-439). The retry + // interval exists to stop timer-driven callers hammering a down API between + // heartbeats; a player pressing a button is not one of those, and making + // them wait up to 10s for anything to happen would make the button look + // broken in exactly the situation it exists for. + it("attempts immediately, inside the interval that holds ensureServerList back", async () => { + vi.useFakeTimers(); + fetchMock.mockRejectedValue(new TypeError("network down")); + expect(await ensureServerList()).toBe("fallback"); + expect(fetchMock).toHaveBeenCalledTimes(1); + + // The control: an ordinary caller in the same moment is held back. + expect(await ensureServerList()).toBe("fallback"); + expect(fetchMock).toHaveBeenCalledTimes(1); + + expect(await retryServerList()).toBe("fallback"); + expect(fetchMock).toHaveBeenCalledTimes(2); + + // And again, still well inside the interval. + await vi.advanceTimersByTimeAsync(100); + await retryServerList(); + expect(fetchMock).toHaveBeenCalledTimes(3); + }); + + it("applies a list that the retry brings back, and clears the offline state", async () => { + vi.useFakeTimers(); + const seen: unknown[] = []; + const listener = (e: Event) => seen.push((e as CustomEvent).detail); + document.addEventListener("backend-reachability", listener); + try { + fetchMock.mockRejectedValue(new TypeError("network down")); + expect(await ensureServerList()).toBe("fallback"); + expect(backendReachable()).toBe(false); + // The page's own values are in charge while the API is unreachable. + expect(ClientEnv.serverWsBase()).toBe("wss://blue.openfront.io"); + + fetchMock.mockImplementation(async () => jsonResponse(API_LIST)); + expect(await retryServerList()).toBe("api"); + expect(backendReachable()).toBe(true); + // Not just the flag: the list the retry fetched is applied, which is + // what makes the bar disappear AND what the next join will use. + expect(ClientEnv.serverWsBase()).toBe("wss://falk2-b.openfront.io"); + expect(seen).toEqual([{ reachable: false }, { reachable: true }]); + } finally { + document.removeEventListener("backend-reachability", listener); + } + }); + + // A repeat-clicker, or a click landing on top of a heartbeat beat, must + // cost one request rather than one per click. + it("joins an attempt already in flight instead of starting a second", async () => { + vi.useFakeTimers(); + let release: (r: Response) => void = () => {}; + fetchMock.mockImplementation( + async () => + await new Promise((resolve) => { + release = resolve; + }), + ); + + const first = retryServerList(); + const second = retryServerList(); + const third = ensureServerList(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + release(jsonResponse(API_LIST)); + expect(await first).toBe("api"); + expect(await second).toBe("api"); + expect(await third).toBe("api"); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("never throws, whatever the fetch does", async () => { + fetchMock.mockImplementation(() => { + throw new Error("fetch itself blew up"); + }); + expect(await retryServerList()).toBe("fallback"); + expect(backendReachable()).toBe(false); + }); +}); + // Today's rollover feel, kept: a player on build X keeps playing on X's // server after Y is released, until they refresh. So the pick prefers an // `open` server on this build, falls back to a `draining` one on this From 59858f70bceb9b5996c9fe9f7bd767d474269aad Mon Sep 17 00:00:00 2001 From: Josh Harris Date: Sat, 12 Sep 2026 11:09:56 +0100 Subject: [PATCH 02/10] Assert the join itself, not the log that precedes it CodeRabbit on #5384. The recovery-path control waited on the "joining lobby" log, which handleJoinLobby writes BEFORE it awaits userAuth, the username seed, the cosmetics refs and the Turnstile token. A regression anywhere in that tail would have left the assertion passing over a join that never happened -- and the far edge is exactly what the test exists to claim. It now waits for joinLobby to have been called once, and checks it was handed the lobby that was dispatched. That also pairs with the refusal test above, which asserts the same mock was never reached: one is the complement of the other, so "exactly once" here means this join and no other. The log assertion is kept, downgraded from the thing being waited on to an ordinary expectation: it still distinguishes "got past the gate" from "got all the way through", which is worth having when this test fails. OPE-439 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PTKyUrxqfwKvf2QxAovR6Z --- tests/client/MainInitialize.test.ts | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/tests/client/MainInitialize.test.ts b/tests/client/MainInitialize.test.ts index d57fe3932f..0a493c4caf 100644 --- a/tests/client/MainInitialize.test.ts +++ b/tests/client/MainInitialize.test.ts @@ -477,8 +477,9 @@ describe("Client.initialize() booted from Main.ts module scope", () => { logSpy.mockClear(); messages.length = 0; mocks.joinLobby.mockReturnValue({ - // Neither settles: the assertion is that the join was ATTEMPTED, and - // the in-game path beyond it is not what this file boots. + // Neither settles: the join is complete once joinLobby has been + // handed the lobby, and the in-game path beyond that is not what + // this file boots. prestart: new Promise(() => {}), join: new Promise(() => {}), stop: vi.fn(), @@ -491,10 +492,18 @@ describe("Client.initialize() booted from Main.ts module scope", () => { }), ); - await vi.waitFor(() => - expect(logSpy).toHaveBeenCalledWith( - expect.stringContaining("joining lobby"), - ), + // The far edge of the funnel, not the "joining lobby" log: that log is + // written BEFORE handleJoinLobby awaits userAuth, the username seed and + // the cosmetics refs, so a regression anywhere in that tail would leave + // the log assertion passing over a join that never happened. joinLobby + // is the call that actually starts one, and the refusal test above + // asserts the same mock was never reached -- so the count being exactly + // one here is the pair of that claim. + await vi.waitFor(() => expect(mocks.joinLobby).toHaveBeenCalledTimes(1)); + // ...and with the lobby that was dispatched, not some other one. + expect(mocks.joinLobby.mock.calls[0][1].gameID).toBe("AbCd1234"); + expect(logSpy).toHaveBeenCalledWith( + expect.stringContaining("joining lobby"), ); expect(messages).not.toContain( translateText("error_modal.backend_unreachable"), From 3039e6d33f49ac1b066c1dba52803e7c1e24664d Mon Sep 17 00:00:00 2001 From: Josh Harris Date: Sat, 12 Sep 2026 12:54:47 +0100 Subject: [PATCH 03/10] Gate on a confirmed outage, not one missed heartbeat Review of #5384. The gates read backendReachable(), which flips on ANY failed attempt -- so a single 4s timeout at t=30s dimmed every multiplayer button and refused every join for at least a retry interval, while the cached list carried on serving perfectly well and the next request would very likely have worked. On the web there was not even a Retry to escape it with. That is worse than the blip it was reacting to. ServerList now counts consecutive unanswered attempts and exposes backendUnreachableConfirmed(), true only once two in a row have failed -- a retry interval's worth of evidence. Any answer resets the count, a failed manual retry counts towards it, and it is never true before the first attempt settles. backendReachable() stays as the raw per-attempt signal; the event carries both as { reachable, confirmed } and fires when either changes, because the second failure moves only `confirmed` and that is the transition every gate acts on. All three gates (GameModeSelector, DetailedGameViewModal, Main's join funnel) and the status bar's offline state read the confirmed value. docs/MultiServer.md updated. Also from the same review: - retryServerList() has a 1s floor of its own. Inside it a second press hands back the same promise rather than starting a request, so someone leaning on the button cannot outpace it; past it, fetchOnce() still dedupes against an attempt in flight. The bar disables Retry while its own attempt is out -- a button that keeps accepting clicks and visibly does nothing reads as broken whatever the throttle underneath is doing. - A refused matchmade join now closes the matchmaking modal, through the same close() its Back button uses (it shuts the queue socket and clears the watchdog). Without it the player sat on "waiting for a game" holding a queue slot for a match they had already been refused. Scoped to source === "matchmaking": a deep link refused while someone is legitimately queued must not cancel their queue. - DetailedGameViewModal's reachability wiring has tests of its own now: mount after a confirmed outage with no event dispatched (the seed), the recovery through the event (the subscribe), and the single-failure control. - The web toast key moves from error_modal.backend_unreachable to common.backend_unreachable. It is a toast raised from three different features, not a modal, and common.* is where the other cross-feature toasts live. - DesktopStatusBar's class doc said it renders nothing on a shell too old to expose the update bridge. It renders the session and outage states there, both of which are the client's own signals; only the update half goes quiet. OPE-439 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PTKyUrxqfwKvf2QxAovR6Z --- docs/MultiServer.md | 33 +++- resources/lang/en.json | 2 +- src/client/GameModeSelector.ts | 69 +++++---- src/client/Main.ts | 25 +++- src/client/ServerList.ts | 141 ++++++++++++++---- src/client/components/DesktopStatusBar.ts | 72 +++++---- .../components/DetailedGameViewModal.ts | 21 +-- tests/DesktopStatusBar.test.ts | 105 ++++++++++--- .../DetailedGameViewModalGatingWiring.test.ts | 128 ++++++++++++++++ tests/GameModeSelectorGating.test.ts | 81 +++++----- tests/ReachabilityGating.test.ts | 66 +++++--- tests/client/MainInitialize.test.ts | 20 ++- tests/client/ServerList.test.ts | 128 ++++++++++++++-- 13 files changed, 676 insertions(+), 215 deletions(-) diff --git a/docs/MultiServer.md b/docs/MultiServer.md index cb22473021..a5509b8993 100644 --- a/docs/MultiServer.md +++ b/docs/MultiServer.md @@ -419,11 +419,34 @@ values. non-OK, malformed or empty: the previous list keeps serving. The API caches its answer for seconds anyway, so a blip must not flip a working page into fallback. Only a client that never got a list falls back. -- **Reachability:** `backendReachable()` is null until the first attempt - settles, true when the API answered at all (a 404 included — reachable, - but no list for this site), false on a timeout or network error. Every - change is announced on the document as `backend-reachability` with - `{ reachable }` for UI to consume. +- **Reachability (two signals, OPE-439):** `backendReachable()` is the raw + per-attempt answer — null until the first attempt settles, true when the + API answered at all (a 404 included: reachable, but no list for this + site), false on a timeout or network error. It is deliberately twitchy, + so nothing player-facing gates on it. + `backendUnreachableConfirmed()` is the debounced one the UI uses: true + only once **two** attempts in a row have gone unanswered, which takes a + retry interval to accumulate. One missed beat is a blip the cached list + serves straight through, and dimming multiplayer for 10s over it would be + worse than the blip; any answer resets the count. Every change to either + value is announced on the document as `backend-reachability` with + `{ reachable, confirmed }`. Consumers seed from the accessor and then + subscribe — the event is one-shot, so a component mounting afterwards + would otherwise never learn the state (OPE-396). +- **Retry:** `retryServerList()` is the player-initiated attempt behind the + desktop status bar's offline Retry. It ignores the heartbeat's retry + interval (a person pressing a button is not a timer) but has a 1s floor + of its own, inside which a second press hands back the same promise; past + that, `fetchOnce()` still dedupes against an attempt already in flight. A + retry that fails counts towards the outage confirmation like any other + attempt. + + What consumes the confirmed signal: the desktop status bar's offline + state (ranked below a session failure, above any update state), and the + multiplayer gates in `GameModeSelector`, `DetailedGameViewModal` and + `Main`'s join funnel — on the web as well as on desktop. Single-player is + never gated, and nothing here touches a game already in progress. + - **Which list:** the desktop shell asks for its injected `serverHost` (its values are exactly the sites); a web page asks for its `siteHost` when rendered behind an apex, else `window.location.host`. Decided with diff --git a/resources/lang/en.json b/resources/lang/en.json index 1decbdd819..322e57dd6a 100644 --- a/resources/lang/en.json +++ b/resources/lang/en.json @@ -469,6 +469,7 @@ "common": { "available": "Available", "back": "Back", + "backend_unreachable": "Can't reach the OpenFront servers. Check your connection and try again.", "cancel": "Cancel", "cap_label": "Cap", "cap_tooltip": "Recipient’s remaining capacity", @@ -679,7 +680,6 @@ } }, "error_modal": { - "backend_unreachable": "Can't reach the OpenFront servers. Check your connection and try again.", "connection_error": "Connection error!", "connection_lost": "Lost connection to the game server and could not reconnect. Refresh the page to try rejoining.", "connection_refused": "Connection refused: {reason}", diff --git a/src/client/GameModeSelector.ts b/src/client/GameModeSelector.ts index 314f98999e..6c7221f88e 100644 --- a/src/client/GameModeSelector.ts +++ b/src/client/GameModeSelector.ts @@ -37,7 +37,7 @@ import { JoinLobbyModal } from "./JoinLobbyModal"; import { PublicLobbySocket } from "./LobbySocket"; import { JoinLobbyEvent } from "./Main"; import { - backendReachable, + backendUnreachableConfirmed, isPinnedToAVersion, type BackendReachabilityDetail, } from "./ServerList"; @@ -70,20 +70,22 @@ const TUTORIAL_CARD_MAX_GAMES = 5; * Whether multiplayer should be available given what we know about the * backend (OPE-439). * - * ONLY a settled `false` gates. `null` means the server-list heartbeat's - * first attempt has not landed yet -- a page is in that state for its first - * few hundred milliseconds, and blocking there would mean every player is - * briefly locked out of multiplayer on every load, on a suspicion we have not - * even tested. Unknown is not unreachable. + * The parameter is ServerList.backendUnreachableConfirmed(), NOT the raw + * backendReachable(), and the difference is load-bearing. That accessor is + * already false for the two states this must never gate: * - * `true` is "the API answered at all", not "the API served a usable list": a - * site with no list is a reachable backend and must not gate. See - * ServerList.backendReachable(). + * - before the first attempt settles. A page is in that state for its first + * few hundred milliseconds, and gating there would lock every player out + * of multiplayer on every load over a suspicion we have not tested yet. + * - after a single missed heartbeat. The cached list is still serving and + * the next request would very likely have worked; taking the game away + * for a retry interval over one blip is worse than the blip. + * + * It is also false when the API answered at all -- a 404 for a site with no + * list is a reachable backend. */ -export function multiplayerAllowedForBackend( - reachable: boolean | null, -): boolean { - return reachable !== false; +export function multiplayerAllowedForBackend(backendOutage: boolean): boolean { + return !backendOutage; } /** @@ -92,7 +94,7 @@ export function multiplayerAllowedForBackend( * A null update/session means that bridge is absent (the web build), so it * gates nothing; any one of the three alone is enough to block. * - * `reachable` is the only one of the three that also applies on the web, + * `backendOutage` is the only one of the three that also applies on the web, * which is why it is a required parameter rather than an optional one: an * entry point that forgets to pass it would silently stay ungated, and a * compile error is the cheapest way to notice. @@ -100,11 +102,11 @@ export function multiplayerAllowedForBackend( export function shouldBlockMultiplayerAction( update: DesktopUpdateState | null, session: DesktopSessionState | null, - reachable: boolean | null, + backendOutage: boolean, ): boolean { if (update !== null && !multiplayerAllowed(update)) return true; if (session !== null && !multiplayerAllowedForSession(session)) return true; - if (!multiplayerAllowedForBackend(reachable)) return true; + if (!multiplayerAllowedForBackend(backendOutage)) return true; return false; } @@ -120,7 +122,7 @@ export function shouldBlockMultiplayerAction( * Only reachability needs the web half: every other reason to refuse here is * desktop-only, and on desktop the bar always carries it. */ -export function reportMultiplayerRefusal(reachable: boolean | null): void { +export function reportMultiplayerRefusal(backendOutage: boolean): void { // Optional-call the method rather than dispatching an event: the bar is a // sibling custom element that may not have upgraded yet, and `?.wiggle?.()` // degrades to a silent no-op in that case instead of firing an event with @@ -133,8 +135,8 @@ export function reportMultiplayerRefusal(reachable: boolean | null): void { // Keyed on the shell, not on the element: is in // index.html on every build and simply renders nothing on the web, so its // presence proves nothing about whether the player can see a reason. - if (!isDesktopShell() && reachable === false) { - showToast(translateText("error_modal.backend_unreachable"), "red"); + if (!isDesktopShell() && backendOutage) { + showToast(translateText("common.backend_unreachable"), "red"); } } @@ -168,10 +170,10 @@ export function shouldBlockJoin( lobby: JoinLobbyEvent, update: DesktopUpdateState | null, session: DesktopSessionState | null, - reachable: boolean | null, + backendOutage: boolean, ): boolean { if (!joinIsGateable(lobby)) return false; - return shouldBlockMultiplayerAction(update, session, reachable); + return shouldBlockMultiplayerAction(update, session, backendOutage); } @customElement("game-mode-selector") @@ -183,9 +185,10 @@ export class GameModeSelector extends LitElement { @state() private viewerSignedIn: boolean = false; @state() private showTrustRequired: boolean = false; @state() private desktopSessionState: DesktopSessionState | null = null; - // Null until the server-list heartbeat's first attempt settles; see - // multiplayerAllowedForBackend for why null never gates. - @state() private backendReachableState: boolean | null = null; + // The DEBOUNCED outage signal, not the raw per-attempt one: see + // multiplayerAllowedForBackend for why one missed heartbeat must not dim + // these buttons. + @state() private backendOutage = false; private serverTimeOffset: number = 0; private defaultLobbyTime: number = 0; @@ -277,10 +280,10 @@ export class GameModeSelector extends LitElement { this.onDesktopSessionState, ); // Seeded unconditionally, unlike the two above: the backend is just as - // unreachable on the web, and the heartbeat's first attempt often settles + // unreachable on the web, and the heartbeat's first attempts often settle // before this element exists (it is started in Main's initialize, we are - // rendered by later), so the event alone would miss it. - this.backendReachableState = backendReachable(); + // rendered by later), so the event alone would miss them. + this.backendOutage = backendUnreachableConfirmed(); document.addEventListener( "backend-reachability", this.onBackendReachability, @@ -358,9 +361,9 @@ export class GameModeSelector extends LitElement { }; private onBackendReachability = (e: Event) => { - this.backendReachableState = ( + this.backendOutage = ( e as CustomEvent - ).detail.reachable; + ).detail.confirmed; }; public stop() { @@ -529,11 +532,11 @@ export class GameModeSelector extends LitElement { !shouldBlockMultiplayerAction( this.desktopUpdateState, this.desktopSessionState, - this.backendReachableState, + this.backendOutage, ) ) return false; - reportMultiplayerRefusal(this.backendReachableState); + reportMultiplayerRefusal(this.backendOutage); return true; } @@ -651,7 +654,7 @@ export class GameModeSelector extends LitElement { shouldBlockMultiplayerAction( this.desktopUpdateState, this.desktopSessionState, - this.backendReachableState, + this.backendOutage, ); return html` `; } + private onRetryClick(): void { + if (this.retrying) return; + this.retrying = true; + retryServerList() + .catch((err: unknown) => { + // retryServerList never rejects; belt and braces, so a change there + // cannot surface as an unhandled rejection from a click handler. + console.error("desktop-status-bar: server list retry failed", err); + }) + .finally(() => { + this.retrying = false; + }); + } + private label(s: DesktopUpdateState) { switch (s.status) { case "downloading": diff --git a/src/client/components/DetailedGameViewModal.ts b/src/client/components/DetailedGameViewModal.ts index 6bde053932..8d76fa86db 100644 --- a/src/client/components/DetailedGameViewModal.ts +++ b/src/client/components/DetailedGameViewModal.ts @@ -20,7 +20,7 @@ import { JoinLobbyModal } from "../JoinLobbyModal"; import { PublicLobbySocket } from "../LobbySocket"; import { JoinLobbyEvent } from "../Main"; import { - backendReachable, + backendUnreachableConfirmed, type BackendReachabilityDetail, } from "../ServerList"; import { UsernameInput } from "../UsernameInput"; @@ -126,9 +126,10 @@ export class DetailedGameViewModal extends BaseModal { @state() private viewerSignedIn: boolean = false; @state() private showTrustRequired: boolean = false; @state() private desktopSessionState: DesktopSessionState | null = null; - // Null until the server-list heartbeat's first attempt settles; see - // multiplayerAllowedForBackend for why null never gates. - @state() private backendReachableState: boolean | null = null; + // The DEBOUNCED outage signal, not the raw per-attempt one: see + // multiplayerAllowedForBackend for why one missed heartbeat must not gate + // this browser's join. + @state() private backendOutage = false; private serverTimeOffset = 0; private countdownTimer: number | null = null; @@ -202,7 +203,7 @@ export class DetailedGameViewModal extends BaseModal { ); // Seeded unconditionally, unlike the two above: an unreachable backend // refuses a join on the web as well as on desktop (OPE-439). - this.backendReachableState = backendReachable(); + this.backendOutage = backendUnreachableConfirmed(); document.addEventListener( "backend-reachability", this.onBackendReachability, @@ -249,9 +250,9 @@ export class DetailedGameViewModal extends BaseModal { }; private onBackendReachability = (e: Event) => { - this.backendReachableState = ( + this.backendOutage = ( e as CustomEvent - ).detail.reachable; + ).detail.confirmed; }; // ---- Slot animation ---- @@ -483,7 +484,7 @@ export class DetailedGameViewModal extends BaseModal { blocked: shouldBlockMultiplayerAction( this.desktopUpdateState, this.desktopSessionState, - this.backendReachableState, + this.backendOutage, ), viewerTrusted: this.viewerTrusted, onClick: () => this.join(lobby), @@ -811,11 +812,11 @@ export class DetailedGameViewModal extends BaseModal { !shouldBlockMultiplayerAction( this.desktopUpdateState, this.desktopSessionState, - this.backendReachableState, + this.backendOutage, ) ) return false; - reportMultiplayerRefusal(this.backendReachableState); + reportMultiplayerRefusal(this.backendOutage); return true; } diff --git a/tests/DesktopStatusBar.test.ts b/tests/DesktopStatusBar.test.ts index 3660735a50..1866008db5 100644 --- a/tests/DesktopStatusBar.test.ts +++ b/tests/DesktopStatusBar.test.ts @@ -3,9 +3,10 @@ import { ClientEnv } from "../src/client/ClientEnv"; import "../src/client/components/DesktopStatusBar"; import { barSource } from "../src/client/components/DesktopStatusBar"; import { - backendReachable, + backendUnreachableConfirmed, ensureServerList, resetServerList, + retryServerList, } from "../src/client/ServerList"; describe("barSource", () => { @@ -14,7 +15,7 @@ describe("barSource", () => { barSource( { status: "current", bytes: 0, total: 0 }, { status: "signed-in" }, - true, + false, ), ).toBe("none"); }); @@ -24,7 +25,7 @@ describe("barSource", () => { barSource( { status: "downloading", bytes: 1, total: 2 }, { status: "signed-in" }, - true, + false, ), ).toBe("update"); }); @@ -47,7 +48,7 @@ describe("barSource", () => { status: "signed-out", reason: "steam-wedged", }, - true, + false, ), ).toBe("session"); }); @@ -68,13 +69,13 @@ describe("barSource", () => { error: { kind: "quota-exceeded", message: "from a newer shell" }, }, { status: "signed-in" }, - true, + false, ), ).toBe("update"); }); it("shows nothing on the web, where neither bridge exists", () => { - expect(barSource(null, null, null)).toBe("none"); + expect(barSource(null, null, false)).toBe("none"); }); // OPE-439. Reachability sits between the two: below the session, because a @@ -84,7 +85,7 @@ describe("barSource", () => { // provably cannot work until the network is back. it("shows the offline state over any update state", () => { expect( - barSource({ status: "current", bytes: 0, total: 0 }, null, false), + barSource({ status: "current", bytes: 0, total: 0 }, null, true), ).toBe("reachability"); expect( barSource( @@ -95,24 +96,25 @@ describe("barSource", () => { error: { kind: "network", message: "offline" }, }, { status: "signed-in" }, - false, + true, ), ).toBe("reachability"); }); it("still shows the session over the offline state", () => { expect( - barSource(null, { status: "signed-out", reason: "network" }, false), + barSource(null, { status: "signed-out", reason: "network" }, true), ).toBe("session"); }); - // No neutral state exists in this bar to hang a "Checking…" on, and - // inventing one would put a permanent strip across the bottom of a healthy - // game for the sake of its first few hundred milliseconds. - it("shows nothing while the first attempt has not settled", () => { - expect(barSource(null, { status: "signed-in" }, null)).toBe("none"); + // The argument is the CONFIRMED outage, so "unsettled" and "missed once" + // both arrive here as false and show nothing. There is no neutral state in + // this bar to hang a "Checking…" on, and inventing one would put a strip + // across the bottom of a healthy game every time one request timed out. + it("shows nothing until an outage is confirmed", () => { + expect(barSource(null, { status: "signed-in" }, false)).toBe("none"); expect( - barSource({ status: "current", bytes: 0, total: 0 }, null, null), + barSource({ status: "current", bytes: 0, total: 0 }, null, false), ).toBe("none"); }); }); @@ -162,6 +164,10 @@ describe("the rendered offline state", () => { vi.stubGlobal("fetch", fetchMock); vi.spyOn(console, "warn").mockImplementation(() => {}); vi.spyOn(console, "info").mockImplementation(() => {}); + // Fake the clock only so the tests can step past the manual-retry floor + // and the heartbeat's retry interval; shouldAdvanceTime keeps real time + // flowing underneath, so vi.waitFor and Lit's microtasks behave normally. + vi.useFakeTimers({ shouldAdvanceTime: true }); }); afterEach(() => { @@ -170,10 +176,24 @@ describe("the rendered offline state", () => { window.BOOTSTRAP_CONFIG = undefined; ClientEnv.reset(); resetServerList(); + vi.useRealTimers(); vi.unstubAllGlobals(); vi.restoreAllMocks(); }); + /** + * Drives enough failed attempts that the outage is confirmed, then steps + * past the manual-retry floor so a click in the test starts an attempt of + * its own rather than joining this one. + */ + async function confirmOutage(): Promise { + await ensureServerList(); + vi.advanceTimersByTime(2_000); + await retryServerList(); + expect(backendUnreachableConfirmed()).toBe(true); + vi.advanceTimersByTime(2_000); + } + it("shows nothing while the backend is fine", async () => { // The control: the bar must not become a permanent fixture just because // this feature exists. @@ -181,16 +201,27 @@ describe("the rendered offline state", () => { async () => new Response("{}", { status: 404 }), ); await ensureServerList(); - expect(backendReachable()).toBe(true); + expect(backendUnreachableConfirmed()).toBe(false); const bar = mountBar(); await bar.updateComplete; expect(bar.textContent?.trim()).toBe(""); }); - it("seeds the offline state from an attempt that failed before it mounted", async () => { + it("shows nothing after a single missed attempt", async () => { + // One timed-out heartbeat is a blip, not an outage. Showing an offline + // bar for it -- while the cached list is still serving perfectly well -- + // would make the bar appear and vanish on any flaky connection. await ensureServerList(); - expect(backendReachable()).toBe(false); + expect(backendUnreachableConfirmed()).toBe(false); + + const bar = mountBar(); + await bar.updateComplete; + expect(bar.textContent?.trim()).toBe(""); + }); + + it("seeds the offline state from failures that happened before it mounted", async () => { + await confirmOutage(); // Mounted AFTER the announcement it would have needed. The accessor is // the only path left, exactly as in OPE-396. @@ -206,17 +237,17 @@ describe("the rendered offline state", () => { await bar.updateComplete; expect(bar.textContent?.trim()).toBe(""); - await ensureServerList(); + await confirmOutage(); await bar.updateComplete; expect(bar.textContent).toContain("desktop_status.offline"); }); it("Retry attempts again immediately, and the bar clears when the API answers", async () => { - await ensureServerList(); + await confirmOutage(); const bar = mountBar(); await bar.updateComplete; - expect(fetchMock).toHaveBeenCalledTimes(1); + const attemptsSoFar = fetchMock.mock.calls.length; // A 404 is an answer: this site has no list, but the backend is up. That // is the boundary the bar keys on, so it is the one worth clearing on. @@ -227,9 +258,37 @@ describe("the rendered offline state", () => { // Immediately, without waiting out the heartbeat's retry interval -- the // whole point of the button. - expect(fetchMock).toHaveBeenCalledTimes(2); - await vi.waitFor(() => expect(backendReachable()).toBe(true)); + expect(fetchMock).toHaveBeenCalledTimes(attemptsSoFar + 1); + await vi.waitFor(() => expect(backendUnreachableConfirmed()).toBe(false)); await bar.updateComplete; expect(bar.textContent?.trim()).toBe(""); }); + + it("disables Retry while its own attempt is still out", async () => { + await confirmOutage(); + const bar = mountBar(); + await bar.updateComplete; + + // A request that never answers, so the in-flight window stays open. + let release: (r: Response) => void = () => {}; + fetchMock.mockImplementation( + async () => + await new Promise((resolve) => { + release = resolve; + }), + ); + const button = retryButton(bar)!; + button.click(); + await bar.updateComplete; + + // A button that keeps accepting clicks while visibly doing nothing reads + // as broken, whatever the throttle underneath is doing. + expect(button.disabled).toBe(true); + const attempts = fetchMock.mock.calls.length; + button.click(); + expect(fetchMock).toHaveBeenCalledTimes(attempts); + + release(new Response("{}", { status: 404 })); + await vi.waitFor(() => expect(backendUnreachableConfirmed()).toBe(false)); + }); }); diff --git a/tests/DetailedGameViewModalGatingWiring.test.ts b/tests/DetailedGameViewModalGatingWiring.test.ts index 85938ab4d0..86c7529802 100644 --- a/tests/DetailedGameViewModalGatingWiring.test.ts +++ b/tests/DetailedGameViewModalGatingWiring.test.ts @@ -1,5 +1,12 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ClientEnv } from "../src/client/ClientEnv"; import type { DesktopUpdateState } from "../src/client/DesktopShell"; +import { + backendUnreachableConfirmed, + ensureServerList, + resetServerList, + retryServerList, +} from "../src/client/ServerList"; import { GameMapType, GameMode } from "../src/core/game/Game"; import type { GameConfig, @@ -224,3 +231,124 @@ describe("the multiplayer gate at DetailedGameViewModal's join()", () => { expect(joinLobby).toHaveBeenCalled(); }); }); + +/** + * OPE-439's half of the same gate. The update and session halves above are + * desktop-only; a confirmed backend outage refuses this browser's join on + * the web too, and its seed-then-subscribe wiring is its own call site with + * its own chance to be wrong. + * + * Driven through the real ServerList module rather than a mock of it: the + * point of the seed test is that the value the component reads is the one + * the heartbeat actually produced. + */ +describe("DetailedGameViewModal and a confirmed backend outage", () => { + let fetchMock: ReturnType; + + /** + * Throws this modal away and mounts a fresh one, so a test can choose what + * the module already knows BEFORE the component exists. The outer + * beforeEach always mounts one; that one is no use for a seeding test. + */ + async function remountModal(): Promise { + modal.remove(); + modal = new DetailedGameViewModal() as unknown as HTMLElement & { + updateComplete: Promise; + }; + document.body.appendChild(modal); + await modal.updateComplete; + await pushLobbies({ ffa: [lobby("public-1", "ffa")] }); + } + + beforeEach(() => { + // ServerList reads the site from ClientEnv, which throws without the + // config the server injects into index.html -- and a site it cannot + // resolve means it never fetches at all. + window.BOOTSTRAP_CONFIG = { + gameEnv: "dev", + numWorkers: 1, + turnstileSiteKey: "", + jwtAudience: "test", + instanceId: "test", + gitCommit: "test", + }; + ClientEnv.reset(); + resetServerList(); + fetchMock = vi.fn(async () => { + throw new TypeError("network down"); + }); + vi.stubGlobal("fetch", fetchMock); + vi.spyOn(console, "warn").mockImplementation(() => {}); + vi.spyOn(console, "info").mockImplementation(() => {}); + }); + + afterEach(() => { + resetServerList(); + ClientEnv.reset(); + window.BOOTSTRAP_CONFIG = undefined; + vi.unstubAllGlobals(); + }); + + /** Two unanswered attempts in a row, which is what confirms an outage. */ + async function confirmOutage(): Promise { + await ensureServerList(); + await retryServerList(); + expect(backendUnreachableConfirmed()).toBe(true); + } + + it("refuses a card click when it mounted after the outage was confirmed", async () => { + await confirmOutage(); + + // No "backend-reachability" event is dispatched below: the ones that + // would have told this component fired before it existed. The accessor + // seed is the only path left (OPE-396, on a new signal). + await remountModal(); + + const card = cardButton("public-1"); + expect(card).not.toBeNull(); + card!.click(); + + expect(joinLobby).not.toHaveBeenCalled(); + expect(wiggle).toHaveBeenCalled(); + }); + + it("marks its cards aria-disabled on that same seed", async () => { + await confirmOutage(); + await remountModal(); + + expect( + modal.querySelectorAll('button[aria-disabled="true"]').length, + ).toBeGreaterThan(0); + }); + + it("allows the join again once the backend answers", async () => { + await confirmOutage(); + await remountModal(); + + // The subscribe half, from the seeded state: a recovery this component + // only ever hears about through the event. + document.dispatchEvent( + new CustomEvent("backend-reachability", { + detail: { reachable: true, confirmed: false }, + }), + ); + await modal.updateComplete; + + cardButton("public-1")!.click(); + + expect(joinLobby).toHaveBeenCalled(); + expect(joinLobby.mock.calls[0][0].detail.gameID).toBe("public-1"); + }); + + it("does not refuse after a single missed attempt", async () => { + // The control, and the bug this debounce exists for: one timed-out + // heartbeat must not take the lobby browser away. + await ensureServerList(); + expect(backendUnreachableConfirmed()).toBe(false); + + await remountModal(); + cardButton("public-1")!.click(); + + expect(joinLobby).toHaveBeenCalled(); + }); +}); diff --git a/tests/GameModeSelectorGating.test.ts b/tests/GameModeSelectorGating.test.ts index 568d76a9da..77f213642c 100644 --- a/tests/GameModeSelectorGating.test.ts +++ b/tests/GameModeSelectorGating.test.ts @@ -9,7 +9,7 @@ import { GameType } from "../src/core/game/Game"; describe("shouldBlockMultiplayerAction", () => { it("allows everything when no desktop update state has arrived", () => { - expect(shouldBlockMultiplayerAction(null, null, null)).toBe(false); + expect(shouldBlockMultiplayerAction(null, null, false)).toBe(false); }); it("allows multiplayer when the client is current", () => { @@ -17,7 +17,7 @@ describe("shouldBlockMultiplayerAction", () => { shouldBlockMultiplayerAction( { status: "current", bytes: 0, total: 0 }, null, - null, + false, ), ).toBe(false); }); @@ -31,14 +31,14 @@ describe("shouldBlockMultiplayerAction", () => { total: 2, }, null, - null, + false, ), ).toBe(true); expect( shouldBlockMultiplayerAction( { status: "staged", bytes: 2, total: 2 }, null, - null, + false, ), ).toBe(true); }); @@ -48,7 +48,7 @@ describe("shouldBlockMultiplayerAction", () => { shouldBlockMultiplayerAction( { status: "blocked", bytes: 0, total: 0 }, null, - null, + false, ), ).toBe(false); }); @@ -63,19 +63,19 @@ describe("shouldBlockMultiplayerAction", () => { }); it("blocks a failed check when Retry is a real remedy", () => { - expect(shouldBlockMultiplayerAction(failed("network"), null, null)).toBe( + expect(shouldBlockMultiplayerAction(failed("network"), null, false)).toBe( true, ); - expect(shouldBlockMultiplayerAction(failed("verify"), null, null)).toBe( + expect(shouldBlockMultiplayerAction(failed("verify"), null, false)).toBe( true, ); }); it("does not block failures no player-side action can change", () => { - expect(shouldBlockMultiplayerAction(failed("refused"), null, null)).toBe( + expect(shouldBlockMultiplayerAction(failed("refused"), null, false)).toBe( false, ); - expect(shouldBlockMultiplayerAction(failed("parse"), null, null)).toBe( + expect(shouldBlockMultiplayerAction(failed("parse"), null, false)).toBe( false, ); }); @@ -89,7 +89,7 @@ describe("shouldBlockMultiplayerAction with a session", () => { shouldBlockMultiplayerAction( healthyUpdate, { status: "signed-in" }, - null, + false, ), ).toBe(false); }); @@ -102,7 +102,7 @@ describe("shouldBlockMultiplayerAction with a session", () => { status: "signed-out", reason: "steam-wedged", }, - null, + false, ), ).toBe(true); }); @@ -114,43 +114,38 @@ describe("shouldBlockMultiplayerAction with a session", () => { { status: "signed-in", }, - null, + false, ), ).toBe(true); }); it("does not block on the web, where neither state exists", () => { - expect(shouldBlockMultiplayerAction(null, null, null)).toBe(false); + expect(shouldBlockMultiplayerAction(null, null, false)).toBe(false); }); }); +// The parameter is ServerList.backendUnreachableConfirmed(), not the raw +// backendReachable(): the states this rule must NOT gate -- nothing tried +// yet, and one missed heartbeat over a still-serving cached list -- are +// already false by the time they reach here. Those are pinned against the +// real module in tests/client/ServerList.test.ts. describe("multiplayerAllowedForBackend", () => { - it("allows multiplayer before the first attempt has settled", () => { - // OPE-439's central rule: unknown is not unreachable. Every page is in - // this state for its first few hundred milliseconds, and gating there - // would lock every player out of multiplayer on every load. - expect(multiplayerAllowedForBackend(null)).toBe(true); + it("allows multiplayer unless an outage is confirmed", () => { + expect(multiplayerAllowedForBackend(false)).toBe(true); }); - it("allows multiplayer when the API answered", () => { - // "Answered" and not "served a usable list": a site with no list at all - // still proves the backend is up. - expect(multiplayerAllowedForBackend(true)).toBe(true); - }); - - it("blocks multiplayer once an attempt has failed outright", () => { - expect(multiplayerAllowedForBackend(false)).toBe(false); + it("blocks multiplayer on a confirmed outage", () => { + expect(multiplayerAllowedForBackend(true)).toBe(false); }); }); -describe("shouldBlockMultiplayerAction with backend reachability", () => { +describe("shouldBlockMultiplayerAction with a backend outage", () => { it("blocks on the web, where both desktop states are absent", () => { - expect(shouldBlockMultiplayerAction(null, null, false)).toBe(true); + expect(shouldBlockMultiplayerAction(null, null, true)).toBe(true); }); - it("does not block on an unknown or reachable backend", () => { - expect(shouldBlockMultiplayerAction(null, null, null)).toBe(false); - expect(shouldBlockMultiplayerAction(null, null, true)).toBe(false); + it("does not block while the backend is fine", () => { + expect(shouldBlockMultiplayerAction(null, null, false)).toBe(false); }); it("still blocks on a desktop reason while the backend is fine", () => { @@ -158,7 +153,7 @@ describe("shouldBlockMultiplayerAction with backend reachability", () => { shouldBlockMultiplayerAction( { status: "staged", bytes: 0, total: 0 }, { status: "signed-in" }, - true, + false, ), ).toBe(true); }); @@ -213,7 +208,7 @@ describe("shouldBlockJoin", () => { const healthy = { status: "current", bytes: 0, total: 0 } as const; it("allows a multiplayer join when both states are healthy", () => { - expect(shouldBlockJoin(mp, healthy, { status: "signed-in" }, true)).toBe( + expect(shouldBlockJoin(mp, healthy, { status: "signed-in" }, false)).toBe( false, ); }); @@ -227,7 +222,7 @@ describe("shouldBlockJoin", () => { status: "signed-out", reason: "steam-wedged", }, - true, + false, ), ).toBe(true); }); @@ -239,7 +234,7 @@ describe("shouldBlockJoin", () => { mp, { status: "staged", bytes: 0, total: 0 }, { status: "signed-in" }, - true, + false, ), ).toBe(true); }); @@ -250,28 +245,26 @@ describe("shouldBlockJoin", () => { solo, { status: "staged", bytes: 0, total: 0 }, { status: "signed-out", reason: "steam-wedged" }, - false, + true, ), ).toBe(false); }); it("does not block on the web, where neither desktop state exists", () => { - expect(shouldBlockJoin(mp, null, null, true)).toBe(false); - // Nor before the heartbeat's first attempt has settled. - expect(shouldBlockJoin(mp, null, null, null)).toBe(false); + expect(shouldBlockJoin(mp, null, null, false)).toBe(false); }); // OPE-439. The one input that gates on the web as well as on desktop. - it("blocks a multiplayer join while the backend is unreachable", () => { - expect(shouldBlockJoin(mp, null, null, false)).toBe(true); - expect(shouldBlockJoin(mp, healthy, { status: "signed-in" }, false)).toBe( + it("blocks a multiplayer join on a confirmed backend outage", () => { + expect(shouldBlockJoin(mp, null, null, true)).toBe(true); + expect(shouldBlockJoin(mp, healthy, { status: "signed-in" }, true)).toBe( true, ); }); - it("never blocks single-player on an unreachable backend", () => { + it("never blocks single-player on a backend outage", () => { // The desktop build's core offline promise: bot games run entirely // in-client, so an unreachable backend is no reason to refuse one. - expect(shouldBlockJoin(solo, null, null, false)).toBe(false); + expect(shouldBlockJoin(solo, null, null, true)).toBe(false); }); }); diff --git a/tests/ReachabilityGating.test.ts b/tests/ReachabilityGating.test.ts index 20ebaa4b8f..f0daa313fa 100644 --- a/tests/ReachabilityGating.test.ts +++ b/tests/ReachabilityGating.test.ts @@ -1,9 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { ClientEnv } from "../src/client/ClientEnv"; import { - backendReachable, + backendUnreachableConfirmed, ensureServerList, resetServerList, + retryServerList, } from "../src/client/ServerList"; import { GameMapType, GameMode } from "../src/core/game/Game"; import type { @@ -98,13 +99,29 @@ function clickEveryButton(): number { } /** Announces a reachability change the way the heartbeat does. */ -async function announce(reachable: boolean): Promise { +async function announce(reachable: boolean, confirmed = false): Promise { document.dispatchEvent( - new CustomEvent("backend-reachability", { detail: { reachable } }), + new CustomEvent("backend-reachability", { + detail: { reachable, confirmed }, + }), ); await selector.updateComplete; } +/** + * Drives the real module through enough failed attempts that the outage is + * confirmed. Uses the manual retry for the second one so the test does not + * have to wait out the heartbeat's retry interval. + */ +async function confirmOutage(): Promise { + fetchMock.mockImplementation(async () => { + throw new TypeError("network down"); + }); + await ensureServerList(); + await retryServerList(); + expect(backendUnreachableConfirmed()).toBe(true); +} + beforeEach(() => { // connectedCallback reads ClientEnv.gameCreationRate(), which throws // without the config the server injects into index.html. No serverHost and @@ -164,7 +181,7 @@ describe("the multiplayer entry points while the backend is unreachable", () => // multiplayer on a suspicion we have not even tested yet. Every page is // in this state for its first few hundred milliseconds. selector = await mountSelector(); - expect(backendReachable()).toBe(null); + expect(backendUnreachableConfirmed()).toBe(false); expect(clickEveryButton()).toBeGreaterThan(0); @@ -176,9 +193,26 @@ describe("the multiplayer entry points while the backend is unreachable", () => expect(messages).toEqual([]); }); - it("dims and refuses every entry point once an attempt fails", async () => { + it("lets everything through after a SINGLE missed attempt", async () => { + // One timed-out heartbeat is a blip. The cached list is still serving, + // the next request would very likely work, and dimming every button for + // a retry interval over it -- with no Retry on the web to escape with -- + // takes the game away for no good reason. selector = await mountSelector(); - await announce(false); + await announce(false, false); + + expect( + selector.querySelectorAll('button[aria-disabled="true"]').length, + ).toBe(0); + expect(clickEveryButton()).toBeGreaterThan(0); + expect(joinOpen).toHaveBeenCalled(); + expect(hostOpen).toHaveBeenCalled(); + expect(messages).toEqual([]); + }); + + it("dims and refuses every entry point once the outage is confirmed", async () => { + selector = await mountSelector(); + await announce(false, true); expect( selector.querySelectorAll('button[aria-disabled="true"]').length, @@ -192,18 +226,18 @@ describe("the multiplayer entry points while the backend is unreachable", () => it("says why, on the web, where there is no status bar to read", async () => { selector = await mountSelector(); - await announce(false); + await announce(false, true); clickEveryButton(); // Refusing silently would look like a broken button, and unlike the // desktop gates there is nothing else on screen naming the reason. - expect(messages).toContain("error_modal.backend_unreachable"); + expect(messages).toContain("common.backend_unreachable"); }); it("re-enables everything when the backend comes back", async () => { selector = await mountSelector(); - await announce(false); + await announce(false, true); clickEveryButton(); expect(joinOpen).not.toHaveBeenCalled(); @@ -225,7 +259,7 @@ describe("the multiplayer entry points while the backend is unreachable", () => open: () => void; } ).open = soloOpen; - await announce(false); + await announce(false, true); clickEveryButton(); @@ -235,16 +269,12 @@ describe("the multiplayer entry points while the backend is unreachable", () => expect(soloOpen).toHaveBeenCalled(); }); - it("gates a selector that mounted after the attempt had already failed", async () => { + it("gates a selector that mounted after the outage was already confirmed", async () => { // The seed half. No "backend-reachability" event is dispatched anywhere - // below: the only one this document will ever see fired while nothing + // below: the only ones this document will ever see fired while nothing // was listening, so the accessor is the sole path by which the selector // can know. This is OPE-396's bug, on a new signal. - fetchMock.mockImplementation(async () => { - throw new TypeError("network down"); - }); - await ensureServerList(); - expect(backendReachable()).toBe(false); + await confirmOutage(); selector = await mountSelector(); @@ -260,7 +290,7 @@ describe("the multiplayer entry points while the backend is unreachable", () => // The control for the seed: a 404 is an answer, so a site with no list // at all is still a reachable backend. await ensureServerList(); - expect(backendReachable()).toBe(true); + expect(backendUnreachableConfirmed()).toBe(false); selector = await mountSelector(); diff --git a/tests/client/MainInitialize.test.ts b/tests/client/MainInitialize.test.ts index 0a493c4caf..2f8be61906 100644 --- a/tests/client/MainInitialize.test.ts +++ b/tests/client/MainInitialize.test.ts @@ -404,7 +404,14 @@ describe("Client.initialize() booted from Main.ts module scope", () => { let messages: string[]; let onMessage: EventListener; - /** Replaces the fetch stub and re-settles the heartbeat's first attempt. */ + /** + * Replaces the fetch stub and drives the module to the state named. + * + * An outage takes TWO unanswered attempts to confirm, and only the + * confirmed signal gates -- so reaching the state under test means making + * both, which the manual retry does without waiting out the heartbeat's + * retry interval. + */ async function settleReachability(reachable: boolean): Promise { vi.stubGlobal( "fetch", @@ -423,7 +430,8 @@ describe("Client.initialize() booted from Main.ts module scope", () => { ); ServerList.resetServerList(); await ServerList.ensureServerList(); - expect(ServerList.backendReachable()).toBe(reachable); + if (!reachable) await ServerList.retryServerList(); + expect(ServerList.backendUnreachableConfirmed()).toBe(!reachable); } beforeAll(async () => { @@ -446,7 +454,7 @@ describe("Client.initialize() booted from Main.ts module scope", () => { ServerList.resetServerList(); }); - it("refuses a join and says why", async () => { + it("refuses a join and says why once the outage is confirmed", async () => { await settleReachability(false); logSpy.mockClear(); messages.length = 0; @@ -459,9 +467,7 @@ describe("Client.initialize() booted from Main.ts module scope", () => { ); await vi.waitFor(() => - expect(messages).toContain( - translateText("error_modal.backend_unreachable"), - ), + expect(messages).toContain(translateText("common.backend_unreachable")), ); // Refused before anything was joined -- not merely reported after. expect(logSpy).not.toHaveBeenCalledWith( @@ -506,7 +512,7 @@ describe("Client.initialize() booted from Main.ts module scope", () => { expect.stringContaining("joining lobby"), ); expect(messages).not.toContain( - translateText("error_modal.backend_unreachable"), + translateText("common.backend_unreachable"), ); }); }); diff --git a/tests/client/ServerList.test.ts b/tests/client/ServerList.test.ts index a5f2df2641..a96666ecf0 100644 --- a/tests/client/ServerList.test.ts +++ b/tests/client/ServerList.test.ts @@ -3,6 +3,7 @@ import { ClientEnv } from "../../src/client/ClientEnv"; import { resetPagePinForTests } from "../../src/client/PagePin"; import { backendReachable, + backendUnreachableConfirmed, ensureServerList, redirectToGameVersion, reloadWouldRescue, @@ -407,6 +408,83 @@ describe("startServerListPolling", () => { }); }); +describe("a confirmed outage, as opposed to one missed beat", () => { + // The gates read backendUnreachableConfirmed(), never the raw signal: the + // heartbeat is expected to miss occasionally while the cached list carries + // on serving, and dimming every multiplayer button for a retry interval + // over one 4s timeout is worse than the timeout. + it("takes two consecutive failures, and any answer resets the count", async () => { + vi.useFakeTimers(); + const seen: unknown[] = []; + const listener = (e: Event) => seen.push((e as CustomEvent).detail); + document.addEventListener("backend-reachability", listener); + try { + expect(backendUnreachableConfirmed()).toBe(false); + + // One failure: reachable flips, but nothing is confirmed yet. + fetchMock.mockRejectedValue(new TypeError("network down")); + await ensureServerList(); + expect(backendReachable()).toBe(false); + expect(backendUnreachableConfirmed()).toBe(false); + expect(seen).toEqual([{ reachable: false, confirmed: false }]); + + // The second one confirms it. `reachable` did not change, so this + // announcement exists only because `confirmed` did -- which is the + // transition every gate acts on. + await vi.advanceTimersByTimeAsync(RETRY_MS); + await ensureServerList(); + expect(backendUnreachableConfirmed()).toBe(true); + expect(seen).toEqual([ + { reachable: false, confirmed: false }, + { reachable: false, confirmed: true }, + ]); + + // A third changes nothing, so it is not announced. + await vi.advanceTimersByTimeAsync(RETRY_MS); + await ensureServerList(); + expect(seen).toHaveLength(2); + + // One answer clears it outright -- no gradual recovery. + fetchMock.mockImplementation(async () => jsonResponse(API_LIST)); + await vi.advanceTimersByTimeAsync(RETRY_MS); + expect(await ensureServerList()).toBe("api"); + expect(backendUnreachableConfirmed()).toBe(false); + expect(seen).toEqual([ + { reachable: false, confirmed: false }, + { reachable: false, confirmed: true }, + { reachable: true, confirmed: false }, + ]); + + // ...and the count restarts: one failure after a success is a blip + // again, not a resumption of the old outage. + fetchMock.mockRejectedValue(new TypeError("network down")); + await vi.advanceTimersByTimeAsync(REFRESH_MS); + await ensureServerList(); + expect(backendUnreachableConfirmed()).toBe(false); + } finally { + document.removeEventListener("backend-reachability", listener); + } + }); + + it("counts a failed manual retry towards the confirmation", async () => { + // Pressing Retry against a backend that is genuinely down should settle + // the question sooner, not reset it. + vi.useFakeTimers(); + fetchMock.mockRejectedValue(new TypeError("network down")); + await ensureServerList(); + expect(backendUnreachableConfirmed()).toBe(false); + + await vi.advanceTimersByTimeAsync(1_500); + await retryServerList(); + expect(backendUnreachableConfirmed()).toBe(true); + }); + + it("is never confirmed before the first attempt settles", async () => { + expect(backendReachable()).toBe(null); + expect(backendUnreachableConfirmed()).toBe(false); + }); +}); + describe("backend reachability", () => { it("reports whether the API answered at all, and announces every change", async () => { const seen: unknown[] = []; @@ -426,20 +504,23 @@ describe("backend reachability", () => { ); expect(await ensureServerList()).toBe("fallback"); expect(backendReachable()).toBe(true); - expect(seen).toEqual([{ reachable: true }]); + expect(seen).toEqual([{ reachable: true, confirmed: false }]); // Unchanged: no second announcement. await vi.advanceTimersByTimeAsync(RETRY_MS); expect(await ensureServerList()).toBe("fallback"); expect(backendReachable()).toBe(true); - expect(seen).toEqual([{ reachable: true }]); + expect(seen).toEqual([{ reachable: true, confirmed: false }]); // A network error is not an answer. fetchMock.mockRejectedValue(new TypeError("network down")); await vi.advanceTimersByTimeAsync(RETRY_MS); expect(await ensureServerList()).toBe("fallback"); expect(backendReachable()).toBe(false); - expect(seen).toEqual([{ reachable: true }, { reachable: false }]); + expect(seen).toEqual([ + { reachable: true, confirmed: false }, + { reachable: false, confirmed: false }, + ]); // Back up again. fetchMock.mockImplementation(async () => jsonResponse(API_LIST)); @@ -447,9 +528,9 @@ describe("backend reachability", () => { expect(await ensureServerList()).toBe("api"); expect(backendReachable()).toBe(true); expect(seen).toEqual([ - { reachable: true }, - { reachable: false }, - { reachable: true }, + { reachable: true, confirmed: false }, + { reachable: false, confirmed: false }, + { reachable: true, confirmed: false }, ]); } finally { document.removeEventListener("backend-reachability", listener); @@ -469,19 +550,42 @@ describe("retryServerList", () => { expect(await ensureServerList()).toBe("fallback"); expect(fetchMock).toHaveBeenCalledTimes(1); - // The control: an ordinary caller in the same moment is held back. + // The control: an ordinary caller in the same moment is held back for a + // full RETRY_MS. The manual retry is not. expect(await ensureServerList()).toBe("fallback"); expect(fetchMock).toHaveBeenCalledTimes(1); expect(await retryServerList()).toBe("fallback"); expect(fetchMock).toHaveBeenCalledTimes(2); - // And again, still well inside the interval. - await vi.advanceTimersByTimeAsync(100); + // Past its own 1s floor, and still far inside RETRY_MS. + await vi.advanceTimersByTimeAsync(1_500); await retryServerList(); expect(fetchMock).toHaveBeenCalledTimes(3); }); + // A player leaning on the button must not outpace the request it starts. + it("throttles a second press inside its floor to the same attempt", async () => { + vi.useFakeTimers(); + fetchMock.mockRejectedValue(new TypeError("network down")); + const first = retryServerList(); + expect(fetchMock).toHaveBeenCalledTimes(1); + await first; + + // Settled, but still inside the floor: the press is a no-op that hands + // back the same promise rather than starting a second request. + await vi.advanceTimersByTimeAsync(300); + const second = retryServerList(); + expect(second).toBe(first); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(800); + const third = retryServerList(); + expect(third).not.toBe(first); + expect(fetchMock).toHaveBeenCalledTimes(2); + await third; + }); + it("applies a list that the retry brings back, and clears the offline state", async () => { vi.useFakeTimers(); const seen: unknown[] = []; @@ -495,12 +599,16 @@ describe("retryServerList", () => { expect(ClientEnv.serverWsBase()).toBe("wss://blue.openfront.io"); fetchMock.mockImplementation(async () => jsonResponse(API_LIST)); + await vi.advanceTimersByTimeAsync(1_500); expect(await retryServerList()).toBe("api"); expect(backendReachable()).toBe(true); // Not just the flag: the list the retry fetched is applied, which is // what makes the bar disappear AND what the next join will use. expect(ClientEnv.serverWsBase()).toBe("wss://falk2-b.openfront.io"); - expect(seen).toEqual([{ reachable: false }, { reachable: true }]); + expect(seen).toEqual([ + { reachable: false, confirmed: false }, + { reachable: true, confirmed: false }, + ]); } finally { document.removeEventListener("backend-reachability", listener); } From 8588344b378c203eb6981ba4ffb34e2c45925c54 Mon Sep 17 00:00:00 2001 From: Josh Harris Date: Sat, 12 Sep 2026 13:04:49 +0100 Subject: [PATCH 04/10] Pin which refused joins cancel the queue The blockedJoin branch that closes the matchmaking modal had no test, so a revert of it would have gone unnoticed -- and both halves of it matter: closing it at all (otherwise the player sits on "waiting for a game", holding a queue slot, over a match they were already refused), and NOT closing it for any other source (otherwise a refused deep link cancels a queue someone is legitimately waiting in). Two tests in the boot harness, one per half. The modal is spied rather than opened for real: opening it would open a queue WebSocket, and the claim under test is only which joins reach close(). The negative case waits for the refusal itself to land before asserting close() was not called, so it cannot pass by simply having tested nothing yet. Checked against a revert: deleting the branch fails "takes a refused matchmade join out of the queue" and nothing else. OPE-439 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PTKyUrxqfwKvf2QxAovR6Z --- tests/client/MainInitialize.test.ts | 84 +++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/tests/client/MainInitialize.test.ts b/tests/client/MainInitialize.test.ts index 2f8be61906..1ba727383c 100644 --- a/tests/client/MainInitialize.test.ts +++ b/tests/client/MainInitialize.test.ts @@ -476,6 +476,90 @@ describe("Client.initialize() booted from Main.ts module scope", () => { expect(mocks.joinLobby).not.toHaveBeenCalled(); }); + /** + * The matchmaking modal dispatches its OWN join once the server matches + * it, so a refusal at the funnel leaves that modal sitting on "waiting + * for a game" over a match the player will never enter -- and, worse, + * holding a queue slot from a screen that is lying to them. + * + * Spied rather than driven for real: opening the modal for real would + * open a queue WebSocket, and the claim under test is only which joins + * reach close(). + */ + function spyOnMatchmakingModal(): { + close: ReturnType; + restore: () => void; + } { + const modal = document.querySelector("matchmaking-modal") as unknown as { + isOpen: () => boolean; + close: () => void; + }; + expect(modal).not.toBeNull(); + const isOpen = vi.spyOn(modal, "isOpen").mockReturnValue(true); + const close = vi.spyOn(modal, "close").mockImplementation(() => {}); + return { + close, + restore: () => { + close.mockRestore(); + isOpen.mockRestore(); + }, + }; + } + + it("takes a refused matchmade join out of the queue", async () => { + await settleReachability(false); + messages.length = 0; + const matchmaking = spyOnMatchmakingModal(); + + try { + document.dispatchEvent( + new CustomEvent("join-lobby", { + detail: { gameID: "AbCd1234", source: "matchmaking" }, + bubbles: true, + }), + ); + + // close() is the same teardown its Back button uses: it shuts the + // queue socket and clears the watchdog, so the player actually + // leaves the queue rather than staring at a stale "waiting" screen. + await vi.waitFor(() => expect(matchmaking.close).toHaveBeenCalled()); + expect(mocks.joinLobby).not.toHaveBeenCalled(); + } finally { + matchmaking.restore(); + } + }); + + it("leaves a live queue alone when the refused join came from elsewhere", async () => { + // The other half of the scoping, and the reason it is not just + // "always close it": someone can be legitimately queued while a deep + // link or a lobby click is refused, and cancelling their queue over + // an unrelated refusal would be its own bug. + await settleReachability(false); + messages.length = 0; + const matchmaking = spyOnMatchmakingModal(); + + try { + document.dispatchEvent( + new CustomEvent("join-lobby", { + detail: { gameID: "AbCd1234", source: "private" }, + bubbles: true, + }), + ); + + // Wait for the refusal itself to land, so "close was not called" is + // about the scoping rather than about nothing having happened yet. + await vi.waitFor(() => + expect(messages).toContain( + translateText("common.backend_unreachable"), + ), + ); + expect(matchmaking.close).not.toHaveBeenCalled(); + expect(mocks.joinLobby).not.toHaveBeenCalled(); + } finally { + matchmaking.restore(); + } + }); + it("lets the same join through once the backend answers", async () => { // The control. Without it a join refused for any unrelated reason -- // the username gate, a listener that never ran -- would pass above. From 4b7daa777fb010f634a39390783a49d8a14b779e Mon Sep 17 00:00:00 2001 From: Josh Harris Date: Sat, 12 Sep 2026 13:17:47 +0100 Subject: [PATCH 05/10] Pin what MatchmakingModal.close() actually tears down The funnel test spies on close() because its claim is WHICH joins reach it, and that spy is only worth something if the real close() genuinely takes the player out of the queue. Nothing asserted that: tests/client/Matchmaking.ts covered the clan-aware joins, the identity gate and the rejection codes, but never the teardown. Two tests against a real modal and its real (fake) socket, in the harness that file already has. The queue is in-memory on the server and keyed to the socket, so "left the queue" IS "the socket is shut" -- and the timers matter just as much, because a watchdog left running after a close reconnects and puts the player straight back in the queue they just left. The second test covers the close frame that a deliberate close itself produces: handled normally that reads as "the service restarted, rejoin", which is the failure intentionalClose exists to prevent. Checked against a revert, both halves: dropping socket.close()/clearWatchdog() fails the first, and dropping intentionalClose as well fails both. OPE-439 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PTKyUrxqfwKvf2QxAovR6Z --- tests/client/Matchmaking.test.ts | 62 ++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/client/Matchmaking.test.ts b/tests/client/Matchmaking.test.ts index 9819506a25..c6cf3345d4 100644 --- a/tests/client/Matchmaking.test.ts +++ b/tests/client/Matchmaking.test.ts @@ -367,3 +367,65 @@ describe("MatchmakingModal identity gate", () => { expect(modal.isOpen()).toBe(false); }); }); + +/** + * What close() actually does, as opposed to who calls it. + * + * Main.blockedJoin calls this modal's close() when it refuses a matchmade + * join (OPE-439), and the test for that spies on close() because its claim is + * which joins reach it. That spy is only worth anything if the real close() + * genuinely takes the player out of the queue -- so that half is pinned here, + * against a real modal and its real socket, where it belongs. + * + * The queue is in-memory on the server and keyed to the socket, so "left the + * queue" IS "the socket is shut". The timers matter just as much: the + * watchdog exists to reconnect through a dropped connection, and a watchdog + * left running after a close would put the player straight back in the queue + * they just left. + */ +describe("MatchmakingModal.close() teardown", () => { + beforeEach(() => { + vi.useFakeTimers(); + sockets.length = 0; + apiMocks.getUserMe.mockReset(); + apiMocks.invalidateUserMe.mockReset(); + vi.stubGlobal("WebSocket", FakeWebSocket); + vi.spyOn(console, "log").mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + vi.useRealTimers(); + }); + + it("shuts the queue socket and cancels the watchdog", async () => { + apiMocks.getUserMe.mockResolvedValue(userMe()); + const { modal, socket } = await openAndJoin("1v1"); + expect(socket.readyState).toBe(FakeWebSocket.OPEN); + + modal.close(); + + expect(socket.readyState).toBe(FakeWebSocket.CLOSED); + // The watchdog fires after 15s of server silence and reconnects, which + // would open a second socket and re-queue the player. Well past that and + // past every reconnect backoff, there is still only the one. + await vi.advanceTimersByTimeAsync(60_000); + expect(sockets).toHaveLength(1); + }); + + it("does not reconnect when the server's close frame lands afterwards", async () => { + // Shutting a socket produces a close frame, and the ordinary handling of + // one is "the service restarted, rejoin". After a deliberate close that + // would silently put the player back in the queue they were just taken + // out of, which is the failure mode intentionalClose exists to prevent. + apiMocks.getUserMe.mockResolvedValue(userMe()); + const { modal, socket } = await openAndJoin("1v1"); + + modal.close(); + socket.serverClose(1011, ""); + + await vi.advanceTimersByTimeAsync(60_000); + expect(sockets).toHaveLength(1); + }); +}); From 18536f5d87dd095bae7d44ceac5711a4c0edcc61 Mon Sep 17 00:00:00 2001 From: Josh Harris Date: Sun, 13 Sep 2026 16:00:40 +0100 Subject: [PATCH 06/10] Count joins from zero in the reachability funnel tests Rebase onto #5383, which added a pinned-page test earlier in the same file that joins a lobby for real and leaves the call on the shared mock. The funnel tests' "was not joined" and "joined exactly once" claims counted from that call. Cleared once at the describe's start. OPE-439 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PTKyUrxqfwKvf2QxAovR6Z --- tests/client/MainInitialize.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/client/MainInitialize.test.ts b/tests/client/MainInitialize.test.ts index 1ba727383c..b29c28ec11 100644 --- a/tests/client/MainInitialize.test.ts +++ b/tests/client/MainInitialize.test.ts @@ -436,6 +436,9 @@ describe("Client.initialize() booted from Main.ts module scope", () => { beforeAll(async () => { ServerList = await import("../../src/client/ServerList"); + // The pinned-page test above joins once and leaves the call on the + // mock; every "was not joined" claim below counts from zero. + mocks.joinLobby.mockClear(); // Test 3 above left the username gate closed; every join below has to // get past it to reach the gate under test. const input = document.querySelector("username-input") as unknown as { From 526186227c282d5db05bf8f5c9ef142245bd72f0 Mon Sep 17 00:00:00 2001 From: Josh Harris Date: Sun, 13 Sep 2026 16:26:43 +0100 Subject: [PATCH 07/10] Stop the join funnel from refusing on a server-list outage backendUnreachableConfirmed() tracks the SERVER-LIST API, not the game servers, and by the time a join-lobby event reaches Main's funnel the client has already reached one. I checked all four dispatch sites: "private" (JoinLobbyModal.checkActiveLobby) fires only after a 200 + exists:true from the game's own server; "host" (HostLobbyModal) only after createLobby() resolved with a server-minted id; "public" (GameModeSelector.validateAndJoin) only from a lobby card delivered over a live PublicLobbySocket; "matchmaking" (Matchmaking.checkGame) only after the queue socket (ClientEnv.jwtIssuer()) matched AND an /exists probe of the game server came back. None of them depends on the list API, so a refusal there could only reject a join already under way -- worst case ejecting a player who pressed F5 mid-game during a list-API blip: checkActiveLobby proves the game is live, the funnel refuses, closes the join modal, which leaves the lobby and resets the URL. That also contradicted docs/MultiServer.md ("nothing here touches a game already in progress"). shouldBlockJoin drops its backendOutage parameter and passes false to shouldBlockMultiplayerAction; Main.blockedJoin no longer reads the accessor and reports with reportMultiplayerRefusal(false) (desktop wiggle only). Everything else keeps the signal: the status bar, the dimmed buttons in GameModeSelector and DetailedGameViewModal, and the web toast for a refused button press, so common.backend_unreachable stays in en.json. OPE-439 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PTKyUrxqfwKvf2QxAovR6Z --- docs/MultiServer.md | 22 ++++- src/client/GameModeSelector.ts | 20 +++-- src/client/Main.ts | 19 ++-- tests/GameModeSelectorGating.test.ts | 50 +++++------ tests/client/MainInitialize.test.ts | 126 ++++++++++++++------------- tests/client/Matchmaking.test.ts | 5 +- 6 files changed, 129 insertions(+), 113 deletions(-) diff --git a/docs/MultiServer.md b/docs/MultiServer.md index a5509b8993..4a32cd7087 100644 --- a/docs/MultiServer.md +++ b/docs/MultiServer.md @@ -441,10 +441,24 @@ values. retry that fails counts towards the outage confirmation like any other attempt. - What consumes the confirmed signal: the desktop status bar's offline - state (ranked below a session failure, above any update state), and the - multiplayer gates in `GameModeSelector`, `DetailedGameViewModal` and - `Main`'s join funnel — on the web as well as on desktop. Single-player is + What consumes the confirmed signal, and what it does: the desktop status + bar's offline state (ranked below a session failure, above any update + state), and the multiplayer _buttons_ in `GameModeSelector` and + `DetailedGameViewModal`, which dim and refuse a press — on the web as well + as on desktop, where the press also raises a + `common.backend_unreachable` toast, since there is no status bar there to + name the reason. + + What it deliberately does **not** do: refuse a join that is already under + way. `Main`'s join funnel (`shouldBlockJoin`) weighs only the desktop + update and session states; reachability is not an input (OPE-439). Every + source that dispatches a join has already reached a server to produce it + — `private` after `checkActiveLobby` read `exists` from the game's own + server, `host` after `createLobby` minted the id, `public` from a lobby + list arriving over a live server socket, `matchmaking` after the queue + matched — so the server-list API's health says nothing about the join in + hand. Refusing there would only ever be wrong, and at worst would eject a + player whose reload had just proved their game is live. Single-player is never gated, and nothing here touches a game already in progress. - **Which list:** the desktop shell asks for its injected `serverHost` diff --git a/src/client/GameModeSelector.ts b/src/client/GameModeSelector.ts index 6c7221f88e..e1d5bb1ae5 100644 --- a/src/client/GameModeSelector.ts +++ b/src/client/GameModeSelector.ts @@ -160,20 +160,28 @@ export function joinIsGateable(lobby: JoinLobbyEvent): boolean { * The whole gate decision for one join, as a pure function so it is testable * without mounting Main's client. Main adds only the shell check (which * decides whether the two desktop states are even read) and the refusal - * feedback around it. + * feedback around it. Both halves it does weigh -- the update state and the + * session state -- are desktop-only. * - * Named for the join rather than for the desktop since OPE-439: the update - * and session halves are still desktop-only, but an unreachable backend - * refuses a join on the web too. + * Backend reachability is deliberately NOT an input here (OPE-439). Every + * source that dispatches a join has already reached a server to produce it: + * "private" only after checkActiveLobby read `exists` from the game's own + * server, "host" only after createLobby minted the id, "public" from a lobby + * list arriving over a live server socket, and "matchmaking" only after the + * queue matched and checkGame confirmed the game exists. The outage signal + * tracks the separate server-list API, whose health says nothing about those + * servers, so refusing here could only reject a join that is already under + * way. Worst case it ejects a player mid-game: a reload during a list-API + * blip proves the game is live, then the refusal closes the join modal, + * which leaves the lobby and resets the URL. */ export function shouldBlockJoin( lobby: JoinLobbyEvent, update: DesktopUpdateState | null, session: DesktopSessionState | null, - backendOutage: boolean, ): boolean { if (!joinIsGateable(lobby)) return false; - return shouldBlockMultiplayerAction(update, session, backendOutage); + return shouldBlockMultiplayerAction(update, session, false); } @customElement("game-mode-selector") diff --git a/src/client/Main.ts b/src/client/Main.ts index f01d9e3e05..c606380e32 100644 --- a/src/client/Main.ts +++ b/src/client/Main.ts @@ -94,7 +94,6 @@ import { } from "./PresenceGroup"; import { RewardsModal } from "./RewardsModal"; import { - backendUnreachableConfirmed, ensureServerList, redirectToGameVersion, startServerListPolling, @@ -1202,26 +1201,21 @@ class Client { * unit-tested; this adds the shell check, the modal cleanup and the * feedback. * - * Two of the three inputs are desktop-only and are read only there. The - * third, a confirmed backend outage, applies to the web too (OPE-439): a - * join that would open a socket to a backend we have repeatedly failed to - * reach is refused rather than left to time out in the lobby. + * Both inputs are desktop-only and are read only there. Backend + * reachability is not among them (OPE-439): by the time a join reaches + * this funnel its source has already reached a server, so the server-list + * API being unreachable is no reason to refuse -- see shouldBlockJoin. * * Says why rather than failing silently, matching what the dimmed buttons * do. */ private blockedJoin(lobby: JoinLobbyEvent): boolean { const desktop = isDesktopShell(); - // Read straight from the module rather than kept in a field: it is a - // synchronous accessor over the heartbeat's own state, so there is no - // event to miss and nothing to seed. - const backendOutage = backendUnreachableConfirmed(); if ( !shouldBlockJoin( lobby, desktop ? this.desktopUpdateState : null, desktop ? getDesktopSessionState() : null, - backendOutage, ) ) { return false; @@ -1244,7 +1238,10 @@ class Client { if (lobby.source === "matchmaking" && this.matchmakingModal?.isOpen()) { this.matchmakingModal.close(); } - reportMultiplayerRefusal(backendOutage); + // false: the web never refuses here any more, so the only feedback left + // is the desktop status bar's wiggle -- the bar is already showing the + // update or session reason that refused this join. + reportMultiplayerRefusal(false); return true; } diff --git a/tests/GameModeSelectorGating.test.ts b/tests/GameModeSelectorGating.test.ts index 77f213642c..76860936fe 100644 --- a/tests/GameModeSelectorGating.test.ts +++ b/tests/GameModeSelectorGating.test.ts @@ -208,22 +208,15 @@ describe("shouldBlockJoin", () => { const healthy = { status: "current", bytes: 0, total: 0 } as const; it("allows a multiplayer join when both states are healthy", () => { - expect(shouldBlockJoin(mp, healthy, { status: "signed-in" }, false)).toBe( - false, - ); + expect(shouldBlockJoin(mp, healthy, { status: "signed-in" })).toBe(false); }); it("blocks a multiplayer join when signed out", () => { expect( - shouldBlockJoin( - mp, - healthy, - { - status: "signed-out", - reason: "steam-wedged", - }, - false, - ), + shouldBlockJoin(mp, healthy, { + status: "signed-out", + reason: "steam-wedged", + }), ).toBe(true); }); @@ -233,8 +226,9 @@ describe("shouldBlockJoin", () => { shouldBlockJoin( mp, { status: "staged", bytes: 0, total: 0 }, - { status: "signed-in" }, - false, + { + status: "signed-in", + }, ), ).toBe(true); }); @@ -244,27 +238,25 @@ describe("shouldBlockJoin", () => { shouldBlockJoin( solo, { status: "staged", bytes: 0, total: 0 }, - { status: "signed-out", reason: "steam-wedged" }, - true, + { + status: "signed-out", + reason: "steam-wedged", + }, ), ).toBe(false); }); it("does not block on the web, where neither desktop state exists", () => { - expect(shouldBlockJoin(mp, null, null, false)).toBe(false); - }); - - // OPE-439. The one input that gates on the web as well as on desktop. - it("blocks a multiplayer join on a confirmed backend outage", () => { - expect(shouldBlockJoin(mp, null, null, true)).toBe(true); - expect(shouldBlockJoin(mp, healthy, { status: "signed-in" }, true)).toBe( - true, - ); + expect(shouldBlockJoin(mp, null, null)).toBe(false); }); - it("never blocks single-player on a backend outage", () => { - // The desktop build's core offline promise: bot games run entirely - // in-client, so an unreachable backend is no reason to refuse one. - expect(shouldBlockJoin(solo, null, null, true)).toBe(false); + // OPE-439. Reachability is not a funnel input at all: by the time a join + // arrives here its source has already reached a server (an /exists probe, + // createLobby, a live lobby socket, or the matchmaking queue), so the + // server-list API's health cannot make this join wrong. Refusing would + // eject a player whose reload just proved their game is live. + it("never blocks a join, whatever the reachability signal says", () => { + expect(shouldBlockJoin(mp, null, null)).toBe(false); + expect(shouldBlockJoin(mp, healthy, { status: "signed-in" })).toBe(false); }); }); diff --git a/tests/client/MainInitialize.test.ts b/tests/client/MainInitialize.test.ts index b29c28ec11..ac5d33acb3 100644 --- a/tests/client/MainInitialize.test.ts +++ b/tests/client/MainInitialize.test.ts @@ -399,7 +399,7 @@ describe("Client.initialize() booted from Main.ts module scope", () => { * Driven through the real ServerList module: the reachability the funnel * reads has to be the one the heartbeat actually produces. */ - describe("the join funnel while the backend is unreachable", () => { + describe("the join funnel lets joins through while the server-list API is unreachable", () => { let ServerList: typeof import("../../src/client/ServerList"); let messages: string[]; let onMessage: EventListener; @@ -437,7 +437,7 @@ describe("Client.initialize() booted from Main.ts module scope", () => { beforeAll(async () => { ServerList = await import("../../src/client/ServerList"); // The pinned-page test above joins once and leaves the call on the - // mock; every "was not joined" claim below counts from zero. + // mock; every join-count claim below counts from zero. mocks.joinLobby.mockClear(); // Test 3 above left the username gate closed; every join below has to // get past it to reach the gate under test. @@ -457,37 +457,16 @@ describe("Client.initialize() booted from Main.ts module scope", () => { ServerList.resetServerList(); }); - it("refuses a join and says why once the outage is confirmed", async () => { - await settleReachability(false); - logSpy.mockClear(); - messages.length = 0; - - document.dispatchEvent( - new CustomEvent("join-lobby", { - detail: { gameID: "AbCd1234", source: "matchmaking" }, - bubbles: true, - }), - ); - - await vi.waitFor(() => - expect(messages).toContain(translateText("common.backend_unreachable")), - ); - // Refused before anything was joined -- not merely reported after. - expect(logSpy).not.toHaveBeenCalledWith( - expect.stringContaining("joining lobby"), - ); - expect(mocks.joinLobby).not.toHaveBeenCalled(); - }); - /** - * The matchmaking modal dispatches its OWN join once the server matches - * it, so a refusal at the funnel leaves that modal sitting on "waiting - * for a game" over a match the player will never enter -- and, worse, - * holding a queue slot from a screen that is lying to them. + * The matchmaking modal's close() is the teardown its Back button uses: + * it shuts the queue socket and clears the watchdog. The funnel reaches + * for it only when it refuses a matchmade join, so it is also the + * sharpest witness that an outage does NOT refuse -- if the funnel had + * gated here, a queued player would have been dropped out of the queue. * * Spied rather than driven for real: opening the modal for real would - * open a queue WebSocket, and the claim under test is only which joins - * reach close(). + * open a queue WebSocket, and the claim under test is only whether any + * join reaches close(). */ function spyOnMatchmakingModal(): { close: ReturnType; @@ -509,55 +488,87 @@ describe("Client.initialize() booted from Main.ts module scope", () => { }; } - it("takes a refused matchmade join out of the queue", async () => { + /** The shape handleJoinLobby expects back; neither promise settles. */ + function stubJoinLobbyReturn(): void { + mocks.joinLobby.mockReturnValue({ + prestart: new Promise(() => {}), + join: new Promise(() => {}), + stop: vi.fn(), + }); + } + + // OPE-439. A confirmed outage means the SERVER-LIST API has missed two + // heartbeats -- it says nothing about the game server this join is + // headed for, which the dispatching source has already reached. The + // sharpest case is this one: a player mid-game reloads during a + // list-API blip, JoinLobbyModal.checkActiveLobby reads `exists` from the + // game's own server, and refusing here would close the join modal, + // leave the lobby and reset the URL -- ejecting them from a game that + // was confirmed live moments earlier. + it("joins a private game the source already confirmed, outage and all", async () => { await settleReachability(false); + logSpy.mockClear(); messages.length = 0; + mocks.joinLobby.mockClear(); + stubJoinLobbyReturn(); const matchmaking = spyOnMatchmakingModal(); try { document.dispatchEvent( new CustomEvent("join-lobby", { - detail: { gameID: "AbCd1234", source: "matchmaking" }, + detail: { gameID: "AbCd1234", source: "private" }, bubbles: true, }), ); - // close() is the same teardown its Back button uses: it shuts the - // queue socket and clears the watchdog, so the player actually - // leaves the queue rather than staring at a stale "waiting" screen. - await vi.waitFor(() => expect(matchmaking.close).toHaveBeenCalled()); - expect(mocks.joinLobby).not.toHaveBeenCalled(); + // The far edge of the funnel, not the "joining lobby" log: that log + // is written BEFORE handleJoinLobby awaits userAuth, the username + // seed and the cosmetics refs, so a regression in that tail would + // leave a log assertion passing over a join that never happened. + await vi.waitFor(() => + expect(mocks.joinLobby).toHaveBeenCalledTimes(1), + ); + // ...and with the lobby that was dispatched, not some other one. + expect(mocks.joinLobby.mock.calls[0][1].gameID).toBe("AbCd1234"); + // No toast either: the funnel refuses nothing, so it has nothing to + // report. The dimmed buttons and the status bar still carry the + // outage, and a refused BUTTON press still says why -- that half is + // covered in ReachabilityGating. + expect(messages).not.toContain( + translateText("common.backend_unreachable"), + ); + // And nothing tore down the queue behind a player's back. + expect(matchmaking.close).not.toHaveBeenCalled(); } finally { matchmaking.restore(); } }); - it("leaves a live queue alone when the refused join came from elsewhere", async () => { - // The other half of the scoping, and the reason it is not just - // "always close it": someone can be legitimately queued while a deep - // link or a lobby click is refused, and cancelling their queue over - // an unrelated refusal would be its own bug. + it("does not cancel a live matchmaking queue during an outage", async () => { + // The same claim on the source that owns that modal: a matchmade join + // arriving during an outage is joined, not refused, so the queue + // teardown never runs. await settleReachability(false); messages.length = 0; + mocks.joinLobby.mockClear(); + stubJoinLobbyReturn(); const matchmaking = spyOnMatchmakingModal(); try { document.dispatchEvent( new CustomEvent("join-lobby", { - detail: { gameID: "AbCd1234", source: "private" }, + detail: { gameID: "AbCd1234", source: "matchmaking" }, bubbles: true, }), ); - // Wait for the refusal itself to land, so "close was not called" is - // about the scoping rather than about nothing having happened yet. await vi.waitFor(() => - expect(messages).toContain( - translateText("common.backend_unreachable"), - ), + expect(mocks.joinLobby).toHaveBeenCalledTimes(1), ); expect(matchmaking.close).not.toHaveBeenCalled(); - expect(mocks.joinLobby).not.toHaveBeenCalled(); + expect(messages).not.toContain( + translateText("common.backend_unreachable"), + ); } finally { matchmaking.restore(); } @@ -565,18 +576,13 @@ describe("Client.initialize() booted from Main.ts module scope", () => { it("lets the same join through once the backend answers", async () => { // The control. Without it a join refused for any unrelated reason -- - // the username gate, a listener that never ran -- would pass above. + // the username gate, a listener that never ran -- would make the cases + // above vacuous, since they too assert a join that goes through. await settleReachability(true); logSpy.mockClear(); messages.length = 0; - mocks.joinLobby.mockReturnValue({ - // Neither settles: the join is complete once joinLobby has been - // handed the lobby, and the in-game path beyond that is not what - // this file boots. - prestart: new Promise(() => {}), - join: new Promise(() => {}), - stop: vi.fn(), - }); + mocks.joinLobby.mockClear(); + stubJoinLobbyReturn(); document.dispatchEvent( new CustomEvent("join-lobby", { @@ -589,9 +595,7 @@ describe("Client.initialize() booted from Main.ts module scope", () => { // written BEFORE handleJoinLobby awaits userAuth, the username seed and // the cosmetics refs, so a regression anywhere in that tail would leave // the log assertion passing over a join that never happened. joinLobby - // is the call that actually starts one, and the refusal test above - // asserts the same mock was never reached -- so the count being exactly - // one here is the pair of that claim. + // is the call that actually starts one. await vi.waitFor(() => expect(mocks.joinLobby).toHaveBeenCalledTimes(1)); // ...and with the lobby that was dispatched, not some other one. expect(mocks.joinLobby.mock.calls[0][1].gameID).toBe("AbCd1234"); diff --git a/tests/client/Matchmaking.test.ts b/tests/client/Matchmaking.test.ts index c6cf3345d4..864005890a 100644 --- a/tests/client/Matchmaking.test.ts +++ b/tests/client/Matchmaking.test.ts @@ -372,8 +372,9 @@ describe("MatchmakingModal identity gate", () => { * What close() actually does, as opposed to who calls it. * * Main.blockedJoin calls this modal's close() when it refuses a matchmade - * join (OPE-439), and the test for that spies on close() because its claim is - * which joins reach it. That spy is only worth anything if the real close() + * join -- on desktop, over a pending update or a lapsed session; backend + * reachability is not a funnel input (OPE-439) -- and the tests for that spy + * on close() because their claim is which joins reach it. That spy is only worth anything if the real close() * genuinely takes the player out of the queue -- so that half is pinned here, * against a real modal and its real socket, where it belongs. * From 4d25f95c992f0be8953a6522680ec96c463c86fa Mon Sep 17 00:00:00 2001 From: Josh Harris Date: Sun, 13 Sep 2026 22:15:28 +0100 Subject: [PATCH 08/10] Back the server list heartbeat's retries off exponentially A failed attempt was retried on a flat 10s interval, forever. That is the right number for a blip and the wrong one for the long tail: a laptop with its lid closed, or a player in a tunnel, keeps firing a 4s request every 10s for as long as the tab is open, and a backend that is genuinely down takes that from every tab at once. retryDelayMs(consecutiveFailures) is now the schedule, and it is a pure function of the count so it can be read (and tested) without a clock: RETRY_BASE_MS (10s, today's value) after the first unanswered attempt, doubling on each further consecutive one, capped at RETRY_MAX_MS (60s). Any answer at all resets the count and so the schedule -- a 404 included, since that is a reachable backend -- which means a page that recovers and then misses once is retried in 10s rather than inheriting the old outage's wait. retryDue() and scheduleNextPoll() both read it; REFRESH_INTERVAL_MS, the success cadence, is untouched. The confirmation rule is unaffected in both letter and timing: two consecutive failures, and the second one is still due a base interval after the first, because the backoff only starts stretching once there IS an outage to back off from. Manual retries keep the semantics they had -- a failed one counts, a successful one resets -- since they go through the same recordAttempt. Also exposes what the Retry button needs to stop offering a press that could only join an attempt already out: attemptInFlight() plus a "server-list-attempt" document event on start and settle. Deliberately not folded into "backend-reachability", which fires only when reachability CHANGES -- an attempt that fails exactly like the last one announces nothing there, and that is precisely the case the button has to see. Test timings adjusted where the schedule moved: the outage test's third attempt now waits 20s and the recovery 40s (and asserts the attempt actually went out, so the wait is evidence rather than an accident), and the heartbeat test carries on past its first failure to pin 10s -> 20s and the return to the base after an answer. OPE-439 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PTKyUrxqfwKvf2QxAovR6Z --- docs/MultiServer.md | 38 +++++--- src/client/ServerList.ts | 106 +++++++++++++++++++--- tests/client/ServerList.test.ts | 150 ++++++++++++++++++++++++++++++-- 3 files changed, 261 insertions(+), 33 deletions(-) diff --git a/docs/MultiServer.md b/docs/MultiServer.md index 4a32cd7087..bd24e3d996 100644 --- a/docs/MultiServer.md +++ b/docs/MultiServer.md @@ -405,16 +405,27 @@ values. - **Fetched at page load, then a heartbeat.** `startServerListPolling()` runs early in `Client.initialize()`: the first fetch overlaps with the - rest of boot, and the list is refreshed every 30s, retried every 10s - after a failed attempt. Each fetch is bounded (4s), so offline - singleplayer waits seconds at worst and never hangs. + rest of boot, and the list is refreshed every 30s on success. Each fetch + is bounded (4s), so offline singleplayer waits seconds at worst and never + hangs. +- **Failed attempts back off.** `retryDelayMs(consecutiveFailures)` is the + schedule, and it is a pure function so it can be read without a clock: 10s + after the first unanswered attempt, doubling on each further consecutive + one (20s, 40s), capped at 60s. **Any** answer at all — a 404 included — + resets it to the base, so a page that recovers and then misses once is + retried in 10s rather than inheriting the old outage's wait. The base is + short because the common case is a blip the next request clears; the cap + exists because a lid-closed laptop should not fire a request every 10s all + night, and by a minute in the player who is still waiting has the Retry + button. The success cadence (30s) is untouched by any of this. - **A click never waits when a list is known.** `ensureServerList()` answers from the cached list whatever its age and revalidates behind the answer (stale-while-revalidate); only a page that has never got a list waits for a fetch — the one in flight, or one it starts. A page with no - list whose last attempt failed under 10s ago starts none: it answers - `fallback` and leaves retrying to the heartbeat, so a caller on a timer - (the matchmaking poll, every second) cannot hammer a down API. + list whose last attempt failed less than the current backoff delay ago + starts none: it answers `fallback` and leaves retrying to the heartbeat, + so a caller on a timer (the matchmaking poll, every second) cannot hammer + a down API. - **A failed refresh keeps the last good list.** Network error, timeout, non-OK, malformed or empty: the previous list keeps serving. The API caches its answer for seconds anyway, so a blip must not flip a working @@ -425,17 +436,20 @@ values. site), false on a timeout or network error. It is deliberately twitchy, so nothing player-facing gates on it. `backendUnreachableConfirmed()` is the debounced one the UI uses: true - only once **two** attempts in a row have gone unanswered, which takes a - retry interval to accumulate. One missed beat is a blip the cached list - serves straight through, and dimming multiplayer for 10s over it would be - worse than the blip; any answer resets the count. Every change to either + only once **two** attempts in a row have gone unanswered, which takes the + base retry delay (10s) to accumulate — the backoff only stretches once + there is an outage to back off from, so confirmation is never slowed by + it. One missed beat is a blip the cached list serves straight through, and + dimming multiplayer for 10s over it would be worse than the blip; any + answer resets the count. Every change to either value is announced on the document as `backend-reachability` with `{ reachable, confirmed }`. Consumers seed from the accessor and then subscribe — the event is one-shot, so a component mounting afterwards would otherwise never learn the state (OPE-396). - **Retry:** `retryServerList()` is the player-initiated attempt behind the - desktop status bar's offline Retry. It ignores the heartbeat's retry - interval (a person pressing a button is not a timer) but has a 1s floor + desktop status bar's offline Retry. It ignores the heartbeat's backoff (a + person pressing a button is not a timer, and once an outage has run a + while that wait is up to a minute) but has a 1s floor of its own, inside which a second press hands back the same promise; past that, `fetchOnce()` still dedupes against an attempt already in flight. A retry that fails counts towards the outage confirmation like any other diff --git a/src/client/ServerList.ts b/src/client/ServerList.ts index be03f15a39..13e46b7d2e 100644 --- a/src/client/ServerList.ts +++ b/src/client/ServerList.ts @@ -19,7 +19,8 @@ import { isReplayShellHost } from "./VersionedReplay"; // // The list is fetched once at page load (bounded, so offline singleplayer // waits seconds at worst and never hangs) and kept warm by a heartbeat: -// every REFRESH_INTERVAL_MS on success, RETRY_INTERVAL_MS after a failure. +// every REFRESH_INTERVAL_MS on success, and on a backing-off schedule after +// a failure (retryDelayMs). // Clicking Join or Create therefore never waits on the network — whatever // the list's age, ensureServerList() answers from the cached copy and // revalidates behind it. Only a page that has never seen a list waits for a @@ -35,21 +36,35 @@ import { isReplayShellHost } from "./VersionedReplay"; // The heartbeat doubles as the client's backend-reachability probe // (backendReachable() for the raw per-attempt answer, // backendUnreachableConfirmed() for the debounced one the UI gates on, and -// the "backend-reachability" document event carrying both). +// the "backend-reachability" document event carrying both), and it says when +// it is busy (attemptInFlight() plus the "server-list-attempt" event), which +// is what the desktop status bar's Retry button disables itself on. // Bounded so an unreachable API costs one short wait, after which the // bootstrap values take over. const FETCH_TIMEOUT_MS = 4_000; // Heartbeat: how long a list is served before it is revalidated in the -// background, and how soon a failed attempt is retried. +// background. const REFRESH_INTERVAL_MS = 30_000; -const RETRY_INTERVAL_MS = 10_000; +// Retry schedule after an unanswered attempt: the first retry comes after +// RETRY_BASE_MS and each further consecutive failure doubles the wait, up to +// RETRY_MAX_MS. Any answer at all resets it to the base. +// +// The point of the backoff is the long tail. A player who closes their laptop +// lid, or sits on a train through a tunnel, should not have the page firing a +// 4s request every 10s for an hour; a backend that is genuinely down should +// not take that traffic from every open tab either. The base is short because +// the common case is a blip that the very next request clears, and the cap is +// a minute because past that point the player has almost certainly stopped +// waiting -- and the Retry button is the escape hatch for anyone who has not. +const RETRY_BASE_MS = 10_000; +const RETRY_MAX_MS = 60_000; // How many attempts in a row must go unanswered before the UI calls it an // outage. One failure is a blip -- a 4s timeout on a flaky connection, a // worker restart mid-beat, a proxy hiccup -- and the cached list keeps // serving straight through it, so gating multiplayer on the first one would // take the game away from a player whose next request would have worked. Two -// in a row, which take a retry interval to accumulate, is evidence. +// in a row, which take RETRY_BASE_MS to accumulate, is evidence. const CONFIRM_OUTAGE_AFTER_FAILURES = 2; // Floor between player-initiated retries, so someone leaning on the button // cannot outpace the request it started. Short enough that a deliberate @@ -183,6 +198,39 @@ export function backendUnreachableConfirmed(): boolean { ); } +/** + * Whether an attempt is out right now, automatic or manual. Synchronous, so + * UI that gates on it can seed itself at mount; the "server-list-attempt" + * event below carries every change. + */ +export function attemptInFlight(): boolean { + return inflight !== null; +} + +/** The detail carried by the "server-list-attempt" document event. */ +export interface ServerListAttemptDetail { + inFlight: boolean; +} + +/** + * Announced on the document when an attempt starts and again when it settles. + * + * Separate from "backend-reachability" on purpose: that one fires only on a + * CHANGE of reachability, so an attempt that fails exactly like the last one + * announces nothing at all -- which is precisely the case the desktop status + * bar's Retry button needs to see, since it disables itself while the + * heartbeat is already trying. Pairs with attemptInFlight() for the same + * reason every other signal here does: the event is one-shot, so a component + * mounting mid-attempt has only the accessor to read (OPE-396). + */ +function announceAttempt(inFlight: boolean): void { + document.dispatchEvent( + new CustomEvent("server-list-attempt", { + detail: { inFlight }, + }), + ); +} + /** The detail carried by the "backend-reachability" document event. */ export interface BackendReachabilityDetail { reachable: boolean; @@ -205,7 +253,7 @@ function recordAttempt(answered: boolean, cause?: unknown): void { const confirmed = backendUnreachableConfirmed(); if (wasReachable === answered && wasConfirmed === confirmed) return; // Only transitions are logged: the heartbeat runs forever and an offline - // player must not get a console line every RETRY_INTERVAL_MS. + // player must not get a console line on every beat. if (!answered && wasReachable !== false) { console.warn("Server list API unreachable, using known values", cause); } else if (!answered && confirmed && !wasConfirmed) { @@ -277,7 +325,9 @@ function fetchOnce(): Promise { }) .finally(() => { inflight = null; + announceAttempt(false); }); + announceAttempt(true); return inflight; } @@ -317,6 +367,11 @@ function runPoll(): void { // Scheduled after each attempt settles, never on a fixed interval: a slow // or hanging fetch must not stack attempts on top of each other. +// +// The retry side reads consecutiveFailures, which recordAttempt has already +// updated for the attempt that just settled -- so an answered attempt that +// carried no list (a 404 for this site) waits the base interval rather than +// inheriting an earlier outage's backoff. function scheduleNextPoll(gotList: boolean): void { if (!polling) return; pollTimer = setTimeout( @@ -324,7 +379,7 @@ function scheduleNextPoll(gotList: boolean): void { pollTimer = null; runPoll(); }, - gotList ? REFRESH_INTERVAL_MS : RETRY_INTERVAL_MS, + gotList ? REFRESH_INTERVAL_MS : retryDelayMs(consecutiveFailures), ); } @@ -392,17 +447,21 @@ export async function ensureServerList(): Promise { * Try the API again right now, at the player's request: the Retry on the * desktop status bar's offline state (OPE-439). * - * Deliberately ignores the heartbeat's retry interval. That interval exists + * Deliberately ignores the heartbeat's retry schedule. That backoff exists * to stop TIMER-driven callers hammering a down API between beats, and a * person pressing a button is not one of those -- holding their click for up - * to RETRY_INTERVAL_MS would make the button look broken in exactly the - * situation it exists for. + * to a minute (RETRY_MAX_MS, once an outage has been going a while) would + * make the button look broken in exactly the situation it exists for. * * It does have a floor of its own, MANUAL_RETRY_MIN_INTERVAL_MS. Inside that * window a second press is a no-op that hands back the same promise, so a * player leaning on the button cannot outpace the request it started. Past * the throttle, fetchOnce() still dedupes: a press landing on top of a - * heartbeat beat joins that attempt rather than starting a second. + * heartbeat beat joins that attempt rather than starting a second. This is + * the last line of defence, not the first: the button that calls this is + * itself disabled while an attempt is out and for a cooldown after a press + * (DesktopStatusBar), and the floor is what holds if anything ever calls + * this without going through such a button. * * A retry that fails counts towards the outage confirmation like any other * attempt -- pressing Retry against a backend that is genuinely down should @@ -433,12 +492,33 @@ async function runManualRetry(): Promise { } } +/** + * How long to wait before retrying, given how many attempts in a row have + * gone unanswered. Pure, so the schedule can be read (and tested) without a + * clock: RETRY_BASE_MS after the first failure, doubling on each further + * consecutive failure, capped at RETRY_MAX_MS. + * + * 1 -> 10s 2 -> 20s 3 -> 40s 4+ -> 60s + * + * Zero (or anything lower, defensively) is the base too: that is the state + * after an ANSWER, and the next beat's schedule should already be back to + * the short interval rather than inheriting the outage's. + */ +export function retryDelayMs(consecutiveFailures: number): number { + if (consecutiveFailures <= 1) return RETRY_BASE_MS; + // Math.min before the shift ever gets big: 2 ** 30 * 10s is still finite, + // but there is no reason to let a long outage compute an absurd number. + const doublings = Math.min(consecutiveFailures - 1, 20); + return Math.min(RETRY_BASE_MS * 2 ** doublings, RETRY_MAX_MS); +} + // Whether a caller may start a fresh attempt, or must leave it to the // heartbeat's next beat. Only a failed attempt holds anything back, and only -// for the retry interval; the cached list (if any) keeps serving meanwhile. +// for the current backoff delay; the cached list (if any) keeps serving +// meanwhile. function retryDue(): boolean { if (lastAttempt === null || !lastAttempt.failed) return true; - return Date.now() - lastAttempt.at >= RETRY_INTERVAL_MS; + return Date.now() - lastAttempt.at >= retryDelayMs(consecutiveFailures); } function apply(): ServerListStatus { diff --git a/tests/client/ServerList.test.ts b/tests/client/ServerList.test.ts index a96666ecf0..13e5cca119 100644 --- a/tests/client/ServerList.test.ts +++ b/tests/client/ServerList.test.ts @@ -2,12 +2,14 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { ClientEnv } from "../../src/client/ClientEnv"; import { resetPagePinForTests } from "../../src/client/PagePin"; import { + attemptInFlight, backendReachable, backendUnreachableConfirmed, ensureServerList, redirectToGameVersion, reloadWouldRescue, resetServerList, + retryDelayMs, retryServerList, serverListSite, serverListUrl, @@ -23,7 +25,10 @@ import { // known, and a failed refresh never throws the last good list away. const REFRESH_MS = 30_000; +// The FIRST retry delay. Later ones double (retryDelayMs), so anywhere a test +// needs a second or third failed attempt it spells the wait out. const RETRY_MS = 10_000; +const RETRY_MAX_MS = 60_000; const OWN = "bfd5563a11111111111111111111111111111111"; const OLD = "5ccc50a722222222222222222222222222222222"; @@ -366,6 +371,37 @@ describe("ensureServerList", () => { }); }); +// The retry schedule on its own, with no clock and no fetch: a failing API +// must not be asked every 10s forever (a lid-closed laptop would keep firing +// a 4s request all night), but the first retry has to stay quick because the +// common case is a blip the very next request clears. +describe("retryDelayMs", () => { + it("waits the base interval after the first failure", () => { + expect(retryDelayMs(1)).toBe(RETRY_MS); + }); + + it("doubles on each further consecutive failure", () => { + expect(retryDelayMs(2)).toBe(2 * RETRY_MS); + expect(retryDelayMs(3)).toBe(4 * RETRY_MS); + }); + + it("caps the wait rather than doubling forever", () => { + // 4 failures is already 80s uncapped, so the cap bites here and stays. + expect(retryDelayMs(4)).toBe(RETRY_MAX_MS); + expect(retryDelayMs(10)).toBe(RETRY_MAX_MS); + expect(retryDelayMs(1_000)).toBe(RETRY_MAX_MS); + expect(Number.isFinite(retryDelayMs(1_000))).toBe(true); + }); + + it("is back at the base with no failures behind it", () => { + // What an ANSWER leaves the counter at. The next beat's schedule must + // not inherit the outage's -- a recovered backend that then misses once + // should be retried in 10s, not in a minute. + expect(retryDelayMs(0)).toBe(RETRY_MS); + expect(retryDelayMs(-1)).toBe(RETRY_MS); + }); +}); + describe("startServerListPolling", () => { it("fetches at page load and keeps a heartbeat, retrying sooner after a failure", async () => { vi.useFakeTimers(); @@ -393,9 +429,27 @@ describe("startServerListPolling", () => { await vi.advanceTimersByTimeAsync(1_000); expect(fetchMock).toHaveBeenCalledTimes(3); + // Two failures deep, the heartbeat backs off: 20s, not another 10s. + await vi.advanceTimersByTimeAsync(2 * RETRY_MS - 1_000); + expect(fetchMock).toHaveBeenCalledTimes(3); + await vi.advanceTimersByTimeAsync(1_000); + expect(fetchMock).toHaveBeenCalledTimes(4); + + // ...and one answer puts it straight back on the short schedule. The + // beat after a success is the refresh cadence, and a failure right after + // that is a first failure again. + fetchMock.mockImplementation(async () => jsonResponse(API_LIST)); + await vi.advanceTimersByTimeAsync(4 * RETRY_MS); + expect(fetchMock).toHaveBeenCalledTimes(5); + fetchMock.mockRejectedValue(new TypeError("network down")); + await vi.advanceTimersByTimeAsync(REFRESH_MS); + expect(fetchMock).toHaveBeenCalledTimes(6); + await vi.advanceTimersByTimeAsync(RETRY_MS); + expect(fetchMock).toHaveBeenCalledTimes(7); + stopServerListPolling(); await vi.advanceTimersByTimeAsync(REFRESH_MS * 2); - expect(fetchMock).toHaveBeenCalledTimes(3); + expect(fetchMock).toHaveBeenCalledTimes(7); }); it("does not poll on a replay shell host", async () => { @@ -428,9 +482,9 @@ describe("a confirmed outage, as opposed to one missed beat", () => { expect(backendUnreachableConfirmed()).toBe(false); expect(seen).toEqual([{ reachable: false, confirmed: false }]); - // The second one confirms it. `reachable` did not change, so this - // announcement exists only because `confirmed` did -- which is the - // transition every gate acts on. + // The second one confirms it, a base retry delay after the first -- + // the backoff only starts stretching once there is an outage to back + // off from, so the confirmation still lands inside the first 10s. await vi.advanceTimersByTimeAsync(RETRY_MS); await ensureServerList(); expect(backendUnreachableConfirmed()).toBe(true); @@ -439,14 +493,18 @@ describe("a confirmed outage, as opposed to one missed beat", () => { { reachable: false, confirmed: true }, ]); - // A third changes nothing, so it is not announced. - await vi.advanceTimersByTimeAsync(RETRY_MS); + // A third changes nothing, so it is not announced. Two failures deep, + // the next attempt is due 20s after the last rather than 10s. + const attemptsBefore = fetchMock.mock.calls.length; + await vi.advanceTimersByTimeAsync(2 * RETRY_MS); await ensureServerList(); + expect(fetchMock).toHaveBeenCalledTimes(attemptsBefore + 1); expect(seen).toHaveLength(2); - // One answer clears it outright -- no gradual recovery. + // One answer clears it outright -- no gradual recovery. Three failures + // deep, so 40s. fetchMock.mockImplementation(async () => jsonResponse(API_LIST)); - await vi.advanceTimersByTimeAsync(RETRY_MS); + await vi.advanceTimersByTimeAsync(4 * RETRY_MS); expect(await ensureServerList()).toBe("api"); expect(backendUnreachableConfirmed()).toBe(false); expect(seen).toEqual([ @@ -647,6 +705,82 @@ describe("retryServerList", () => { }); }); +// What the desktop status bar's Retry button disables itself on. The +// reachability event is no use for it: that one fires only when reachability +// CHANGES, so an attempt that fails exactly like the last one announces +// nothing -- and the button still has to grey out while it is out. +describe("attemptInFlight and the server-list-attempt event", () => { + it("is true from the moment an attempt starts until it settles, and says so", async () => { + const seen: unknown[] = []; + const listener = (e: Event) => seen.push((e as CustomEvent).detail); + document.addEventListener("server-list-attempt", listener); + try { + expect(attemptInFlight()).toBe(false); + + let release: (r: Response) => void = () => {}; + fetchMock.mockImplementation( + async () => + await new Promise((resolve) => { + release = resolve; + }), + ); + + const pending = ensureServerList(); + expect(attemptInFlight()).toBe(true); + expect(seen).toEqual([{ inFlight: true }]); + + release(jsonResponse(API_LIST)); + await pending; + expect(attemptInFlight()).toBe(false); + expect(seen).toEqual([{ inFlight: true }, { inFlight: false }]); + } finally { + document.removeEventListener("server-list-attempt", listener); + } + }); + + it("announces one start per attempt, however many callers join it", async () => { + const seen: unknown[] = []; + const listener = (e: Event) => seen.push((e as CustomEvent).detail); + document.addEventListener("server-list-attempt", listener); + try { + let release: (r: Response) => void = () => {}; + fetchMock.mockImplementation( + async () => + await new Promise((resolve) => { + release = resolve; + }), + ); + + const first = retryServerList(); + const second = retryServerList(); + const third = ensureServerList(); + expect(seen).toEqual([{ inFlight: true }]); + + release(jsonResponse(API_LIST)); + await Promise.all([first, second, third]); + expect(seen).toEqual([{ inFlight: true }, { inFlight: false }]); + } finally { + document.removeEventListener("server-list-attempt", listener); + } + }); + + it("settles even when the fetch throws outright", async () => { + const seen: unknown[] = []; + const listener = (e: Event) => seen.push((e as CustomEvent).detail); + document.addEventListener("server-list-attempt", listener); + try { + fetchMock.mockImplementation(() => { + throw new Error("fetch itself blew up"); + }); + await ensureServerList(); + expect(attemptInFlight()).toBe(false); + expect(seen).toEqual([{ inFlight: true }, { inFlight: false }]); + } finally { + document.removeEventListener("server-list-attempt", listener); + } + }); +}); + // Today's rollover feel, kept: a player on build X keeps playing on X's // server after Y is released, until they refresh. So the pick prefers an // `open` server on this build, falls back to a `draining` one on this From 60633faacce024657f6f9893c23a5ea19a60e158 Mon Sep 17 00:00:00 2001 From: Josh Harris Date: Sun, 13 Sep 2026 22:16:33 +0100 Subject: [PATCH 09/10] Stop the offline Retry button being spammable The button's only protection was the 1s floor inside retryServerList(), which is a throttle on the MODULE, not on the button: against a stubbed or fast-failing backend an attempt settles in milliseconds, so the button came straight back and a player watching an outage could sit there clicking it, each click a real request. It also stayed live while the heartbeat was already asking, where a press could only ever join the attempt out and so offered something it could not do. Two conditions now disable it, composing into "whichever ends later" without either knowing about the other: - an attempt is in flight, whoever started it -- seeded from attemptInFlight() at mount and kept current by "server-list-attempt", the same accessor-plus-event shape everything else here uses because the event is one-shot (OPE-396); - a 5s cooldown after a press (RETRY_BUTTON_COOLDOWN_MS). Long enough that leaning on it costs nothing, short enough that someone who has just plugged their network back in is not left staring at a dead button. During an automatic attempt the label and title read desktop_status.retrying rather than greying out for no visible reason: a disabled control with no explanation is the complaint this started as. The click handler does NOT set the in-flight flag itself -- retryServerList announces the start synchronously when it actually starts a fetch, so a press the 1s floor swallows (which starts no attempt, and so announces no settle) cannot leave the button stuck on. The floor stays as the last line of defence for any future caller that does not come through a button like this one. New key, en.json only: desktop_status.retrying. OPE-439 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PTKyUrxqfwKvf2QxAovR6Z --- docs/MultiServer.md | 35 +++++-- resources/lang/en.json | 3 +- src/client/components/DesktopStatusBar.ts | 90 +++++++++++++---- tests/DesktopStatusBar.test.ts | 114 ++++++++++++++++++++++ 4 files changed, 212 insertions(+), 30 deletions(-) diff --git a/docs/MultiServer.md b/docs/MultiServer.md index bd24e3d996..4028a4e854 100644 --- a/docs/MultiServer.md +++ b/docs/MultiServer.md @@ -441,19 +441,34 @@ values. there is an outage to back off from, so confirmation is never slowed by it. One missed beat is a blip the cached list serves straight through, and dimming multiplayer for 10s over it would be worse than the blip; any - answer resets the count. Every change to either - value is announced on the document as `backend-reachability` with - `{ reachable, confirmed }`. Consumers seed from the accessor and then - subscribe — the event is one-shot, so a component mounting afterwards - would otherwise never learn the state (OPE-396). + answer resets the count. Every change to either value is announced on the + document as `backend-reachability` with `{ reachable, confirmed }`. + Consumers seed from the accessor and then subscribe — the event is + one-shot, so a component mounting afterwards would otherwise never learn + the state (OPE-396). +- **Busy (a third signal):** `attemptInFlight()` says whether an attempt is + out right now, automatic or manual, and every start and settle is + announced as `server-list-attempt` with `{ inFlight }`. Separate from + `backend-reachability` because that one fires only on a **change**: an + attempt that fails exactly like the last one announces nothing, which is + precisely the case the Retry button has to see. - **Retry:** `retryServerList()` is the player-initiated attempt behind the desktop status bar's offline Retry. It ignores the heartbeat's backoff (a person pressing a button is not a timer, and once an outage has run a - while that wait is up to a minute) but has a 1s floor - of its own, inside which a second press hands back the same promise; past - that, `fetchOnce()` still dedupes against an attempt already in flight. A - retry that fails counts towards the outage confirmation like any other - attempt. + while that wait is up to a minute) but has a 1s floor of its own, inside + which a second press hands back the same promise; past that, + `fetchOnce()` still dedupes against an attempt already in flight. A retry + that fails counts towards the outage confirmation like any other attempt. + + The floor is the last line of defence rather than the first. The button + itself is disabled under **either** of two conditions, so it comes back + whenever the later of them ends: while any server-list attempt is in + flight (`attemptInFlight()` / `server-list-attempt`), whoever started it — + during an automatic one it reads `desktop_status.retrying` rather than + sitting greyed out for no visible reason — and for a 5s cooldown after a + press (`RETRY_BUTTON_COOLDOWN_MS` in `DesktopStatusBar`), since a stubbed + or fast failure settles in milliseconds and would otherwise hand the + button straight back to a player clicking at an outage. What consumes the confirmed signal, and what it does: the desktop status bar's offline state (ranked below a session failure, above any update diff --git a/resources/lang/en.json b/resources/lang/en.json index 322e57dd6a..de8962924b 100644 --- a/resources/lang/en.json +++ b/resources/lang/en.json @@ -584,7 +584,8 @@ }, "desktop_status": { "offline": "Offline: can't reach the OpenFront servers", - "retry": "Retry" + "retry": "Retry", + "retrying": "Retrying…" }, "desktop_update": { "blocked": "A Steam update is required for the latest version", diff --git a/src/client/components/DesktopStatusBar.ts b/src/client/components/DesktopStatusBar.ts index 5b37a2282a..f620c5a74e 100644 --- a/src/client/components/DesktopStatusBar.ts +++ b/src/client/components/DesktopStatusBar.ts @@ -11,14 +11,25 @@ import { type DesktopUpdateState, } from "../DesktopShell"; import { + attemptInFlight, backendUnreachableConfirmed, retryServerList, type BackendReachabilityDetail, + type ServerListAttemptDetail, } from "../ServerList"; import { translateText } from "../Utils"; const WIGGLE_CLASS = "animate-bounce"; +// How long Retry stays disabled after a press, on top of however long that +// press's own attempt takes. The fetch is bounded at 4s, so without this the +// button would come back within seconds of a failure and a player watching an +// outage could sit there clicking it -- each click a real request. Five +// seconds is long enough that leaning on it costs nothing and short enough +// that someone who has just plugged their network back in is not left +// waiting on a button that looks broken. +const RETRY_BUTTON_COOLDOWN_MS = 5_000; + /** * Which state the single bottom slot shows, in one fixed order rather than a * precedence matrix: @@ -83,11 +94,17 @@ export class DesktopStatusBar extends LitElement { @state() private updateState: DesktopUpdateState | null = null; @state() private sessionState: DesktopSessionState | null = null; @state() private backendOutage = false; - // True from a Retry press until that attempt settles, so the button cannot - // be pressed again while its own request is still out. ServerList throttles - // and dedupes underneath, but a button that keeps accepting clicks and - // visibly does nothing reads as broken. - @state() private retrying = false; + // Whether ANY server-list attempt is out, this bar's own Retry press or a + // heartbeat beat. ServerList throttles and dedupes underneath, but a button + // that keeps accepting clicks and visibly does nothing reads as broken -- + // and while the heartbeat is already asking, a press could only ever join + // the attempt that is out, so offering it is a lie. + @state() private attempting = false; + // Whether the post-press cooldown is still running. The button is disabled + // while EITHER this or `attempting` holds, so the two compose into "until + // whichever ends later" without either needing to know about the other. + @state() private coolingDown = false; + private cooldownTimer: number | undefined; private unsubscribe: (() => void) | null = null; @@ -101,6 +118,12 @@ export class DesktopStatusBar extends LitElement { ).detail.confirmed; }; + private onAttempt = (e: Event) => { + this.attempting = ( + e as CustomEvent + ).detail.inFlight; + }; + // The bar's own element, so wiggle() can restart the animation with a real // synchronous class removal + reflow + re-add. Routing that through a Lit // @state does NOT work: Lit batches writes into one microtask render and @@ -140,10 +163,12 @@ export class DesktopStatusBar extends LitElement { // this element upgrades. if (isDesktopShell()) { this.backendOutage = backendUnreachableConfirmed(); + this.attempting = attemptInFlight(); document.addEventListener( "backend-reachability", this.onBackendReachability, ); + document.addEventListener("server-list-attempt", this.onAttempt); } } @@ -156,7 +181,9 @@ export class DesktopStatusBar extends LitElement { "backend-reachability", this.onBackendReachability, ); + document.removeEventListener("server-list-attempt", this.onAttempt); window.clearTimeout(this.wiggleTimer); + window.clearTimeout(this.cooldownTimer); } /** Draws attention when the player tries to do something the update gates. */ @@ -248,39 +275,64 @@ export class DesktopStatusBar extends LitElement { /** * Retry for the offline state: ask the server list to try the API again - * right now rather than waiting out the heartbeat's retry interval. + * right now rather than waiting out the heartbeat's backoff, which after a + * few failures is up to a minute long. * * Nothing is rendered from the result. A successful attempt flips * backendReachable() to true, which dispatches "backend-reachability", * which is what makes this whole bar disappear -- so the button's own * feedback is the bar going away. A failed one leaves the bar as it is, * which is also correct. + * + * Disabled while an attempt is out (whoever started it) and for + * RETRY_BUTTON_COOLDOWN_MS after a press, whichever ends later. While the + * heartbeat is the one asking, the label says so rather than sitting there + * greyed out for no visible reason: a dead button with no explanation is + * the thing this feature was reported as. */ private reachabilityAction() { + const disabled = this.retryDisabled(); return html``; } + private retryDisabled(): boolean { + return this.attempting || this.coolingDown; + } + private onRetryClick(): void { - if (this.retrying) return; - this.retrying = true; - retryServerList() - .catch((err: unknown) => { - // retryServerList never rejects; belt and braces, so a change there - // cannot surface as an unhandled rejection from a click handler. - console.error("desktop-status-bar: server list retry failed", err); - }) - .finally(() => { - this.retrying = false; - }); + // The rendered `disabled` already stops a real click getting here; this + // guard is for the synthetic ones (a double click delivered in the same + // task, before Lit has re-rendered) and costs nothing. + if (this.retryDisabled()) return; + this.coolingDown = true; + window.clearTimeout(this.cooldownTimer); + this.cooldownTimer = window.setTimeout(() => { + this.coolingDown = false; + }, RETRY_BUTTON_COOLDOWN_MS); + // `attempting` is not set here: retryServerList dispatches + // "server-list-attempt" synchronously when it starts a fetch, and that + // one event covers this press and the heartbeat alike. Setting it here + // too would leave it stuck on for a press the 1s floor swallowed, which + // starts no attempt and so announces no settle. + retryServerList().catch((err: unknown) => { + // retryServerList never rejects; belt and braces, so a change there + // cannot surface as an unhandled rejection from a click handler. + console.error("desktop-status-bar: server list retry failed", err); + }); } private label(s: DesktopUpdateState) { diff --git a/tests/DesktopStatusBar.test.ts b/tests/DesktopStatusBar.test.ts index 1866008db5..4c16d2fb60 100644 --- a/tests/DesktopStatusBar.test.ts +++ b/tests/DesktopStatusBar.test.ts @@ -9,6 +9,9 @@ import { retryServerList, } from "../src/client/ServerList"; +// Matches RETRY_BUTTON_COOLDOWN_MS in the component. +const COOLDOWN_MS = 5_000; + describe("barSource", () => { it("shows nothing when both states are healthy", () => { expect( @@ -136,12 +139,18 @@ describe("the rendered offline state", () => { return bar; } + // The button, whichever label it is wearing. `retrying` has `retry` as a + // prefix, so this matches on the shared stem rather than either key. function retryButton(bar: HTMLElement): HTMLButtonElement | undefined { return Array.from(bar.querySelectorAll("button")).find((b) => b.textContent?.includes("desktop_status.retry"), ); } + function buttonLabel(bar: HTMLElement): string { + return retryButton(bar)!.textContent!.trim(); + } + beforeEach(() => { // The bar renders nothing on the web, so every assertion here needs a // shell. No `update` bridge on it: a shell too old to expose one must @@ -291,4 +300,109 @@ describe("the rendered offline state", () => { release(new Response("{}", { status: 404 })); await vi.waitFor(() => expect(backendUnreachableConfirmed()).toBe(false)); }); + + // The press's own attempt fails in milliseconds against a stubbed fetch, + // so without a cooldown the button would come straight back and a player + // watching an outage could sit there clicking it -- one real request each. + it("keeps Retry disabled for the cooldown after a press, then re-enables it", async () => { + await confirmOutage(); + const bar = mountBar(); + await bar.updateComplete; + + retryButton(bar)!.click(); + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalled()); + // The attempt itself has settled by now; the cooldown is what is still + // holding the button. + await bar.updateComplete; + expect(retryButton(bar)!.disabled).toBe(true); + + vi.advanceTimersByTime(COOLDOWN_MS - 1); + await bar.updateComplete; + expect(retryButton(bar)!.disabled).toBe(true); + + vi.advanceTimersByTime(1); + await bar.updateComplete; + expect(retryButton(bar)!.disabled).toBe(false); + expect(buttonLabel(bar)).toBe("desktop_status.retry"); + }); + + it("costs one attempt for a rapid double click", async () => { + await confirmOutage(); + const bar = mountBar(); + await bar.updateComplete; + const before = fetchMock.mock.calls.length; + + const button = retryButton(bar)!; + button.click(); + button.click(); + button.click(); + + expect(fetchMock).toHaveBeenCalledTimes(before + 1); + }); + + // The automatic half of the same protection: while the heartbeat is + // already asking, a press could only ever join the attempt that is out, so + // the button says what is happening instead of offering a no-op. + it("disables Retry and says so while an automatic attempt is in flight", async () => { + await confirmOutage(); + const bar = mountBar(); + await bar.updateComplete; + expect(retryButton(bar)!.disabled).toBe(false); + + // Failing the attempt, not answering it: an answer would clear the + // outage and take this whole bar away before the assertion. + let fail: (e: unknown) => void = () => {}; + fetchMock.mockImplementation( + async () => + await new Promise((_resolve, reject) => { + fail = reject; + }), + ); + // Nobody clicked anything: this is the heartbeat's own beat. Stepping + // past the backoff first, since two failures are already behind us. + vi.advanceTimersByTime(60_000); + const beat = ensureServerList(); + await bar.updateComplete; + + expect(retryButton(bar)!.disabled).toBe(true); + expect(buttonLabel(bar)).toBe("desktop_status.retrying"); + expect(retryButton(bar)!.title).toBe("desktop_status.retrying"); + + // Settling re-enables it: no click happened, so no cooldown is owed. + fail(new TypeError("network down")); + await beat; + await bar.updateComplete; + expect(retryButton(bar)!.disabled).toBe(false); + expect(buttonLabel(bar)).toBe("desktop_status.retry"); + }); + + it("stays disabled after an attempt settles if the click's cooldown is still running", async () => { + await confirmOutage(); + const bar = mountBar(); + await bar.updateComplete; + + let fail: (e: unknown) => void = () => {}; + fetchMock.mockImplementation( + async () => + await new Promise((_resolve, reject) => { + fail = reject; + }), + ); + retryButton(bar)!.click(); + await bar.updateComplete; + expect(retryButton(bar)!.disabled).toBe(true); + + // The attempt ends well inside the cooldown. Whichever ends LATER is the + // one that governs, so the button is still held. + vi.advanceTimersByTime(1_000); + fail(new TypeError("network down")); + await vi.waitFor(() => + expect(buttonLabel(bar)).toBe("desktop_status.retry"), + ); + expect(retryButton(bar)!.disabled).toBe(true); + + vi.advanceTimersByTime(COOLDOWN_MS); + await bar.updateComplete; + expect(retryButton(bar)!.disabled).toBe(false); + }); }); From 4577bb2c16c9af02db8e95458f2de73408aaf408 Mon Sep 17 00:00:00 2001 From: Josh Harris Date: Mon, 14 Sep 2026 11:15:07 +0100 Subject: [PATCH 10/10] Gate on reachability only where the list API is needed The backend-reachability signal is the health of one thing: the server-list API. It is not an "is the network up" light, and it says nothing about whether any given game server is up. State that rule once, at the top of GameModeSelector.ts, and make every call site follow it. Gated (API-dependent): Create, Ranked and Join-by-code. Each has to resolve a server for something nothing has told the client about, so a dead list API really does mean the click cannot work. Not gated (socket-sourced): every public and hosted lobby card, in the homepage selector and in DetailedGameViewModal alike. The card is in front of the player because a game server sent it over a socket that is still open, which is the only liveness the join needs. Those cards were dimming and refusing on the list API's health while Main's funnel -- by its own docblock -- refused to weigh reachability on the very same join. They now call shouldBlockSocketSourcedAction, the same predicate with the reachability input nailed shut, for both the dimming and the click-through, so the two cannot drift. DetailedGameViewModal no longer tracks the signal at all. Web players also get the escape hatch desktop has. Desktop refuses into a status bar with a Retry button; the web has no bar, so a refused click now IS the retry -- reportMultiplayerRefusal probes before raising the toast, which is what makes "Check your connection and try again" true. Without it the only way out was the heartbeat's next beat, up to RETRY_MAX_MS away. Throttled by ServerList.manualRetryAvailable(): nothing while an attempt is in flight, nothing for MANUAL_RETRY_COOLDOWN_MS after the last player-initiated one. That cooldown moves out of DesktopStatusBar so both shells' affordances share one number and one clock. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PTKyUrxqfwKvf2QxAovR6Z --- docs/MultiServer.md | 95 +++++---- src/client/GameModeSelector.ts | 173 +++++++++++++--- src/client/Main.ts | 5 +- src/client/ServerList.ts | 53 ++++- src/client/components/DesktopStatusBar.ts | 16 +- .../components/DetailedGameViewModal.ts | 59 ++---- .../DetailedGameViewModalGatingWiring.test.ts | 79 +++++--- tests/GameModeSelectorGating.test.ts | 69 +++++++ tests/ReachabilityGating.test.ts | 188 +++++++++++++++++- tests/client/ServerList.test.ts | 69 +++++++ 10 files changed, 665 insertions(+), 141 deletions(-) diff --git a/docs/MultiServer.md b/docs/MultiServer.md index 4028a4e854..cc7240c8f7 100644 --- a/docs/MultiServer.md +++ b/docs/MultiServer.md @@ -452,43 +452,64 @@ values. `backend-reachability` because that one fires only on a **change**: an attempt that fails exactly like the last one announces nothing, which is precisely the case the Retry button has to see. -- **Retry:** `retryServerList()` is the player-initiated attempt behind the - desktop status bar's offline Retry. It ignores the heartbeat's backoff (a - person pressing a button is not a timer, and once an outage has run a - while that wait is up to a minute) but has a 1s floor of its own, inside - which a second press hands back the same promise; past that, - `fetchOnce()` still dedupes against an attempt already in flight. A retry - that fails counts towards the outage confirmation like any other attempt. - - The floor is the last line of defence rather than the first. The button - itself is disabled under **either** of two conditions, so it comes back - whenever the later of them ends: while any server-list attempt is in - flight (`attemptInFlight()` / `server-list-attempt`), whoever started it — - during an automatic one it reads `desktop_status.retrying` rather than - sitting greyed out for no visible reason — and for a 5s cooldown after a - press (`RETRY_BUTTON_COOLDOWN_MS` in `DesktopStatusBar`), since a stubbed - or fast failure settles in milliseconds and would otherwise hand the - button straight back to a player clicking at an outage. - - What consumes the confirmed signal, and what it does: the desktop status - bar's offline state (ranked below a session failure, above any update - state), and the multiplayer _buttons_ in `GameModeSelector` and - `DetailedGameViewModal`, which dim and refuse a press — on the web as well - as on desktop, where the press also raises a - `common.backend_unreachable` toast, since there is no status bar there to - name the reason. - - What it deliberately does **not** do: refuse a join that is already under - way. `Main`'s join funnel (`shouldBlockJoin`) weighs only the desktop - update and session states; reachability is not an input (OPE-439). Every - source that dispatches a join has already reached a server to produce it - — `private` after `checkActiveLobby` read `exists` from the game's own - server, `host` after `createLobby` minted the id, `public` from a lobby - list arriving over a live server socket, `matchmaking` after the queue - matched — so the server-list API's health says nothing about the join in - hand. Refusing there would only ever be wrong, and at worst would eject a - player whose reload had just proved their game is live. Single-player is - never gated, and nothing here touches a game already in progress. +- **Retry:** `retryServerList()` is the player-initiated attempt. It + ignores the heartbeat's backoff (a person pressing a button is not a + timer, and once an outage has run a while that wait is up to a minute) but + has a 1s floor of its own, inside which a second press hands back the same + promise; past that, `fetchOnce()` still dedupes against an attempt already + in flight. A retry that fails counts towards the outage confirmation like + any other attempt. + + The floor is the last line of defence rather than the first. Above it sits + one policy, `manualRetryAvailable()`, shared by both shells' affordances + and reading one clock: no retry while any server-list attempt is in flight + (`attemptInFlight()` / `server-list-attempt`), whoever started it, and + none for `MANUAL_RETRY_COOLDOWN_MS` (5s) after the last player-initiated + one — a stubbed or fast failure settles in milliseconds and would + otherwise hand the affordance straight back to a player clicking at an + outage. + + The two affordances: + - **Desktop:** the status bar's offline Retry, disabled under either + condition above so it comes back whenever the later of them ends. During + an automatic attempt it reads `desktop_status.retrying` rather than + sitting greyed out for no visible reason. + - **Web:** there is no status bar, so the refused click _is_ the retry. + `reportMultiplayerRefusal` probes when `manualRetryAvailable()` says it + would do something, and raises the `common.backend_unreachable` toast + either way — which is what makes that toast's "try again" true. Without + it a web player's only way out would be the heartbeat's next beat, up to + `RETRY_MAX_MS` away. + +- **What reachability may gate, and what it may not.** The rule, stated + once at the top of `GameModeSelector.ts` and referenced from every call + site: the signal is the health of **one** thing, the server-list API. It + is not a general "is the network up" light, and it says nothing about + whether any given _game_ server is up. So it gates exactly the actions + that cannot begin until that API answers, because nothing has yet told the + client which server to talk to. + - _Gated (API-dependent):_ Create/host a lobby, Ranked/matchmaking, and + the join-by-code modal, in `GameModeSelector`. These dim and refuse a + press — `shouldBlockMultiplayerAction` with + `backendUnreachableConfirmed()` — on the web as well as on desktop. + - _Not gated (socket-sourced):_ every public or hosted lobby card, in the + homepage selector and in `DetailedGameViewModal` alike, and every join + that reaches `Main`'s funnel. These call + `shouldBlockSocketSourcedAction`, the same predicate with the + reachability input nailed shut, so a card neither dims nor refuses over + a list-API outage; `DetailedGameViewModal` does not subscribe to the + signal at all. + + A card is in front of the player because a game server sent it over a + socket that is still open, which is the only liveness that join needs. + Likewise every join source has already reached a server to produce its + event — `private` after `checkActiveLobby` read `exists` from the game's + own server, `host` after `createLobby` minted the id, `public` from that + live lobby feed, `matchmaking` after the queue matched. Refusing on the + list API's health could only ever reject a join that is already under way, + and at worst would eject a player whose reload had just proved their game + is live. Single-player is never gated either way, and nothing here touches + a game already in progress. - **Which list:** the desktop shell asks for its injected `serverHost` (its values are exactly the sites); a web page asks for its `siteHost` diff --git a/src/client/GameModeSelector.ts b/src/client/GameModeSelector.ts index e1d5bb1ae5..5e93b3f14e 100644 --- a/src/client/GameModeSelector.ts +++ b/src/client/GameModeSelector.ts @@ -39,6 +39,8 @@ import { JoinLobbyEvent } from "./Main"; import { backendUnreachableConfirmed, isPinnedToAVersion, + manualRetryAvailable, + retryServerList, type BackendReachabilityDetail, } from "./ServerList"; import { SinglePlayerModal } from "./SinglePlayerModal"; @@ -67,8 +69,43 @@ const TUTORIAL_ACTION = const TUTORIAL_CARD_MAX_GAMES = 5; /** + * THE REACHABILITY RULE (OPE-439). Stated once, here; every other call site + * in this feature points back at this comment rather than restating it. + * + * The backend-reachability signal is the health of ONE thing: the server-list + * API (`/cluster.json`), as observed by ServerList's heartbeat. It is not a + * general "is the internet up" light, and in particular it says nothing about + * whether any given GAME server is up. + * + * So it may gate exactly one category of action: the ones that cannot even + * begin without that API answering first, because nothing has yet told the + * client which server to talk to. + * + * GATED (API-dependent): creating/hosting a lobby, entering matchmaking, + * opening the join-by-code modal. Each has to resolve a server for + * something the client has heard nothing about, so a dead list API really + * does mean the click cannot work. These dim, and refuse with + * reportMultiplayerRefusal. + * + * NOT GATED (socket-sourced): anything whose target arrived over a live + * game-server socket -- every card in the public lobby feed, in both the + * homepage selector and the detailed browser -- and every join that reaches + * Main's funnel (shouldBlockJoin). The card's very existence is proof that + * the game server behind it is up and talking to us, which is the only + * liveness that join needs. Refusing there could only ever reject a join + * that is already under way, over the health of an unrelated API. These + * neither dim nor refuse on reachability: they call + * shouldBlockSocketSourcedAction, which is the same predicate with the + * reachability input nailed shut. + * + * The other two inputs (desktop update state, desktop session state) apply to + * both categories, which is why the two predicates differ only in this one + * argument. + * + * --- + * * Whether multiplayer should be available given what we know about the - * backend (OPE-439). + * backend. * * The parameter is ServerList.backendUnreachableConfirmed(), NOT the raw * backendReachable(), and the difference is load-bearing. That accessor is @@ -97,7 +134,11 @@ export function multiplayerAllowedForBackend(backendOutage: boolean): boolean { * `backendOutage` is the only one of the three that also applies on the web, * which is why it is a required parameter rather than an optional one: an * entry point that forgets to pass it would silently stay ungated, and a - * compile error is the cheapest way to notice. + * compile error is the cheapest way to notice. Pass + * backendUnreachableConfirmed() only from an API-dependent entry point; a + * socket-sourced one calls shouldBlockSocketSourcedAction instead, so that + * "reachability does not apply here" is a named decision rather than a + * `false` literal someone has to interpret. */ export function shouldBlockMultiplayerAction( update: DesktopUpdateState | null, @@ -111,7 +152,30 @@ export function shouldBlockMultiplayerAction( } /** - * Tells the player why a multiplayer action was refused. + * The same gate for an action whose target arrived over a live game-server + * socket: a public or hosted lobby card, in either browser, and every join + * that reaches Main's funnel (shouldBlockJoin below wraps this). + * + * Reachability is not an input, by the rule at the top of this file: the card + * is in front of the player because a game server sent it over a socket that + * is still open, so the server-list API's health cannot make joining it + * wrong. The desktop update and session states still apply -- they are + * statements about this client, not about any server. + * + * A function rather than `shouldBlockMultiplayerAction(u, s, false)` at four + * call sites so the dimming and the click-through of a given control cannot + * drift apart, and so grep finds every place the rule is exercised. + */ +export function shouldBlockSocketSourcedAction( + update: DesktopUpdateState | null, + session: DesktopSessionState | null, +): boolean { + return shouldBlockMultiplayerAction(update, session, false); +} + +/** + * Tells the player why a multiplayer action was refused -- and, on the web, + * acts as the retry it tells them to make. * * On desktop the status bar is already showing the reason and its remedy, so * the click lands there as a wiggle rather than as a message that would say @@ -121,6 +185,22 @@ export function shouldBlockMultiplayerAction( * * Only reachability needs the web half: every other reason to refuse here is * desktop-only, and on desktop the bar always carries it. + * + * The refused click also PROBES on the web, and that is the point rather than + * a nicety. Desktop has a Retry button; the web has nothing, so without this + * the only way out of the gated state is the heartbeat's own next beat -- + * which backs off to as much as RETRY_MAX_MS once an outage has run a while. + * A message reading "try again" over a button where trying again provably did + * nothing is worse than no message. So the click the player makes IS the + * retry, and the message is true. + * + * Throttled by ServerList.manualRetryAvailable(), the same policy (and the + * same clock) as the desktop button's disabled state: nothing while an + * attempt is already out, nothing for MANUAL_RETRY_COOLDOWN_MS after the last + * one. A player clicking at an outage gets the message every time and a + * request at most every few seconds. Nothing is rendered from the result: a + * successful probe flips the reachability signal, which is what un-dims the + * buttons -- the feedback is the gate going away. */ export function reportMultiplayerRefusal(backendOutage: boolean): void { // Optional-call the method rather than dispatching an event: the bar is a @@ -136,6 +216,13 @@ export function reportMultiplayerRefusal(backendOutage: boolean): void { // index.html on every build and simply renders nothing on the web, so its // presence proves nothing about whether the player can see a reason. if (!isDesktopShell() && backendOutage) { + if (manualRetryAvailable()) { + retryServerList().catch((err: unknown) => { + // retryServerList never rejects; belt and braces, so a change there + // cannot surface as an unhandled rejection from a click handler. + console.error("server list retry from a refused click failed", err); + }); + } showToast(translateText("common.backend_unreachable"), "red"); } } @@ -163,17 +250,24 @@ export function joinIsGateable(lobby: JoinLobbyEvent): boolean { * feedback around it. Both halves it does weigh -- the update state and the * session state -- are desktop-only. * - * Backend reachability is deliberately NOT an input here (OPE-439). Every - * source that dispatches a join has already reached a server to produce it: - * "private" only after checkActiveLobby read `exists` from the game's own - * server, "host" only after createLobby minted the id, "public" from a lobby - * list arriving over a live server socket, and "matchmaking" only after the - * queue matched and checkGame confirmed the game exists. The outage signal - * tracks the separate server-list API, whose health says nothing about those - * servers, so refusing here could only reject a join that is already under - * way. Worst case it ejects a player mid-game: a reload during a list-API - * blip proves the game is live, then the refusal closes the join modal, - * which leaves the lobby and resets the URL. + * Backend reachability is deliberately NOT an input here -- the rule at the + * top of this file, which is why this defers to + * shouldBlockSocketSourcedAction. Every source that dispatches a join has + * already reached a server to produce it: "private" only after + * checkActiveLobby read `exists` from the game's own server, "host" only + * after createLobby minted the id, "public" from a lobby list arriving over a + * live server socket, and "matchmaking" only after the queue matched and + * checkGame confirmed the game exists. The outage signal tracks the separate + * server-list API, whose health says nothing about those servers, so refusing + * here could only reject a join that is already under way. Worst case it + * ejects a player mid-game: a reload during a list-API blip proves the game + * is live, then the refusal closes the join modal, which leaves the lobby and + * resets the URL. + * + * The controls one step earlier in the funnel -- the lobby cards in this + * component and in DetailedGameViewModal, which are where a "public" join + * comes from -- hold to the same rule for the same reason, so a card is + * neither dimmed nor refused over a list-API outage. */ export function shouldBlockJoin( lobby: JoinLobbyEvent, @@ -181,7 +275,7 @@ export function shouldBlockJoin( session: DesktopSessionState | null, ): boolean { if (!joinIsGateable(lobby)) return false; - return shouldBlockMultiplayerAction(update, session, false); + return shouldBlockSocketSourcedAction(update, session); } @customElement("game-mode-selector") @@ -526,16 +620,21 @@ export class GameModeSelector extends LitElement { } /** - * Refuses the action and tells the player why. Returns true when the caller - * should stop. + * Refuses an API-DEPENDENT action (Create, Ranked, Join by code) and tells + * the player why. Returns true when the caller should stop. + * + * The reachability half applies here -- see the rule at the top of this + * file: none of these three can resolve a server without the list API. A + * lobby card goes through blockedFromLobbyJoin below instead. * * Deliberately NOT implemented with the `disabled` attribute the way * renderSmallActionCard handles invalid input: a disabled control (and * `pointer-events-none` alongside it) swallows the click, leaving nothing to - * trigger the wiggle. The button stays clickable and merely stops being - * actionable. + * trigger the wiggle -- and, on the web, nothing to trigger the retry that + * reportMultiplayerRefusal makes of it. The button stays clickable and + * merely stops being actionable. */ - private blockedFromMultiplayer(): boolean { + private blockedFromApiAction(): boolean { if ( !shouldBlockMultiplayerAction( this.desktopUpdateState, @@ -548,8 +647,27 @@ export class GameModeSelector extends LitElement { return true; } + /** + * The same, for the public-lobby card: the desktop states still refuse, a + * list-API outage never does. Its lobby came over a live game-server socket + * (the rule at the top of this file), so there is no reachability reason to + * refuse and nothing to retry -- hence `false` to the refusal report, which + * leaves the desktop wiggle as the only feedback. + */ + private blockedFromLobbyJoin(): boolean { + if ( + !shouldBlockSocketSourcedAction( + this.desktopUpdateState, + this.desktopSessionState, + ) + ) + return false; + reportMultiplayerRefusal(false); + return true; + } + private openRankedMenu = () => { - if (this.blockedFromMultiplayer()) return; + if (this.blockedFromApiAction()) return; if (!this.validateUsername()) return; window.showPage?.("page-ranked"); }; @@ -573,13 +691,13 @@ export class GameModeSelector extends LitElement { }; private openHostLobby = () => { - if (this.blockedFromMultiplayer()) return; + if (this.blockedFromApiAction()) return; if (!this.validateUsername()) return; (document.querySelector("host-lobby-modal") as HostLobbyModal)?.open(); }; private openJoinLobby = () => { - if (this.blockedFromMultiplayer()) return; + if (this.blockedFromApiAction()) return; if (!this.validateUsername()) return; (document.querySelector("join-lobby-modal") as JoinLobbyModal)?.open(); }; @@ -710,16 +828,19 @@ export class GameModeSelector extends LitElement { // with pointer-events-none) swallows the click, and the click is what // makes the update bar wiggle. `blocked` only dims and reports // aria-disabled; validateAndJoin does the refusing. + // + // Socket-sourced, so a list-API outage neither dims this nor refuses it: + // the same predicate validateAndJoin uses, for the reason in the rule at + // the top of this file. return lobbyCard({ lobby, subtitle: titleContent, timeDisplay, timeDisplayUppercase, disabled: !this.inputValid, - blocked: shouldBlockMultiplayerAction( + blocked: shouldBlockSocketSourcedAction( this.desktopUpdateState, this.desktopSessionState, - this.backendOutage, ), viewerTrusted: this.viewerTrusted, onClick: () => this.validateAndJoin(lobby), @@ -727,7 +848,7 @@ export class GameModeSelector extends LitElement { } private validateAndJoin(lobby: PublicGameInfo) { - if (this.blockedFromMultiplayer()) return; + if (this.blockedFromLobbyJoin()) return; if (!this.validateUsername()) return; if (!canJoinTrustedLobby(lobby, this.viewerTrusted)) { this.showTrustRequired = true; diff --git a/src/client/Main.ts b/src/client/Main.ts index c606380e32..28cfaa8b01 100644 --- a/src/client/Main.ts +++ b/src/client/Main.ts @@ -1204,7 +1204,10 @@ class Client { * Both inputs are desktop-only and are read only there. Backend * reachability is not among them (OPE-439): by the time a join reaches * this funnel its source has already reached a server, so the server-list - * API being unreachable is no reason to refuse -- see shouldBlockJoin. + * API being unreachable is no reason to refuse. The lobby cards one step + * earlier hold to the same rule, so nothing dims or refuses on it there + * either -- see shouldBlockJoin, and the rule at the top of + * GameModeSelector.ts. * * Says why rather than failing silently, matching what the dimmed buttons * do. diff --git a/src/client/ServerList.ts b/src/client/ServerList.ts index 13e46b7d2e..a665360114 100644 --- a/src/client/ServerList.ts +++ b/src/client/ServerList.ts @@ -70,6 +70,21 @@ const CONFIRM_OUTAGE_AFTER_FAILURES = 2; // cannot outpace the request it started. Short enough that a deliberate // second press still works, which is the whole point of a manual retry. const MANUAL_RETRY_MIN_INTERVAL_MS = 1_000; +/** + * How long a player-initiated retry stays unavailable after one, on top of + * however long that attempt itself takes. The fetch is bounded at 4s, so + * without this a failure would hand the affordance back within seconds and a + * player watching an outage could sit there firing real requests at it. Five + * seconds is long enough that leaning on it costs nothing and short enough + * that someone who has just plugged their network back in is not left waiting + * on something that looks broken. + * + * One number for both affordances: the desktop status bar's Retry button + * (which disables itself for this long after a press) and the web's refused + * multiplayer click, which doubles as a retry because there is no bar there + * to press. manualRetryAvailable() below is what the latter asks. + */ +export const MANUAL_RETRY_COOLDOWN_MS = 5_000; export type ServerListStatus = // The list is loaded and a server for this build was picked: own-server @@ -207,6 +222,33 @@ export function attemptInFlight(): boolean { return inflight !== null; } +/** + * Whether asking for a player-initiated retry right now would actually probe + * the API, rather than be swallowed. + * + * For callers that have no button of their own to disable -- the web's + * refused multiplayer click (GameModeSelector.reportMultiplayerRefusal), + * which is the web's stand-in for the desktop status bar's Retry. The bar + * renders its own disabled state from the same two conditions, so the two + * affordances share one policy and one clock: + * + * - nothing while an attempt is already out, whoever started it. A press + * landing on top of one could only join it, so offering it is a lie. + * - nothing for MANUAL_RETRY_COOLDOWN_MS after the last player-initiated + * attempt, so a player clicking at an outage cannot turn each click into a + * request. + * + * Advisory, not enforcement: retryServerList() has its own floor and + * fetchOnce() dedupes underneath, so a caller that skips this check still + * cannot hammer the API. This exists so such a caller can tell whether its + * retry did anything. + */ +export function manualRetryAvailable(): boolean { + if (attemptInFlight()) return false; + if (lastManualRetry === null) return true; + return Date.now() - lastManualRetry.at >= MANUAL_RETRY_COOLDOWN_MS; +} + /** The detail carried by the "server-list-attempt" document event. */ export interface ServerListAttemptDetail { inFlight: boolean; @@ -444,8 +486,12 @@ export async function ensureServerList(): Promise { } /** - * Try the API again right now, at the player's request: the Retry on the - * desktop status bar's offline state (OPE-439). + * Try the API again right now, at the player's request (OPE-439). Two + * callers, one per shell: the Retry on the desktop status bar's offline + * state, and -- because the web has no such bar -- a refused multiplayer + * click on the web, which doubles as that press + * (GameModeSelector.reportMultiplayerRefusal). Both gate themselves on + * manualRetryAvailable()'s policy first. * * Deliberately ignores the heartbeat's retry schedule. That backoff exists * to stop TIMER-driven callers hammering a down API between beats, and a @@ -461,7 +507,8 @@ export async function ensureServerList(): Promise { * the last line of defence, not the first: the button that calls this is * itself disabled while an attempt is out and for a cooldown after a press * (DesktopStatusBar), and the floor is what holds if anything ever calls - * this without going through such a button. + * this without going through such a button, or through + * manualRetryAvailable(). * * A retry that fails counts towards the outage confirmation like any other * attempt -- pressing Retry against a backend that is genuinely down should diff --git a/src/client/components/DesktopStatusBar.ts b/src/client/components/DesktopStatusBar.ts index f620c5a74e..1a47f57c98 100644 --- a/src/client/components/DesktopStatusBar.ts +++ b/src/client/components/DesktopStatusBar.ts @@ -13,6 +13,7 @@ import { import { attemptInFlight, backendUnreachableConfirmed, + MANUAL_RETRY_COOLDOWN_MS, retryServerList, type BackendReachabilityDetail, type ServerListAttemptDetail, @@ -22,13 +23,10 @@ import { translateText } from "../Utils"; const WIGGLE_CLASS = "animate-bounce"; // How long Retry stays disabled after a press, on top of however long that -// press's own attempt takes. The fetch is bounded at 4s, so without this the -// button would come back within seconds of a failure and a player watching an -// outage could sit there clicking it -- each click a real request. Five -// seconds is long enough that leaning on it costs nothing and short enough -// that someone who has just plugged their network back in is not left -// waiting on a button that looks broken. -const RETRY_BUTTON_COOLDOWN_MS = 5_000; +// press's own attempt takes. The number lives in ServerList because the web +// shares it: with no bar to press there, a refused multiplayer click is the +// retry (GameModeSelector.reportMultiplayerRefusal), throttled by +// manualRetryAvailable() on this same cooldown and the same clock. /** * Which state the single bottom slot shows, in one fixed order rather than a @@ -285,7 +283,7 @@ export class DesktopStatusBar extends LitElement { * which is also correct. * * Disabled while an attempt is out (whoever started it) and for - * RETRY_BUTTON_COOLDOWN_MS after a press, whichever ends later. While the + * MANUAL_RETRY_COOLDOWN_MS after a press, whichever ends later. While the * heartbeat is the one asking, the label says so rather than sitting there * greyed out for no visible reason: a dead button with no explanation is * the thing this feature was reported as. @@ -322,7 +320,7 @@ export class DesktopStatusBar extends LitElement { window.clearTimeout(this.cooldownTimer); this.cooldownTimer = window.setTimeout(() => { this.coolingDown = false; - }, RETRY_BUTTON_COOLDOWN_MS); + }, MANUAL_RETRY_COOLDOWN_MS); // `attempting` is not set here: retryServerList dispatches // "server-list-attempt" synchronously when it starts a fetch, and that // one event covers this press and the heartbeat alike. Setting it here diff --git a/src/client/components/DetailedGameViewModal.ts b/src/client/components/DetailedGameViewModal.ts index 8d76fa86db..4f98613f44 100644 --- a/src/client/components/DetailedGameViewModal.ts +++ b/src/client/components/DetailedGameViewModal.ts @@ -14,15 +14,11 @@ import { } from "../DesktopShell"; import { reportMultiplayerRefusal, - shouldBlockMultiplayerAction, + shouldBlockSocketSourcedAction, } from "../GameModeSelector"; import { JoinLobbyModal } from "../JoinLobbyModal"; import { PublicLobbySocket } from "../LobbySocket"; import { JoinLobbyEvent } from "../Main"; -import { - backendUnreachableConfirmed, - type BackendReachabilityDetail, -} from "../ServerList"; import { UsernameInput } from "../UsernameInput"; import { calculateServerTimeOffset, @@ -126,10 +122,10 @@ export class DetailedGameViewModal extends BaseModal { @state() private viewerSignedIn: boolean = false; @state() private showTrustRequired: boolean = false; @state() private desktopSessionState: DesktopSessionState | null = null; - // The DEBOUNCED outage signal, not the raw per-attempt one: see - // multiplayerAllowedForBackend for why one missed heartbeat must not gate - // this browser's join. - @state() private backendOutage = false; + // No backend-reachability state, deliberately. Every lobby this browser + // shows arrived over a live game-server socket, and by the reachability + // rule (GameModeSelector, top of file) the server-list API's health may not + // gate such a join -- so there is nothing here for the signal to decide. private serverTimeOffset = 0; private countdownTimer: number | null = null; @@ -201,13 +197,6 @@ export class DetailedGameViewModal extends BaseModal { "desktop-session-state", this.onDesktopSessionState, ); - // Seeded unconditionally, unlike the two above: an unreachable backend - // refuses a join on the web as well as on desktop (OPE-439). - this.backendOutage = backendUnreachableConfirmed(); - document.addEventListener( - "backend-reachability", - this.onBackendReachability, - ); } disconnectedCallback() { @@ -220,10 +209,6 @@ export class DetailedGameViewModal extends BaseModal { "desktop-session-state", this.onDesktopSessionState, ); - document.removeEventListener( - "backend-reachability", - this.onBackendReachability, - ); this.onClose(); super.disconnectedCallback(); } @@ -249,12 +234,6 @@ export class DetailedGameViewModal extends BaseModal { this.desktopSessionState = (e as CustomEvent).detail; }; - private onBackendReachability = (e: Event) => { - this.backendOutage = ( - e as CustomEvent - ).detail.confirmed; - }; - // ---- Slot animation ---- // // When the lobby at the top of a pane starts, the one queued behind it takes @@ -481,10 +460,12 @@ export class DetailedGameViewModal extends BaseModal { // Gated, not disabled: `disabled` also sets pointer-events-none and would // swallow the click that's supposed to make the update bar wiggle. join() // does the actual refusing. - blocked: shouldBlockMultiplayerAction( + // + // Socket-sourced: the desktop update and session states dim a card, a + // list-API outage never does. Same predicate join() refuses on. + blocked: shouldBlockSocketSourcedAction( this.desktopUpdateState, this.desktopSessionState, - this.backendOutage, ), viewerTrusted: this.viewerTrusted, onClick: () => this.join(lobby), @@ -802,21 +783,25 @@ export class DetailedGameViewModal extends BaseModal { * Refuses the action and draws attention to the update bar. Returns true * when the caller should stop. * - * Mirrors GameModeSelector's blockedByUpdate() (deliberately not shared: it - * touches this component's own state field) -- see that file for why this - * nudges the bar instead of relying on `disabled`, which would swallow the - * click. + * Mirrors GameModeSelector's blockedFromLobbyJoin() (deliberately not + * shared: it touches this component's own state fields) -- see that file for + * why this nudges the bar instead of relying on `disabled`, which would + * swallow the click. + * + * Every lobby here came over a live game-server socket, so reachability is + * not an input and there is no reachability reason to report: `false` to the + * refusal report leaves the desktop wiggle as the only feedback, which is + * all the update and session states need. */ - private blockedFromMultiplayer(): boolean { + private blockedFromLobbyJoin(): boolean { if ( - !shouldBlockMultiplayerAction( + !shouldBlockSocketSourcedAction( this.desktopUpdateState, this.desktopSessionState, - this.backendOutage, ) ) return false; - reportMultiplayerRefusal(this.backendOutage); + reportMultiplayerRefusal(false); return true; } @@ -825,7 +810,7 @@ export class DetailedGameViewModal extends BaseModal { // Checked -- and the bar nudged -- before close(): a blocked attempt must // leave the modal open and tell the player why, not vanish silently. This // sits above the hosted/public branch below so both paths are covered. - if (this.blockedFromMultiplayer()) return; + if (this.blockedFromLobbyJoin()) return; // Also before close(): the popup explains how to become trusted, so it // must stay on screen with the browser rather than vanish with it. if (!canJoinTrustedLobby(lobby, this.viewerTrusted)) { diff --git a/tests/DetailedGameViewModalGatingWiring.test.ts b/tests/DetailedGameViewModalGatingWiring.test.ts index 86c7529802..e309194481 100644 --- a/tests/DetailedGameViewModalGatingWiring.test.ts +++ b/tests/DetailedGameViewModalGatingWiring.test.ts @@ -233,14 +233,19 @@ describe("the multiplayer gate at DetailedGameViewModal's join()", () => { }); /** - * OPE-439's half of the same gate. The update and session halves above are - * desktop-only; a confirmed backend outage refuses this browser's join on - * the web too, and its seed-then-subscribe wiring is its own call site with - * its own chance to be wrong. + * OPE-439, from the other side. The update and session halves above are + * desktop-only and they DO refuse a card click. A confirmed backend outage + * must not, on either shell: every lobby this browser lists arrived over a + * live game-server socket, and the outage signal tracks the separate + * server-list API, whose health says nothing about that server. That is the + * reachability rule at the top of GameModeSelector.ts, and Main's join funnel + * already follows it -- these cards are where the very joins that funnel lets + * through are produced, so refusing them one step earlier would contradict it. * - * Driven through the real ServerList module rather than a mock of it: the - * point of the seed test is that the value the component reads is the one - * the heartbeat actually produced. + * Driven through the real ServerList module rather than a mock of it: a + * mocked accessor could only prove this component ignores something; this + * proves it is unmoved by the signal the heartbeat actually produces, seeded + * before it mounted or announced afterwards. */ describe("DetailedGameViewModal and a confirmed backend outage", () => { let fetchMock: ReturnType; @@ -296,53 +301,75 @@ describe("DetailedGameViewModal and a confirmed backend outage", () => { expect(backendUnreachableConfirmed()).toBe(true); } - it("refuses a card click when it mounted after the outage was confirmed", async () => { + it("still joins a public card when it mounted during a confirmed outage", async () => { await confirmOutage(); - - // No "backend-reachability" event is dispatched below: the ones that - // would have told this component fired before it existed. The accessor - // seed is the only path left (OPE-396, on a new signal). await remountModal(); const card = cardButton("public-1"); expect(card).not.toBeNull(); card!.click(); - expect(joinLobby).not.toHaveBeenCalled(); - expect(wiggle).toHaveBeenCalled(); + expect(joinLobby).toHaveBeenCalled(); + expect(joinLobby.mock.calls[0][0].detail.gameID).toBe("public-1"); + // Nothing was refused, so nothing is reported: no wiggle on desktop and + // no toast on the web. + expect(wiggle).not.toHaveBeenCalled(); }); - it("marks its cards aria-disabled on that same seed", async () => { + it("still opens the join modal for a hosted card during that outage", async () => { await confirmOutage(); await remountModal(); + await pushLobbies({ hosted: [lobby("hosted-1", "hosted")] }); - expect( - modal.querySelectorAll('button[aria-disabled="true"]').length, - ).toBeGreaterThan(0); + cardButton("hosted-1")!.click(); + + expect(joinModalOpen).toHaveBeenCalledWith({ lobbyId: "hosted-1" }); }); - it("allows the join again once the backend answers", async () => { + it("does not dim its cards during that outage", async () => { await confirmOutage(); await remountModal(); - // The subscribe half, from the seeded state: a recovery this component - // only ever hears about through the event. + expect(modal.querySelectorAll('button[aria-disabled="true"]').length).toBe( + 0, + ); + }); + + it("is unmoved by an outage announced while it is mounted", async () => { + // The subscribe half of the old wiring, from the other direction: this + // component no longer listens for "backend-reachability" at all, and an + // event that would once have dimmed every card now changes nothing. + await remountModal(); document.dispatchEvent( new CustomEvent("backend-reachability", { - detail: { reachable: true, confirmed: false }, + detail: { reachable: false, confirmed: true }, }), ); await modal.updateComplete; + expect(modal.querySelectorAll('button[aria-disabled="true"]').length).toBe( + 0, + ); cardButton("public-1")!.click(); expect(joinLobby).toHaveBeenCalled(); - expect(joinLobby.mock.calls[0][0].detail.gameID).toBe("public-1"); }); - it("does not refuse after a single missed attempt", async () => { - // The control, and the bug this debounce exists for: one timed-out - // heartbeat must not take the lobby browser away. + it("still refuses on a DESKTOP reason during an outage", async () => { + // The control that keeps this from being "the gate was deleted": the + // update state is a statement about this client rather than about any + // server, so it refuses the same card the outage may not. + await confirmOutage(); + await remountModal(); + await setUpdateState({ status: "staged", bytes: 0, total: 0 }); + + cardButton("public-1")!.click(); + + expect(joinLobby).not.toHaveBeenCalled(); + expect(wiggle).toHaveBeenCalled(); + }); + + it("does not refuse after a single missed attempt either", async () => { await ensureServerList(); expect(backendUnreachableConfirmed()).toBe(false); diff --git a/tests/GameModeSelectorGating.test.ts b/tests/GameModeSelectorGating.test.ts index 76860936fe..35f0475485 100644 --- a/tests/GameModeSelectorGating.test.ts +++ b/tests/GameModeSelectorGating.test.ts @@ -4,6 +4,7 @@ import { multiplayerAllowedForBackend, shouldBlockJoin, shouldBlockMultiplayerAction, + shouldBlockSocketSourcedAction, } from "../src/client/GameModeSelector"; import { GameType } from "../src/core/game/Game"; @@ -260,3 +261,71 @@ describe("shouldBlockJoin", () => { expect(shouldBlockJoin(mp, healthy, { status: "signed-in" })).toBe(false); }); }); + +/** + * The other half of the reachability rule (GameModeSelector, top of file): + * whatever the server-list API is doing, it may not gate an action whose + * target arrived over a live game-server socket -- a public or hosted lobby + * card, in either browser. The card exists because a game server sent it over + * a socket that is still open, which is the only liveness the join needs. + * + * This predicate takes no reachability argument AT ALL, which is the point: + * there is no value a caller could pass that would make a card refuse on the + * list API's health. The desktop update and session states still apply -- + * those are statements about this client, not about any server. + */ +describe("shouldBlockSocketSourcedAction", () => { + const healthy = { status: "current", bytes: 0, total: 0 } as const; + + it("allows a card click on the web, where no desktop state exists", () => { + expect(shouldBlockSocketSourcedAction(null, null)).toBe(false); + }); + + it("allows a card click when both desktop states are healthy", () => { + expect( + shouldBlockSocketSourcedAction(healthy, { status: "signed-in" }), + ).toBe(false); + }); + + it("still blocks a card click while an update is pending", () => { + expect( + shouldBlockSocketSourcedAction( + { status: "staged", bytes: 0, total: 0 }, + { + status: "signed-in", + }, + ), + ).toBe(true); + }); + + it("still blocks a card click while signed out", () => { + expect( + shouldBlockSocketSourcedAction(healthy, { + status: "signed-out", + reason: "steam-wedged", + }), + ).toBe(true); + }); + + // The finding this rule answers: the funnel refused to gate a "public" join + // on reachability while the card that produces it dimmed and refused one + // step earlier, on exactly the same lobby. Now both ask the same question. + it("agrees with the funnel on the join its card produces", () => { + const publicJoin = { gameID: "g", source: "public" } as any; + for (const update of [ + null, + healthy, + { status: "staged", bytes: 0, total: 0 } as const, + ]) { + for (const session of [ + null, + { status: "signed-in" } as const, + { status: "signed-out", reason: "steam-wedged" } as const, + ]) { + expect(shouldBlockSocketSourcedAction(update, session)).toBe( + shouldBlockJoin(publicJoin, update, session), + ); + } + } + }); +}); diff --git a/tests/ReachabilityGating.test.ts b/tests/ReachabilityGating.test.ts index f0daa313fa..9c1af050a9 100644 --- a/tests/ReachabilityGating.test.ts +++ b/tests/ReachabilityGating.test.ts @@ -1,8 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { ClientEnv } from "../src/client/ClientEnv"; import { + attemptInFlight, backendUnreachableConfirmed, ensureServerList, + MANUAL_RETRY_COOLDOWN_MS, resetServerList, retryServerList, } from "../src/client/ServerList"; @@ -54,6 +56,11 @@ let wiggle: ReturnType; let joinLobby: ReturnType; let messages: string[]; let fetchMock: ReturnType; +// Added to the real clock, so a test can step past the manual-retry cooldown +// without waiting out five real seconds. Only Date.now is moved: ServerList's +// throttles are all clock comparisons, and faking timers wholesale would +// stall Lit's own scheduling. +let clockOffset: number; function stub(tag: string, methods: Record): void { const el = document.createElement(tag); @@ -98,6 +105,15 @@ function clickEveryButton(): number { return buttons.length; } +/** + * The rendered public-lobby card's button. Socket-sourced: whatever the + * server-list API is doing, this one must keep working (the reachability rule + * at the top of GameModeSelector.ts). + */ +function lobbyCardButton(): HTMLButtonElement | null { + return selector.querySelector("button.group"); +} + /** Announces a reachability change the way the heartbeat does. */ async function announce(reachable: boolean, confirmed = false): Promise { document.dispatchEvent( @@ -140,6 +156,9 @@ beforeEach(() => { fetchMock = vi.fn(async () => new Response("{}", { status: 404 })); vi.stubGlobal("fetch", fetchMock); + clockOffset = 0; + const realNow = Date.now.bind(Date); + vi.spyOn(Date, "now").mockImplementation(() => realNow() + clockOffset); vi.spyOn(console, "warn").mockImplementation(() => {}); vi.spyOn(console, "info").mockImplementation(() => {}); @@ -210,7 +229,10 @@ describe("the multiplayer entry points while the backend is unreachable", () => expect(messages).toEqual([]); }); - it("dims and refuses every entry point once the outage is confirmed", async () => { + it("dims and refuses the API-DEPENDENT entry points once the outage is confirmed", async () => { + // Create, Ranked and Join-by-code each have to resolve a server for + // something nothing has told this client about, so a dead list API really + // does mean the click cannot work. selector = await mountSelector(); await announce(false, true); @@ -221,7 +243,42 @@ describe("the multiplayer entry points while the backend is unreachable", () => expect(joinOpen).not.toHaveBeenCalled(); expect(hostOpen).not.toHaveBeenCalled(); - expect(joinLobby).not.toHaveBeenCalled(); + }); + + it("still joins the public lobby card during a confirmed outage", async () => { + // The rule (GameModeSelector, top of file): this lobby arrived over a + // live game-server socket, which is the only liveness the join needs. The + // server-list API's health says nothing about that server, and Main's + // funnel would let the very same join through -- so refusing here would + // only reject a join that is already under way. + selector = await mountSelector(); + await announce(false, true); + + const card = lobbyCardButton(); + expect(card).not.toBeNull(); + card!.click(); + + expect(joinLobby).toHaveBeenCalled(); + expect(joinLobby.mock.calls[0][0].detail.gameID).toBe("public-1"); + }); + + it("does not dim the public lobby card during a confirmed outage", async () => { + selector = await mountSelector(); + await announce(false, true); + + expect(lobbyCardButton()?.getAttribute("aria-disabled")).toBe("false"); + }); + + it("says nothing when a card click goes through during an outage", async () => { + // The toast is for a REFUSAL. A join that proceeded has nothing to + // apologise for, and telling the player the servers are unreachable while + // taking them into a game would be a lie. + selector = await mountSelector(); + await announce(false, true); + + lobbyCardButton()!.click(); + + expect(messages).toEqual([]); }); it("says why, on the web, where there is no status bar to read", async () => { @@ -284,6 +341,9 @@ describe("the multiplayer entry points while the backend is unreachable", () => clickEveryButton(); expect(joinOpen).not.toHaveBeenCalled(); expect(hostOpen).not.toHaveBeenCalled(); + // ...and the card that came over the socket still joins, from the same + // seed. + expect(joinLobby).toHaveBeenCalled(); }); it("does not gate a selector that mounted after an attempt SUCCEEDED", async () => { @@ -299,3 +359,127 @@ describe("the multiplayer entry points while the backend is unreachable", () => expect(hostOpen).toHaveBeenCalled(); }); }); + +/** + * The web's escape hatch. Desktop has the status bar's Retry button; the web + * has no bar at all, so without this the only way out of a gated state is the + * heartbeat's next beat -- and that backs off to as much as RETRY_MAX_MS once + * an outage has run a while. A toast reading "Check your connection and try + * again" over a button where trying again provably does nothing is worse than + * no toast, so on the web the refused click IS the retry. + * + * Driven through the real ServerList module: the point is that the click + * reaches the same probe the desktop button does, throttled by the same + * policy (manualRetryAvailable) and the same clock. + */ +describe("a refused multiplayer click on the web", () => { + /** Past the shared manual-retry cooldown, so a probe is available again. */ + function advancePastRetryCooldown(): void { + clockOffset += MANUAL_RETRY_COOLDOWN_MS + 1; + } + + /** Mounts a selector against a module that has already confirmed an outage. */ + async function mountGated(): Promise { + await confirmOutage(); + selector = await mountSelector(); + // confirmOutage's own manual retry started the cooldown; step past it so + // each test starts from "a probe is available". + advancePastRetryCooldown(); + } + + it("probes the API again", async () => { + await mountGated(); + const before = fetchMock.mock.calls.length; + + clickEveryButton(); + + expect(messages).toContain("common.backend_unreachable"); + expect(fetchMock.mock.calls.length).toBe(before + 1); + }); + + it("collapses a flurry of refused clicks into a single probe", async () => { + // Three gated entry points are clicked in that one pass. The first starts + // an attempt; while it is in flight the rest could only join it, so + // offering them a request each would just point traffic at a backend that + // is already known to be struggling. + await mountGated(); + const before = fetchMock.mock.calls.length; + + clickEveryButton(); + + expect(fetchMock.mock.calls.length).toBe(before + 1); + }); + + it("does not probe again inside the cooldown, but still says why", async () => { + await mountGated(); + clickEveryButton(); + await vi.waitFor(() => expect(attemptInFlight()).toBe(false)); + const after = fetchMock.mock.calls.length; + messages.length = 0; + + clickEveryButton(); + + expect(fetchMock.mock.calls.length).toBe(after); + // The player is still being refused, so they are still told so: the + // throttle is on the request, not on the explanation. + expect(messages).toContain("common.backend_unreachable"); + }); + + it("probes again once the cooldown has elapsed", async () => { + await mountGated(); + clickEveryButton(); + await vi.waitFor(() => expect(attemptInFlight()).toBe(false)); + const after = fetchMock.mock.calls.length; + + advancePastRetryCooldown(); + clickEveryButton(); + + expect(fetchMock.mock.calls.length).toBe(after + 1); + }); + + it("lets the player back in when the probe finds the backend up", async () => { + // End to end: a refused click starts the probe, the probe answers, the + // reachability event that carries the answer un-dims the buttons, and the + // next click goes through. No Retry button involved anywhere. + await mountGated(); + fetchMock.mockImplementation( + async () => + new Response("{}", { + status: 404, + }), + ); + + clickEveryButton(); + await vi.waitFor(() => expect(backendUnreachableConfirmed()).toBe(false)); + await selector.updateComplete; + + expect( + selector.querySelectorAll('button[aria-disabled="true"]').length, + ).toBe(0); + expect(clickEveryButton()).toBeGreaterThan(0); + expect(hostOpen).toHaveBeenCalled(); + expect(joinOpen).toHaveBeenCalled(); + }); + + it("does not probe when the click was not refused", async () => { + // The control. A healthy page clicks the same buttons; nothing here may + // turn an ordinary click into an extra request. + selector = await mountSelector(); + const before = fetchMock.mock.calls.length; + + clickEveryButton(); + + expect(fetchMock.mock.calls.length).toBe(before); + }); + + it("does not probe on a card click that goes through", async () => { + // Socket-sourced: it was never refused, so there is nothing to retry. + await mountGated(); + const before = fetchMock.mock.calls.length; + + lobbyCardButton()!.click(); + + expect(joinLobby).toHaveBeenCalled(); + expect(fetchMock.mock.calls.length).toBe(before); + }); +}); diff --git a/tests/client/ServerList.test.ts b/tests/client/ServerList.test.ts index 13e5cca119..a17bf67da5 100644 --- a/tests/client/ServerList.test.ts +++ b/tests/client/ServerList.test.ts @@ -6,6 +6,8 @@ import { backendReachable, backendUnreachableConfirmed, ensureServerList, + MANUAL_RETRY_COOLDOWN_MS, + manualRetryAvailable, redirectToGameVersion, reloadWouldRescue, resetServerList, @@ -596,6 +598,73 @@ describe("backend reachability", () => { }); }); +/** + * The policy above retryServerList's own floor, shared by both shells' + * player-initiated retries so they cannot drift: the desktop status bar's + * Retry button (which renders its disabled state from the same two + * conditions) and, on the web where there is no such button, a refused + * multiplayer click (GameModeSelector.reportMultiplayerRefusal). The web + * caller has nothing to disable, so it asks this instead. + */ +describe("manualRetryAvailable", () => { + it("is available before anyone has asked", () => { + expect(manualRetryAvailable()).toBe(true); + }); + + it("is unavailable while an attempt is out, whoever started it", async () => { + let release: (r: Response) => void = () => {}; + fetchMock.mockImplementation( + async () => + await new Promise((resolve) => { + release = resolve; + }), + ); + + // The heartbeat's own beat, not a manual one: a press landing on top of + // it could only join the attempt already out, so offering it is a lie. + const beat = ensureServerList(); + expect(attemptInFlight()).toBe(true); + expect(manualRetryAvailable()).toBe(false); + + release(jsonResponse(API_LIST)); + await beat; + expect(manualRetryAvailable()).toBe(true); + }); + + it("is unavailable for the cooldown after a player-initiated attempt", async () => { + vi.useFakeTimers(); + fetchMock.mockRejectedValue(new TypeError("network down")); + + await retryServerList(); + // Settled in milliseconds, which is exactly the case the cooldown exists + // for: without it a stubbed or fast failure hands the affordance straight + // back to someone clicking at an outage. + expect(attemptInFlight()).toBe(false); + expect(manualRetryAvailable()).toBe(false); + + await vi.advanceTimersByTimeAsync(MANUAL_RETRY_COOLDOWN_MS - 1); + expect(manualRetryAvailable()).toBe(false); + + await vi.advanceTimersByTimeAsync(1); + expect(manualRetryAvailable()).toBe(true); + }); + + it("counts a retry the floor swallowed as the same cooldown", async () => { + vi.useFakeTimers(); + fetchMock.mockRejectedValue(new TypeError("network down")); + await retryServerList(); + await vi.advanceTimersByTimeAsync(500); + + // Inside the 1s floor: no new attempt, and no new cooldown either -- the + // window still ends MANUAL_RETRY_COOLDOWN_MS after the attempt that ran. + await retryServerList(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(MANUAL_RETRY_COOLDOWN_MS - 500); + expect(manualRetryAvailable()).toBe(true); + }); +}); + describe("retryServerList", () => { // The Retry on the desktop status bar's offline state (OPE-439). The retry // interval exists to stop timer-driven callers hammering a down API between