From 4ff3e4e1b735542e49d1752758a4429e17551e5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 12 Nov 2026 14:12:45 +0100 Subject: [PATCH] fix(notifications): rest a channel that had nobody to send to A send with no browser subscribed, no iPhone paired or a transport the operator has not set up no longer counts as a failure, but nothing else replaced it. The mood reminder and the daily briefing retry every minute until delivered, so such a channel wrote a skipped row each minute, for good; the dispatch still counted as an attempt, so the watchdog and status alerts took it as announced; and the status screen said "active" with no hint why nothing arrived. The channel now rests for an hour after such a send, with the reason on its status, and the send does not count as an attempt. Subscribing a browser or pairing an iPhone ends the rest at once. --- src/app/api/devices/__tests__/route.test.ts | 27 +++++++++++++++++++ src/app/api/devices/route.ts | 9 +++++++ .../__tests__/dispatcher.test.ts | 25 ++++++++++++++--- src/lib/notifications/channel-state.ts | 21 +++++++++++++++ src/lib/notifications/dispatcher.ts | 12 ++++++--- src/lib/notifications/retry-policy.ts | 8 ++++++ src/lib/openapi/routes/notifications.ts | 2 +- 7 files changed, 97 insertions(+), 7 deletions(-) diff --git a/src/app/api/devices/__tests__/route.test.ts b/src/app/api/devices/__tests__/route.test.ts index a06d413e0..d82648c3b 100644 --- a/src/app/api/devices/__tests__/route.test.ts +++ b/src/app/api/devices/__tests__/route.test.ts @@ -17,6 +17,7 @@ vi.mock("@/lib/db", () => ({ $transaction: vi.fn(), notificationChannel: { upsert: vi.fn(), + updateMany: vi.fn(), }, auditLog: { create: vi.fn(), @@ -89,6 +90,9 @@ beforeEach(() => { vi.mocked(prisma.notificationChannel.upsert).mockResolvedValue({ id: "ch-1", } as never); + vi.mocked(prisma.notificationChannel.updateMany).mockResolvedValue({ + count: 0, + }); vi.mocked(prisma.auditLog.create).mockResolvedValue({} as never); // `resetAllMocks` strips the top-level `.mockResolvedValue(...)` // declaration too, so re-arm the rate-limit success path here. @@ -219,6 +223,29 @@ describe("POST /api/devices", () => { expect(args.data.apnsEnvironment).toBe("sandbox"); }); + it("ends a pause that was waiting for a paired iPhone", async () => { + // A reminder with no iPhone paired rests the APNs channel for an hour; + // pairing one must not leave the next reminder waiting it out. + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + const res = await POST( + req({ + token: "abcd1234efgh5678", + bundleId: "io.healthlog.app", + apnsToken: "deadbeef".repeat(8), + apnsEnvironment: "sandbox", + }), + ); + expect(res.status).toBe(201); + expect(prisma.notificationChannel.updateMany).toHaveBeenCalledWith({ + where: { + userId: "user-1", + type: "APNS", + lastFailureReason: "apns_no_devices", + }, + data: { nextRetryAt: null, lastFailureReason: null }, + }); + }); + it("rejects re-registration of an apnsToken owned by another user with 409", async () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(prisma.device.findFirst).mockResolvedValue({ diff --git a/src/app/api/devices/route.ts b/src/app/api/devices/route.ts index 36f24678b..f8d8e5a71 100644 --- a/src/app/api/devices/route.ts +++ b/src/app/api/devices/route.ts @@ -272,6 +272,15 @@ export const POST = apiHandler(async (request: NextRequest) => { }, update: {}, }); + // The iPhone a no-device pause was waiting for: deliver at once. + await tx.notificationChannel.updateMany({ + where: { + userId: user.id, + type: "APNS", + lastFailureReason: "apns_no_devices", + }, + data: { nextRetryAt: null, lastFailureReason: null }, + }); } return { id: device.id }; diff --git a/src/lib/notifications/__tests__/dispatcher.test.ts b/src/lib/notifications/__tests__/dispatcher.test.ts index e5b80df3c..c03eaf7b8 100644 --- a/src/lib/notifications/__tests__/dispatcher.test.ts +++ b/src/lib/notifications/__tests__/dispatcher.test.ts @@ -211,13 +211,15 @@ describe("dispatchNotification — nobody to send to", () => { // Turning push off on the last browser deletes its subscription and leaves // the channel on. Counted as failures, the next five reminders switched the // channel off for good, and subscribing again later did not bring it back. + // Not counted either, the jobs that retry until delivered asked again every + // minute, and a watchdog read the attempt as an announced alert. it.each([ ["WEB_PUSH", "web_push_no_subscriptions"], ["WEB_PUSH", "web_push_vapid_not_configured"], ["APNS", "apns_no_devices"], ["APNS", "apns_not_configured"], ] as const)( - "%s answering %s leaves the channel state alone", + "%s answering %s rests the channel without counting a failure or an attempt", async (type, reason) => { vi.mocked(prisma.notificationChannel.findMany).mockResolvedValueOnce([ { @@ -241,8 +243,25 @@ describe("dispatchNotification — nobody to send to", () => { }); expect(sender).toHaveBeenCalledTimes(1); - expect(outcome.dispatched).toBe(false); - expect(prisma.notificationChannel.update).not.toHaveBeenCalled(); + expect(outcome).toEqual({ + dispatched: false, + channelsAttempted: 0, + channelsSucceeded: 0, + }); + expect(prisma.notificationChannel.update).toHaveBeenCalledTimes(1); + const { data } = vi.mocked(prisma.notificationChannel.update).mock + .calls[0][0] as { data: Record }; + // The reason for the status screen and an hour's rest; the counter and + // the switch stay as they were. + expect(data).toEqual({ + lastFailureAt: expect.any(Date), + lastFailureReason: reason, + nextRetryAt: expect.any(Date), + }); + expect( + (data.nextRetryAt as Date).getTime() - + (data.lastFailureAt as Date).getTime(), + ).toBe(60 * 60 * 1000); expect(auditLog).not.toHaveBeenCalled(); }, ); diff --git a/src/lib/notifications/channel-state.ts b/src/lib/notifications/channel-state.ts index 62e7cc937..a915cd961 100644 --- a/src/lib/notifications/channel-state.ts +++ b/src/lib/notifications/channel-state.ts @@ -8,6 +8,7 @@ import { prisma } from "@/lib/db"; import { auditLog } from "@/lib/auth/audit"; import { + NO_RECIPIENT_COOLDOWN_MS, nextRetryAt, shouldAutoDisableAfterTransient, type SendOutcome, @@ -129,6 +130,26 @@ export async function recordChannelTransientFailure( return { autoDisabled: false, nextRetryAt: retryAt }; } +/** + * Record a send that had nobody to go to. Not a failure: the counter stays + * where it is. The reason is kept, so the status screen says why nothing + * arrives, and the channel rests for `NO_RECIPIENT_COOLDOWN_MS`. + */ +export async function recordChannelNoRecipient( + channel: ChannelRef, + reason: string, + now: Date = new Date(), +): Promise { + await prisma.notificationChannel.update({ + where: { id: channel.id }, + data: { + lastFailureAt: now, + lastFailureReason: reason, + nextRetryAt: new Date(now.getTime() + NO_RECIPIENT_COOLDOWN_MS), + }, + }); +} + /** * Re-enable a channel that was previously auto-disabled. Clears the * disabled-reason + counter and the cooldown so the next dispatch diff --git a/src/lib/notifications/dispatcher.ts b/src/lib/notifications/dispatcher.ts index 4b102c7c2..610b771d5 100644 --- a/src/lib/notifications/dispatcher.ts +++ b/src/lib/notifications/dispatcher.ts @@ -20,6 +20,7 @@ import type { SendOutcome } from "@/lib/notifications/retry-policy"; import { isChannelInCooldown, recordChannelHardReject, + recordChannelNoRecipient, recordChannelSuccess, recordChannelTransientFailure, } from "@/lib/notifications/channel-state"; @@ -49,7 +50,8 @@ import { * not set up. The sender writes a `skipped` ledger row. The channel has not * failed, so these leave its failure counter alone; counted as failures, * turning push off on the last device would switch the channel off for good - * after five reminders. + * after five reminders. The channel rests for a while instead + * (`recordChannelNoRecipient`), and the send does not count as an attempt. */ const NO_RECIPIENT_REASONS: ReadonlySet = new Set([ "web_push_no_subscriptions", @@ -74,8 +76,8 @@ const NO_RECIPIENT_REASONS: ReadonlySet = new Set([ * 6. Call the appropriate sender — which now returns a `SendOutcome` so * the dispatcher can classify hard rejects (410, blocked-by-user) vs * soft errors (5xx, 429, network) and update channel state accordingly. - * A send that had nobody to go to (`NO_RECIPIENT_REASONS`) changes no - * channel state. + * A send that had nobody to go to (`NO_RECIPIENT_REASONS`) pauses the + * channel without counting a failure. * * Also handles legacy Telegram config stored on the User model by * auto-migrating it to a NotificationChannel record on first dispatch. @@ -432,6 +434,10 @@ export async function dispatchNotification( } if (outcome.reason && NO_RECIPIENT_REASONS.has(outcome.reason)) { + // Nothing went anywhere: a caller that reads an attempt as "the + // alert was announced" must not read this one so. + channelsAttempted -= 1; + await recordChannelNoRecipient(ref, outcome.reason, now); getEvent()?.addMeta( `notification_skip_${channel.type.toLowerCase()}_no_recipient`, outcome.reason, diff --git a/src/lib/notifications/retry-policy.ts b/src/lib/notifications/retry-policy.ts index 958024563..d6b493abe 100644 --- a/src/lib/notifications/retry-policy.ts +++ b/src/lib/notifications/retry-policy.ts @@ -76,6 +76,14 @@ export const BACKOFF_SCHEDULE_MS: readonly number[] = Object.freeze([ export const MAX_CONSECUTIVE_FAILURES = 5; +/** + * How long a channel rests after a send that had nobody to go to (no + * browser subscribed, no iPhone paired, a transport the operator has not set + * up). The jobs that retry until delivered would otherwise ask again every + * minute. Subscribing or pairing ends the rest at once. + */ +export const NO_RECIPIENT_COOLDOWN_MS = 60 * 60 * 1_000; + /** * Given the new `consecutiveFailures` value (1, 2, …) AFTER incrementing, * return the wall-clock time when a retry is allowed. Returns null once the diff --git a/src/lib/openapi/routes/notifications.ts b/src/lib/openapi/routes/notifications.ts index d83d218e4..a226567f0 100644 --- a/src/lib/openapi/routes/notifications.ts +++ b/src/lib/openapi/routes/notifications.ts @@ -105,7 +105,7 @@ const channelDeliveryStatus = z state: z .enum(["active", "auto_disabled", "manually_disabled", "sending_paused"]) .describe( - "Server-derived. `auto_disabled` — the dispatcher turned it off after repeated hard rejects (`disabledReason` says why). `manually_disabled` — the user turned it off. `sending_paused` — still enabled but inside a retry cooldown until `nextRetryAt`. `active` — enabled and sending.", + "Server-derived. `auto_disabled` — the dispatcher turned it off after repeated hard rejects (`disabledReason` says why). `manually_disabled` — the user turned it off. `sending_paused` — still enabled but inside a retry cooldown until `nextRetryAt`, after a transient failure or after a send that had nobody to go to (no browser subscribed, no iPhone paired, a transport the operator has not set up; `lastFailureReason` names it, `consecutiveFailures` is not raised). `active` — enabled and sending.", ), disabledReason: z .string()