From 257deff2f885dc6562503a2dfccda5dff18d63cc Mon Sep 17 00:00:00 2001 From: Marcus Kempe Date: Thu, 24 Sep 2026 22:13:07 +0200 Subject: [PATCH 1/2] Cut round trips on polled endpoints and per-request auth counterState and dbStatus each become one statement (the database size is memoised for a minute), the REST guard hands back the event it resolved instead of handlers looking it up again, the finished-runner count only runs when it can change the capability set, the course-map window preview shares the tile render gate, and controlCompletionStatus loads only the requested course's runners and their cards. Co-authored-by: Cursor --- docs/map-tile-rendering.md | 2 +- docs/perf-polling-and-auth-round-trips.md | 69 +++++++++ e2e/online-input-config.spec.ts | 7 +- .../integration/control-completion.test.ts | 135 ++++++++++++++++++ .../src/__tests__/integration/event.test.ts | 41 +++++- .../api/src/__tests__/permissions.test.ts | 27 ++++ packages/api/src/__tests__/ttl-memo.test.ts | 50 +++++++ packages/api/src/course-maps/routes.ts | 47 +++--- packages/api/src/map-render-limits.ts | 20 +++ packages/api/src/map-tiles.ts | 62 ++++---- packages/api/src/permissions.ts | 41 ++++-- packages/api/src/restGuard.ts | 32 +++-- packages/api/src/routers/course.ts | 61 +++++--- packages/api/src/routers/event.ts | 100 +++++++------ packages/api/src/ttl-memo.ts | 34 +++++ 15 files changed, 587 insertions(+), 141 deletions(-) create mode 100644 docs/perf-polling-and-auth-round-trips.md create mode 100644 packages/api/src/__tests__/integration/control-completion.test.ts create mode 100644 packages/api/src/__tests__/ttl-memo.test.ts create mode 100644 packages/api/src/ttl-memo.ts diff --git a/docs/map-tile-rendering.md b/docs/map-tile-rendering.md index 1784697..a93b22c 100644 --- a/docs/map-tile-rendering.md +++ b/docs/map-tile-rendering.md @@ -123,7 +123,7 @@ map bug: unrelated queries time out while someone pans a map. | `map_tiles` table | Shared | The real cache. Written with `ON CONFLICT DO NOTHING`, so concurrent renders of the same block across instances are harmless. | | SVG cache | Per process | Parsed map SVGs, `MAP_SVG_CACHE_EVENTS` of them. Amortises the ~70 ms parse and the OCAD read. | | In-flight block map | Per process | A viewport fetches ~20 tiles at once; they collapse onto one render per block. | -| Render semaphore | Per process | Bounds concurrent block renders (`MAP_RENDER_CONCURRENCY`). One permit covers a block's composite **and** ink rasters, run side by side. Foreground requests are served before pre-cache work, so background rendering never queues a user behind a whole sweep. | +| Render semaphore | Per process | `renderGate()` in `map-render-limits.ts` bounds concurrent rasterisations (`MAP_RENDER_CONCURRENCY`). One permit covers a block's composite **and** ink rasters, run side by side. The course-map layout preview (`/api/maps/:nameId/window.png`) takes permits from the same pool. Foreground requests are served before pre-cache work, so background rendering never queues a user behind a whole sweep. | | Queue bound | Per process | At most `MAP_RENDER_MAX_QUEUE` foreground blocks may wait for a permit. Beyond that the tile route answers **503 + `Retry-After: 5`** immediately instead of letting the request sit until the platform kills it. | Everything above the database is a per-process optimisation, and losing diff --git a/docs/perf-polling-and-auth-round-trips.md b/docs/perf-polling-and-auth-round-trips.md new file mode 100644 index 0000000..b7cdba3 --- /dev/null +++ b/docs/perf-polling-and-auth-round-trips.md @@ -0,0 +1,69 @@ +# Performance: polling endpoints and per-request auth round trips + +**Date:** 2026-09-24 +**Context:** follow-up sweep after +[bugfix-map-tile-cold-render-cloud-timeouts.md](bugfix-map-tile-cold-render-cloud-timeouts.md). +Cloud Run request logs since 2026-09-10 (requests ≥ 0.5 s, tiles +excluded) were grouped by endpoint; the recurring entries were all +polls or per-request plumbing, not user actions. + +The scarce resource on the cloud deployment is Cloud SQL +(`db-f1-micro`, ~22 usable connections, non-trivial per-query latency +through the connector). Everything below trades sequential round trips +for single statements or for not asking at all. + +## What changed + +| Endpoint / path | Before | After | +|---|---|---| +| `competition.counterState` (every 5 s per tab) | 9 sequential queries — one `MAX(updated_at)` per table in a loop, plus punches, events and a Prisma aggregate | 1 statement with scalar subselects | +| `competition.dbStatus` (every 3 s per tab, public) | 3 queries including `pg_database_size()` each time | 1 statement; database size memoised for 60 s (`ttl-memo.ts`) | +| `assertRestAccess` + route handler | Guard resolved the event, handler resolved it again; `countFinishedRunners` on every authenticated request | Guard returns the `EventRef`; handlers reuse it. The finished-runner count runs only when it can change the capability set (`finishedCountMatters`) | +| `/api/maps/:nameId/window.png` | Full print-window rasterisation outside the render semaphore | Shares `renderGate()` with tiles; refused with `503 Retry-After` when the queue is full | +| `course.controlCompletionStatus` (every 15 s) | Loaded every classed runner and every card's full `punches_raw` for the event | Scoped to the requested course's runners and to the cards those runners carry; one regex per control instead of one per runner per code | + +### Why the finished-runner count is usually redundant + +`resolveEventCapabilities` adds `event.view` / `results.view` / +`courses.view` for every signed-in user once the event is *completed*, +and completion is `date < today || finishedRunners > 0`. The count is +therefore irrelevant when the date is already in the past, or when the +user's grants already include all three capabilities — which is every +club member with a view role. Only a user without those grants, on a +current or future event, still triggers the count. Semantics are +unchanged; `permissions.test.ts` covers the decision table. + +### REST guard contract + +`assertRestAccess()` now returns `EventRef | null` instead of `boolean`. +Existing `if (!(await assertRestAccess(...)))` call sites keep working; +new handlers should take the returned event rather than looking the slug +up again. Pass `event:` when the handler had to resolve it first (the +tile-progress poll does, because an unknown event is not an error +there). + +## Verification + +- `permissions.test.ts` — `finishedCountMatters` decision table. +- `ttl-memo.test.ts` — TTL, shared in-flight computation, failures not cached. +- `integration/event.test.ts` — `counterState` returns all nine legacy + keys; `oClub` ignores removed runners while `oRunner` does not. +- `integration/control-completion.test.ts` — course-scoped vs. aggregate + counting, direct course assignment overriding the class course, + alternate punch codes, removed / unclassed runners, cards nobody + carries. The same suite passes against the previous implementation, so + the narrowing changed nothing observable. +- `integration/map-tiles.test.ts`, `integration/course-maps.test.ts` — + routes through the changed guard (404 / 200 / 503 paths). + +## Not changed, and why + +- `/api/version`, `/sw.js`, first `users.me` at 4–12 s: instance cold + start (Node + Prisma + Cloud SQL connector handshake). Only + `--min-instances=1` removes it. +- `lease.status` every 10 s: two indexed reads; its slow tail tracks + instance saturation, not the query. +- `identityFromRequest` still looks the user up per request. Caching + identity is an auth decision, not a performance one. +- `maps.pdf` still fetches the full event row (needs `name` / + `organizerName`); it is a one-off export. diff --git a/e2e/online-input-config.spec.ts b/e2e/online-input-config.spec.ts index dbd1e2e..e68b7db 100644 --- a/e2e/online-input-config.spec.ts +++ b/e2e/online-input-config.spec.ts @@ -100,14 +100,17 @@ test.describe("Online Input panel", () => { await page.getByTestId("online-input-new-target").selectOption("2"); // PunchFinish await page.getByTestId("online-input-add-mapping").click(); + // The row appears after the mutation round trip *and* the config + // refetch it triggers; under a full sharded run that can exceed the + // default 5 s expect timeout. const mapping = page.getByTestId("online-input-mapping-100"); - await expect(mapping).toBeVisible(); + await expect(mapping).toBeVisible({ timeout: 15000 }); await expect(mapping).toContainText("100"); await expect(mapping).toContainText("Finish"); // Remove it again await mapping.getByRole("button", { name: "Remove mapping" }).click(); - await expect(mapping).toHaveCount(0); + await expect(mapping).toHaveCount(0, { timeout: 15000 }); }); test("Reset button on lastId becomes enabled after a poll has advanced it", async ({ diff --git a/packages/api/src/__tests__/integration/control-completion.test.ts b/packages/api/src/__tests__/integration/control-completion.test.ts new file mode 100644 index 0000000..34db5ac --- /dev/null +++ b/packages/api/src/__tests__/integration/control-completion.test.ts @@ -0,0 +1,135 @@ +/** + * `course.controlCompletionStatus` is polled every 15 s by the dashboard + * and the map panel. These tests pin its counting rules so the query can + * be narrowed (runners on the course only, cards those runners carry) + * without changing what it reports: + * + * - a runner belongs to a course directly (`runner.courseId`) or via + * its class (`class.courseId`), the direct assignment winning; + * - `passed` counts runners whose card holds any of the control's + * punch codes; + * - with no `courseId` every course in the event is aggregated. + */ + +import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { + createTestEvent, + disconnect, + type TestEventContext, +} from "../helpers/test-db.js"; +import { makeCaller } from "../helpers/caller.js"; + +let ctx: TestEventContext; +let caller: ReturnType; +let courseASeq: number; +let courseBSeq: number; + +beforeAll(async () => { + ctx = await createTestEvent("control_completion"); + caller = makeCaller(ctx.event); + const db = ctx.db; + const eventId = ctx.eventId; + + const c31 = await db.control.create({ + data: { eventId, codes: "31;131", xpos: 0, ypos: 0 }, + select: { id: true }, + }); + const c32 = await db.control.create({ + data: { eventId, codes: "32", xpos: 10, ypos: 0 }, + select: { id: true }, + }); + const c40 = await db.control.create({ + data: { eventId, codes: "40", xpos: 20, ypos: 0 }, + select: { id: true }, + }); + + const courseA = await db.course.create({ + data: { eventId, name: "A", lengthM: 1000 }, + select: { id: true, seq: true }, + }); + const courseB = await db.course.create({ + data: { eventId, name: "B", lengthM: 1000 }, + select: { id: true, seq: true }, + }); + courseASeq = courseA.seq; + courseBSeq = courseB.seq; + await db.courseControl.createMany({ + data: [ + { courseId: courseA.id, position: 0, controlId: c31.id }, + { courseId: courseA.id, position: 1, controlId: c32.id }, + { courseId: courseB.id, position: 0, controlId: c40.id }, + ], + }); + + const classA = await db.class.create({ + data: { eventId, name: "H21", courseId: courseA.id }, + select: { id: true }, + }); + const classB = await db.class.create({ + data: { eventId, name: "D21", courseId: courseB.id }, + select: { id: true }, + }); + + await db.runner.createMany({ + data: [ + // On A via class; punched 31 and 32. + { eventId, name: "A via class, full", classId: classA.id, cardNo: 1001 }, + // On A via class; punched only the alternate code of control 31. + { eventId, name: "A via class, alt code", classId: classA.id, cardNo: 1002 }, + // Class says B, but assigned directly to A; no card read yet. + { eventId, name: "A direct", classId: classB.id, courseId: courseA.id, cardNo: 1003 }, + // On B via class; punched 40. + { eventId, name: "B via class", classId: classB.id, cardNo: 2001 }, + // Removed runner on A — must not count. + { eventId, name: "A removed", classId: classA.id, cardNo: 1004, removed: true }, + // No class at all — never counted (matches the dashboard's rule). + { eventId, name: "unclassed", courseId: courseA.id, cardNo: 1005 }, + ], + }); + await db.card.createMany({ + data: [ + { eventId, cardNo: 1001, punchesRaw: "31-100.0;32-200.0" }, + { eventId, cardNo: 1002, punchesRaw: "131-100.0" }, + { eventId, cardNo: 1004, punchesRaw: "31-100.0;32-200.0" }, + { eventId, cardNo: 1005, punchesRaw: "31-100.0;32-200.0" }, + { eventId, cardNo: 2001, punchesRaw: "40-300.0" }, + // A card nobody on this event carries: must not influence anything. + { eventId, cardNo: 9999, punchesRaw: "31-1.0;32-1.0;40-1.0" }, + ], + }); +}, 60_000); + +afterAll(async () => { + await ctx?.cleanup(); + await disconnect(); +}, 30_000); + +const byCode = (rows: Array<{ code: number; total: number; passed: number }>) => + Object.fromEntries(rows.map((r) => [r.code, { total: r.total, passed: r.passed }])); + +describe("course.controlCompletionStatus", () => { + it("scoped to a course counts only that course's runners and their cards", async () => { + const rows = byCode( + await caller.course.controlCompletionStatus({ courseId: courseASeq }), + ); + // Three live, classed runners on A: two via class, one direct. + expect(rows[31]).toEqual({ total: 3, passed: 2 }); // 31 or 131 + expect(rows[32]).toEqual({ total: 3, passed: 1 }); + expect(rows[40]).toBeUndefined(); + }); + + it("a runner's direct course assignment overrides the class course", async () => { + const rows = byCode( + await caller.course.controlCompletionStatus({ courseId: courseBSeq }), + ); + // "A direct" is in class D21 (→ B) but assigned to A, so B has one runner. + expect(rows[40]).toEqual({ total: 1, passed: 1 }); + }); + + it("without a course aggregates every course in the event", async () => { + const rows = byCode(await caller.course.controlCompletionStatus()); + expect(rows[31]).toEqual({ total: 3, passed: 2 }); + expect(rows[32]).toEqual({ total: 3, passed: 1 }); + expect(rows[40]).toEqual({ total: 1, passed: 1 }); + }); +}); diff --git a/packages/api/src/__tests__/integration/event.test.ts b/packages/api/src/__tests__/integration/event.test.ts index 5ddd5e0..5d7d0c5 100644 --- a/packages/api/src/__tests__/integration/event.test.ts +++ b/packages/api/src/__tests__/integration/event.test.ts @@ -325,12 +325,47 @@ describe("event.counterState (legacy alias)", () => { const ref = (await resolveEvent(slug))!; const caller = makeCaller(ref); try { - await caller.class.create({ name: "H21" }); + const cls = await caller.class.create({ name: "H21" }); const counters = await caller.event.counterState(); - expect(typeof counters.oRunner).toBe("number"); - expect(typeof counters.oClass).toBe("number"); + // Every legacy key the web hook diffs against, all numeric ms. + for (const key of [ + "oRunner", + "oClass", + "oCourse", + "oControl", + "oCard", + "oTeam", + "oPunch", + "oEvent", + "oClub", + ]) { + expect(typeof counters[key]).toBe("number"); + expect(Number.isInteger(counters[key])).toBe(true); + } expect(counters.oClass).toBeGreaterThan(0); + expect(counters.oEvent).toBeGreaterThan(0); + expect(counters.oRunner).toBe(0); expect(counters.oPunch).toBe(0); + + // A runner bumps oRunner and oClub alike; a *removed* runner only + // bumps oRunner — oClub tracks live entries, which is what the + // club list on the web keys off. + await caller.runner.create({ name: "A", classId: cls.id, clubName: "OK A" }); + const after = await caller.event.counterState(); + expect(after.oRunner).toBeGreaterThan(0); + expect(after.oClub).toBe(after.oRunner); + + await new Promise((r) => setTimeout(r, 5)); + await caller.runner.create({ name: "B", classId: cls.id, clubName: "OK B" }); + const db = prisma(); + const b = await db.runner.findFirstOrThrow({ + where: { eventId: ref.id, name: "B" }, + select: { id: true }, + }); + await db.runner.update({ where: { id: b.id }, data: { removed: true } }); + const removed = await caller.event.counterState(); + expect(removed.oRunner).toBeGreaterThan(after.oRunner); + expect(removed.oClub).toBeLessThan(removed.oRunner); } finally { await publicCaller.event.delete({ nameId: slug }); await publicCaller.event.purgeDeleted(); diff --git a/packages/api/src/__tests__/permissions.test.ts b/packages/api/src/__tests__/permissions.test.ts index a3b8cee..dde17e8 100644 --- a/packages/api/src/__tests__/permissions.test.ts +++ b/packages/api/src/__tests__/permissions.test.ts @@ -1,7 +1,9 @@ import { describe, it, expect } from "vitest"; import { ALL_CAPABILITIES, + COMPLETION_CAPABILITIES, effectiveCapabilities, + finishedCountMatters, isEventCompleted, } from "../permissions.js"; import type { AuthUser } from "../auth.js"; @@ -115,3 +117,28 @@ describe("isEventCompleted", () => { expect(isEventCompleted("2099-12-31", 0)).toBe(false); }); }); + +// The finished-runner count runs on every authenticated request purely to +// decide whether completion should add the three view capabilities. It +// is skipped whenever it cannot change the outcome. +describe("finishedCountMatters", () => { + it("is false when the date alone completes the event", () => { + expect(finishedCountMatters([["courses.edit"]], "2020-01-01")).toBe(false); + }); + + it("is false when the grants already include every completion capability", () => { + expect(finishedCountMatters([[...COMPLETION_CAPABILITIES]], "2099-12-31")).toBe(false); + // Spread across groups counts too. + expect( + finishedCountMatters( + [["event.view"], ["results.view", "courses.view"]], + "2099-12-31", + ), + ).toBe(false); + }); + + it("is true for a current or future event whose grants leave a completion capability out", () => { + expect(finishedCountMatters([["courses.view", "courses.edit"]], "2099-12-31")).toBe(true); + expect(finishedCountMatters([], "2099-12-31")).toBe(true); + }); +}); diff --git a/packages/api/src/__tests__/ttl-memo.test.ts b/packages/api/src/__tests__/ttl-memo.test.ts new file mode 100644 index 0000000..45b00df --- /dev/null +++ b/packages/api/src/__tests__/ttl-memo.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from "vitest"; +import { ttlMemo } from "../ttl-memo.js"; + +describe("ttlMemo", () => { + it("computes once within the ttl and again after it", async () => { + let clock = 0; + let calls = 0; + const memo = ttlMemo(1000, async () => ++calls, () => clock); + + expect(await memo.get()).toBe(1); + clock = 999; + expect(await memo.get()).toBe(1); + clock = 1000; + expect(await memo.get()).toBe(2); + expect(calls).toBe(2); + }); + + it("shares one in-flight computation between concurrent callers", async () => { + let calls = 0; + let release!: (v: number) => void; + const memo = ttlMemo(1000, () => { + calls++; + return new Promise((r) => (release = r)); + }); + const a = memo.get(); + const b = memo.get(); + release(42); + expect(await Promise.all([a, b])).toEqual([42, 42]); + expect(calls).toBe(1); + }); + + it("does not cache a failure", async () => { + let calls = 0; + const memo = ttlMemo(1000, async () => { + calls++; + if (calls === 1) throw new Error("boom"); + return "ok"; + }); + await expect(memo.get()).rejects.toThrow("boom"); + expect(await memo.get()).toBe("ok"); + }); + + it("clear() forces a recompute", async () => { + let calls = 0; + const memo = ttlMemo(1000, async () => ++calls); + await memo.get(); + memo.clear(); + expect(await memo.get()).toBe(2); + }); +}); diff --git a/packages/api/src/course-maps/routes.ts b/packages/api/src/course-maps/routes.ts index 3ab25a8..c79e2a6 100644 --- a/packages/api/src/course-maps/routes.ts +++ b/packages/api/src/course-maps/routes.ts @@ -9,6 +9,12 @@ import { import { z } from "zod"; import { prisma } from "../db.js"; import { assertRestAccess } from "../restGuard.js"; +import { + RENDER_BUSY_RETRY_AFTER_S, + RenderBusyError, + renderGate, + renderMaxQueue, +} from "../map-render-limits.js"; import { composeMapPageSvg, renderBaseMapWindow } from "./map-page-svg.js"; import { graphicToResolved } from "./graphics.js"; import { getBaseMapInfo, loadBaseMapSvg } from "./map-source.js"; @@ -314,15 +320,10 @@ export function registerCourseMapRoutes( }; }>("/api/maps/:nameId/window.png", async (req, reply) => { const { nameId } = req.params; - if (!(await assertRestAccess(req, reply, { nameId, cap: "courses.view" }))) { - return; - } + const event = await assertRestAccess(req, reply, { nameId, cap: "courses.view" }); + if (!event) return; try { const db = prisma(); - const event = await db.event.findUnique({ where: { nameId } }); - if (!event || event.removed) { - return reply.code(404).send({ error: "Event not found" }); - } const cx = Number(req.query.cx); const cy = Number(req.query.cy); if (!Number.isFinite(cx) || !Number.isFinite(cy)) { @@ -421,10 +422,18 @@ export function registerCourseMapRoutes( dataLayer: layer === "ink" ? "map-ink" : "map-full", })}`; const { renderAsync } = await import("@resvg/resvg-js"); - const rendered = await renderAsync(svg, { - fitTo: { mode: "width", value: widthPx }, - ...(layer === "ink" ? {} : { background: "white" }), - }); + // Same permit pool as the slippy tiles: a layout-editor preview is + // a full print-window rasterisation and must not run on top of + // `MAP_RENDER_CONCURRENCY` tile renders. Refused with 503 when the + // queue is full, like a tile; the editor simply retries. + const rendered = await renderGate().run( + () => + renderAsync(svg, { + fitTo: { mode: "width", value: widthPx }, + ...(layer === "ink" ? {} : { background: "white" }), + }), + { maxQueue: renderMaxQueue() }, + ); const body = Buffer.from(rendered.asPng()); const etag = `"${createHash("sha256").update(body).digest("base64url")}"`; windowPngCache.set(cacheKey, { body, etag }); @@ -435,6 +444,13 @@ export function registerCourseMapRoutes( .header("X-Cache", "miss") .send(body); } catch (error) { + if (error instanceof RenderBusyError) { + return reply + .code(503) + .header("Retry-After", String(RENDER_BUSY_RETRY_AFTER_S)) + .header("Cache-Control", "no-store") + .send({ error: "Map renderer busy", waiting: error.waiting }); + } return reply.code(400).send({ error: error instanceof Error ? error.message : "Could not render map window", @@ -446,18 +462,13 @@ export function registerCourseMapRoutes( Params: { nameId: string; id: string }; }>("/api/maps/:nameId/graphics/:id", async (req, reply) => { const { nameId } = req.params; - if (!(await assertRestAccess(req, reply, { nameId, cap: "courses.view" }))) { - return; - } + const event = await assertRestAccess(req, reply, { nameId, cap: "courses.view" }); + if (!event) return; const id = Number(req.params.id); if (!Number.isInteger(id) || id <= 0) { return reply.code(400).send({ error: "Invalid graphic id" }); } const db = prisma(); - const event = await db.event.findUnique({ where: { nameId } }); - if (!event || event.removed) { - return reply.code(404).send({ error: "Event not found" }); - } const graphic = await db.graphic.findFirst({ where: { id: BigInt(id), diff --git a/packages/api/src/map-render-limits.ts b/packages/api/src/map-render-limits.ts index 5cb945b..a5a1513 100644 --- a/packages/api/src/map-render-limits.ts +++ b/packages/api/src/map-render-limits.ts @@ -136,6 +136,13 @@ export function evictForInsert(cache: Map, cap: number): void { } } +/** + * Seconds a client refused with 503 should wait before asking again. + * Long enough for a block to finish and free a permit; short enough that + * a viewport fills in visibly rather than stalling. + */ +export const RENDER_BUSY_RETRY_AFTER_S = 5; + /** * Thrown by `Semaphore.run` when a bounded foreground task finds the * queue already full. Callers turn it into a fast 503 + Retry-After so @@ -217,3 +224,16 @@ export class Semaphore { this.available++; } } + +let processGate: Semaphore | null = null; + +/** + * The one render semaphore for the process. Every resvg rasterisation of + * a base map — slippy tiles and the course-map layout preview alike — + * goes through it, so the two cannot together exceed + * `MAP_RENDER_CONCURRENCY` and compete for the same two vCPUs. + */ +export function renderGate(): Semaphore { + processGate ??= new Semaphore(renderConcurrency()); + return processGate; +} diff --git a/packages/api/src/map-tiles.ts b/packages/api/src/map-tiles.ts index e6d6cdd..b9f2b54 100644 --- a/packages/api/src/map-tiles.ts +++ b/packages/api/src/map-tiles.ts @@ -42,7 +42,7 @@ */ import type { FastifyInstance } from "fastify"; -import { prisma, onMapUpload } from "./db.js"; +import { prisma, onMapUpload, resolveEvent } from "./db.js"; import { assertRestAccess } from "./restGuard.js"; import { ocadBoundsToWgs84, @@ -70,15 +70,15 @@ import { type ViewBox, } from "./map-window.js"; import { + RENDER_BUSY_RETRY_AFTER_S, RenderBusyError, - Semaphore, blockTiles, evictForInsert, precacheBlockDelayMs, precacheEnabled, precacheMaxZoom, precacheMinZoom, - renderConcurrency, + renderGate, renderMaxQueue, supersample, svgCacheEvents, @@ -94,12 +94,7 @@ import { ensureEventMapRenderKey } from "./map-render-cache.js"; const TILE_SIZE = 256; -/** - * Seconds a refused (503) tile client should wait before asking again. - * Long enough for a block to finish and free a permit; short enough that - * a viewport fills in visibly rather than stalling. - */ -export const RENDER_BUSY_RETRY_AFTER_S = 5; +export { RENDER_BUSY_RETRY_AFTER_S }; /** * Everything a tile request needs to know about the event's map, read @@ -161,11 +156,7 @@ const TRANSPARENT_TILE_PNG = Buffer.from( "base64", ); -let renderGate: Semaphore | null = null; -function gate(): Semaphore { - renderGate ??= new Semaphore(renderConcurrency()); - return renderGate; -} +const gate = renderGate; /** Drop in-process caches for a render key (profile/rotation change). */ export function invalidateRenderKey(renderKey: string): void { @@ -194,15 +185,6 @@ function invalidateEvent(eventId: bigint): void { void eventId; } -async function resolveEventId(nameId: string): Promise { - if (!nameId) return null; - const row = await prisma().event.findUnique({ - where: { nameId }, - select: { id: true }, - }); - return row?.id ?? null; -} - const tileKey = (z: number, x: number, y: number) => `${z}/${x}/${y}`; // ─── Map source ───────────────────────────────────────────── @@ -821,13 +803,20 @@ export function registerMapTileRoutes(server: FastifyInstance): void { const rawDbName = req.headers["x-competition-id"]; const nameId = (Array.isArray(rawDbName) ? rawDbName[0] : rawDbName) ?? ""; - if (nameId && !(await assertRestAccess(req, reply, { nameId, cap: "courses.view", allowKiosk: true }))) { - return; - } - const eventId = await resolveEventId(nameId); - if (eventId === null) { + // An unknown or missing event is not an error here — the poll simply + // has nothing to report — so resolve first and only then run the guard. + const ref = nameId ? await resolveEvent(nameId) : null; + if (!ref) { return reply.send({ total: 0, done: 0, rendering: false }); } + const event = await assertRestAccess(req, reply, { + nameId, + cap: "courses.view", + allowKiosk: true, + event: ref, + }); + if (!event) return; + const eventId = event.id; const meta = await ensureEventMapRenderKey(prisma(), eventId); if (!meta) return reply.send({ total: 0, done: 0, rendering: false }); const progress = await tileProgress(meta); @@ -856,14 +845,15 @@ export function registerMapTileRoutes(server: FastifyInstance): void { return reply.code(400).send({ error: "Invalid tile request" }); } - if (!(await assertRestAccess(req, reply, { nameId, cap: "courses.view", allowKiosk: true }))) { - return; - } - - const eventId = await resolveEventId(nameId); - if (eventId === null) { - return reply.code(404).send({ error: "Unknown event" }); - } + // One lookup for auth and routing alike: the guard resolves the + // event and hands it back (404 already sent when it is unknown). + const event = await assertRestAccess(req, reply, { + nameId, + cap: "courses.view", + allowKiosk: true, + }); + if (!event) return; + const eventId = event.id; const meta = await ensureEventMapRenderKey(prisma(), eventId); if (!meta) { diff --git a/packages/api/src/permissions.ts b/packages/api/src/permissions.ts index 1c2c9af..b24087e 100644 --- a/packages/api/src/permissions.ts +++ b/packages/api/src/permissions.ts @@ -55,13 +55,35 @@ export function effectiveCapabilities(args: { for (const c of group) caps.add(c); } if (args.eventCompleted) { - caps.add("event.view"); - caps.add("results.view"); - caps.add("courses.view"); + for (const c of COMPLETION_CAPABILITIES) caps.add(c); } return caps; } +/** What a completed event grants every signed-in user. */ +export const COMPLETION_CAPABILITIES = [ + "event.view", + "results.view", + "courses.view", +] as const satisfies readonly Capability[]; + +/** + * Whether `countFinishedRunners` can affect the capability set at all. + * It only feeds `isEventCompleted`, which only adds + * `COMPLETION_CAPABILITIES` — so when the date already completes the + * event, or the grants already carry all three, the count is a wasted + * query on every authenticated request. + */ +export function finishedCountMatters( + grants: Capability[][], + eventDate: string, +): boolean { + if (isEventCompleted(eventDate, 0)) return false; + const granted = new Set(); + for (const group of grants) for (const c of group) granted.add(c); + return !COMPLETION_CAPABILITIES.every((c) => granted.has(c)); +} + const FINISHED_STATUSES = [ "ok", "missing_punch", @@ -117,14 +139,17 @@ export async function resolveEventCapabilities(args: { if (!args.user) return new Set(); if (args.user.isAdmin) return new Set(ALL_CAPABILITIES); - const [grants, finishedCount] = await Promise.all([ - loadGrantCapabilities(args.db, args.eventId, args.user.id), - countFinishedRunners(args.db, args.eventId), - ]); + const grants = await loadGrantCapabilities(args.db, args.eventId, args.user.id); + const day = eventDateString(args.eventDate); + // Only count finished runners when the answer can still swing the + // capability set; on most requests it cannot (see finishedCountMatters). + const finishedCount = finishedCountMatters(grants, day) + ? await countFinishedRunners(args.db, args.eventId) + : 0; return effectiveCapabilities({ user: args.user, grants, - eventCompleted: isEventCompleted(eventDateString(args.eventDate), finishedCount), + eventCompleted: isEventCompleted(day, finishedCount), authEnabled: true, }); } diff --git a/packages/api/src/restGuard.ts b/packages/api/src/restGuard.ts index 32b8fc4..f403c91 100644 --- a/packages/api/src/restGuard.ts +++ b/packages/api/src/restGuard.ts @@ -8,7 +8,7 @@ import { resolveUser, type AuthUser, } from "./auth.js"; -import { prisma, resolveEvent } from "./db.js"; +import { prisma, resolveEvent, type EventRef } from "./db.js"; import { resolveEventCapabilities } from "./permissions.js"; import { kioskKeyMatches, KIOSK_KEY_HEADER } from "./trpc.js"; @@ -54,7 +54,12 @@ export async function assertClubRestAccess( } /** - * Returns true if the request may proceed. On failure the reply is already sent. + * Returns the resolved event if the request may proceed, or `null` after + * sending the failure reply (404 unknown event, 401, 403). + * + * The event is always resolved — auth on or off — and returned so route + * handlers reuse it instead of looking the slug up a second time. On the + * tile path that second lookup ran for every tile in a viewport. */ export async function assertRestAccess( req: FastifyRequest, @@ -63,24 +68,27 @@ export async function assertRestAccess( nameId: string; cap: Capability; allowKiosk?: boolean; + /** Already-resolved event, when the caller had to look it up anyway. */ + event?: EventRef | null; }, -): Promise { - const { user, authEnabled, kioskKey } = await identityFromRequest(req); - if (!authEnabled) return true; - - const event = await resolveEvent(args.nameId); +): Promise { + const [{ user, authEnabled, kioskKey }, event] = await Promise.all([ + identityFromRequest(req), + args.event !== undefined ? args.event : resolveEvent(args.nameId), + ]); if (!event) { void reply.code(404).send({ error: "Unknown event" }); - return false; + return null; } + if (!authEnabled) return event; if (args.allowKiosk && kioskKeyMatches(kioskKey, event.kioskKey ?? null)) { - return true; + return event; } if (!user) { void reply.code(401).send({ error: "Not authenticated" }); - return false; + return null; } const caps = await resolveEventCapabilities({ @@ -92,9 +100,9 @@ export async function assertRestAccess( }); if (!caps.has(args.cap)) { void reply.code(403).send({ error: `Missing capability ${args.cap}` }); - return false; + return null; } - return true; + return event; } /** diff --git a/packages/api/src/routers/course.ts b/packages/api/src/routers/course.ts index 0271ede..3d2ed0b 100644 --- a/packages/api/src/routers/course.ts +++ b/packages/api/src/routers/course.ts @@ -1235,16 +1235,44 @@ export const courseRouter = router({ const classCourse = new Map(); for (const c of classes) if (c.courseId) classCourse.set(c.id, c.courseId); + // Polled every 15 s by the dashboard and the map panel. When a + // course is given, only its runners matter — either assigned to it + // directly or via their class — so ask the database for just those + // rather than every entry in the event. + const courseClassIds = courseFilter + ? classes.filter((c) => c.courseId === courseFilter.id).map((c) => c.id) + : []; const runners = await ctx.db.runner.findMany({ - where: { eventId, removed: false, classId: { not: null } }, + where: courseFilter + ? { + eventId, + removed: false, + classId: { not: null }, + OR: [ + { courseId: courseFilter.id }, + { courseId: null, classId: { in: courseClassIds } }, + ], + } + : { eventId, removed: false, classId: { not: null } }, select: { id: true, cardNo: true, classId: true, courseId: true }, }); if (runners.length === 0) return []; - const cards = await ctx.db.card.findMany({ - where: { eventId, removed: false }, - select: { cardNo: true, punchesRaw: true }, - }); + // Only the cards those runners carry; `punches_raw` is the whole + // punch list per card, so this is the bulk of the payload. + const cardNos = [ + ...new Set( + runners + .map((r) => r.cardNo) + .filter((n): n is number => typeof n === "number" && n > 0), + ), + ]; + const cards = cardNos.length + ? await ctx.db.card.findMany({ + where: { eventId, removed: false, cardNo: { in: cardNos } }, + select: { cardNo: true, punchesRaw: true }, + }) + : []; const cardByNo = new Map( cards.map((c) => [c.cardNo, c.punchesRaw]), ); @@ -1274,19 +1302,18 @@ export const courseRouter = router({ .split(";") .map((s) => parseInt(s.trim(), 10)) .filter((n) => !isNaN(n) && n > 0); - const codeSet = new Set(codes); + // Quick scan of the packed punch string for any matching code. + // Format is `code-time;code-time;...` so one regex per control + // catches it without parsing every punch into objects. + const anyCode = codes.length + ? new RegExp(`(?:^|;)(?:${codes.join("|")})-`) + : null; let passed = 0; - for (const r of expectedRunners) { - const raw = cardByNo.get(r.cardNo ?? -1); - if (!raw) continue; - // Quick scan of the packed punch string for any matching code. - // Format is `code-time;code-time;...` so a simple regex - // catches it without parsing every punch into objects. - const hit = codes.some((c) => - new RegExp(`(?:^|;)${c}-`).test(raw), - ); - if (hit) passed++; - void codeSet; + if (anyCode) { + for (const r of expectedRunners) { + const raw = cardByNo.get(r.cardNo ?? -1); + if (raw && anyCode.test(raw)) passed++; + } } out.push({ // Same public ID space as controlCoordinates so MapPanel can diff --git a/packages/api/src/routers/event.ts b/packages/api/src/routers/event.ts index 243071c..23bce6e 100644 --- a/packages/api/src/routers/event.ts +++ b/packages/api/src/routers/event.ts @@ -32,6 +32,20 @@ import type { } from "@oxygen/shared"; import { clearSheetsCache, testGoogleSheetPush } from "../sheetsBackup.js"; import { runnerStatusToValue, valueToRunnerStatus } from "../statusConvert.js"; +import { ttlMemo } from "../ttl-memo.js"; + +/** + * `pg_database_size()` stats every file under the database directory, + * which is not free on a shared-core Cloud SQL instance and was being + * asked every 3 s per open tab by the load indicator. The figure only + * moves slowly, so a minute-old answer is as good as a fresh one. + */ +const databaseSizeBytes = ttlMemo(60_000, async () => { + const rows = await prisma().$queryRawUnsafe>( + `SELECT pg_database_size(current_database())::bigint AS size`, + ); + return Number(rows[0]?.size ?? 0); +}).get; const eventKindFields = { kind: z.enum(EVENT_KINDS).default("competition"), @@ -622,39 +636,41 @@ export const eventRouter = router({ */ counterState: viewProcedure.query(async ({ ctx }) => { const eventId = ctx.event.id; - const tables = [ - { legacy: "oRunner", table: "runners" }, - { legacy: "oClass", table: "classes" }, - { legacy: "oCourse", table: "courses" }, - { legacy: "oControl", table: "controls" }, - { legacy: "oCard", table: "cards" }, - { legacy: "oTeam", table: "teams" }, - ] as const; - const out: Record = {}; - for (const { legacy, table } of tables) { - const row = await ctx.db.$queryRawUnsafe>( - `SELECT EXTRACT(EPOCH FROM MAX(updated_at)) * 1000 AS ms FROM oxygen.${table} WHERE event_id = $1`, - eventId, - ); - out[legacy] = Math.floor(Number(row[0]?.ms) || 0); - } - const punches = await ctx.db.$queryRawUnsafe>( - `SELECT EXTRACT(EPOCH FROM MAX(imported_at)) * 1000 AS ms FROM oxygen.punches WHERE event_id = $1`, + // Every open tab polls this every 5 s. One statement with scalar + // subselects instead of nine sequential round trips: on Cloud SQL + // the round trip, not the scan, is what the poll was paying for. + const ms = (column: string, table: string, extra = "") => + `(SELECT EXTRACT(EPOCH FROM MAX(${column})) * 1000 FROM oxygen.${table} WHERE event_id = $1${extra})`; + const rows = await ctx.db.$queryRawUnsafe< + Array> + >( + `SELECT + ${ms("updated_at", "runners")} AS "oRunner", + ${ms("updated_at", "classes")} AS "oClass", + ${ms("updated_at", "courses")} AS "oCourse", + ${ms("updated_at", "controls")} AS "oControl", + ${ms("updated_at", "cards")} AS "oCard", + ${ms("updated_at", "teams")} AS "oTeam", + ${ms("imported_at", "punches")} AS "oPunch", + (SELECT EXTRACT(EPOCH FROM MAX(updated_at)) * 1000 FROM oxygen.events WHERE id = $1) AS "oEvent", + ${ms("updated_at", "runners", " AND removed = false")} AS "oClub"`, eventId, ); - out.oPunch = Math.floor(Number(punches[0]?.ms) || 0); - const eventRow = await ctx.db.$queryRawUnsafe>( - `SELECT EXTRACT(EPOCH FROM MAX(updated_at)) * 1000 AS ms FROM oxygen.events WHERE id = $1`, - eventId, - ); - out.oEvent = Math.floor(Number(eventRow[0]?.ms) || 0); - const club = await ctx.db.runner.aggregate({ - _max: { updatedAt: true }, - where: { eventId, removed: false }, - }); - out.oClub = club._max.updatedAt - ? Math.floor(club._max.updatedAt.getTime()) - : 0; + const row = rows[0] ?? {}; + const out: Record = {}; + for (const key of [ + "oRunner", + "oClass", + "oCourse", + "oControl", + "oCard", + "oTeam", + "oPunch", + "oEvent", + "oClub", + ]) { + out[key] = Math.floor(Number(row[key]) || 0); + } return out; }), @@ -722,6 +738,8 @@ export const eventRouter = router({ */ dbStatus: publicProcedure.query(async () => { try { + // Polled every 3 s by every tab showing the load indicator, so the + // stats and the active-backend count come back in one round trip. const stats = await prisma().$queryRawUnsafe< Array<{ numbackends: number | bigint; @@ -737,35 +755,29 @@ export const eventRouter = router({ deadlocks: number | bigint; temp_bytes: number | bigint; stats_reset: Date | null; + active: number | bigint; }> >(` SELECT numbackends, xact_commit, xact_rollback, tup_returned, tup_fetched, tup_inserted, tup_updated, tup_deleted, blks_read, blks_hit, deadlocks, temp_bytes, - stats_reset + stats_reset, + (SELECT count(*)::bigint FROM pg_stat_activity + WHERE datname = current_database() AND state = 'active') AS active FROM pg_stat_database WHERE datname = current_database() `); const row = stats[0]; if (!row) return null; - const active = await prisma().$queryRawUnsafe< - Array<{ active: bigint }> - >(` - SELECT count(*)::bigint AS active - FROM pg_stat_activity - WHERE datname = current_database() AND state = 'active' - `); - const dbSize = await prisma().$queryRawUnsafe< - Array<{ size: bigint }> - >(`SELECT pg_database_size(current_database())::bigint AS size`); + const dbSizeBytes = await databaseSizeBytes(); const n = (v: number | bigint): number => Number(v); return { // Connection pool / activity backends: n(row.numbackends), - activeBackends: Number(active[0]?.active ?? 0), + activeBackends: n(row.active ?? 0), // Transaction throughput (xact_commit + xact_rollback ~ qps) xactCommit: n(row.xact_commit), @@ -785,7 +797,7 @@ export const eventRouter = router({ // Health deadlocks: n(row.deadlocks), tempBytes: n(row.temp_bytes), - dbSizeBytes: Number(dbSize[0]?.size ?? 0), + dbSizeBytes, // ISO timestamp the stats counters were last reset (uptime // proxy for the rate computations on the client). diff --git a/packages/api/src/ttl-memo.ts b/packages/api/src/ttl-memo.ts new file mode 100644 index 0000000..fb071a2 --- /dev/null +++ b/packages/api/src/ttl-memo.ts @@ -0,0 +1,34 @@ +/** + * Tiny time-bounded memo for values that are expensive to compute and + * harmless to serve a little stale — the size of the database for a load + * indicator, for instance. Concurrent callers during a refresh share the + * in-flight promise, and a failed refresh is not cached. + */ +export function ttlMemo( + ttlMs: number, + compute: () => Promise, + now: () => number = Date.now, +): { get(): Promise; clear(): void } { + let value: { at: number; result: T } | null = null; + let inFlight: Promise | null = null; + + return { + async get() { + const t = now(); + if (value && t - value.at < ttlMs) return value.result; + if (inFlight) return inFlight; + inFlight = compute() + .then((result) => { + value = { at: now(), result }; + return result; + }) + .finally(() => { + inFlight = null; + }); + return inFlight; + }, + clear() { + value = null; + }, + }; +} From 00ca4edbc17b20fd19729729cc587e1a4fd45218 Mon Sep 17 00:00:00 2001 From: Marcus Kempe Date: Thu, 24 Sep 2026 22:23:24 +0200 Subject: [PATCH 2/2] Stop the E2E suite from timing out on GitHub runners Four full stacks on a 4 vCPU runner ran every shard at half speed and pushed 5 s expect budgets over; the shard count now follows the core count. The runner create/edit/delete test derives its name and card from the retry index so a slow first attempt cannot turn into a duplicate-card dialog on retry, and the two long course-editor tests declare themselves slow instead of racing the 30 s test timeout. Co-authored-by: Cursor --- AGENTS.md | 6 +++--- docs/e2e-sharding.md | 16 ++++++++++++++-- e2e/course-editor.spec.ts | 7 +++++++ e2e/phase2.spec.ts | 32 ++++++++++++++++++++++---------- scripts/e2e-sharded.mjs | 21 ++++++++++++++++++--- 5 files changed, 64 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index efd4d3b..fc98ca0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,7 +23,7 @@ See `docs/architecture.md` for the full system architecture. | TypeScript build | `pnpm build` | All 3 packages must compile cleanly | | Unit tests | `pnpm test` | Vitest across shared, api, web (518+ tests) | | Integration tests | `pnpm --filter api exec vitest run --config vitest.integration.config.ts` | 69 tests, requires the test Postgres container (`pnpm test:db:up`) | -| E2E tests | `pnpm test:e2e` | 195 tests, Playwright — full runs are sharded across 4 isolated stacks (see `docs/e2e-sharding.md`); `pnpm test:e2e e2e/foo.spec.ts` runs a single spec unsharded | +| E2E tests | `pnpm test:e2e` | 195 tests, Playwright — full runs are sharded across up to 4 isolated stacks, scaled to the machine (see `docs/e2e-sharding.md`); `pnpm test:e2e e2e/foo.spec.ts` runs a single spec unsharded | | E2E tests (serial) | `pnpm test:e2e:serial` | Escape hatch: plain single-stack `playwright test` | | Test coverage | `pnpm test:coverage` | V8 coverage reports (HTML + LCOV) | | Lint | `pnpm lint` | ESLint | @@ -90,7 +90,7 @@ This is a TDD-first project. All new features and bug fixes must be developed te - **Unit tests**: `packages/*/src/__tests__/*.test.ts` — Vitest, jsdom (web) / node (api). Fast, deterministic, no database. - **Integration tests**: `packages/api/src/__tests__/integration/*.test.ts` — Vitest against the dedicated `postgres-oxygen-test` container on `:5433`. The harness (`helpers/load-env.ts`) refuses to run if `DATABASE_URL` resolves to port 5432 (dev DB) — set `INTEGRATION_DATABASE_URL` to override. Per-suite isolation comes from giving each suite its own `Event` row and relying on `ON DELETE CASCADE`. -- **E2E tests**: `e2e/*.spec.ts` — Playwright, Chromium, single worker, sequential within a stack. Full user flows through the browser. Full runs are parallelized by `scripts/e2e-sharded.mjs`, which launches 4 isolated stacks (own DB `oxygen_e2e_`, own API, own Vite server) and splits the spec files across them — serial semantics are preserved inside each shard. See `docs/e2e-sharding.md`. +- **E2E tests**: `e2e/*.spec.ts` — Playwright, Chromium, single worker, sequential within a stack. Full user flows through the browser. Full runs are parallelized by `scripts/e2e-sharded.mjs`, which launches up to 4 isolated stacks — `min(4, cores/2)` — (own DB `oxygen_e2e_`, own API, own Vite server) and splits the spec files across them — serial semantics are preserved inside each shard. See `docs/e2e-sharding.md`. ## 5. Flaky Test Policy @@ -108,7 +108,7 @@ in your final message and explain why. 1. **`pnpm build`** — Zero TypeScript errors across all three packages. 2. **`pnpm test`** — All unit tests pass. Always required. 3. **Integration tests** — Run for any DB-related changes. Always required for features: `pnpm --filter api exec vitest run --config vitest.integration.config.ts` -4. **E2E tests** — During iterative development, run only the spec files covering the affected area (`pnpm test:e2e e2e/specific-file.spec.ts` — runs unsharded against the default stack). Before declaring a task complete, run the full suite once: `pnpm test:e2e` (sharded across 4 isolated stacks, ~2-3 min; see `docs/e2e-sharding.md`). For minor fixes confined to `docs/` or other non-shipping files, E2E can be skipped — state so in your final message. +4. **E2E tests** — During iterative development, run only the spec files covering the affected area (`pnpm test:e2e e2e/specific-file.spec.ts` — runs unsharded against the default stack). Before declaring a task complete, run the full suite once: `pnpm test:e2e` (sharded across up to 4 isolated stacks, ~3-5 min; see `docs/e2e-sharding.md`). For minor fixes confined to `docs/` or other non-shipping files, E2E can be skipped — state so in your final message. 5. **Rebuild Docker** — Run `docker compose -f docker-compose.host-db.yml up --build -d` so the running stack reflects the latest code. **Required for every change that touches `packages/api/`, `packages/web/`, `packages/shared/`, `docker/`, any `Dockerfile`, `docker-compose*.yml`, or `pnpm-lock.yaml`.** You may skip it only for changes confined to `docs/`, `AGENTS.md`, `.claude/`, or test fixtures that don't ship in either image — and when you skip it, state so in your final message. Verify the output ends with both `Image oxygen-api Built` / `Image oxygen-web Built` and both containers `Started`; treat anything else as a failure. 6. **Major-version drift report** — After all other steps pass, run `pnpm outdated -r --long` and list any **direct** dependencies (production or dev) with a major-version update available. Format each as `package: current → latest — one-line note on what changes / "no notable changes documented"`. Informational only; do not bump majors as part of an unrelated PR. The user decides whether to act. diff --git a/docs/e2e-sharding.md b/docs/e2e-sharding.md index 1c3ec3d..c9f44d9 100644 --- a/docs/e2e-sharding.md +++ b/docs/e2e-sharding.md @@ -14,7 +14,7 @@ semantics the suite has always had. ``` pnpm test:e2e │ - └── scripts/e2e-sharded.mjs (default N=4, override with E2E_SHARDS) + └── scripts/e2e-sharded.mjs (N = min(4, cores/2) by default, override with E2E_SHARDS) ├── shard 1: playwright test → vite :4201 → api :4101 → db oxygen_e2e_1 (+ eventor stub :4301) ├── shard 2: playwright test → vite :4202 → api :4102 → db oxygen_e2e_2 (+ eventor stub :4302) ├── shard 3: playwright test → vite :4203 → api :4103 → db oxygen_e2e_3 (+ eventor stub :4303) @@ -30,7 +30,7 @@ reference events — nothing to provision manually. | Command | What happens | |---------|--------------| -| `pnpm test:e2e` | Full suite, sharded across 4 stacks (~2-3 min) | +| `pnpm test:e2e` | Full suite, sharded across `min(4, floor(cores/2))` stacks — 4 on a 16-core dev box (~3-5 min), 2 on a 4 vCPU GitHub runner | | `pnpm test:e2e e2e/kiosk.spec.ts` | Selective run — single plain Playwright process on its own isolated stack (ports 4100/4200, db `oxygen_e2e`), no sharding. Isolated ports mean it works while `pnpm dev` is running | | `E2E_SHARDS=2 pnpm test:e2e` | Fewer shards (lower peak CPU/RAM) | | `pnpm test:e2e:serial` | Escape hatch: plain `playwright test`, identical to the pre-sharding behavior | @@ -117,3 +117,15 @@ deliberately avoid the dev servers (3002/5173) and the Docker stack - **Port already in use** — a previous run crashed without cleanup; kill leftover `tsx`/`vite` processes bound to 41xx/42xx ports. - **Machine too loaded** — `E2E_SHARDS=2 pnpm test:e2e`. +- **Timeouts only in CI** — each shard is a whole stack (API + Vite + + eventor stub + Chromium) and wants about two cores. Four shards on a + 4 vCPU runner ran every shard at half speed (6-10 min instead of 3-5) + and pushed tests with 5 s `expect` budgets over the edge; the default + now scales with `os.availableParallelism()`. Tests that are + legitimately long (many round trips, tile rendering in the background) + should say so with `test.slow()` rather than the whole suite growing + its timeouts. Tests that create named records must also be + **retry-safe**: Playwright retries twice in CI, and a retry that finds + the previous attempt's runner / card in place fails for a different + reason than the first attempt did (`phase2.spec.ts` derives its names + and card number from `testInfo.retry`). diff --git a/e2e/course-editor.spec.ts b/e2e/course-editor.spec.ts index fa8291f..4d1bfdc 100644 --- a/e2e/course-editor.spec.ts +++ b/e2e/course-editor.spec.ts @@ -299,6 +299,10 @@ test.describe("Course editor", () => { }); test("build a course by clicking, reorder, undo/redo, persist", async ({ page }) => { + // Many round trips (place, reorder, undo, redo, reload) plus tile + // rendering in the background: legitimately long, and it runs into + // the 30 s test timeout on a loaded CI runner. + test.slow(); await selectCompetition(page); await ensureCoursesAndMap(page); await openEditor(page); @@ -763,6 +767,9 @@ test.describe("Course editor", () => { }); test("suggests a description from the base map for a placed control", async ({ page }) => { + // Waits on the base-map object lookup after placing a control; slow + // under CI load. + test.slow(); page.on("dialog", (dialog) => dialog.accept()); await selectCompetition(page); diff --git a/e2e/phase2.spec.ts b/e2e/phase2.spec.ts index 75a8e6f..5d95765 100644 --- a/e2e/phase2.spec.ts +++ b/e2e/phase2.spec.ts @@ -68,7 +68,16 @@ test.describe("Runner Management", () => { await expect(page.locator("span", { hasText: "runners" })).toBeVisible(); }); - test("should create, edit, and delete a runner", async ({ page }) => { + test("should create, edit, and delete a runner", async ({ page }, testInfo) => { + // Unique per attempt: a retry after a slow first attempt must not + // find the previous attempt's runner (the dialog would then show the + // duplicate-card warning and never close — the deterministic CI + // failure of September 2026). + const tag = `${testInfo.retry}-${Date.now().toString(36).slice(-4)}`; + const createdName = `Test Runner E2E ${tag}`; + const updatedName = `Test Runner Updated ${tag}`; + const cardNo = String(990000 + (Date.now() % 9000) + testInfo.retry); + await goToTab(page, "Runners"); await expect(page.locator("span", { hasText: "runners" })).toBeVisible({ timeout: 10000 }); @@ -80,33 +89,36 @@ test.describe("Runner Management", () => { ).toBeVisible({ timeout: 3000 }); const dialog = page.getByTestId("registration-dialog"); - await dialog.locator("input[placeholder='First Last']").fill("Test Runner E2E"); + await dialog.locator("input[placeholder='First Last']").fill(createdName); await dialog.getByTestId("reg-class").click(); await expect(dialog.getByText("Öppen 1", { exact: true })).toBeVisible({ timeout: 3000 }); await dialog.getByText("Öppen 1", { exact: true }).click(); - await dialog.locator("input[placeholder='e.g. 500123']").fill("999999"); + await dialog.locator("input[placeholder='e.g. 500123']").fill(cardNo); await dialog.getByTestId("reg-submit").click(); - await expect(page.getByText("Test Runner E2E")).toBeVisible({ timeout: 5000 }); + // Dialog closes, then the list refetches — two round trips on a + // loaded CI runner. + await expect(dialog).not.toBeVisible({ timeout: 15000 }); + await expect(page.getByText(createdName)).toBeVisible({ timeout: 15000 }); // EDIT (inline autosave) - await page.getByRole("cell", { name: "Test Runner E2E" }).click(); + await page.getByRole("cell", { name: createdName }).click(); const expandedPanel = page.locator(".bg-blue-50\\/60"); await expect(expandedPanel).toBeVisible({ timeout: 3000 }); const nameInput = expandedPanel.locator("input").first(); await nameInput.clear(); - await nameInput.fill("Test Runner Updated"); - await expect(page.getByText("Saved")).toBeVisible({ timeout: 3000 }); + await nameInput.fill(updatedName); + await expect(page.getByText("Saved")).toBeVisible({ timeout: 10000 }); - await page.getByRole("cell", { name: "Test Runner Updated", exact: true }).first().click(); + await page.getByRole("cell", { name: updatedName, exact: true }).first().click(); await expect(expandedPanel).not.toBeVisible({ timeout: 3000 }); // DELETE page.on("dialog", (dialog) => dialog.accept()); - const updatedRow = page.locator("tr").filter({ hasText: "Test Runner Updated" }).first(); + const updatedRow = page.locator("tr").filter({ hasText: updatedName }).first(); await updatedRow.getByTitle("Remove runner").click(); - await expect(page.getByText("Test Runner Updated")).not.toBeVisible({ timeout: 5000 }); + await expect(page.getByText(updatedName)).not.toBeVisible({ timeout: 10000 }); await expect(page.locator("span", { hasText: "runners" })).toBeVisible(); }); diff --git a/scripts/e2e-sharded.mjs b/scripts/e2e-sharded.mjs index 3328af8..a1bf649 100644 --- a/scripts/e2e-sharded.mjs +++ b/scripts/e2e-sharded.mjs @@ -16,7 +16,7 @@ * E2E_WEB_PORT / E2E_EVENTOR_PORT / E2E_DB_NAME to wire everything up. * * Usage: - * pnpm test:e2e # full suite, sharded (default 4) + * pnpm test:e2e # full suite, sharded (default: min(4, cores/2)) * pnpm test:e2e e2e/kiosk.spec.ts # selective run → single plain * # playwright process, no sharding * E2E_SHARDS=2 pnpm test:e2e # fewer shards (lower peak load) @@ -26,12 +26,25 @@ */ import { spawn } from "node:child_process"; import { readdirSync } from "node:fs"; +import os from "node:os"; import path from "node:path"; import readline from "node:readline"; import { fileURLToPath } from "node:url"; const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); -const SHARD_COUNT = Math.max(1, Number(process.env.E2E_SHARDS ?? 4)); +/** + * Each shard is a full stack — API, Vite, eventor stub and a Chromium — + * so it wants about two cores to itself. Default to half the available + * parallelism, capped at four: a 16-core dev box keeps four shards, a + * 4 vCPU GitHub runner gets two instead of four stacks fighting for the + * same cores (which is what turned tight `expect` budgets into failures + * after the map specs started rendering two tile layers). + */ +const DEFAULT_SHARDS = Math.max( + 1, + Math.min(4, Math.floor(os.availableParallelism() / 2)), +); +const SHARD_COUNT = Math.max(1, Number(process.env.E2E_SHARDS ?? DEFAULT_SHARDS)); const API_PORT_BASE = 4100; const WEB_PORT_BASE = 4200; const EVENTOR_PORT_BASE = 4300; @@ -115,7 +128,9 @@ if (hasFileFilter) { lightest.weight += WEIGHTS[file] ?? DEFAULT_WEIGHT; } - console.log(`Running ${specs.length} spec files across ${SHARD_COUNT} shards:`); + console.log( + `Running ${specs.length} spec files across ${SHARD_COUNT} shards (${os.availableParallelism()} cores available):`, + ); shards.forEach((s, i) => { console.log( ` shard ${i + 1} (api :${API_PORT_BASE + i + 1}, web :${WEB_PORT_BASE + i + 1}, eventor :${EVENTOR_PORT_BASE + i + 1}, db oxygen_e2e_${i + 1}, weight ${s.weight}):`,