diff --git a/src/lib/__tests__/geo-offline-detection.test.ts b/src/lib/__tests__/geo-offline-detection.test.ts index 178439514..2379c9c6b 100644 --- a/src/lib/__tests__/geo-offline-detection.test.ts +++ b/src/lib/__tests__/geo-offline-detection.test.ts @@ -1,32 +1,45 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + mkdtempSync, + readFileSync, + rmSync, + unlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; /** - * v1.4.27 R5 — runtime detection + one-shot admin notification for the - * offline GeoLite2 tier. + * Runtime detection + once-per-state admin notification for the offline + * GeoLite2 tier. * * The CI workflow drops an `.empty` marker into `assets/geolite2/` * whenever `MAXMIND_LICENSE_KEY` is unset, so the Docker COPY still * lands a non-empty directory in `/opt/geolite2/`. The runtime resolver - * detects the marker on first fallback, fires a single localised - * notification to every admin, and stays silent for the rest of the - * process lifetime. + * detects the marker on first fallback and fires a localised + * notification to every admin. * - * The tests cover the three states the design pins: + * Issue #851: the sent-latch used to be a module-level boolean, so every + * worker boot re-sent the same notice while the configuration never + * changed. The anchor now persists in the `notification_events` ledger + * (via `claimNotificationEvent`) and is released when the state exits — + * databases configured, or online lookups disabled. The tests pin the + * full state machine: * - * 1. `.empty` marker present, City MMDB absent → notification fires - * on first public-IP lookup. - * 2. No marker, City MMDB present → no notification fires. - * 3. Marker present, repeat lookups → notification fires once and - * never again. + * 1. Entering the unconfigured state sends, once, to every admin. + * 2. Staying in the state — further lookups, and simulated worker + * restarts — stays silent. + * 3. Leaving the state releases the anchor; re-entering sends again. + * 4. `IP_GEO_LOOKUP_DISABLED=1` counts as having left the state: with + * no egress there is nothing to warn about. * * `mmdb-lib` is stubbed via the same shape the existing * `geo-asn.test.ts` uses; `fs` is left real because `offlineGeoReady` - * reads real files at the temp-dir path picked per test, and the - * `dispatchLocalisedNotification` + `prisma.user.findMany` are mocked - * so the test never reaches a database or a sender. + * reads real files at the temp-dir path picked per test. `@/lib/db` is + * mocked with an in-memory `notification_events` store that outlives + * `vi.resetModules()`, which is what lets a test simulate a restart + * while the "database" keeps its rows. `safeFetch` is stubbed so the + * online-fallback path never leaves the process. */ vi.mock("mmdb-lib", () => ({ @@ -41,15 +54,76 @@ vi.mock("mmdb-lib", () => ({ }, })); +vi.mock("@/lib/safe-fetch", () => ({ + safeFetch: vi.fn(async () => ({ ok: false, status: 503 })), +})); + const dispatchSpy = vi.fn(async () => undefined); vi.mock("@/lib/notifications/dispatch-localised", () => ({ dispatchLocalisedNotification: dispatchSpy, })); +interface EventRow { + recordUserId: string; + eventType: string; + dedupKey: string; + createdAt: Date; +} + +/** In-memory `notification_events` — survives `vi.resetModules()`. */ +const eventStore: EventRow[] = []; + const findManySpy = vi.fn(async () => [{ id: "admin-1" }]); + +const txStub = { + $queryRaw: async () => [{ locked: 1 }], + notificationEvent: { + findFirst: async (args: { + where: { + recordUserId: string; + eventType: string; + dedupKey: string; + createdAt: { gte: Date }; + }; + }) => + eventStore.find( + (row) => + row.recordUserId === args.where.recordUserId && + row.eventType === args.where.eventType && + row.dedupKey === args.where.dedupKey && + row.createdAt >= args.where.createdAt.gte, + ) ?? null, + create: async (args: { + data: { recordUserId: string; eventType: string; dedupKey: string }; + }) => { + const row: EventRow = { ...args.data, createdAt: new Date() }; + eventStore.push(row); + return row; + }, + }, +}; + vi.mock("@/lib/db", () => ({ prisma: { user: { findMany: findManySpy }, + $transaction: async (fn: (tx: typeof txStub) => Promise) => + fn(txStub), + notificationEvent: { + deleteMany: async (args: { + where: { eventType: string; dedupKey: string }; + }) => { + const before = eventStore.length; + for (let i = eventStore.length - 1; i >= 0; i--) { + if ( + eventStore[i].eventType === args.where.eventType && + eventStore[i].dedupKey === args.where.dedupKey + ) { + eventStore.splice(i, 1); + } + } + return { count: before - eventStore.length }; + }, + }, }, })); @@ -57,7 +131,7 @@ const ORIGINAL_ENV = { ...process.env }; let tmpRoot: string; async function flushMicrotasks(): Promise { - // The notification path is fire-and-forget — `void notify…()` — + // The notification path is fire-and-forget — `void evaluate…()` — // and pulls in `@/lib/db` + `@/lib/notifications/dispatch-localised` // through dynamic `import()` calls, so each dispatch needs several // microtask + macrotask flushes before the spy registers the call. @@ -75,6 +149,7 @@ beforeEach(() => { dispatchSpy.mockClear(); findManySpy.mockClear(); findManySpy.mockResolvedValue([{ id: "admin-1" }]); + eventStore.length = 0; vi.resetModules(); }); @@ -85,7 +160,7 @@ afterEach(() => { vi.unstubAllGlobals(); }); -describe("offline-geo runtime detection (v1.4.27 R5)", () => { +describe("offline-geo runtime detection", () => { it("offlineGeoReady() is false when the .empty marker is present", async () => { writeFileSync(join(tmpRoot, ".empty"), ""); const { offlineGeoReady } = await import("../geo"); @@ -125,10 +200,8 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { expect(offlineGeoReady()).toBe(true); }); - it("fires the one-shot admin notification when the marker is present and lookupIpLocation falls back", async () => { + it("fires the admin notification when the marker is present and lookupIpLocation falls back", async () => { writeFileSync(join(tmpRoot, ".empty"), ""); - // Disable the online fallback so the test does not hit the network. - process.env.IP_GEO_LOOKUP_DISABLED = "1"; const { lookupIpLocation } = await import("../geo"); await lookupIpLocation("8.8.8.8"); @@ -152,7 +225,6 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { it("names the real online provider in the notification params", async () => { writeFileSync(join(tmpRoot, ".empty"), ""); - process.env.IP_GEO_LOOKUP_DISABLED = "1"; process.env.IP_GEO_LOOKUP_URL = "https://ip-api.example/json"; const { lookupIpLocation } = await import("../geo"); @@ -204,6 +276,20 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { // The stubbed reader returns null for every IP, which is fine — the // lookup will still go through the online path, but the offline // check sees a healthy directory so the notification stays silent. + const { lookupIpLocation } = await import("../geo"); + + await lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + + expect(dispatchSpy).not.toHaveBeenCalled(); + expect(findManySpy).not.toHaveBeenCalled(); + }); + + it("does NOT fire the notification when online lookups are disabled", async () => { + // With IP_GEO_LOOKUP_DISABLED=1 no address leaves the host, so there + // is no egress for the notice to warn about — the disabled host has + // left the state exactly like one that mounted the databases. + writeFileSync(join(tmpRoot, ".empty"), ""); process.env.IP_GEO_LOOKUP_DISABLED = "1"; const { lookupIpLocation } = await import("../geo"); @@ -214,9 +300,8 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { expect(findManySpy).not.toHaveBeenCalled(); }); - it("fires the notification exactly once across repeated fallbacks (process-level latch)", async () => { + it("fires the notification exactly once across repeated fallbacks in one process", async () => { writeFileSync(join(tmpRoot, ".empty"), ""); - process.env.IP_GEO_LOOKUP_DISABLED = "1"; const { lookupIpLocation, lookupIpAsn } = await import("../geo"); await lookupIpLocation("8.8.8.8"); @@ -232,7 +317,6 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { it("falls back silently when no admin user is configured", async () => { writeFileSync(join(tmpRoot, ".empty"), ""); - process.env.IP_GEO_LOOKUP_DISABLED = "1"; findManySpy.mockResolvedValueOnce([]); const { lookupIpLocation } = await import("../geo"); @@ -253,4 +337,72 @@ describe("offline-geo runtime detection (v1.4.27 R5)", () => { expect(result).toBeNull(); expect(dispatchSpy).toHaveBeenCalledTimes(1); }); + + it("is once per configuration state: silent across restarts, resent only after exit + re-entry (issue #851)", async () => { + const cityDb = join(tmpRoot, "GeoLite2-City.mmdb"); + + // ── Boot 1: unconfigured → the notice goes out and anchors durably. + writeFileSync(join(tmpRoot, ".empty"), ""); + let geo = await import("../geo"); + await geo.lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(eventStore).toHaveLength(1); + + // ── Boot 2: worker restart, state unchanged → the persisted anchor + // keeps it silent. This is the #851 repeat, pinned closed. + vi.resetModules(); + geo = await import("../geo"); + await geo.lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(eventStore).toHaveLength(1); + + // ── Boot 3: the operator mounts the databases → state exits, the + // anchor is released, still no new send. + unlinkSync(join(tmpRoot, ".empty")); + writeFileSync(cityDb, Buffer.from("city-stub")); + vi.resetModules(); + geo = await import("../geo"); + await geo.lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(eventStore).toHaveLength(0); + + // ── Boot 4: the databases are gone again → genuine re-entry into the + // unconfigured state, and the notice fires afresh. + unlinkSync(cityDb); + writeFileSync(join(tmpRoot, ".empty"), ""); + vi.resetModules(); + geo = await import("../geo"); + await geo.lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(2); + expect(eventStore).toHaveLength(1); + }); + + it("re-evaluates after a runtime database fetch resets the reader cache", async () => { + // The runtime GeoLite2 fetch calls `resetGeoLite2ReaderCache()` after + // placing fresh databases. The next lookup must observe the healthy + // state and release the anchor without waiting for a restart. + writeFileSync(join(tmpRoot, ".empty"), ""); + const geo = await import("../geo"); + + await geo.lookupIpLocation("8.8.8.8"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(eventStore).toHaveLength(1); + + unlinkSync(join(tmpRoot, ".empty")); + writeFileSync( + join(tmpRoot, "GeoLite2-City.mmdb"), + Buffer.from("city-stub"), + ); + geo.resetGeoLite2ReaderCache(); + + await geo.lookupIpLocation("1.1.1.1"); + await flushMicrotasks(); + expect(dispatchSpy).toHaveBeenCalledTimes(1); + expect(eventStore).toHaveLength(0); + }); }); diff --git a/src/lib/geo.ts b/src/lib/geo.ts index 3efa3cd5a..6c7b2f9a6 100644 --- a/src/lib/geo.ts +++ b/src/lib/geo.ts @@ -76,15 +76,25 @@ * `MAXMIND_LICENSE_KEY` secret — the CI workflow drops an `.empty` * marker into the geo asset directory when the key is unset. * `offlineGeoReady()` is the canonical check used by `/api/version` - * and the admin status surface, and the lookup paths fire a one-shot - * admin notification on the first fallback so the maintainer hears - * about the gap from the running app. + * and the admin status surface, and the lookup paths fire an admin + * notification on the first fallback so the maintainer hears about the + * gap from the running app. The notification is once per CONFIGURATION + * STATE, not once per process: the sent-anchor persists in the + * `notification_events` ledger and is released when the state exits + * (databases configured, or lookups disabled), so a worker restart does + * not re-send while nothing changed (issue #851) and a genuine re-entry + * into the unconfigured state does. */ import fs from "node:fs"; import path from "node:path"; import { Reader as MmdbReader } from "mmdb-lib"; import type { AsnResponse, CityResponse } from "mmdb-lib/lib/reader/response"; import { getEvent } from "@/lib/logging/context"; +import { + claimNotificationEvent, + releaseNotificationEvent, + CONFIG_NOTICE_EVENT_TYPE, +} from "@/lib/notifications/reminder-dedup"; import { safeFetch } from "@/lib/safe-fetch"; interface IpwhoIsResponse { @@ -257,13 +267,16 @@ function getAsnReader(): MmdbReader | null { * calls this after it places freshly downloaded databases, so a running worker * picks them up without a restart — the readers latch `null` on the first miss, * so without this a process that booted before the files landed would keep - * resolving online forever. Also clears the one-shot "offline unavailable" - * admin-notification latch, since the offline tier may now be present. + * resolving online forever. Also clears the "offline unavailable" + * notice-evaluation latch, so the next lookup re-evaluates the + * configuration state — and, finding the offline tier present, releases + * the persisted sent-anchor so a later loss of the databases notifies + * again. */ export function resetGeoLite2ReaderCache(): void { cache.city = undefined; cache.asn = undefined; - notifiedThisProcess = false; + noticeEvaluatedThisProcess = false; } /** @@ -277,7 +290,7 @@ export function __resetGeoLite2CacheForTests(): void { GEO_CACHE.clear(); } -// ── Offline readiness + one-shot admin notification ───────────────── +// ── Offline readiness + once-per-state admin notification ──────────── // // The CI workflow drops an `.empty` marker into the geo asset directory // when the maintainer has not configured `MAXMIND_LICENSE_KEY`, in @@ -286,12 +299,36 @@ export function __resetGeoLite2CacheForTests(): void { // `/api/version`, the admin status surface, and the resolver's own // offline-first branch, so the three cannot disagree. // -// `notifiedThisProcess` is a module-level latch so the admin only -// receives the "offline geo is disabled" notification once per worker -// boot — every subsequent fallback hit is silent. Test-only reset is -// folded into the cache reset above. +// The notification is once per CONFIGURATION STATE. A module-level latch +// alone re-sent it on every worker boot while the state never changed +// (issue #851: same notice again and again on a host that has simply not +// mounted the databases). The durable anchor is a per-admin +// `notification_events` claim — the same ledger every other repeat-prone +// notice anchors in — released when the state exits, i.e. when the +// databases are configured or the online lookup is disabled. Entering the +// unconfigured state sends once; staying in it is silent across restarts; +// leaving and re-entering sends again. +// +// `noticeEvaluatedThisProcess` remains as a cheap gate so the ledger is +// consulted once per worker boot (and again after a runtime database +// fetch resets it), not on every lookup. Test-only reset is folded into +// the cache reset above. -let notifiedThisProcess = false; +let noticeEvaluatedThisProcess = false; + +/** Durable anchor identity for the offline-geo notice in `notification_events`. */ +const OFFLINE_GEO_NOTICE_DEDUP_KEY = "geo:offline-unavailable"; + +/** + * Whether the host is in the state the notice describes: the offline tier + * is not configured AND lookups still egress to the online provider. With + * `IP_GEO_LOOKUP_DISABLED=1` there is no egress to warn about, so the + * disabled host counts as having left the state — exactly like a host + * that mounted the databases. + */ +function offlineGeoNoticeStateActive(): boolean { + return !offlineGeoReady() && process.env.IP_GEO_LOOKUP_DISABLED !== "1"; +} /** * Where the admin notification sends someone who wants the offline tier. @@ -311,17 +348,28 @@ export function offlineGeoReady(): boolean { } } -async function notifyOfflineGeoUnavailable(): Promise { - if (notifiedThisProcess) return; - notifiedThisProcess = true; +async function evaluateOfflineGeoNotice(): Promise { + if (noticeEvaluatedThisProcess) return; + noticeEvaluatedThisProcess = true; // Defer the resolve so we don't pay the cost of pulling the - // Prisma client into the import graph until the first fallback — + // Prisma client into the import graph until the first evaluation — // most calls are cache hits or private-IP short-circuits. try { - const [{ prisma }, { dispatchLocalisedNotification }] = await Promise.all([ - import("@/lib/db"), - import("@/lib/notifications/dispatch-localised"), - ]); + const { prisma } = await import("@/lib/db"); + + if (!offlineGeoNoticeStateActive()) { + // The state the notice describes does not hold. Release any + // persisted sent-anchor so a future re-entry into the unconfigured + // state notifies again — this is the "state exit" edge. + await releaseNotificationEvent(prisma, { + eventType: CONFIG_NOTICE_EVENT_TYPE, + dedupKey: OFFLINE_GEO_NOTICE_DEDUP_KEY, + }); + return; + } + + const { dispatchLocalisedNotification } = + await import("@/lib/notifications/dispatch-localised"); const admins = await prisma.user.findMany({ where: { role: "ADMIN" }, select: { id: true }, @@ -332,10 +380,21 @@ async function notifyOfflineGeoUnavailable(): Promise { ); return; } - getEvent()?.addWarning( - `geo: offline databases unavailable, every lookup goes online to ${resolveGeoProviderHost()} — notifying admins`, - ); for (const admin of admins) { + // The anchor is unwindowed (`since` at epoch): it holds until the + // state exit above releases it, however many restarts happen in + // between. An admin who already has the anchor claimed nothing and + // hears nothing. + const claimed = await claimNotificationEvent(prisma, { + recordUserId: admin.id, + eventType: CONFIG_NOTICE_EVENT_TYPE, + dedupKey: OFFLINE_GEO_NOTICE_DEDUP_KEY, + since: new Date(0), + }); + if (!claimed) continue; + getEvent()?.addWarning( + `geo: offline databases unavailable, every lookup goes online to ${resolveGeoProviderHost()} — notifying admin`, + ); await dispatchLocalisedNotification({ userId: admin.id, titleKey: "notifications.admin.offlineGeoUnavailableTitle", @@ -610,15 +669,17 @@ export function lookupIpAsn( ip: string | null, ): { asn: number; carrier: string | null } | null { if (!ip || PRIVATE_IP.test(ip)) return null; + // First public-IP lookup of the process evaluates the offline-geo + // configuration state: unconfigured-with-egress sends the admin notice + // (once per state, anchored durably), configured-or-disabled releases + // the anchor so a later re-entry notifies again. Runs on the healthy + // path too — that is where the state-exit edge is observed. + void evaluateOfflineGeoNotice(); const reader = getAsnReader(); if (!reader) { - // No offline ASN data — alert once per process. The online tier can - // still fill the carrier from its ISP field, but the ASN number and - // the authoritative carrier come from the MMDB, so the admin is told - // once that the offline tier is not being read. - if (!offlineGeoReady()) { - void notifyOfflineGeoUnavailable(); - } + // No offline ASN data. The online tier can still fill the carrier + // from its ISP field, but the ASN number and the authoritative + // carrier come from the MMDB. return null; } try { diff --git a/src/lib/jobs/reminder/__tests__/notification-retention.test.ts b/src/lib/jobs/reminder/__tests__/notification-retention.test.ts index 2573d6698..4314ad55d 100644 --- a/src/lib/jobs/reminder/__tests__/notification-retention.test.ts +++ b/src/lib/jobs/reminder/__tests__/notification-retention.test.ts @@ -49,8 +49,14 @@ describe("notification retention", () => { expect(h.pushDeleteMany).toHaveBeenCalledWith({ where: { createdAt: { lt: expect.any(Date) } }, }); + // State-scoped admin-config-notice anchors live as long as the state + // they describe; the horizon prune must leave them alone or the + // once-per-state notice re-fires every 90 days. expect(h.eventDeleteMany).toHaveBeenCalledWith({ - where: { createdAt: { lt: expect.any(Date) } }, + where: { + createdAt: { lt: expect.any(Date) }, + eventType: { not: "ADMIN_CONFIG_NOTICE" }, + }, }); expect(h.authorizationDeleteMany).toHaveBeenCalledWith({ where: { authorizedAt: { lt: expect.any(Date) } }, diff --git a/src/lib/jobs/reminder/cleanup-handlers.ts b/src/lib/jobs/reminder/cleanup-handlers.ts index e793bce69..98ad16be9 100644 --- a/src/lib/jobs/reminder/cleanup-handlers.ts +++ b/src/lib/jobs/reminder/cleanup-handlers.ts @@ -38,6 +38,7 @@ import { cleanupExpiredMoodTombstones, cleanupExpiredIntakeTombstones, } from "@/lib/jobs/measurement-tombstone-cleanup"; +import { CONFIG_NOTICE_EVENT_TYPE } from "@/lib/notifications/reminder-dedup"; import { getWorkerPrisma } from "./shared"; const MOOD_REMINDER_RETENTION_DAYS = 90; @@ -125,7 +126,12 @@ export async function handleMoodReminderCleanup( * so anything older than the 90-day retention window is dead weight * inflating the table and the `(user_id, created_at DESC)` index. Record-only * notification events are claims rather than delivery diagnostics, but after - * the same horizon their dedup windows have elapsed and they have no reader. + * the same horizon their dedup windows have elapsed and they have no reader — + * with one carve-out: state-scoped admin-config-notice anchors + * (`CONFIG_NOTICE_EVENT_TYPE`) hold for as long as the configuration state + * they describe and are released explicitly on state exit, so the horizon + * delete must not touch them or the once-per-state notice re-fires every + * 90 days. * * The DELETE is unbounded by user. The event and attempt ledgers carry direct * `created_at` indexes, and final egress claims carry `authorized_at`, so each @@ -151,7 +157,10 @@ export async function handlePushAttemptCleanup( where: { createdAt: { lt: cutoff } }, }); const deletedEvents = await p.notificationEvent.deleteMany({ - where: { createdAt: { lt: cutoff } }, + where: { + createdAt: { lt: cutoff }, + eventType: { not: CONFIG_NOTICE_EVENT_TYPE }, + }, }); const deletedAuthorizations = await p.notificationEgressAuthorization.deleteMany({ diff --git a/src/lib/notifications/reminder-dedup.ts b/src/lib/notifications/reminder-dedup.ts index 16f5ee741..e5b8c89f8 100644 --- a/src/lib/notifications/reminder-dedup.ts +++ b/src/lib/notifications/reminder-dedup.ts @@ -33,6 +33,18 @@ import type { PrismaClient } from "@/generated/prisma/client"; */ export const REMINDER_DEDUP_LOOKBACK_MS = 48 * 60 * 60 * 1000; +/** + * Event type for state-scoped admin configuration notices ("offline geo is + * not configured" and its future siblings). These anchors are not windowed: + * they hold for as long as the configuration state itself holds — days or + * years — and are released explicitly when the state exits, so the notice + * can fire again on re-entry and never in between. The retention prune in + * `cleanup-handlers.ts` excludes this event type for exactly that reason; a + * horizon delete would re-open the once-per-state notice every 90 days + * (issue #851 wearing a new clock). + */ +export const CONFIG_NOTICE_EVENT_TYPE = "ADMIN_CONFIG_NOTICE"; + /** * Dedup key for one medication dose slot in one phase on one local day. * @@ -118,3 +130,34 @@ export async function claimNotificationEvent( return false; } } + +/** + * Release every claim on one event across all record users. The counterpart + * to `claimNotificationEvent` for state-scoped anchors: when the condition a + * notice describes stops holding, its anchors are deleted so the next entry + * into the condition sends a fresh notice. Deliberately not scoped to a + * single `recordUserId` — the state is host-wide, so its exit releases the + * anchor for every admin at once. + * + * Never throws. A failed release leaves the anchors standing, which + * suppresses a future notice rather than duplicating one — the safe side of + * this particular trade, and the next state exit retries it. + */ +export async function releaseNotificationEvent( + prisma: PrismaClient, + input: { + eventType: string; + dedupKey: string; + }, +): Promise { + try { + await prisma.notificationEvent.deleteMany({ + where: { + eventType: input.eventType, + dedupKey: input.dedupKey, + }, + }); + } catch { + // Swallowed by design; see above. + } +}