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, + }, + }, ], });