fix(e2e): close the throwaway-customer leak at its source - #678
Conversation
## 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 <github@sebastianmendel.de>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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 <github@sebastianmendel.de>
|
|
Self-review (Copilot review unavailable — monthly quota exhausted; diff reviewed manually). Checked on head 1cd4194: the leak is closed at the source with two independent guards — an API-based Verification includes a genuine A/B repro (simulated mid-test failure leaks the row pre-fix, leaves the table at its three seed rows post-fix, with the test's own error preserved) and a planted-stale-row sweep test that also proves the age bound spares fresh rows. The two local |
What this closes
#675 diagnosed a scheduled-run failure down to a leaked throwaway customer and hardened the consumers —
createWorklogEntrynow picks the seeded bookable customer by name instead of taking the first option. The leak itself was left open:e2e/admin-inline-edit.spec.tscreates a global, project-lessE2EInline_<ts>_<rand>customer and deletes it in afinallythat drives the row's Delete button and swallows every error. A test that dies mid-UI — with a modal covering that button, or the row detached — leaves the row behind, and becausedb-e2eis a persistent volume and the customer list is name-sorted, that row sorts ahead ofFreizeitand pollutes every later spec in the shard that reads the list. This PR closes it at the source.e2e/admin/admin-ui.spec.tsturned out to be the worse of the two: its UI delete is the test's final assertion and it had nofinallyat all, so any failure before that point leaked unconditionally.Defense in depth
Per-test cleanup that survives a failed test.
deleteThrowawayCustomers()resolves the row overGET /getAllCustomersand removes it withPOST /customer/delete— the same JSON endpoint the admin UI uses. No page state is involved, so it works from afinallyafter any UI failure; an open modal can no longer block it, which is why theEscape-then-delete dance in the inline-edit spec could go away. It matches on the name prefix, so the renamed variants (-edited,-draft,_Renamed) are covered without the caller enumerating which name actually landed.A cleanup failure is reported, not swallowed. A row that could not be removed produces a
console.errorline and acleanup-failedTestInfo annotation, so CI surfaces it instead of it becoming the next shard's mystery flake. Nothing in the helper throws, so afinallycalling it can never replace the test's own error with a teardown error. The verdict comes from re-reading the customer list rather than from the delete's status code:/customer/deleteanswers 422 both for a genuine failure and for a row the parallel worker already removed, and reporting on the status alone made every concurrent sweep cry leak (observed, then fixed).A sweep so a leak cannot outlive the spec.
sweepStaleThrowawayCustomers()runs in thebeforeEachof both specs that create throwaway rows and drops ones left by an earlier run that crashed hard enough to skip its ownfinally, so a shard heals itself instead of needing a manualdb-e2ecleanup. Only rows whose embedded timestamp is older than ten minutes are touched —fullyParallelis on with two workers per CI shard, so 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.tsdeliberately stays a UI click: exercising it is that test's subject. The newfinallyis only a safety net around it.Sibling check
.admin-crud-toolbar/Add-modal entity creation exists in exactly the two specs fixed here — no other spec creates an admin entity, so the customer-shaped leak has no further instances. The worklog specs (worklog-crud,worklog-grid-editing,date-formatviacleanupWorklogEntries, andsummary-refreshviacleanupStamped) create throwaway entries rather than customers. They share the best-effort shape, but the exposure differs in kind: entries are per-user, are matched by their own unique stamp rather than positionally, and are already cleaned by a prefix sweep rather than by exact identity — so a leaked entry cannot reorder a relation list the way a leaked customer does.cleanupWorklogEntriesdoes still scrape the DOM rather than the API, which is the same root cause in a milder form; that is worth a separate change, and it is not folded in here because the specs it serves are the onese2e/AGENTS.mdmarks as CI-authoritative and therefore not verifiable on this branch.Verification
Ran against a local e2e stack (
ghcr.io/netresearch/timetracker:e2e, compose profilee2e, own project namespace,E2E_BASE_URL=http://localhost:8768), asserting on thecustomerstable directly after each run.Baseline on unmodified
origin/main(3e2a2fe):admin-inline-edit13/13 pass;date-format2/2 fail ongetByLabel(/Date format|Datumsformat/i)never resolving — a pre-existing local-vs-CI divergence, identical before and after this change.The leak reproduced and closed, same tree and containers, A/B via a temporary probe that opens the Edit modal and then throws: with the pre-fix cleanup the row survives in
db-e2e(E2EInline_1786772591271_324639left behind); with this one the table holds only the three seed rows, and the test still reportsError: simulated mid-test failurerather than a teardown error.The sweep: a planted stale row (
E2EInline_9999999999_999999-draft, plus anE2ECustomer_..._Renamed) is removed on the next run, while a row stamped with the currentDate.now()is left untouched — so the age bound that protects a concurrent worker is doing real work rather than the sweep simply deleting everything.Shard-1-like combination (
admin-inline-edit+admin/admin-ui+date-format,--workers=2) with a stale row planted first: 18 passed, the 2 pre-existingdate-formatfailures unchanged, the planted row swept, and thecustomerstable back to exactly its three seed rows — no cross-spec residue.The diff is confined to
e2e/.