From 78572987950fc7fec5a35abb86f0b8e72dee27b2 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Sat, 15 Aug 2026 07:46:14 +0200 Subject: [PATCH 1/2] fix(e2e): close the throwaway-customer leak at its source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## The leak The admin specs create a global, project-less customer to mutate so they never touch the shared seed rows, then delete it in a `finally`. That delete drove the row's Delete button and swallowed every error, so a test that died mid-UI — with a modal covering the button, or the row detached — left the row behind. `db-e2e` is persistent and the customer list is name-sorted, so a leaked `E2E*` row sorts ahead of the seeded bookable customer and derails every later spec in the shard that reads that list. #675 hardened those consumers to pick by name; the leak itself stayed open. `e2e/admin/admin-ui.spec.ts` was the worse of the two: its UI delete is the test's final assertion and there was no `finally` at all, so any earlier failure leaked unconditionally. ## Two independent guards `deleteThrowawayCustomers()` resolves the row over `/getAllCustomers` and deletes it with `POST /customer/delete`. No page state is involved, so it still works from a `finally` after any UI failure. It matches on the name prefix, which covers the renamed variants (`-edited`, `-draft`, `_Renamed`) the callers previously had to enumerate. A row it cannot remove is reported as a TestInfo annotation plus a console line rather than swallowed, and nothing in it throws, so it can never mask the test's own failure. The verdict comes from re-reading the list, not from the delete's status code: `/customer/delete` answers 422 both for a real failure and for a row the parallel worker already removed. `sweepStaleThrowawayCustomers()` runs in `beforeEach` and drops rows left by an earlier run that crashed hard enough to skip its own cleanup, so a shard heals itself instead of needing a manual DB cleanup. Only rows whose embedded timestamp is older than ten minutes are touched: under `fullyParallel` the sibling worker may have a fresh throwaway row live at that very moment, and the per-test timeout is 30s. The UI delete in `admin-ui.spec.ts` stays a UI click — it is that test's subject. The new `finally` is only a safety net around it. ## Verified Against a local e2e stack, same tree, same containers: with the pre-fix cleanup, a test failing while the Edit modal is open leaves the row in `db-e2e`; with this one the table is clean and the test still reports its own error rather than a teardown error. A planted stale row is swept while a freshly-created one is left untouched, so the age bound is doing real work. `admin-inline-edit` (13) and `admin/admin-ui` (5) pass. Claude-Session: https://claude.ai/code/session_01AcqcEjgwcQfp3vpnFa3gh6 Signed-off-by: Sebastian Mendel --- e2e/admin-inline-edit.spec.ts | 50 ++++------- e2e/admin/admin-ui.spec.ts | 71 +++++++++------ e2e/helpers/admin-fixtures.ts | 158 ++++++++++++++++++++++++++++++++++ 3 files changed, 221 insertions(+), 58 deletions(-) create mode 100644 e2e/helpers/admin-fixtures.ts diff --git a/e2e/admin-inline-edit.spec.ts b/e2e/admin-inline-edit.spec.ts index af9dc7846..78aa784c8 100644 --- a/e2e/admin-inline-edit.spec.ts +++ b/e2e/admin-inline-edit.spec.ts @@ -1,10 +1,15 @@ import { test, expect, type Page } from '@playwright/test'; import { login } from './helpers/auth'; +import { + INLINE_EDIT_PREFIX, + deleteThrowawayCustomers, + sweepStaleThrowawayCustomers, + throwawayCustomerName, +} from './helpers/admin-fixtures'; const ADD = /^(Hinzufügen|Add)$/i; const SAVE = /^(Speichern|Save)$/i; const EDIT = /^(Bearbeiten|Edit)$/i; -const DELETE = /^(Löschen|Delete)$/i; /** A throwaway Customers row to mutate, by name. */ function adminRow(page: Page, name: string) { @@ -18,9 +23,7 @@ function adminRow(page: Page, name: string) { * shared seed rows, so re-runs stay idempotent and leave no residue. */ async function createThrowawayCustomer(page: Page): Promise { - // Date.now() alone can collide when parallel workers create a row in the same - // millisecond (a unique-name DB violation); a random suffix makes it collision-safe. - const name = `E2EInline_${Date.now()}_${Math.floor(Math.random() * 1_000_000)}`; + const name = throwawayCustomerName(INLINE_EDIT_PREFIX); await page.locator('.admin-crud-toolbar button.primary-button').filter({ hasText: ADD }).click(); const form = page.locator('.modal form.stack-form'); await expect(form).toBeVisible(); @@ -32,24 +35,6 @@ async function createThrowawayCustomer(page: Page): Promise { return name; } -/** Best-effort delete of the throwaway customer (native confirm), for finally blocks. */ -async function deleteThrowawayCustomer(page: Page, name: string): Promise { - try { - const row = adminRow(page, name); - // Of the two names passed across a finally block (pre/post rename), only one - // row actually exists — skip the missing one instead of letting its delete - // button locator wait out the full timeout (a ~30s stall on every run). Bound - // the click for the same reason; the row is present, so it resolves at once. - if ((await row.count()) === 0) return; - page.once('dialog', (dialog) => dialog.accept()); - await row.getByRole('button', { name: DELETE }).click({ timeout: 2000 }); - await expect(row).toHaveCount(0); - } catch { - // Swallow — a mid-test failure may leave the page in a state where delete - // can't complete; never mask the original failure with a cleanup error. - } -} - /** * Inline (spreadsheet-style) cell editing on the SolidJS Administration tables, * built on the use:gridNav directive. @@ -58,6 +43,11 @@ test.describe('Admin inline cell editing', () => { test.beforeEach(async ({ page }) => { // i.myself is a PL (ROLE_ADMIN), so the Administration page is reachable. await login(page, 'i.myself', 'myself123'); + // Heal the shared db-e2e before touching the list: a previous run killed + // mid-test leaves an E2EInline row that sorts ahead of the seed rows and + // derails later specs (#675). Age-bounded, so a sibling worker's live row + // is never touched. + await sweepStaleThrowawayCustomers(page); await page.goto('/ui/admin'); await page.waitForSelector('table.admin-table [role="gridcell"]', { timeout: 15000 }); }); @@ -90,9 +80,8 @@ test.describe('Admin inline cell editing', () => { await expect(page.getByRole('gridcell', { name: updated, exact: true })).toBeVisible(); } finally { // The row carries `updated` on success, or `name` if the rename never landed - // (mid-test failure); whichever is present, delete it best-effort. - await deleteThrowawayCustomer(page, updated); - await deleteThrowawayCustomer(page, name); + // (mid-test failure) — deleting by name prefix covers both without guessing. + await deleteThrowawayCustomers(page, name); } }); @@ -214,13 +203,10 @@ test.describe('Admin inline cell editing', () => { await row.getByRole('button', { name: EDIT }).click(); await expect(page.locator('.modal input[type="text"]').first()).toHaveValue(draft); } finally { - // Close the modal (Escape-dismissible) so the row's Delete icon is clickable, - // then delete: the background auto-save may have renamed the row to `draft`, - // so cover both names best-effort. - await page.keyboard.press('Escape').catch(() => undefined); - await expect(page.locator('.modal')).toHaveCount(0).catch(() => undefined); - await deleteThrowawayCustomer(page, draft); - await deleteThrowawayCustomer(page, name); + // The background auto-save may have renamed the row to `draft`; the prefix + // delete covers both. No need to dismiss the modal first — the API delete + // does not go through the row's Delete icon, so an open modal cannot block it. + await deleteThrowawayCustomers(page, name); } }); diff --git a/e2e/admin/admin-ui.spec.ts b/e2e/admin/admin-ui.spec.ts index 8c4270743..d9140a59b 100644 --- a/e2e/admin/admin-ui.spec.ts +++ b/e2e/admin/admin-ui.spec.ts @@ -1,4 +1,10 @@ import { test, expect, Page } from '@playwright/test'; +import { + ADMIN_UI_PREFIX, + deleteThrowawayCustomers, + sweepStaleThrowawayCustomers, + throwawayCustomerName, +} from '../helpers/admin-fixtures'; import { login } from '../helpers/auth'; import { waitForGrid } from '../helpers/grid'; import { goToAdminPage } from '../helpers/navigation'; @@ -45,6 +51,9 @@ test.describe('Administration UI', () => { test.beforeEach(async ({ page }) => { // i.myself is type PL → ROLE_ADMIN. await login(page, 'i.myself', 'myself123'); + // Heal the shared db-e2e before touching the list — see the same call in + // admin-inline-edit.spec.ts and helpers/admin-fixtures.ts. + await sweepStaleThrowawayCustomers(page); await waitForGrid(page); await goToAdminPage(page); }); @@ -64,34 +73,44 @@ test.describe('Administration UI', () => { }); test('creates, edits and deletes a customer', async ({ page }) => { - const name = `E2ECustomer_${Date.now()}`; + const name = throwawayCustomerName(ADMIN_UI_PREFIX); - // Create. A customer must be global or have teams (server-enforced), so - // mark it global to keep the fixture self-contained. - await page.locator('.admin-crud-toolbar button.primary-button').filter({ hasText: ADD }).click(); - const form = page.locator('.modal form.stack-form'); - await expect(form).toBeVisible(); - await form.locator('.field input[type="text"]').first().fill(name); - await form.getByRole('checkbox', { name: 'Global', exact: true }).check(); - await form.locator('button[type="submit"]').filter({ hasText: SAVE }).click(); - await expect(page.locator('.modal')).toHaveCount(0); - await expect(row(page, name)).toHaveCount(1); + // Deleting through the UI is this test's SUBJECT, so it stays a UI click. The + // finally is the safety net: fail anywhere before that click — or on it — and + // the row would otherwise outlive the test and poison the name-sorted customer + // list for every later spec in the shard (#675). + try { + // Create. A customer must be global or have teams (server-enforced), so + // mark it global to keep the fixture self-contained. + await page.locator('.admin-crud-toolbar button.primary-button').filter({ hasText: ADD }).click(); + const form = page.locator('.modal form.stack-form'); + await expect(form).toBeVisible(); + await form.locator('.field input[type="text"]').first().fill(name); + await form.getByRole('checkbox', { name: 'Global', exact: true }).check(); + await form.locator('button[type="submit"]').filter({ hasText: SAVE }).click(); + await expect(page.locator('.modal')).toHaveCount(0); + await expect(row(page, name)).toHaveCount(1); - // Edit (rename). The action buttons are icon-only, so match the accessible - // name (aria-label), not visible text. - const renamed = `${name}_Renamed`; - await row(page, name).getByRole('button', { name: EDIT }).click(); - const editForm = page.locator('.modal form.stack-form'); - await expect(editForm).toBeVisible(); - const nameInput = editForm.locator('.field input[type="text"]').first(); - await nameInput.fill(renamed); - await editForm.locator('button[type="submit"]').filter({ hasText: SAVE }).click(); - await expect(page.locator('.modal')).toHaveCount(0); - await expect(row(page, renamed)).toHaveCount(1); + // Edit (rename). The action buttons are icon-only, so match the accessible + // name (aria-label), not visible text. + const renamed = `${name}_Renamed`; + await row(page, name).getByRole('button', { name: EDIT }).click(); + const editForm = page.locator('.modal form.stack-form'); + await expect(editForm).toBeVisible(); + const nameInput = editForm.locator('.field input[type="text"]').first(); + await nameInput.fill(renamed); + await editForm.locator('button[type="submit"]').filter({ hasText: SAVE }).click(); + await expect(page.locator('.modal')).toHaveCount(0); + await expect(row(page, renamed)).toHaveCount(1); - // Delete (native confirm) - page.once('dialog', (dialog) => dialog.accept()); - await row(page, renamed).getByRole('button', { name: DELETE }).click(); - await expect(row(page, renamed)).toHaveCount(0); + // Delete (native confirm) + page.once('dialog', (dialog) => dialog.accept()); + await row(page, renamed).getByRole('button', { name: DELETE }).click(); + await expect(row(page, renamed)).toHaveCount(0); + } finally { + // A no-op on the happy path (the UI delete already removed the row); the + // prefix match covers `name` and `name_Renamed` alike. + await deleteThrowawayCustomers(page, name); + } }); }); diff --git a/e2e/helpers/admin-fixtures.ts b/e2e/helpers/admin-fixtures.ts new file mode 100644 index 000000000..8971164a0 --- /dev/null +++ b/e2e/helpers/admin-fixtures.ts @@ -0,0 +1,158 @@ +import { test, type Page } from '@playwright/test'; + +/** + * Throwaway-customer lifecycle for the Administration specs. + * + * Those specs create a global, project-less customer to mutate so they never touch + * the shared seed rows. Such a row is dangerous residue: the customer list is + * name-sorted and shared run-wide, so an `E2E*` row that outlives its test sorts + * ahead of the seeded bookable customer and derails every later spec that reads the + * list (see SEEDED_BOOKABLE_CUSTOMER in helpers/worklog.ts). #675 hardened those + * consumers to pick by name; this module closes the leak at the source. + * + * Two independent guards, so no single failure can leak a row past the spec: + * - deleteThrowawayCustomers() deletes over the HTTP API, not the UI — it works + * from a `finally` even when the test died mid-interaction with a modal open, + * and it reports a delete it could not perform instead of swallowing it. + * - sweepStaleThrowawayCustomers() drops rows left behind by an earlier run that + * crashed hard enough to skip its own cleanup. + */ + +/** Name prefix per spec, so a sweep can tell throwaway rows from seed rows. */ +export const INLINE_EDIT_PREFIX = 'E2EInline'; +export const ADMIN_UI_PREFIX = 'E2ECustomer'; + +const THROWAWAY_PREFIXES = [INLINE_EDIT_PREFIX, ADMIN_UI_PREFIX]; + +/** + * `__`, plus whatever a spec appends while mutating it + * (`-edited`, `-draft`, `_Renamed`). The captured epoch ms is what makes the sweep + * safe under `fullyParallel` — see sweepStaleThrowawayCustomers. The digit run is + * bounded (no unbounded quantifier) and the pattern is anchored. + */ +const THROWAWAY_NAME = new RegExp(`^(?:${THROWAWAY_PREFIXES.join('|')})_(\\d{10,16})_`); + +/** + * A row younger than this may belong to a test running right now in the other + * worker (`fullyParallel: true`, 2 workers per CI shard), so the sweep leaves it + * alone. Playwright's per-test timeout is 30s, so a row this old is provably + * residue from an earlier run and can never be in use. + */ +const STALE_AFTER_MS = 10 * 60 * 1000; + +interface CustomerRow { + customer: { id: number; name: string }; +} + +/** + * Report a cleanup that did not happen. Deliberately non-fatal: this runs from a + * `finally`, where throwing would replace the test's own error with a teardown one. + * The annotation surfaces the leak in the HTML report, the console line in the CI + * log — a silent leak is what let a row survive the spec in the first place. + */ +function reportLeak(message: string): void { + console.error(`[e2e cleanup] ${message}`); + try { + test.info().annotations.push({ type: 'cleanup-failed', description: message }); + } catch { + // Called outside a test (no TestInfo) — the console line above still stands. + } +} + +/** Every customer the admin list shows, or null when the read itself failed. */ +async function readCustomers(page: Page): Promise { + try { + const response = await page.request.get('/getAllCustomers'); + if (!response.ok()) { + reportLeak(`GET /getAllCustomers answered ${response.status()} — cannot verify throwaway customers were removed`); + return null; + } + return (await response.json()) as CustomerRow[]; + } catch (error) { + reportLeak(`GET /getAllCustomers failed (${String(error)}) — cannot verify throwaway customers were removed`); + return null; + } +} + +/** + * Delete every given customer, then report the ones that are still there. + * + * The verdict comes from re-reading the list, not from the delete's status code: + * `/customer/delete` answers 422 both when the row could not go (a real leak) and + * when it was already gone — which is the NORMAL outcome when the sibling worker + * swept the same stale row a moment earlier. Reporting on the status alone made + * every parallel sweep cry leak; the list says what actually happened, and says it + * in any locale. + */ +async function deleteRows(page: Page, rows: CustomerRow['customer'][]): Promise { + if (rows.length === 0) { + return; + } + for (const row of rows) { + await page.request.post('/customer/delete', { data: { id: row.id } }).catch(() => undefined); + } + const remaining = await readCustomers(page); + if (remaining === null) { + return; + } + for (const row of rows) { + if (remaining.some((candidate) => candidate.customer.id === row.id)) { + reportLeak(`LEAKED customer "${row.name}" (id ${row.id}) — /customer/delete did not remove it; it will pollute the name-sorted customer list for the rest of this shard`); + } + } +} + +/** A throwaway name for `prefix`, collision-safe across parallel workers. + * Date.now() alone can collide when two workers create a row in the same + * millisecond (a unique-name DB violation); the random suffix rules that out. */ +export function throwawayCustomerName(prefix: string): string { + return `${prefix}_${Date.now()}_${Math.floor(Math.random() * 1_000_000)}`; +} + +/** + * Delete the throwaway customer named `base` and every variant a spec renamed it + * to (`base-edited`, `base-draft`, `base_Renamed`, …) — matching on the prefix + * means the caller never has to enumerate which name actually landed. + * + * Goes over the HTTP API rather than the row's Delete button on purpose: a test + * that failed mid-UI can leave a modal open or the row detached, which is exactly + * when the UI delete silently gave up and the row leaked. Nothing here throws, so + * a `finally` calling it cannot mask the test's own failure; a delete that does not + * happen is reported instead. + */ +export async function deleteThrowawayCustomers(page: Page, base: string): Promise { + const customers = await readCustomers(page); + if (customers === null) { + return; + } + await deleteRows( + page, + customers.map((row) => row.customer).filter((customer) => customer.name.startsWith(base)), + ); +} + +/** + * Drop throwaway customers left behind by an EARLIER run — a run killed hard + * enough to skip its own `finally` still poisons the shared, persistent db-e2e for + * every run after it. Call it from a beforeEach of the specs that create them, so + * the shard heals itself instead of needing a manual DB cleanup. + * + * Only rows older than STALE_AFTER_MS are touched: under `fullyParallel` a sibling + * test in the other worker may have a fresh throwaway row live at this very moment, + * and deleting it would break that test. + */ +export async function sweepStaleThrowawayCustomers(page: Page): Promise { + const customers = await readCustomers(page); + if (customers === null) { + return; + } + const now = Date.now(); + const stale = customers.map((row) => row.customer).filter((customer) => { + const stamp = THROWAWAY_NAME.exec(customer.name); + return stamp !== null && now - Number(stamp[1]) > STALE_AFTER_MS; + }); + if (stale.length > 0) { + console.error(`[e2e cleanup] sweeping ${stale.length} stale throwaway customer(s) from an earlier run: ${stale.map((customer) => customer.name).join(', ')}`); + await deleteRows(page, stale); + } +} From 33d0d6f01004861ab91c29177c0daceb3a1cd7d3 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Sat, 15 Aug 2026 07:51:21 +0200 Subject: [PATCH 2/2] fix(e2e): use crypto.randomInt for the throwaway-name suffix SonarCloud flags Math.random as a security hotspot (typescript:S2245) in the new helper, blocking the PR quality gate. Nothing here is security-relevant - the suffix only de-duplicates names across parallel workers - but crypto.randomInt costs nothing and clears the finding at the source instead of a Sonar UI review. Claude-Session: https://claude.ai/code/session_01AcqcEjgwcQfp3vpnFa3gh6 Signed-off-by: Sebastian Mendel --- e2e/helpers/admin-fixtures.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/e2e/helpers/admin-fixtures.ts b/e2e/helpers/admin-fixtures.ts index 8971164a0..fa4ae3cae 100644 --- a/e2e/helpers/admin-fixtures.ts +++ b/e2e/helpers/admin-fixtures.ts @@ -1,3 +1,5 @@ +import { randomInt } from 'node:crypto'; + import { test, type Page } from '@playwright/test'; /** @@ -104,9 +106,11 @@ async function deleteRows(page: Page, rows: CustomerRow['customer'][]): Promise< /** A throwaway name for `prefix`, collision-safe across parallel workers. * Date.now() alone can collide when two workers create a row in the same - * millisecond (a unique-name DB violation); the random suffix rules that out. */ + * millisecond (a unique-name DB violation); the random suffix rules that out. + * crypto.randomInt only because Sonar flags Math.random as a hotspot — + * nothing here is security-relevant, the suffix just de-duplicates names. */ export function throwawayCustomerName(prefix: string): string { - return `${prefix}_${Date.now()}_${Math.floor(Math.random() * 1_000_000)}`; + return `${prefix}_${Date.now()}_${randomInt(1_000_000)}`; } /**