From 74dbb1a2e44c9303c8d62a293be0ec2b474f24da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 06:35:40 +0200 Subject: [PATCH] test(e2e): give the notification journey a server and a scope of its own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The journey's isolation went one way only. `test.describe.configure` orders the tests inside the file; the suite is fullyParallel with two workers, so everything else runs beside it — and its own trigger, the admin reminder sweep, walked the whole instance and dispatched for every account that had an overdue dose. With the mail env pointed at the local responder, that would have started sending real messages for accounts this file has never heard of, and the exact-equality assertion on the accepted recipients would have been reading them. The sweep now names the account it then counts, and the comments that defended the opposite direction say what the exposure actually is. Its own server, too, and for two reasons. The `SMTP_*` vars decide whether the Email card renders at all, so on the shared server they put a card the axe scan and the card-anatomy sweep had never measured onto `/settings/integrations` for every spec that opens the route. And unset, `HEALTHLOG_PROCESS_TYPE` starts the pg-boss reminder scheduler inside the process serving requests, whose every-fifteen-minutes tick either mints the missed-dose row that makes the sweep skip its own dispatch or dispatches beside it. A third server carries both: mail configured, `web`, and only this spec talks to it. The shared server keeps `all` on measured grounds rather than taste. Worker status is per-process and in-memory, so a `web` process reports the worker stopped, `/admin` renders that row destructive-red, and axe fails it at 3.97:1 against the muted tile — a real contrast defect any operator running the documented web/worker split already sees, and not this branch's to fix. That limit is written where the server is defined. Also here: the stub draws its port per run instead of pinning one, and says so by name when the port is taken; teardown ends its sockets and survives a setup that never got as far as starting it; the ledger reset stops deleting rows whose subject is this account but whose recipient is somebody else, and clears intake events explicitly rather than through a cascade; the attempt poll refuses a window larger than the twenty rows the diagnostic returns; and the header names the precondition the file cannot enforce and the internal reasons it pins on purpose. Verified against a production build and a throwaway Postgres: the journey three times over at one worker, then the a11y and settings geometry specs at two. Both assertions broken once to see them fail. --- e2e/notification-dispatch-journey.spec.ts | 125 +++++++++++++++++++--- e2e/setup/global-setup.ts | 12 ++- e2e/setup/notification-fixture.ts | 16 ++- e2e/setup/notification-server.ts | 43 ++++++++ e2e/setup/smtp-stub.ts | 61 ++++++++++- playwright.config.ts | 66 +++++++++--- 6 files changed, 283 insertions(+), 40 deletions(-) create mode 100644 e2e/setup/notification-server.ts diff --git a/e2e/notification-dispatch-journey.spec.ts b/e2e/notification-dispatch-journey.spec.ts index 4e6db6c54..ea8b68a90 100644 --- a/e2e/notification-dispatch-journey.spec.ts +++ b/e2e/notification-dispatch-journey.spec.ts @@ -35,11 +35,37 @@ * a form-control id — or the account's own API responses, never rendered copy: * the labels are i18n-driven and this account renders them in English only by * cookie. + * + * What isolates this file, and what does not. `test.describe.configure({ mode: + * "serial" })` orders the tests INSIDE it; the suite runs `fullyParallel` with + * two workers, so every other spec file is live against the same database + * throughout. Three things carry the isolation instead: the account is this + * file's own (`E2E_NOTIFY`), the server is this file's own (mail configured, no + * reminder scheduler — `e2e/setup/notification-server.ts`), and the reminder + * sweep is told which account to walk, so the one trigger here that could + * dispatch for the whole instance no longer does. + * + * What is left standing is a precondition rather than a mechanism: nothing else + * delivers email in this run, because no other e2e account configures an EMAIL + * channel and no other server is pointed at the responder. `smtp.accepted()` is + * asserted by exact equality on the strength of that, and a spec that seeds an + * email channel for another account has to say so here. + * + * `reason: "apns_not_configured"` and `reason: "client_managed"` are internal + * sender / dispatcher constants rather than published contract, and they are + * pinned on purpose: the ledger reason is the only place the DECISION is + * legible, and the whole claim of the client-managed opt-in is that the reason + * changes while everything else does not. A rename that reaches here should + * read as a decision to make, not as a mystery break. */ import type { Page } from "@playwright/test"; -import { NOTIFY_STORAGE_STATE_PATH } from "./setup/global-setup"; -import { resetNotificationFixture } from "./setup/notification-fixture"; +import { E2E_NOTIFY, NOTIFY_STORAGE_STATE_PATH } from "./setup/global-setup"; +import { NOTIFICATION_BASE_URL } from "./setup/notification-server"; +import { + notificationAccountId, + resetNotificationFixture, +} from "./setup/notification-fixture"; import { startSmtpStub, type SmtpStub } from "./setup/smtp-stub"; import { expect, test } from "./setup/test"; @@ -144,12 +170,24 @@ async function readDiagnostic(page: Page): Promise { * `expected` is asserted, not merely awaited — an EXTRA attempt is the failure * this journey exists to catch (a channel that was switched off delivering * anyway), and a poll that stops at "at least" would never see it. + * + * The diagnostic returns the trailing TWENTY rows and no more + * (`src/app/api/admin/notifications/diagnostic/route.ts`). The fixture resets + * the ledger to zero before every test and the largest cascade here is two, so + * the ceiling is far away — but a poll that ran past it could never converge + * and would report "the dispatch wrote N delivery attempt(s)" while the real + * answer was "the diagnostic stopped counting". The ceiling is asserted up + * front so that failure names itself. */ async function attemptsSince( page: Page, before: number, expected: number, ): Promise { + expect( + before + expected, + "the diagnostic returns at most 20 attempts, so a larger window could never converge", + ).toBeLessThanOrEqual(20); await expect .poll(async () => (await readDiagnostic(page)).recentPushAttempts.length, { message: `the dispatch wrote ${expected} delivery attempt(s)`, @@ -345,25 +383,43 @@ async function createOverdueMedication(page: Page): Promise { } interface SweepResult { + scoped: boolean; medications: Array<{ name: string; + user: string; schedules: Array<{ notificationSent: boolean }>; }>; } /** - * Run the reminder sweep the admin notifications panel drives. This is the - * product's own trigger for the medication-reminder dispatch, and it goes - * through `dispatchNotification` — the cascade, the per-channel preference - * read and the client-managed gate — rather than calling a sender directly. + * Run the reminder sweep the admin notifications panel drives, NAMING this + * account. This is the product's own trigger for the medication-reminder + * dispatch, and it goes through `dispatchNotification` — the cascade, the + * per-channel preference read and the client-managed gate — rather than + * calling a sender directly. + * + * The selector is not a convenience. Without it the sweep walks every active + * medication on the instance and dispatches for each overdue slot, which makes + * this file a writer into every other account's delivery ledger — the exact + * inverse of the interference it is trying to avoid, and one that would start + * sending real mail the moment another account had a channel and an overdue + * dose. Scoped, the run reaches one account: the one it then counts. */ -async function runReminderSweep(page: Page): Promise { +async function runReminderSweep(page: Page, userId: string): Promise { const res = await fromPage( page, "/api/admin/notifications/reminder-check", - { method: "POST" }, + { method: "POST", body: { userId } }, ); expect(res.status, "the sweep ran").toBe(200); + expect( + res.data?.scoped, + "the sweep walked one account, not the instance", + ).toBe(true); + expect( + [...new Set(res.data?.medications.map((m) => m.user))], + "every medication the sweep touched belongs to this account", + ).toEqual([E2E_NOTIFY.username]); const mine = res.data?.medications.find((m) => m.name === MEDICATION_NAME); expect(mine, "the sweep saw this account's overdue medication").toBeTruthy(); expect( @@ -381,7 +437,16 @@ function attemptFor( } test.describe("notification preferences drive the dispatch decision", () => { - test.use({ storageState: NOTIFY_STORAGE_STATE_PATH }); + /** + * Its own server as well as its own account. That process is the one carrying + * the mail configuration, and the one with no reminder scheduler inside it — + * `e2e/setup/notification-server.ts` says why each matters and what the + * shared server keeping the scheduler still leaves standing. + */ + test.use({ + storageState: NOTIFY_STORAGE_STATE_PATH, + baseURL: NOTIFICATION_BASE_URL, + }); /** * Serial, because every test configures the one account's channels and then @@ -391,20 +456,48 @@ test.describe("notification preferences drive the dispatch decision", () => { test.describe.configure({ mode: "serial" }); /** - * The sweep walks every active medication on the instance and the ledger - * write that follows is fire-and-forget, so the budget covers a loaded - * runner rather than the 30 s default. + * The ledger write that follows a dispatch is fire-and-forget and the tests + * poll for it, so the budget covers a loaded runner rather than the 30 s + * default. */ test.setTimeout(120_000); let smtp: SmtpStub; + let accountId: string; - test.beforeAll(async () => { + test.beforeAll(async ({ playwright }) => { smtp = await startSmtpStub(); + accountId = await notificationAccountId(); + + // The one precondition this file cannot arrange for itself: the server it + // talks to has to have been started with `SMTP_*`. `playwright.config.ts` + // hands them to the server it starts, but `reuseExistingServer` (the local + // default) and `E2E_SKIP_WEB_SERVER=1` both attach to a server started + // without them — and the symptom is a missing `#email-recipient` field + // thirty lines into the first test, which points nowhere near the cause. + const api = await playwright.request.newContext({ + baseURL: NOTIFICATION_BASE_URL, + storageState: NOTIFY_STORAGE_STATE_PATH, + }); + try { + const res = await api.get("/api/settings/email"); + const body = (await res.json()) as { data: { smtpConfigured: boolean } }; + expect( + body.data?.smtpConfigured, + `the server on ${NOTIFICATION_BASE_URL} was started without SMTP_* — a reused ` + + `dev server or E2E_SKIP_WEB_SERVER=1 does not carry them. Start it with ` + + `SMTP_HOST=127.0.0.1 SMTP_PORT=${smtp.port} SMTP_FROM=.`, + ).toBe(true); + } finally { + await api.dispose(); + } }); test.afterAll(async () => { - await smtp.close(); + // Optional-chained: a `beforeAll` that threw before the stub resolved + // leaves this undefined, and a teardown TypeError buries the real cause + // under its own. + await smtp?.close(); }); test.beforeEach(async () => { @@ -467,7 +560,7 @@ test.describe("notification preferences drive the dispatch decision", () => { await createOverdueMedication(page); const before = await attemptCount(page); - await runReminderSweep(page); + await runReminderSweep(page, accountId); // Two arms, and exactly two: the paired phone and the enabled email // channel. A third row would mean the switched-off relay was dialled. @@ -520,7 +613,7 @@ test.describe("notification preferences drive the dispatch decision", () => { expect(resolved.data?.medication.clientManaged).toBe(true); const before = await attemptCount(page); - await runReminderSweep(page); + await runReminderSweep(page, accountId); const attempts = await attemptsSince(page, before, 2); diff --git a/e2e/setup/global-setup.ts b/e2e/setup/global-setup.ts index bd0f24e99..2cc432cc0 100644 --- a/e2e/setup/global-setup.ts +++ b/e2e/setup/global-setup.ts @@ -98,9 +98,15 @@ export const E2E_GUARDIAN = { * * It has one, rather than borrowing the shared fixture, because the journey's * verdicts are counts over ONE account's `push_attempts` ledger — "one email - * attempt, no ntfy attempt, the APNs arm skipped and for which reason". Any - * other spec that happened to trigger a delivery for the shared account would - * land inside that window and flip a count that is not about it. + * attempt, no ntfy attempt, the APNs arm skipped and for which reason". + * + * The direction that matters is the one that is easy to state backwards. The + * exposure is not mainly that another spec writes into this account's window; + * it is that this journey's own trigger — the admin reminder sweep — dispatches + * for whatever accounts it is pointed at, and pointed at the instance it + * reaches all of them. A dedicated account gives the sweep something to name, + * so the run stays a writer for exactly one ledger and a reader of exactly the + * same one. * * ADMIN because the two surfaces the journey reads the dispatch decision * through — the reminder trigger and the notification diagnostic — are diff --git a/e2e/setup/notification-fixture.ts b/e2e/setup/notification-fixture.ts index 23363b05a..cd48bae00 100644 --- a/e2e/setup/notification-fixture.ts +++ b/e2e/setup/notification-fixture.ts @@ -77,9 +77,23 @@ export async function resetNotificationFixture(): Promise { userId, ]); await pool.query("DELETE FROM devices WHERE user_id = $1", [userId]); + // `user_id` and `recipient_user_id`, and deliberately NOT + // `record_user_id`: that column is the health-record SUBJECT, so a + // delivery ABOUT this account sent TO a guardian carries it while the + // recipient is somebody else. Deleting on it would take a row out of + // another account's ledger. The diagnostic this journey reads filters on + // the recipient, so the two agree. await pool.query( `DELETE FROM push_attempts - WHERE user_id = $1 OR recipient_user_id = $1 OR record_user_id = $1`, + WHERE user_id = $1 OR recipient_user_id = $1`, + [userId], + ); + // The medication delete cascades to its intake events, but a stale event + // is what makes the sweep decide a dose was already taken and skip its + // dispatch, so the statement is written out rather than inherited from a + // foreign key a future fixture might stop relying on. + await pool.query( + "DELETE FROM medication_intake_events WHERE user_id = $1", [userId], ); await pool.query("DELETE FROM medications WHERE user_id = $1", [userId]); diff --git a/e2e/setup/notification-server.ts b/e2e/setup/notification-server.ts new file mode 100644 index 000000000..053d30205 --- /dev/null +++ b/e2e/setup/notification-server.ts @@ -0,0 +1,43 @@ +/** + * The third E2E server: the one configured for mail, and the one with no + * scheduler. + * + * Two things the notification-dispatch journey needs are wrong for every other + * spec, so it gets its own process rather than bending the shared one. + * + * **Mail.** `SMTP_HOST` / `SMTP_PORT` / `SMTP_FROM` are operator config, and + * `isEmailConfigured()` is what decides whether the Email card renders at all + * (`src/components/settings/email-card.tsx`). Putting them on the shared server + * would put that card on `/settings/integrations` for every spec that opens the + * route — the axe scan and the card-anatomy geometry sweep included — and make + * this branch responsible for a surface it does not touch. Here, the shipped + * "no SMTP, no card" state stays what every other spec observes. + * + * **No scheduler.** Unset, `HEALTHLOG_PROCESS_TYPE` reads "all" and + * `src/instrumentation.ts` starts the pg-boss reminder worker inside the + * process serving requests, which schedules `medication-reminder-check` every + * fifteen minutes. The journey keeps a medication overdue for the length of a + * test, and a tick inside that window either mints the missed-dose row that + * makes the sweep skip its own dispatch or dispatches beside it and adds a + * ledger row the count was not expecting. `web` is the shipped, documented + * value for HTTP-without-scheduler (docs/self-hosting/scaling.md); the pg-boss + * PRODUCER still starts, so a route that enqueues work keeps working, and the + * journey's own trigger is a route rather than a job. + * + * The shared server keeps "all" deliberately, and the reason is measured + * rather than assumed: `getWorkerStatus()` is per-process in-memory, so a `web` + * process reports the worker stopped, `/admin` renders that row + * `text-destructive`, and axe fails it at 3.97:1 against the muted tile. That + * is a real contrast defect, visible to any operator running the documented + * web/worker split, and it is not this branch's to fix. What it means here is + * that the scheduler still exists on the suite's shared process: it is off in + * the process the journey drives, not off in the run. + * + * With `E2E_SKIP_WEB_SERVER=1` no server is started for you and all three have + * to be running — the same contract the other two already carry. + */ +export const NOTIFICATION_PORT = 3200; + +export const NOTIFICATION_BASE_URL = + process.env.E2E_NOTIFICATION_BASE_URL ?? + `http://localhost:${NOTIFICATION_PORT}`; diff --git a/e2e/setup/smtp-stub.ts b/e2e/setup/smtp-stub.ts index d640cc492..d63ea8e5a 100644 --- a/e2e/setup/smtp-stub.ts +++ b/e2e/setup/smtp-stub.ts @@ -27,9 +27,31 @@ import { createServer, type Server, type Socket } from "node:net"; /** * Port the stub listens on, and the port `playwright.config.ts` hands the app - * server as `SMTP_PORT`. Named here so the config and the spec cannot drift. + * server as `SMTP_PORT`. + * + * It cannot be an OS-assigned port-0 binding: the config has to put `SMTP_PORT` + * into the web server's environment before any worker process exists, and the + * stub itself does not start until the spec's `beforeAll` — so by the time a + * kernel-assigned port would be known, the server it has to reach is already + * running. The port is therefore CHOSEN once per run, at config load, from the + * IANA dynamic range, and published through `SMTP_STUB_PORT` in the + * environment so the config, the server and the spec all read the one value. + * Two suites on one machine draw different numbers instead of colliding on a + * constant, and a collision that does happen fails by name in `listen` below + * rather than as a missing form field thirty lines into a test. + * + * An operator pinning it by hand (`SMTP_STUB_PORT=3925 pnpm e2e`) wins, which + * is what makes an externally started server — `E2E_SKIP_WEB_SERVER=1` — able + * to carry the same value. */ -export const SMTP_STUB_PORT = 3925; +export function resolveSmtpStubPort(): number { + const pinned = Number(process.env.SMTP_STUB_PORT); + if (Number.isInteger(pinned) && pinned > 0 && pinned < 65_536) return pinned; + // 49152–65535, the dynamic/private range no registered service claims. + const chosen = 49_152 + Math.floor(Math.random() * (65_536 - 49_152)); + process.env.SMTP_STUB_PORT = String(chosen); + return chosen; +} export const SMTP_STUB_HOST = "127.0.0.1"; @@ -37,6 +59,8 @@ export const SMTP_STUB_HOST = "127.0.0.1"; export const SMTP_STUB_FROM = "healthlog-e2e@healthlog.test"; export interface SmtpStub { + /** The port this responder bound, which is the one the app was handed. */ + readonly port: number; /** Envelope recipients of every message the stub accepted, oldest first. */ accepted(): readonly string[]; /** Forget everything accepted so far. */ @@ -49,8 +73,16 @@ const CRLF = "\r\n"; /** Start the responder and resolve once it is accepting connections. */ export async function startSmtpStub(): Promise { const accepted: string[] = []; + const port = resolveSmtpStubPort(); + // Every established socket, so `close()` can end them. `server.close()` + // resolves only once the last connection is gone and never ends one itself, + // so a socket abandoned mid-DATA by a timed-out test would otherwise hold + // teardown open until Playwright's own timeout fires over it. + const open = new Set(); const server: Server = createServer((socket: Socket) => { + open.add(socket); + socket.on("close", () => open.delete(socket)); let buffer = ""; let inData = false; let envelope: string[] = []; @@ -113,20 +145,39 @@ export async function startSmtpStub(): Promise { }); await new Promise((resolve, reject) => { - server.once("error", reject); - server.listen(SMTP_STUB_PORT, SMTP_STUB_HOST, () => { - server.removeListener("error", reject); + server.once("error", (err: NodeJS.ErrnoException) => { + // The one failure worth naming. A crashed prior run, a second suite, or + // a hand-pinned port already in use all arrive here, and the bare + // `EADDRINUSE` that used to propagate took the whole `beforeAll` down + // with a message that named neither the port nor the stub. + reject( + err.code === "EADDRINUSE" + ? new Error( + `[smtp-stub] port ${port} on ${SMTP_STUB_HOST} is already in use — ` + + "another run's responder is still listening, or SMTP_STUB_PORT " + + "was pinned to a port something else holds. Free it, or unset " + + "SMTP_STUB_PORT to draw a fresh one.", + { cause: err }, + ) + : err, + ); + }); + server.listen(port, SMTP_STUB_HOST, () => { + server.removeAllListeners("error"); resolve(); }); }); return { + port, accepted: () => accepted, reset: () => { accepted.length = 0; }, close: () => new Promise((resolve) => { + for (const socket of open) socket.destroy(); + open.clear(); server.close(() => resolve()); }), }; diff --git a/playwright.config.ts b/playwright.config.ts index b2c689661..6f9ecee3e 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -1,13 +1,17 @@ import { defineConfig, devices } from "@playwright/test"; +import { + NOTIFICATION_BASE_URL, + NOTIFICATION_PORT, +} from "./e2e/setup/notification-server"; import { SSR_PREFETCH_BASE_URL, SSR_PREFETCH_PORT, } from "./e2e/setup/ssr-prefetch-server"; import { + resolveSmtpStubPort, SMTP_STUB_FROM, SMTP_STUB_HOST, - SMTP_STUB_PORT, } from "./e2e/setup/smtp-stub"; /** @@ -24,6 +28,14 @@ import { * To run locally: `pnpm dlx playwright install --with-deps chromium` * once, then `pnpm e2e`. */ +/** + * Drawn once here, before any server or worker exists, and published through + * `process.env.SMTP_STUB_PORT` — which the web server inherits and which every + * worker process re-reads when it loads this config, so all three agree on one + * number without a constant to collide on. + */ +const smtpStubPort = resolveSmtpStubPort(); + export default defineConfig({ testDir: "./e2e", testIgnore: ["setup/**"], @@ -176,11 +188,12 @@ export default defineConfig({ // through stable data attributes, not a mobile layout, so it runs in // one project. "apple-health-import.spec.ts", - // The notification-dispatch journey binds a local SMTP responder on a - // fixed port and reads GLOBAL verdicts off one account's delivery - // ledger — "one email attempt, no ntfy attempt, the APNs arm skipped - // for this reason". A second project would fight it for the port and - // add attempts to the window it counts. It proves a dispatch decision + // The notification-dispatch journey binds a local SMTP responder and + // reads verdicts off one account's delivery ledger — "one email + // attempt, no ntfy attempt, the APNs arm skipped for this reason". A + // second project would run the file in a second worker process, whose + // stub would find the port already bound, and would add attempts to + // the window the first one counts. It proves a dispatch decision // through stable attributes and the account's own API, not a mobile // layout, so it runs in one project. "notification-dispatch-journey.spec.ts", @@ -223,15 +236,6 @@ export default defineConfig({ // Disable the prefetch for the e2e server — the suite keeps the // deterministic client-fetch path. DASHBOARD_SSR_PREFETCH: "false", - // The one delivery channel a notification journey can exercise - // for real on one machine. Its transport is operator config, not - // account input, so it never crosses the SSRF floor that refuses - // a local host for ntfy / webhook / Web Push. The responder these - // point at is started by the journey itself; see - // `e2e/setup/smtp-stub.ts` for why no other channel can be. - SMTP_HOST: SMTP_STUB_HOST, - SMTP_PORT: String(SMTP_STUB_PORT), - SMTP_FROM: SMTP_STUB_FROM, }, }, // The SHIPPED configuration, on its own port. @@ -262,5 +266,37 @@ export default defineConfig({ DASHBOARD_SSR_PREFETCH: "true", }, }, + // The mail-configured, scheduler-free server, on its own port. + // + // Only `notification-dispatch-journey.spec.ts` talks to it, and + // `e2e/setup/notification-server.ts` carries the whole reasoning: why + // the SMTP env must not reach the shared server (it would render the + // Email card for every spec that opens `/settings/integrations`), why + // this process must not run the reminder scheduler (a tick inside the + // journey's window is a writer no spec can see), and what the shared + // server keeping "all" still leaves standing. + { + command: `PORT=${NOTIFICATION_PORT} HOSTNAME=127.0.0.1 ${JSON.stringify(process.execPath)} .next/standalone/server.js`, + url: `${NOTIFICATION_BASE_URL}/api/version`, + timeout: 60_000, + reuseExistingServer: !process.env.CI, + stdout: "ignore", + stderr: "pipe", + env: { + ...process.env, + NATIVE_CANVAS: "off", + DASHBOARD_SSR_PREFETCH: "false", + HEALTHLOG_PROCESS_TYPE: "web", + // The one delivery channel a notification journey can exercise for + // real on one machine. Its transport is operator config, not + // account input, so it never crosses the SSRF floor that refuses a + // local host for ntfy / webhook / Web Push. The responder these + // point at is started by the journey itself; see + // `e2e/setup/smtp-stub.ts` for why no other channel can be. + SMTP_HOST: SMTP_STUB_HOST, + SMTP_PORT: String(smtpStubPort), + SMTP_FROM: SMTP_STUB_FROM, + }, + }, ], });